{{ message }}
bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types - #19414
Merged
Merged
Conversation
pablogsal
changed the title
pablogsal
force-pushed
the
bpo-40217
branch
4 times, most recently
from
April 7, 2020 20:25
ea96f0b to
88f807c
Compare
Member
Author
tim-one
reviewed
Apr 7, 2020
pablogsal
marked this pull request as ready for review
April 7, 2020 21:27
pablogsal
force-pushed
the
bpo-40217
branch
4 times, most recently
from
April 7, 2020 21:58
353f682 to
ab1ca32
Compare
pablogsal
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit ab1ca32 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
vstinner
approved these changes
Apr 23, 2020
vstinner
left a comment
vstinner
left a comment
Member
There was a problem hiding this comment.
LGTM, but I have a few minor remarks.
| if (obj == NULL) | ||
| return PyErr_NoMemory(); | ||
|
|
||
| obj = obj; |
| /* note that we need to add one, for the sentinel and space for the | ||
| provided tp-traverse: See bpo-40217 for more details */ | ||
|
|
||
| if (PyType_IS_GC(type)) |
Member
There was a problem hiding this comment.
PEP 7: please add { ... }. Same remark for other if.
| } | ||
| else if (slot->slot == Py_tp_traverse) { | ||
|
|
||
| /* Types created by PyType_FromSpec own a strong reference to their |
Member
|
@pablogsal: I added "skip news" to be able to merge your PR, but IMHO it's worth it to add a NEWS entry for it (I suggest the C API category). Would you mind to add one? |
Member
Author
Will do but I didn't had time to add your suggestions for this PR before you merged it. Should I add this in the future PR? |
Member
pablogsal
added a commit
to pablogsal/cpython
that referenced
this pull request
May 23, 2020
…r PyType_FromSpec types (pythonGH-19414)" This reverts commit 0169d30.
miss-islington
pushed a commit
that referenced
this pull request
May 27, 2020
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
https://bugs.python.org/issue40217
Types created by PyType_FromSpec own a strong reference to their type, but this was added in Python 3.8. The tp_traverse function needs to call Py_VISIT on the type but all existing traverse functions cannot be updated (especially the ones from existing user functions) so we need to provide a tp_traverse that manually calls Py_VISIT(Py_TYPE(self)) and then call the provided tp_traverse. In this way, user functions do not need to be updated, preserve backwards compatibility.
To do this, we store the user-provided traverse function at the end of the type (we need to allocate space for it) so we can call it from our PyType_FromSpec_tp_traverse wrapper.