Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-94673: More Per-Interpreter Fields for Builtin Static Types#103912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
ericsnowcurrently merged 13 commits intopython:mainfromericsnowcurrently:per-interpreter-static-types-fieldsMay 3, 2023
Merged
gh-94673: More Per-Interpreter Fields for Builtin Static Types#103912
ericsnowcurrently merged 13 commits intopython:mainfromericsnowcurrently:per-interpreter-static-types-fieldsMay 3, 2023
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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
48354b9 to7ccc2e4Compare7ccc2e4 to9937406Compare9937406 to071ef3fComparebedevere-bot commentedMay 2, 2023
🤖 New build scheduled with the buildbot fleet by@ericsnowcurrently for commit071ef3f 🤖 If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
bedevere-bot commentedMay 2, 2023
🤖 New build scheduled with the buildbot fleet by@ericsnowcurrently for commitcd1dd10 🤖 If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again. |
3c4000c to2771f4eComparerwgk commentedMay 20, 2023
git bisect got me here. I'm testing Python 3.12 withpybind11 (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. |
Member
markshannon commentedMay 22, 2023
You are deferencing a NULL pointer, in pybind11 code. Is there some field that is expected to be non-NULL and is now NULL? |
rwgk commentedMay 22, 2023
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. |
rwgk commentedMay 23, 2023
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.
|
rwgk commentedMay 23, 2023
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 |
rwgk commentedMay 23, 2023
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 |
Member
sunmy2019 commentedMay 27, 2023
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 |
rwgk commentedMay 27, 2023
That's beginning to look like a major API change. Is |
Member
sunmy2019 commentedMay 27, 2023
After this PR, yes, for builtin static types. Look at the PR title. |
rwgk commentedMay 27, 2023
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? |
Member
encukou commentedMay 30, 2023
|
Member
encukou commentedMay 30, 2023
I remember a discussion on changing |
This was referencedMay 30, 2023
Merged
ericsnowcurrently added a commit that referenced this pull requestMay 31, 2023
…tic Builtin Types (gh-105115)Ingh-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 requestMay 31, 2023
…ll Static Builtin Types (pythongh-105115)Inpythongh-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 commit7be667d)Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently pushed a commit that referenced this pull requestJun 1, 2023
…All Static Builtin Types (gh-105115) (gh-105124)Ingh-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 commit7be667d)Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
ericsnowcurrently added a commit that referenced this pull requestJun 1, 2023
gh-105122)When I added the relevant condition to type_ready_set_bases() ingh-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 requestJun 1, 2023
…Ready() (pythongh-105122)When I added the relevant condition to type_ready_set_bases() inpythongh-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 commit1469393)Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently pushed a commit that referenced this pull requestJun 1, 2023
…_Ready() (gh-105122) (gh-105211)When I added the relevant condition to type_ready_set_bases() ingh-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 commit1469393)Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
Sign up for freeto 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.
Uh oh!
There was an error while loading.Please reload this page.
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. (Seegh-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.