gh-94673: More Per-Interpreter Fields for Builtin Static Types #103912
Conversation
This file contains 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
ericsnowcurrently
force-pushed
the
per-interpreter-static-types-fields
branch
3 times, most recently
from
48354b9 to
7ccc2e4
Compare
ericsnowcurrently
force-pushed
the
per-interpreter-static-types-fields
branch
from
7ccc2e4 to
9937406
Compare
ericsnowcurrently
force-pushed
the
per-interpreter-static-types-fields
branch
from
9937406 to
071ef3f
Compare
ericsnowcurrently
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 071ef3f 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
ericsnowcurrently
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit cd1dd10 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
ericsnowcurrently
force-pushed
the
per-interpreter-static-types-fields
branch
from
3c4000c to
2771f4e
Compare
|
git bisect got me here. I'm testing Python 3.12 with pybind11 (master @ https://github.com/pybind/pybind11/tree/d72ffb448c58b4ffb08b5ad629bc788646e2d59e). I double-checked that this is true: The gdb backtrace is below.
Does this ring any bells? Is there something obvious that we need to do differently in pybind11? Steps to reproduce are involved, roughly:
I can send more details or try to reduce as needed. Please let me know. |
|
You are deferencing a NULL pointer, in pybind11 code. Is there some field that is expected to be non-NULL and is now NULL? |
I didn't mean to suggest it's a CPython bug. All we know at the moment is that the pybind11 unit tests do not load anymore after this PR was merged. I didn't want to dive in (possibly spending a significant amount of time) without asking here first, in case it's something obvious to you. I'll look closer to get a better understanding. |
|
Attached is a much smaller reproducer and shorter gdb backtrace, JIC this rings any bells. The crash is in the context of pybind11 multiple inheritance code, which hasn't changed in any significant way for years, although there was some back and forth around the time 3.11 was released. I still have to dig up references.
|
Found it: https://github.com/pybind/pybind11/pull/4142/files (NOT merged) I just applied that change locally, but it doesn't make a difference: still exit 0 with last good, segfault with first bad. If anybody has conclusive advice regarding |
|
The pybind11 patch below fixes the segfault. All pybind11 unit tests pass (excluding those depending on numpy). From what I saw while debugging, it appears |
NULL indicates the correct value is stored elsewhere. Currently, there are only private APIs that can get access to these values. Namely, we need to make |
That's beginning to look like a major API change. Is |
After this PR, yes, for builtin static types. Look at the PR title. |
On one end of the spectrum: Will we be OK with something like my patch under #103912 (comment) (because we don't need the bases of "Builtin Static Types")? On the other end of the spectrum: Will we have to make deeper changes in in the binding tools to handle interpreter-specific fields? |
|
|
|
I remember a discussion on changing |
This was referenced May 30, 2023
Merged
ericsnowcurrently
added a commit
that referenced
this pull request
May 31, 2023
…tic Builtin Types (gh-105115) In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process.
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
May 31, 2023
…ll Static Builtin Types (pythongh-105115) In pythongh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process. (cherry picked from commit 7be667d) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently
pushed a commit
that referenced
this pull request
Jun 1, 2023
…All Static Builtin Types (gh-105115) (gh-105124) In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process. (cherry picked from commit 7be667d) Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
ericsnowcurrently
added a commit
that referenced
this pull request
Jun 1, 2023
gh-105122) When I added the relevant condition to type_ready_set_bases() in gh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types. We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific.
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Jun 1, 2023
…Ready() (pythongh-105122) When I added the relevant condition to type_ready_set_bases() in pythongh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types. We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific. (cherry picked from commit 1469393) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently
pushed a commit
that referenced
this pull request
Jun 1, 2023
…_Ready() (gh-105122) (gh-105211) When I added the relevant condition to type_ready_set_bases() in gh-103912, I had missed that the function also sets tp_base and ob_type (if necessary). That led to problems for third-party static types. We fix that here, by making those extra operations distinct and by adjusting the condition to be more specific. (cherry picked from commit 1469393) Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This involves moving
tp_dict,tp_bases, andtp_mrotoPyInterpreterState, in the same way we did fortp_subclasses. Those three fields are effectively const for builtin static types (unliketp_subclasses). In theory we only need to make their values immortal, along with their contents. However, that isn't such a simple proposition. (See gh-103823.) In the meantime the simplest solution is to move the fields into the interpreter.One alternative is to statically allocate the values, but that's its own can of worms.