Skip to content

Serialize whether VarDecls are top-level #29489

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

Closed
wants to merge 9 commits into from

Conversation

adrian-prantl
Copy link
Contributor

The current way that VarDecl::isLazilyInitializedGlobal() is implemented does
not work in the debugger, since the DeclContext of all VarDecls are deserialized
Swift modules. By adding a bit to the VarDecl we can recover the fact that a
VarDecl was in fact a global even in the debugger.

rdar://problem/58939370

@adrian-prantl
Copy link
Contributor Author

Any pointers for how to test this would be appreciated.

@adrian-prantl adrian-prantl requested a review from xedin January 28, 2020 00:34
@xedin
Copy link
Contributor

xedin commented Jan 28, 2020

I can pull this change into my branch and run full suite test to see whether it fixes the problem.

@xedin
Copy link
Contributor

xedin commented Jan 28, 2020

@adrian-prantl I have amed your commit to #29024 and started full test.

@@ -69,7 +69,8 @@ ManagedValue
SILGenFunction::emitGlobalVariableRef(SILLocation loc, VarDecl *var) {
assert(!VarLocs.count(var));

if (var->isLazilyInitializedGlobal()) {
// In the debugger, no variables are lazy initialized.
if (!SGM.SwiftModule->getDebugClient() && var->isLazilyInitializedGlobal()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I accidentally left this is.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, I can re-apply and test :)

@adrian-prantl adrian-prantl force-pushed the 58939370 branch 2 times, most recently from adf05b5 to af3ffb1 Compare January 28, 2020 01:36
@xedin
Copy link
Contributor

xedin commented Jan 28, 2020

I have re-applied the changes and restarted testing.

@adrian-prantl
Copy link
Contributor Author

test with swiftlang/llvm-project#674
@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 35370b54a200807d04b4277e54520cee7f4f4700

@adrian-prantl
Copy link
Contributor Author

@xedin You'll also need swiftlang/llvm-project#674 for the LLDB tests to succeed. This PR tests whether it also works without your PR. If yes I will land this, cherry-pick both to master-rebranch and master-next and then you should be able to merge your PR.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 35370b54a200807d04b4277e54520cee7f4f4700

@xedin
Copy link
Contributor

xedin commented Jan 28, 2020

Sounds good! Going to keep an eye on this PR.

@adrian-prantl
Copy link
Contributor Author

test with swiftlang/llvm-project#674
@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - af3ffb167077b83275dc8a3d00f6797b08921771

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - af3ffb167077b83275dc8a3d00f6797b08921771

xedin added 2 commits January 28, 2020 12:31
Diagnose an attempt to reference a top-level name shadowed by
a local member e.g.

```swift
extension Sequence {
  func test() -> Int {
    return max(1, 2)
  }
}
```

Here `min` refers to a global function `min<T>(_: T, _: T)` in `Swift`
module and can only be accessed by adding `Swift.` to it, because `Sequence`
has a member named `min` which accepts a single argument.
@adrian-prantl
Copy link
Contributor Author

test with swiftlang/llvm-project#674
@swift-ci smoke test

xedin and others added 5 commits January 28, 2020 12:32
Stop filtering outer overload choices while trying to pre-check
expression, instead have it always fetch those and use new
fix to only attempt them in diagnostic mode (unless it's min/max
situation with conditional conformances).
Although such functionality is not yet supported we have to
mirror AST lookup and add such members into results, otherwise
there is a risk of inner/outer results mismatch.
@adrian-prantl
Copy link
Contributor Author

test with swiftlang/llvm-project#674
@swift-ci smoke test

The current way that VarDecl::isLazilyInitializedGlobal() is implemented does
not work in the debugger, since the DeclContext of all VarDecls are deserialized
Swift modules. By adding a bit to the VarDecl we can recover the fact that a
VarDecl was in fact a global even in the debugger.

<rdar://problem/58939370>
This is in preparation to a change in serialization of global variables where
this detail will matter.
@adrian-prantl
Copy link
Contributor Author

test with swiftlang/llvm-project#674
@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - b6871acdfb57e975b271dae7a935b1ea9f601c9b

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - b6871acdfb57e975b271dae7a935b1ea9f601c9b

@adrian-prantl
Copy link
Contributor Author

test with swiftlang/llvm-project#674
@swift-ci test

Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

This looks good, but it appears you've cherry-picked @xedin's changes into this PR too. Can we land the two sets of changes separately?

@@ -5410,11 +5411,7 @@ bool VarDecl::isLazilyInitializedGlobal() const {

// Top-level global variables in the main source file and in the REPL are not
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the above checks (hasClangNode() and isDebuggerVar()) needed at all then? The bit should not be set in those cases either.

@xedin
Copy link
Contributor

xedin commented Jan 30, 2020

@slavapestov We actually going to do it vice versa - I have cherry-picked Adrian's changes to my PR to make sure everything works correctly, currently waiting on full performance suite to complete and we are going to land my PR together with llvm side.

@slavapestov
Copy link
Contributor

@xedin Ok, sure. In that case, can you simplify VarDecl::isLazilyInitializedGlobal() in your PR then?

@xedin
Copy link
Contributor

xedin commented Jan 30, 2020

@slavapestov Sure! I'm going to let full test finish and commit the simplification to my PR.

@adrian-prantl
Copy link
Contributor Author

Abandoning this in favor of @xedin's all-in-one PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants