Re-implement DebuggerContextChange using a new mechanism #33795
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
Expression evaluation in lldb wraps the entire user-written expression in a new function body, which puts any new declarations written by the user in local context. There is a mechanism where declarations can get moved to the top level, if they're only valid at the top level (imports, extensions etc), or if the name of the declaration begins with '$'. This mechanism used to actually add the declaration to the SourceFile's TopLevelDecls list, which would break ASTScope invariants about source ranges being monotonically increasing and non-overlapping. Instead, we use the new 'hoisted' flag to mark the declarations as hoisted, which leaves them syntactically in their original location in the AST, but treats them as top level in SILGen and IRGen. Part of <rdar://problem/53971116>.
|
@swift-ci Please smoke test |
DougGregor
approved these changes
Sep 3, 2020
davidungar
suggested changes
Sep 3, 2020
Comment on lines
+306
to
+309
| /// Whether this declaration is syntactically scoped inside of | ||
| /// a local context, but should behave like a top-level | ||
| /// declaration for name lookup purposes. This is used by | ||
| /// lldb. |
Comment on lines
+108
to
+110
| D->setHoisted(); | ||
| SF->addHoistedDecl(D); | ||
| getDebuggerClient()->didGlobalize(D); |
There was a problem hiding this comment.
There is an invariant here, I'm guessing that these three always go together. If so, how about factoring this out, and lines 118-121 into a separate function? I'm guessing you've also added assertions for the invariant.
Comment on lines
+224
to
+228
| for (auto *D : SF->getTopLevelDecls()) | ||
| visitTopLevelDecl(D); | ||
|
|
||
| for (auto *D : SF->getHoistedDecls()) | ||
| visitTopLevelDecl(D); |
There was a problem hiding this comment.
This might be the place for the consistency-checking assertion.
|
I discussed the suggested improvements with @davidungar offline. We both agree they’re good but I’m going to apply them to a follow-up PR since I have more cleanups in this area and I don’t want to run PR tests again. |
|
Works for me! Thanks, @Slava -- this is great stuff! |
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.
Expression evaluation in lldb wraps the entire user-written expression
in a new function body, which puts any new declarations written by the
user in local context.
There is a mechanism where declarations can get moved to the top level,
if they're only valid at the top level (imports, extensions etc), or
if the name of the declaration begins with '$'. This mechanism used to
actually add the declaration to the SourceFile's TopLevelDecls list,
which would break ASTScope invariants about source ranges being
monotonically increasing and non-overlapping.
Instead, we use the new 'hoisted' flag to mark the declarations as
hoisted, which leaves them syntactically in their original location
in the AST, but treats them as top level in SILGen and IRGen.
Part of rdar://problem/53971116.