[ClangImporter] Break cycle in IAM extensions #71027
Merged
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
Under a set of circumstances that I cannot fully characterize, it was possible for `ClangImporter::Implementation::importDeclContextOf()` to end up recursing until stack overflow while trying to import a global as a member. The cycle was caused by `addExtension()` using a newly-created extension’s lazy member loader to load its members before the extension had been added to ClangImporter’s `extensionPoints` table. Loading the members would try to import the IAM members again, which would cause `importDeclContextOf()` to try to create another extension to contain them, which `addExtension()` would force to load those members. Repeat until stack overflow. I have fixed this bug by registering the extension in the `extensionPoints` map before its member loader is set. This ensures that if `importDeclContextOf()` gets called for any reason after that point, it will return the existing extension instead of creating a new one, avoiding the cycle. Unfortunately, I was not able to figure out the exact conditions that were necessary to reproduce this bug, so I have not included a test. However, I have tested with the affected project locally and confirmed that this change lets it build. All existing tests pass. Fixes rdar://120854030.
|
@swift-ci please test |
nkcsgexi
approved these changes
Jan 22, 2024
DougGregor
reviewed
Jan 22, 2024
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.
Under a set of circumstances that I cannot fully characterize, it was possible for
ClangImporter::Implementation::importDeclContextOf()to end up recursing until stack overflow while trying to import a global as a member.The cycle was caused by
addExtension()using a newly-created extension’s lazy member loader to load its members before the extension had been added to ClangImporter’sextensionPointstable. Loading the members would try to import the IAM members again, which would causeimportDeclContextOf()to try to create another extension to contain them, whichaddExtension()would force to load those members. Repeat until stack overflow.I have fixed this bug by registering the extension in the
extensionPointsmap before its member loader is set. This ensures that ifimportDeclContextOf()gets called for any reason after that point, it will return the existing extension instead of creating a new one, avoiding the cycle.Unfortunately, I was not able to figure out the exact conditions that were necessary to reproduce this bug, so I have not included a test. However, I have tested with the affected project locally and confirmed that this change lets it build. All existing tests pass.
(In fact, there appears to be a hole in the test suite around the entire code path for lazily loading the members of these extensions—I can deliberately break it and all tests will still pass. But I haven't yet been able to write a new test that exercises it, and the affected project has been waiting very patiently for their code to start building again.)
Fixes rdar://120854030.