-
Notifications
You must be signed in to change notification settings - Fork 341
[lldb] Implement diagnostics detecting when a variable is not captured #10772
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
base: swift/release/6.2
Are you sure you want to change the base?
[lldb] Implement diagnostics detecting when a variable is not captured #10772
Conversation
…m#142052) This PR adds the missing move operators for VariableList: this class is just a wrapper around a vector, so it can use the default move operations. Subsequent patches will want to return VariableLists from functions, so the move operation is required (the copy constructors are deleted). It also fixes constness for a method in DiagnosticManager returning its list of diagnostics. Previously, the method always returned a const list, even though the method was not const itself. Subsequent patches will make use of the ability to mutate the diagnostics. (cherry picked from commit 03d1f3d)
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be very helpful!
"was not captured.\nHint: the variable may be available " | ||
"in a parent frame.", | ||
var_name, parent_func_name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this a separate function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thinking is that:
- I expect us to one day unify this with the
v
case - To separate the logic of "when to emit the diagnostics" from the logic of "how to write the diagnostics". Anyone debugging this in the future will likely want to debug one or the other.
using Node = swift::Demangle::Node; | ||
|
||
/// Returns the first child of `node` whose kind is in `kinds`. | ||
NodePointer getFirstChildOfKind(NodePointer node, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to SwiftDemangle.h
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of that file!
I've moved them there, but beware that that header has no corresponding cpp file, so we should not let it grow too wildly.
lang_plugin->FindParentOfClosureWithVariable(missing_var_name, sc); | ||
if (!parent_func) | ||
return ""; | ||
return llvm::formatv("Current frame is a closure.\nA variable named '{0}' " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the text duplicated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second commit implements the v diagnostics in a language agnostic way. It is what should be upstreamed to LLVM once we can support this in C++. I punted on doing this because C++ mangling is a bit to work with.
I see.
d57be96
to
ed8ae16
Compare
@swift-ci test |
Addressed review comments, fixed linux test issue by specifying an extra regex to match against, fixed windows testing issue by adding the |
ed8ae16
to
d864c20
Compare
I've just realized why Linux was failing: I had added a test looking for a variable in an async function, which is not supposed to be working on linux right now. |
@swift-ci test |
@swift-ci test macos platform |
1 similar comment
@swift-ci test macos platform |
CI is being troublesome right now, the macOS job is timing out |
|
||
llvm::SmallVector<Function *> parents; | ||
Function *root = sc.function; | ||
if (root == nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (root == nullptr) | |
if (!root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why do you prefer this version? Let's say that that you encounter the two versions of this line out in the wild. One of them tells you root
is implicitly convertible to bool, the other one tells you root
is a pointer. Do you believe the former is better?
@swift-ci test macos platform |
test.assertIn(expected_error, error) | ||
|
||
test.expect(f"frame variable {var_name}", substrs=[expected_error], error=True) | ||
test.expect(f"frame variable {var_name} + 1", substrs=[expected_error], error=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frame variable
doesn't support expressions. The expression would be interpreted as a request to print 3 variables (var_name
, +
, and 1
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Updated
return llvm::formatv("Current frame is a closure.\nA variable named '{0}' " | ||
"exists in function '{1}', but it " | ||
"was not captured.\nHint: the variable may be available " | ||
"in a parent frame.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the hint contradictory (specifically the phrase "may be available")? The message says "A variable named <v> exists in function <f>" – doesn't that mean a variable is guaranteed to be available in a parent frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the function where a closure is declared is not necessarily a parent frame of the frame where the closure was called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see my misunderstanding, the diagnostic doesn't attempt check the stack for the function which has the found variable, which I assumed it would. I had pictured this as a runtime check, but it's more of a static check?
This ties back to my other question #10772 (comment): In cases where the found function is on the stack (ex: call chains into a non-escaping closures in Swift), then should lldb print the variable using the ancestor frame where it exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is impossible to do this:
func make_closure(arg: Int) {
if (arg < 0} return { /*empty closure */}
closure = make_closure(arg - 1);
closure();
return {
print ("hello from closure")
}
}
Let's say we are stopped inside the print line and we try to do a expr arg
. There might be N
frames, all with the same name make_closure
. Which one has the correct arg
variable?
Even if we somehow found out the correct frame, this would require a lot of changes in the debugger, as the current code assumes expressions are running with variables all in the same frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's known that a variable exists in an ancestor frame
This is a false premise: it is not known that the variable exists in an ancestor frame. Only that it exists in an ancestor scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The misunderstanding I've had might not be unique to me. What do you think of:
A variable named '{0}' existed where the current closure was defined (function '{1}'), but it was not captured. If '{1}' is in the call stack, select that frame and try again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think either version works, but IIRC @adrian-prantl had some thoughts on being more verbose?
Since it's known that a variable exists in an ancestor frame, was it discussed to use that variable? In other words, should lldb print the variable for the user, and if not, why not? |
d864c20
to
dcd9331
Compare
@swift-ci test |
Do we have a way of determining which block the closure was captured from? You might see:
Then we'll find bar and say "bar was in the parent frame but wasn't captured." would be confusing... If you knew the defining block, you could crawl outward from there so you only say it is in the parent frame if it really was capturable. |
I thought about that, but I'm not sure we have a way of doing this. We really only have the mangled names to go by... We could potentially find the line where the variable was defined and compare this to the line where the closure is defined? |
If you have the defining source line for the closure, then you should be able to find the block that contains that line in the parent function. That would also be more efficient since you wouldn't be running through all the blocks in the function, just the ones visible to the defining block. |
We have a
Here, it would have been useful if the SymbolContextScope hierarchy mirrored the dwarf hierarchy, but AFAICT that is lost in translation. We can see that here: Did you have something else in mind? |
Would a line number comparison suffice / approximate this well enough? |
…vars in "frame var" This commit adds the language-agnostic logic to support diagnosing when a `frame var` command failed because a variable is not visible in the current frame, but the current frame is a "closure-like"(*) function and it could have access to the variable, had it been captured by the closure. (*) A lambda in C++, a block in C or Objective C, a closure in Swift. The logic relies on language plugins being able to produce the name of the function where the current closure is defined, given the mangled name of the closure. Since this commit does not implement support for this in any languages, it is NFCI. It contains all the necessary pieces to upstream the code, once we implement Language support for at least one upstream language.
This commit implements SwiftLanguage::GetParentNameIfClosure, effectively adding support for diagnosing when a `frame var` command inside a closure targets a variable that was not captured.
This commit implements the same diagnostic that was previously implemented for frame var. The strategy here is slightly different due to how expression evaluation works. We have to pattern match this diagnostic from the compiler: ``` error: <EXPR>:6:1: cannot find 'NAME' in scope 1 + NAME ^~~~ ``` If we find it, we trigger the usual search for a variable named `NAME`. If it is found, the diagnostic is replaced with: ``` error: <EXPR>:6:5: Current frame is a closure. A variable named 'NAME' exists in function 'foo(_:)', but it was not captured. Hint: the variable may be available in a parent frame. 1 + NAME ^~~~ ``` Implementation note: the pattern matching could be a _lot_ simpler if we don't insist on replacing the original diagnostic and, instead, append to it.
dcd9331
to
632fab8
Compare
Replaced |
If we have the Function object for the closure, we should have the line where it is defined. That line most likely also has line table entry in the parent function. That should be the block that determines the variable scope of the closure. If this is really hard, this is probably over-kill, but then we need to make it clear that we are just guessing that it's a problem with capture, and not just a misspelling. |
This series of commits implement diagnostics for when a variable could have been available for
v
orexpr
commads, had it been captured inside a closure.v
diagnostics in a language agnostic way. It is what should be upstreamed to LLVM once we can support this in C++. I punted on doing this because C++ mangling is a bit<redacted>
to work with.v
by doing the demangling work inside the SwiftLanguage plugin.expr
.