[3.9] bpo-43710: Change layout of pystate struct to preserve intra-version ABI compatibiliy. #25160
Conversation
markshannon
changed the title
There was a problem hiding this comment.
3.9 originally had int, char, char, int. #24501 replaced it with int, int, char, int, which was an ABI break. This PR is betting that int, short, int will restore the ABI. It sounds like a reasonable bet, unless we both have sizeof(short) != 2 * sizeof(char) and sizeof(short) > alignof(int).
| @@ -69,10 +69,10 @@ PyAPI_FUNC(void) Py_LeaveRecursiveCall(void); | |||
|
|
|||
| #define Py_ALLOW_RECURSION \ | |||
| do { unsigned char _old = PyThreadState_GET()->recursion_critical;\ | |||
There was a problem hiding this comment.
recursion_critical needs fixing up here too. Are you sure you grepped the whole codebase?
There was a problem hiding this comment.
How did this even compile and pass CI?
There was a problem hiding this comment.
I suppose this macro is only used in third-party code? Which of course makes wonder whether it still works as expected.
|
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be put in the comfy chair! |
|
PR 25179 is a simple revert of the offending change. I consider that a safer choice for a rushed 3.9.4 than attempting to fix a bugfix of a non-critical issue under time pressure. Up to ambv to decide. |
|
I agree with @gpshead |
This PR replaces the original (in 3.9.0)
with
restoring the ABI broken by #24501
Since
recursion_headroom,overflowed, andrecursion_criticalall serve the same purpose, indicating that RecursionError should not be raised unless overflow become excessive, we can safely merge them.I choose to use
short recursion_headroomrather than usingchar recursion_headroomand leavingrecursion_criticalin place, so that assignments torecursion_criticalin code compiled for 3.9.0 will be visible as changes torecursion_headroom.The layout will be the same (at least for all compilers where
sizeof(short) == sizeof(char)*2).https://bugs.python.org/issue43710