| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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++? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
Ok, make sense. Can you open an issue like @picnixz asked? Also edit the PR title to associate it with the issue. |
Sorry, something went wrong.
|
Created the issue #144490 @picnixz, @colesbury: Thank you so much for the review. |
Sorry, something went wrong.
There was a problem hiding this comment.
Please also add an #include to test_cppext as well, like this:
cpython/Lib/test/test_cppext/extension.cpp
Lines 15 to 19 in b6d8aa4
That'll make sure we don't break this again someday.
Sorry, something went wrong.
| PyObject *value; | ||
| #ifdef Py_GIL_DISABLED | ||
| value = _Py_atomic_load_ptr(&cell->ob_ref); | ||
| value = (PyObject *)_Py_atomic_load_ptr(&cell->ob_ref); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Most other headers use _PyObject_CAST so I think we should use that instead.
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you. I grepped for _Py_STATIC_CAST and _PyObject_CAST. There were more usages of _PyObject_CAST, so I changed it to that.
Sorry, something went wrong.
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
|
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
@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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Ooops! I wrote #144536 to fix test_cppext (test the internal C API).
/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. |
Sorry, something went wrong.
Added in #144536 |
Sorry, something went wrong.
|
Thanks @yoney for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
Sorry, something went wrong.
) (cherry picked from commit a2495ff) Co-authored-by: Alper <alperyoney@fb.com>
|
GH-144555 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
Thanks, I trust that Victor will figure out testing. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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