python
Move `#include` directives before `extern "C" {` in internal headers
Feature or enhancement
Some of our internal headers (e.g., pycore_ceval.h) contain #include directives inside the extern "C" { blocks. This can cause problems if system headers are included within an extern "C" { block and the header is compiled as C++ code. For a similar issue with the public headers, see #110845.
As far as I can tell, nobody has complained about this yet, but I think we might as well address this sooner rather later.
cc @ericsnowcurrently @gpshead @vstinner
Yes, it sounds like a good idea.
It took me a while, but I think that I understood the C++20 module issue: https://github.com/python/cpython/issues/110845#issuecomment-1931113272
If someone asks, let's make them happy, but I don't think we need to do this proactively. Internal headers should be compiled as C.
If someone asks, let's make them happy, but I don't think we need to do this proactively. Internal headers should be compiled as C.
I suggest to close this issue in this case.
@albanD asked me about this a week or two ago in the context of PyTorch. I'm not sure if it's still an issue or if it's been worked around.
@albanD, please speak if this is an issue for you. I'll close this in a month if no one complains.
In TorchDynamo eval_frame.c was implemented in C (as opposed to C++ like the rest of PyTorch), because CPython headers (internal/pycore_frame.h, internal/pycore_pystate.h, etc) did not work from C++.
This was a few years ago at this point, so I forget the exact error -- but the workaround was to implement that one file in C. We would prefer for that file to be implemented in C++.
Thanks for considering our request. On top of what @jansel mentioned above, I would add:
- Some definitions we need for subtype dealloc use, we end up just putting the "extern" in our code to not have to deal with including the corresponding header (pycore_weakref in this case): https://github.com/pytorch/pytorch/blob/1cc9354cb0a1ac2dcd63c73b37f5bec0dec0cba3/torch/csrc/utils/python_compat.h#L36-L37
- For the part where this is not as simple as just an extern and we need full definitions, we have built a cpython_defs layer (that is heavily used within the eval_frame.c Jason is mentioning above and has its own .c file compiled as plain C) that allows us to include/copy any code we need from CPython and re-expose it in a C++ friendly way.
As you might imagine, this is fairly brittle (very dependent on cpython core include changes) and prevents us from using c++ as with the rest of our codebase.
Some definitions we need for subtype dealloc use, we end up just putting the "extern" in our code to not have to deal with including the corresponding header (pycore_weakref in this case): https://github.com/pytorch/pytorch/blob/1cc9354cb0a1ac2dcd63c73b37f5bec0dec0cba3/torch/csrc/utils/python_compat.h#L36-L37
For _PyWeakref_ClearRef, see https://github.com/capi-workgroup/decisions/issues/25
Is there anything that should be exposed as public (potentially unstable) API, that we could document, test and support?
If you want the compiled code to match CPython, you will want to compile it as C. Especially if you've already taught your build system to do that. I don't think the extern "C" { itself is a major problem -- though it's likely to be the first issue in the file.
From a quick look at the code, it seems that you're reaching into the kind internals that change as CPython is optimized. I'm afraid I don't have advice for you other than: think about what could be contributed to CPython, and what could be put behind a supported API boundary. (Also: if you're copying code from CPython, please mind the licence.)
I will close this issue. If you must use internal headers, then I think compiling them as C is the way to go. Discussion about bigger issues than moving extern "C" { would be better on Discourse.