msg390855 - (view) |
Author: Andrew V. Jones (andrewvaughanj) * |
Date: 2021-04-12 15:16 |
With Python 3.9.4, and when compiling with Visual Studio 2019, we have noticed that the variable `_Py_ctype_table` is *not* scoped with in an `extern "C"` block, and where the Python library (`python39.lib`) *has* been compiled with a C compiler.
This causes an issue when trying to refer to `_Py_ctype_table` from a C++ file, as the compiler tries to name-mangle the _use_ of `_Py_ctype_table`, but the linker cannot then tie the mangled name to non-mangled named from `python39.lib`.
Example:
```
#include "Python.h"
int main() { return _Py_ctype_table[0]; }
```
Compilation:
```
cl.exe /Fe:test.exe /TP /I include test.cpp /link libs/python39.lib
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29336 for x64
Copyright (C) Microsoft Corporation. All rights reserved.
test.cpp
Microsoft (R) Incremental Linker Version 14.28.29336.0
Copyright (C) Microsoft Corporation. All rights reserved.
/out:test.exe
libs/python39.lib
test.obj
test.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) unsigned int const * const _Py_ctype_table" (__imp_?_Py_ctype_table@@3QBIB) referenced in function main
test.exe : fatal error LNK1120: 1 unresolved externals
```
With `cl.exe`:
```
cl.exe /Bv
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29336 for x64
Copyright (C) Microsoft Corporation. All rights reserved.
Compiler Passes:
Z:\home\avj\visual_studio\MSVC\14.28.29333\bin\HostX64\x64\cl.exe: Version 19.28.29336.0
Z:\home\avj\visual_studio\MSVC\14.28.29333\bin\HostX64\x64\c1.dll: Version 19.28.29336.0
Z:\home\avj\visual_studio\MSVC\14.28.29333\bin\HostX64\x64\c1xx.dll: Version 19.28.29336.0
Z:\home\avj\visual_studio\MSVC\14.28.29333\bin\HostX64\x64\c2.dll: Version 19.28.29336.0
Z:\home\avj\visual_studio\MSVC\14.28.29333\bin\HostX64\x64\c1xx.dll: Version 19.28.29336.0
Z:\home\avj\visual_studio\MSVC\14.28.29333\bin\HostX64\x64\link.exe: Version 14.28.29336.0
Z:\home\avj\visual_studio\MSVC\14.28.29333\bin\HostX64\x64\mspdb140.dll: Version 14.28.29336.0
Z:\home\avj\visual_studio\MSVC\14.28.29333\bin\HostX64\x64\1033\clui.dll: Version 19.28.29336.0
```
A naïve check of Python.h (e126547c07) seems to suggest that:
* There are 82 includes
* 64 of these contain `extern "C"`
* 8 do not contain `extern "C"`
* The remaining 10 are either system includes or pyconfig.h
For the 8 that *do not* contain `extern "C"`, none of these use `PyAPI_DATA`. This leads me to believe that it is an oversight that `pyctype.h` does not have `extern "C"`
|
msg390856 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2021-04-12 15:36 |
In fact, _Py_ctype_table is limited to the internal parts of the interpreter. So in this case, this one could not be used in an external tool.
You can read: https://docs.python.org/3/c-api/stable.html
I am not sure that you correctly use the API.
|
msg390858 - (view) |
Author: Andrew V. Jones (andrewvaughanj) * |
Date: 2021-04-12 15:46 |
> In fact, _Py_ctype_table is limited to the internal parts of the interpreter. So in this case, this one could not be used in an external tool.
>
Hmm, so why is this "exposed" by the "world-facing" `Python.h` file?
I should say: we found this bug via Cython; and it was Cython that was accessing/referring to `_Py_ctype_table` -- our Cythonated code pulls in C++ headers, so we need to compile these files as C++.
I am happy to re-assign this as a Cython bug, but the fact it is fixed with an `extern "C"` in Python.h, really makes it feel like it is a Python-proper issue and not a "user" issue.
|
msg390860 - (view) |
Author: Andrew V. Jones (andrewvaughanj) * |
Date: 2021-04-12 15:55 |
> I am happy to re-assign this as a Cython bug, but the fact it is fixed with an `extern "C"` in Python.h, really makes it feel like it is a Python-proper issue and not a "user" issue.
>
Just to extend on this:
1) The Cython-generated code uses `Py_ISSPACE` (and not `_Py_ctype_table`), but the expansion of the macro `Py_ISSPACE` then adds `_Py_ctype_table` to the user's code
2) The "user-fix" is to wrap `#include <Python.h>` in `extern "C"` -- however, given other parts of Python.h already do this, it seems extraneous to expect a C++ user to wrap Python.h in this way
|
msg390868 - (view) |
Author: Andrew V. Jones (andrewvaughanj) * |
Date: 2021-04-12 16:55 |
> 1) The Cython-generated code uses `Py_ISSPACE` (and not `_Py_ctype_table`), but the expansion of the macro `Py_ISSPACE` then adds `_Py_ctype_table` to the user's code
>
I wrote this up as a Cython bug here (just to see if the Cython team consider this "their" bug): https://github.com/cython/cython/issues/4111
|
msg390890 - (view) |
Author: Stefan Behnel (scoder) * |
Date: 2021-04-12 19:22 |
These macros are a sort-of documented part of the C-API. At least, they were mentioned in a What's New document:
https://docs.python.org/3/whatsnew/2.7.html?highlight=py_isspace#build-and-c-api-changes
They were added here, for the Py2.7 release:
https://github.com/python/cpython/commit/8374981fb4d781d5ddbca313656bd3f32b49cef4
It looks to me like the header file should use "extern C".
@Eric, do you agree?
|
msg390896 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2021-04-12 20:14 |
These files were probably added as part of str.format() or short float repr. I think the fact that they've been moved to Include/cpython means that user code shouldn't be using them. See https://bugs.python.org/issue35134 and https://vstinner.github.io/split-include-directory-python38.html. Probably best to ask Victor.
|
msg390900 - (view) |
Author: Andrew V. Jones (andrewvaughanj) * |
Date: 2021-04-12 20:27 |
> I think the fact that they've been moved to Include/cpython means that user
> code shouldn't be using them.
>
I think it is fine to say that they shouldn't be used, but then we get this from Victor's blog:
> It was decided that internal header files must not be included implicitly by
> the generic #include <Python.h>, but included explicitly.
>
So, is it the case that we have two issues here:
1) Cython is using stuff it shouldn't (I can do a PR against Cython)
2) Python.h is exposing more than it should (so, if Python "core" wants something from pyctype.h, it should be explicitly including pyctype.h and not getting via Python.h)
?
|
msg390910 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2021-04-12 21:49 |
See https://github.com/python/cpython/blob/master/Include/README.rst for the organization of the C API.
|
msg390949 - (view) |
Author: Stefan Behnel (scoder) * |
Date: 2021-04-13 10:00 |
The macros were moved to "Includes/cpython/", not to "Includes/internal/". That suggests to me that they should use "extern C", so that C++ code that wants to make use of CPython internals can use them.
Basically, the way I see it, *all* header files in "Includes/" and "Includes/cpython/" should work with C++ code and thus have an "extern C". Only the header files in "Includes/internal/" should not have them.
Regardless, I've removed the macro usage from Cython so that the current state doesn't hurt our users.
|
msg390954 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2021-04-13 11:27 |
New changeset 54db51c9114ac49030832f5134979ca866ffd21c by Andrew V. Jones in branch 'master':
bpo-43816: Add extern "C" to Include/cpython/pyctype.h (GH-25365)
https://github.com/python/cpython/commit/54db51c9114ac49030832f5134979ca866ffd21c
|
msg390957 - (view) |
Author: miss-islington (miss-islington) |
Date: 2021-04-13 11:45 |
New changeset 47a894d338f436ea8f513f1ad2cc7e1b086e99cc by Miss Islington (bot) in branch '3.8':
bpo-43816: Add extern "C" to Include/cpython/pyctype.h (GH-25365)
https://github.com/python/cpython/commit/47a894d338f436ea8f513f1ad2cc7e1b086e99cc
|
msg390958 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2021-04-13 12:20 |
New changeset 15ad30d88fead718b7eeff8c54454b82753d520e by Miss Islington (bot) in branch '3.9':
bpo-43816: Add extern "C" to Include/cpython/pyctype.h (GH-25365) (GH-25387)
https://github.com/python/cpython/commit/15ad30d88fead718b7eeff8c54454b82753d520e
|
msg390959 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2021-04-13 12:21 |
It's now fixed. Thanks Andrew V. Jones for the fix.
|
msg390960 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2021-04-13 12:22 |
The workaround for this issue is to put extern "C" around #include <Python.h>, or around the header file which includes <Python.h>.
|
msg390963 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2021-04-13 13:31 |
Stéphane Wirtel: "In fact, _Py_ctype_table is limited to the internal parts of the interpreter. So in this case, this one could not be used in an external tool. You can read: https://docs.python.org/3/c-api/stable.html"
You're right that pyctype.h is excluded from the limited C API. But almost all C extensions use the "regular" C API which includes header files in Include/cpython/.
The internal C API is something different: header files in Include/internal/.
I know that it's complicated, that why a README was written :-)
https://github.com/python/cpython/blob/master/Include/README.rst
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:44 | admin | set | github: 87982 |
2021-04-13 13:31:26 | vstinner | set | messages:
+ msg390963 |
2021-04-13 12:22:57 | vstinner | set | messages:
+ msg390960 |
2021-04-13 12:21:07 | vstinner | set | status: open -> closed versions:
+ Python 3.8 messages:
+ msg390959
resolution: fixed stage: patch review -> resolved |
2021-04-13 12:20:21 | vstinner | set | messages:
+ msg390958 |
2021-04-13 11:45:32 | miss-islington | set | messages:
+ msg390957 |
2021-04-13 11:28:51 | miss-islington | set | pull_requests:
+ pull_request24120 |
2021-04-13 11:28:20 | miss-islington | set | nosy:
+ miss-islington pull_requests:
+ pull_request24119
|
2021-04-13 11:27:31 | vstinner | set | messages:
+ msg390954 |
2021-04-13 10:00:00 | scoder | set | messages:
+ msg390949 |
2021-04-12 21:49:29 | vstinner | set | messages:
+ msg390910 |
2021-04-12 20:27:49 | andrewvaughanj | set | messages:
+ msg390900 |
2021-04-12 20:14:14 | eric.smith | set | nosy:
+ vstinner messages:
+ msg390896
|
2021-04-12 19:22:56 | scoder | set | versions:
+ Python 3.10 nosy:
+ scoder, eric.smith
messages:
+ msg390890
type: compile error |
2021-04-12 16:55:56 | andrewvaughanj | set | messages:
+ msg390868 |
2021-04-12 16:06:54 | xtreak | set | components:
+ C API |
2021-04-12 15:55:57 | andrewvaughanj | set | messages:
+ msg390860 |
2021-04-12 15:46:55 | andrewvaughanj | set | messages:
+ msg390858 |
2021-04-12 15:36:32 | matrixise | set | nosy:
+ matrixise messages:
+ msg390856
|
2021-04-12 15:19:01 | andrewvaughanj | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request24099 |
2021-04-12 15:16:23 | andrewvaughanj | create | |