FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-144490: Fix C++ compatibility in pycore_cell.h by yoney · Pull Request #144482 · python/cpython · GitHub

/ cpython Public

gh-144490: Fix C++ compatibility in pycore_cell.h - #144482

Merged
ZeroIntensity merged 3 commits into
python:mainfrom
yoney:pycore_cell
Feb 7, 2026
Merged

gh-144490: Fix C++ compatibility in pycore_cell.h#144482
ZeroIntensity merged 3 commits into
python:mainfrom
yoney:pycore_cell

Conversation

yoney commented Feb 4, 2026
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

The header already includes an extern "C" guard, suggesting it is intended to be C++ compatible. Adding a cast allows it to compile without -fpermissive.

cc: @colesbury

picnixz commented Feb 4, 2026

Copy link
Copy Markdown
Member

Do we do this kind of stuff for every calls? do we support -fpermissive flags? should we do it? I would prefer having an issue (maybe an old one) for other places that could be affected by this.

Copy link
Copy Markdown
Contributor

The internal headers (pycore_xxx.h) are often not C++ compatible, even though they all have extern "C" { guards (often in the wrong places).

The change itself seems fine, but I'm curious how this came up. What's including pycore_cell.h from C++?

Copy link
Copy Markdown
Contributor

To answer @picnixz, we generally do this sort of thing when people ask, especially when the changes are tiny, but we don't proactively make the internal header files C++ compatible.

yoney commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

What's including pycore_cell.h from C++?

This came up while I’m adapting CinderX to FT-Python: C++ codegen path for LOAD_DEREF/STORE_DEREF ends up including pycore_cell.h and calling the internal helpers as a good starting point (vs. generating the atomic-swap sequence right away). I initially worked around it with wrappers on our side, but since the header already has extern "C" guards and the change is tiny, I wanted to check whether it’s okay to make this compile cleanly from C++.

Also agreed that pycore_* headers aren’t guaranteed to be C++-compatible and may change; in CinderX we already adapt to internal changes per version.

Copy link
Copy Markdown
Contributor

Ok, make sense. Can you open an issue like @picnixz asked? Also edit the PR title to associate it with the issue.

yoney changed the title Fix C++ compatibility in _PyCell_GetStackRef gh-144490: Fix C++ compatibility in pycore_cell.h Feb 4, 2026

yoney commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

Created the issue #144490

@picnixz, @colesbury: Thank you so much for the review.

ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please also add an #include to test_cppext as well, like this:

#ifdef TEST_INTERNAL_C_API
// gh-135906: Check for compiler warnings in the internal C API
# include "internal/pycore_backoff.h"
# include "internal/pycore_frame.h"
#endif

That'll make sure we don't break this again someday.

Comment thread Include/internal/pycore_cell.h Outdated
PyObject *value;
#ifdef Py_GIL_DISABLED
value = _Py_atomic_load_ptr(&cell->ob_ref);
value = (PyObject *)_Py_atomic_load_ptr(&cell->ob_ref);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Up to you, but I believe we've used _Py_STATIC_CAST for things like this in the past. That will expand to static_cast<type>(expr) in C++, and then the usual (type)(expr) in C.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Most other headers use _PyObject_CAST so I think we should use that instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you. I grepped for _Py_STATIC_CAST and _PyObject_CAST. There were more usages of _PyObject_CAST, so I changed it to that.

vstinner commented Feb 5, 2026

Copy link
Copy Markdown
Member

Please also add an #include to test_cppext as well

Just including the header is not enough to reproduce the issue. test_cppext just pass with the following change:

diff --git a/Lib/test/test_cppext/extension.cpp b/Lib/test/test_cppext/extension.cpp
index f95655eccde..881140eb695 100644
--- a/Lib/test/test_cppext/extension.cpp
+++ b/Lib/test/test_cppext/extension.cpp
@@ -15,6 +15,7 @@
 #ifdef TEST_INTERNAL_C_API
    // gh-135906: Check for compiler warnings in the internal C API
 #  include "internal/pycore_backoff.h"
+#  include "internal/pycore_cell.h"
 #  include "internal/pycore_frame.h"
 #endif
 

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Copy link
Copy Markdown
Contributor

Just including the header is not enough to reproduce the issue. test_cppext just pass with the following change:

@vstinner - I think BaseTests in Lib/test/test_cppext/__init__.py is missing handling of TEST_INTERNAL_C_API. It doesn't have the env['TEST_INTERNAL_C_API'] = str(int(self.TEST_INTERNAL_C_API)) line from test_cext

Also, in case it's not clear, the error here is limited to Py_GIL_DISABLED builds.

yoney commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Please also add an #include to test_cppext as well, like this:

I’m not sure if this is the correct way to run it. When I set TEST_INTERNAL_C_API=1, the test immediately emits many C++ errors. I tried fixing some of them by passing CPPFLAGS, but there are still a lot of errors, so I stopped. Below are a few sample error lines; many seem to originate from mimalloc. It’s also possible that I’m invoking the test incorrectly.

[cpython]$ TEST_INTERNAL_C_API=1 ./python -m test test.test_cppext -u cpu=1
...
      CC env var: 'gcc'
      CFLAGS env var: <missing>
      CPPFLAGS env var: <missing>
      extra_compile_args: ['-Werror', '-DMODULE_NAME=_testcpp11ext', '-std=c++11', '-pedantic-errors', '-Wno-long-long', '-DTEST_INTERNAL_C_API=1']
      running bdist_wheel
      running build
      running build_ext
      building '_testcpp11ext' extension
      creating build/temp.linux-x86_64-cpython-315t-pydebug
      g++ -fno-strict-overflow -Wsign-compare -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -g -Og -Wall -fPIC -I/home/dev/workspace/cpython/build/test_python_worker_21579æ/tempcwd/env/include -I/home/dev/workspace/cpython/Include -I/home/dev/workspace/cpython -c extension.cpp -o build/temp.linux-x86_64-cpython-315t-pydebug/extension.o -Werror -DMODULE_NAME=_testcpp11ext -std=c++11 -pedantic-errors -Wno-long-long -DTEST_INTERNAL_C_API=1
      In file included from /usr/include/features.h:524,
                       from /usr/include/assert.h:35,
                       from /home/dev/workspace/cpython/Include/Python.h:20,
                       from extension.cpp:13:
      /home/dev/workspace/cpython/Include/internal/mimalloc/mimalloc/types.h:73:33: error: expected unqualified-id before ‘noexcept’
         73 | #define mi_decl_throw           __THROW
            |                                 ^~~~~~~
      /home/dev/workspace/cpython/Include/internal/mimalloc/mimalloc/types.h:611:31: note: in expansion of macro ‘mi_decl_throw’
        611 | mi_decl_noreturn mi_decl_cold mi_decl_throw
            |                               ^~~~~~~~~~~~~
      In file included from /home/dev/workspace/cpython/Include/internal/pycore_mimalloc.h:44,
                       from /home/dev/workspace/cpython/Include/internal/pycore_tstate.h:14,
                       from /home/dev/workspace/cpython/Include/internal/pycore_interp_structs.h:15,
                       from /home/dev/workspace/cpython/Include/internal/pycore_backoff.h:15,
                       from extension.cpp:17:
      /home/dev/workspace/cpython/Include/internal/mimalloc/mimalloc/internal.h: In function ‘bool _mi_is_aligned(void*, size_t)’:
      /home/dev/workspace/cpython/Include/internal/mimalloc/mimalloc/types.h:613:49: error: ‘_mi_assert_fail’ was not declared in this scope; did you mean ‘__assert_fail’?
        613 | #define mi_assert(expr)     ((expr) ? (void)0 : _mi_assert_fail(#expr,__FILE__,__LINE__,__func__))
            |                                                 ^~~~~~~~~~~~~~~
      /home/dev/workspace/cpython/Include/internal/mimalloc/mimalloc/types.h:619:31: note: in expansion of macro ‘mi_assert’
        619 | #define mi_assert_internal    mi_assert
            |                               ^~~~~~~~~
...

Note: This is on the unmodified main branch; I did not add any includes.

vstinner commented Feb 6, 2026

Copy link
Copy Markdown
Member

@vstinner - I think BaseTests in Lib/test/test_cppext/init.py is missing handling of TEST_INTERNAL_C_API. It doesn't have the env['TEST_INTERNAL_C_API'] = str(int(self.TEST_INTERNAL_C_API)) line from test_cext

Ooops! I wrote #144536 to fix test_cppext (test the internal C API).

When I set TEST_INTERNAL_C_API=1, the test immediately emits many C++ errors

      /home/dev/workspace/cpython/Include/internal/mimalloc/mimalloc/types.h:73:33: error: expected unqualified-id before ‘noexcept’
         73 | #define mi_decl_throw           __THROW
            |                                 ^~~~~~~

Yeah, I already saw C++ errors/warnings on mimalloc. I'm not sure how to deal with it.

yoney commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Please also add an #include to test_cppext as well, like this:

Added in #144536

colesbury added the needs backport to 3.14 bugs and security fixes label Feb 6, 2026
ZeroIntensity merged commit a2495ff into python:main Feb 7, 2026
47 checks passed

Copy link
Copy Markdown

Thanks @yoney for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 7, 2026
)

(cherry picked from commit a2495ff)

Co-authored-by: Alper <alperyoney@fb.com>

bedevere-app Bot commented Feb 7, 2026

Copy link
Copy Markdown

GH-144555 is a backport of this pull request to the 3.14 branch.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Feb 7, 2026

Copy link
Copy Markdown
Member

Thanks, I trust that Victor will figure out testing.

ZeroIntensity pushed a commit that referenced this pull request Feb 7, 2026
…H-144555)

gh-144490: Fix C++ compatibility in pycore_cell.h (GH-144482)
(cherry picked from commit a2495ff)

Co-authored-by: Alper <alperyoney@fb.com>
thunder-coding pushed a commit to thunder-coding/cpython that referenced this pull request Feb 15, 2026
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL