Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix serialization of redefined nodes #7413

Merged
merged 2 commits into from Aug 29, 2019

Conversation

@msullivan
Copy link
Collaborator

commented Aug 29, 2019

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.

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.
@msullivan msullivan requested review from JukkaL and ilevkivskyi Aug 29, 2019
@JukkaL
JukkaL approved these changes Aug 29, 2019
Copy link
Collaborator

left a comment

Thanks for fixing this! I wonder if this could happen with --allow-redefinition, which adds ' suffixes to names.

@msullivan msullivan merged commit 811c201 into master Aug 29, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@msullivan msullivan deleted the class-redef-crash branch Aug 29, 2019
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 added 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 added 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.