Skip to content

[4.0] Serialization: Create a deserialized EnumDecl before deserializing its dependency types. #9875

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 23, 2017
Merged

[4.0] Serialization: Create a deserialized EnumDecl before deserializing its dependency types. #9875

merged 1 commit into from
May 23, 2017

Conversation

@jckarter
Copy link
Contributor

@jckarter jckarter commented May 23, 2017

Explanation: Breaks a circularity that can occur when two enums recur through each other.

Scope: Deserialization crash when two enums reference each other from different separately-compiled files that get merged together into one module.

Issue: rdar://problem/32337278

Risk: Low, small bug fix

Testing: Swift CI, project from Radar, source compatibility suite

@jckarter
Copy link
Contributor Author

jckarter commented May 23, 2017

@swift-ci Please test

@jckarter
Copy link
Contributor Author

jckarter commented May 23, 2017

@swift-ci Please test source compatibility

@jckarter jckarter requested a review from jrose-apple May 23, 2017 18:18
@jckarter
Copy link
Contributor Author

jckarter commented May 23, 2017

@jrose-apple Does this look OK for the 4.0 branch?

jrose-apple
jrose-apple suggested changes May 23, 2017
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is exactly the situation we're trying to prevent. Once a type has been stored in declOrOffset, it might be referenced by other parts of the code, which would be a problem if it turns out to have an error in it.

@jckarter
Copy link
Contributor Author

jckarter commented May 23, 2017

How can we avoid this problem, then?

@jrose-apple
Copy link
Contributor

jrose-apple commented May 23, 2017

Hm. Since merge-modules happens on swiftmodules that we just built (or that didn't need rebuilding), we could just skip dependency checking altogether in that phase. We won't have cycles in any other situations.

@swift-ci
Copy link
Contributor

swift-ci commented May 23, 2017

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - b0dbf54e791b54e72874cecfcdea63cb06505e49
Test requested by - @jckarter

@swift-ci
Copy link
Contributor

swift-ci commented May 23, 2017

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - b0dbf54e791b54e72874cecfcdea63cb06505e49
Test requested by - @jckarter

@jckarter
Serialization: Exclude types from the same module as dependencies of …
a7bd12d 
…an enum.

There can be a circularity when two enums recur through each other, and deserialization currently is not set up to robustly detect and avoid these circularities. This should avoid regressions, but re-exposes some possible cases that should require recovery in mix-and-match situations. Short-term fix for rdar://problem/32337278.
@jckarter jckarter force-pushed the enum-deserialization-circularity-4.0 branch from b0dbf54 to a7bd12d Compare May 23, 2017 20:26
@jckarter jckarter changed the title Serialization: Create a deserialized EnumDecl before deserializing its dependency types. [4.0] Serialization: Create a deserialized EnumDecl before deserializing its dependency types. May 23, 2017
@jckarter
Copy link
Contributor Author

jckarter commented May 23, 2017

@swift-ci Please test

@jckarter
Copy link
Contributor Author

jckarter commented May 23, 2017

@swift-ci Please test source compatibility

jrose-apple
jrose-apple approved these changes May 23, 2017
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New version looks good for 4.0!

@jckarter
Copy link
Contributor Author

jckarter commented May 23, 2017

@swift-ci Please test

@jckarter
Copy link
Contributor Author

jckarter commented May 23, 2017

@swift-ci Please test source compatibility

@tkremenek tkremenek merged commit 3ea9a3b into swiftlang:swift-4.0-branch May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jrose-apple jrose-apple jrose-apple approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jckarter @jrose-apple @swift-ci @tkremenek