Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C API] The internal C API <pycore_dict.h> cannot be used in C++ #108216

Closed
2 tasks done
P403n1x87 opened this issue Aug 21, 2023 · 17 comments
Closed
2 tasks done

[C API] The internal C API <pycore_dict.h> cannot be used in C++ #108216

P403n1x87 opened this issue Aug 21, 2023 · 17 comments
Labels
topic-C-API type-bug An unexpected behavior, bug, or error

Comments

@P403n1x87
Copy link
Contributor

P403n1x87 commented Aug 21, 2023

Bug report

Checklist

  • I am confident this is a bug in CPython, not a bug in a third-party project
  • I have searched the CPython issue tracker,
    and am confident this bug has not been reported before

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Output from running 'python -VV' on the command line:

3.12.0rc1

A clear and concise description of the bug:

This looks like a new instance of #67832.

@vstinner This seems to occur in 3.12 when importing e.g. pycore_dict.h in a C++ module https://github.com/P403n1x87/echion/actions/runs/5775912109/job/16065738879

In file included from /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/internal/pycore_atomic.h:15,
                       from /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/internal/pycore_runtime.h:12,
                       from /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/internal/pycore_dict.h:13,
                       from ./echion/mirrors.h:14,
                       from echion/coremodule.cc:29:
      /usr/lib/gcc/x86_64-linux-gnu/9/include/stdatomic.h: At global scope:
      /usr/lib/gcc/x86_64-linux-gnu/9/include/stdatomic.h:40:9: error: ‘_Atomic’ does not name a type
         40 | typedef _Atomic _Bool atomic_bool;
            |         ^~~~~~~

https://github.com/P403n1x87/echion/blob/29275a13aea67a8d008471523f38bc4477499a98/echion/mirrors.h#L14

The same job with clang on OSX doesn't show the same issue: https://github.com/P403n1x87/echion/actions/runs/5775912109/job/16065739714.

Linked PRs

@P403n1x87 P403n1x87 added the type-bug An unexpected behavior, bug, or error label Aug 21, 2023
@P403n1x87 P403n1x87 changed the title Cannot compile C++ extensions with GCC when importing from core Cannot compile C++ extensions with GCC when including core headers Aug 21, 2023
@vstinner
Copy link
Member

This looks like a new instance of #67832.

This old issue was about including the public C API <Python.h>. This issue is about using the internal C API in C++.

@vstinner vstinner changed the title Cannot compile C++ extensions with GCC when including core headers [C API] The internal C API <pycore_dict.h> cannot be used in C++ Aug 21, 2023
@vstinner
Copy link
Member

What is your use case for using the internal C API?

https://github.com/P403n1x87/echion/ description:

Near-zero-overhead, in-process CPython frame stack sampler with async support

It looks like a Python profiler.

@vstinner
Copy link
Member

In Python 3.11, I added test_cppext to check that the Python C API is usable in C++, but only the public C API.

@P403n1x87
Copy link
Contributor Author

It looks like a Python profiler.

Indeed it is a profiler (actually just a frame stack sample fwiw). The internal API is used to access internal fields of structures from copies of Python objects so that it can all be done without the GIL

vstinner added a commit to vstinner/cpython that referenced this issue Aug 21, 2023
* Add missing includes
* Remove unused includes
* Mention at least once included symbol
* Sort includes
vstinner added a commit to vstinner/cpython that referenced this issue Aug 21, 2023
* Add missing includes
* Remove unused includes
* Mention at least once included symbol
* Sort includes
* Update Tools/cases_generator/generate_cases.py used to generated
  pycore_opcode_metadata.h.
* Update Parser/asdl_c.py used to generate pycore_ast.h.
* Cleanup also includes in _testcapimodule.c and _testinternalcapi.c.
@vstinner
Copy link
Member

With Python 3.11, it's possible to include <pycore_dict.h> in C++? In 3.11, there is no include.

Python 3.12 added two includes:

#include "pycore_dict_state.h"
#include "pycore_runtime.h"         // _PyRuntime

PEP 684: A Per-Interpreter GIL modified DICT_NEXT_VERSION() to access interp->dict_state.global_version: the counter is now "per-interpreter". The problem is that accessing PyInterprepter members requires to include pycore_interp.h is a giant beast: pycore_interp.h includes the problematic pycore_atomic.h.

vstinner added a commit that referenced this issue Aug 21, 2023
* Add missing includes.
* Remove unused includes.
* Update old include/symbol names to newer names.
* Mention at least one included symbol.
* Sort includes.
* Update Tools/cases_generator/generate_cases.py used to generated
  pycore_opcode_metadata.h.
* Update Parser/asdl_c.py used to generate pycore_ast.h.
* Cleanup also includes in _testcapimodule.c and _testinternalcapi.c.
vstinner added a commit to vstinner/cpython that referenced this issue Aug 21, 2023
Sub-set of pycore_dict.h which can be used by debuggers and
profilers.

pycore_dict.h includes pycore_dict_struct.h.
vstinner added a commit to vstinner/cpython that referenced this issue Aug 21, 2023
Sub-set of pycore_dict.h which can be used by debuggers and
profilers.

pycore_dict.h includes pycore_dict_struct.h.
vstinner added a commit to vstinner/cpython that referenced this issue Aug 21, 2023
pycore_dict_struct.h is a sub-set of pycore_dict.h which can be used
by debuggers and profilers and should be usable in C++.

pycore_dict.h includes pycore_dict_struct.h (no API change).
vstinner added a commit to vstinner/cpython that referenced this issue Aug 21, 2023
Add a new internal pycore_dict_struct.h header file. It can be used
by debuggers and profilers to inspect a Python dictionary without
having to call Python functions. It should be usable in C++.

pycore_dict.h includes pycore_dict_struct.h (no API change).
@vstinner
Copy link
Member

I proposed PR #108235 which adds <pycore_dict_struct.h>: a sub-set of <pycore_dict.h> which should be usable in C++. It has no include and so does not cause C++ issue on <pycore_atomic.h>.

@P403n1x87
Copy link
Contributor Author

With Python 3.11, it's possible to include <pycore_dict.h> in C++? In 3.11, there is no include.

Yep I add the import of pycore_dict if PY_VERSION_HEX >= 0x030b0000 and it compiles fine

https://github.com/P403n1x87/echion/blob/29275a13aea67a8d008471523f38bc4477499a98/echion/mirrors.h#L12-L15

@methane
Copy link
Member

methane commented Aug 22, 2023

Maybe workaround:

#ifdef HAVE_STD_ATOMIC
#undef HAVE_STD_ATOMIC
#endif
#include <pycore_dict.h>

@vstinner
Copy link
Member

@P403n1x87: Does @methane's workaround work for you? If yes, there is no need to change the header file, right?

@P403n1x87
Copy link
Contributor Author

@vstinner will try to test today and report

@P403n1x87
Copy link
Contributor Author

P403n1x87 commented Aug 28, 2023

@vstinner @methane's workaround seems to work as it makes all the atomic-related errors disappear

GH job: https://github.com/P403n1x87/echion/actions/runs/5997079612/job/16262783675?pr=8

PR: P403n1x87/echion#8

With that "noise" reduced, I spotted this other seemingly GCC-only issue

      In file included from /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/objimpl.h:227,
                       from /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/Python.h:45,
                       from echion/coremodule.cc:6:
      /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/internal/pycore_gc.h: At global scope:
      /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/cpython/objimpl.h:85:30: error: ‘int PyObject_GC_IsFinalized(PyObject*)’ was declared ‘extern’ and later ‘static’ [-fpermissive]
         85 | #  define _PyGC_FINALIZED(o) PyObject_GC_IsFinalized(o)
            |                              ^~~~~~~~~~~~~~~~~~~~~~~
      /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/internal/pycore_gc.h:84:19: note: in expansion of macro ‘_PyGC_FINALIZED’
         84 | static inline int _PyGC_FINALIZED(PyObject *op) {
            |                   ^~~~~~~~~~~~~~~
      In file included from /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/Python.h:45,
                       from echion/coremodule.cc:6:
      /opt/hostedtoolcache/Python/3.12.0-rc.1/x64/include/python3.12/objimpl.h:209:17: note: previous declaration of ‘int PyObject_GC_IsFinalized(PyObject*)’
        209 | PyAPI_FUNC(int) PyObject_GC_IsFinalized(PyObject *);
            |                 ^~~~~~~~~~~~~~~~~~~~~~~

but this likely a different issue.

@vstinner
Copy link
Member

I removed # define _PyGC_FINALIZED(o) in the main branch, but my backport to 3.12 was rejected. Just undefine the macro manually.

@vstinner
Copy link
Member

@vstinner @methane's workaround seems to work as it makes all the atomic-related errors disappear

In that case, I propose to not add pycore_dict_struct.h for now. I suggest you using:

#include <Python.h>

// Workaround pycore_atomic.h build error:
// https://github.com/python/cpython/issues/108216
#ifdef HAVE_STD_ATOMIC
#  undef HAVE_STD_ATOMIC
#endif

// Workaround _PyGC_FINALIZED() build error on Python <= 3.12:
// https://github.com/python/cpython/issues/105268
#undef _PyGC_FINALIZED

#include <pycore_dict.h>

Thanks for the feedback. I close the issue.

@erlend-aasland erlend-aasland closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2023
@erlend-aasland
Copy link
Contributor

Ops, sorry for the noise, Victor! I didn't see there was a related PR that fixed some of the issues.

@vstinner
Copy link
Member

This issue was not fixed. Only workaround is offered as a way to avoid the issue 😁

@erlend-aasland
Copy link
Contributor

Aha, so we close it as not planned then! :)

@erlend-aasland erlend-aasland closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2023
@vstinner
Copy link
Member

Oh. I tried but apparently failed to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants