Repository navigation
Fix serialization of redefined nodes - #7413
Merged
Merged
Conversation
Currently they will get incorrectly serialized as xrefs because the symbol table name doesn't match the fullname. Fix this. (Though I don't love the way I've fixed it, if somebody has a suggestion for something nicer.) This could cause nondeterministc crashes under python 3.5 when it led to crossrefs to crossrefs, since random dict iteration order meant that the referenced crossref might not be resolved. Add some asserts to catch these things earlier.
JukkaL
approved these changes
Aug 29, 2019
JukkaL
left a comment
JukkaL
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for fixing this! I wonder if this could happen with --allow-redefinition, which adds ' suffixes to names.
msullivan
added a commit
that referenced
this pull request
Sep 11, 2019
Redefined nodes sometimes have busted internal references (like a self argument that has the class as its type even though it won't be able to look it up) which can cause crashes. Since they can't have references from outside themselves, we don't lose anything by not serializing them. Fixes a crash I observed at Dropbox. This partially reverts #7413, which introduced this crash (while fixing another), but leaves the test case it added.
JukkaL
pushed a commit
that referenced
this pull request
Sep 12, 2019
Redefined nodes sometimes have busted internal references (like a self argument that has the class as its type even though it won't be able to look it up) which can cause crashes. Since they can't have references from outside themselves, we don't lose anything by not serializing them. Fixes a crash I observed at Dropbox. This partially reverts #7413, which introduced this crash (while fixing another), but leaves the test case it added.
JukkaL
pushed a commit
that referenced
this pull request
Sep 12, 2019
Redefined nodes sometimes have busted internal references (like a self argument that has the class as its type even though it won't be able to look it up) which can cause crashes. Since they can't have references from outside themselves, we don't lose anything by not serializing them. Fixes a crash I observed at Dropbox. This partially reverts #7413, which introduced this crash (while fixing another), but leaves the test case it added.
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
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.
Currently they will get incorrectly serialized as xrefs because
the symbol table name doesn't match the fullname.
Fix this. (Though I don't love the way I've fixed it, if somebody has
a suggestion for something nicer.)
This could cause nondeterministc crashes under python 3.5 when it led
to crossrefs to crossrefs, since random dict iteration order meant
that the referenced crossref might not be resolved. Add some asserts
to catch these things earlier.