[lldb-dap] Improving 'variables' hover requests. #146773
Conversation
This partially fixes llvm#146559. When hovering over variables while debugging with lldb-dap we are receiving hover requests that include symbols. For example, if you have: ``` MyClass *foo; ``` and hover over 'foo', the request will contain the expression `expression="*foo"` and we're evaluating the dereference, so you end up with different hover results vs the variables view. A more complete solution would be to implement an [EvaluatableExpressionProvider](https://code.visualstudio.com/api/references/vscode-api#EvaluatableExpressionProvider) in the VS Code extension. For example, if you have a declaration without any whitespace like ```c char*foo = "bar"; ``` And try to hover over 'foo', the request will be `expression="char*foo"`.
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis partially fixes #146559. When hovering over variables while debugging with lldb-dap we are receiving hover requests that include symbols. For example, if you have: and hover over 'foo', the request will contain the expression A more complete solution would be to implement an For example, if you have a declaration without any whitespace like char*foo = "bar";And try to hover over 'foo', the request will be Full diff: https://github.com/llvm/llvm-project/pull/146773.diff 2 Files Affected:
diff --git a/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py b/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py
index 55fb4a961e783..30ee7a1631e29 100644
--- a/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py
+++ b/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py
@@ -92,7 +92,7 @@ def test_readMemory(self):
)
self.continue_to_next_stop()
- ptr_deref = self.dap_server.request_evaluate("*rawptr")["body"]
+ ptr_deref = self.dap_server.request_evaluate("*rawptr", context="repl")["body"]
memref = ptr_deref["memoryReference"]
# We can read the complete string
diff --git a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp
index e1556846dff19..e2161eb999e82 100644
--- a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp
@@ -181,7 +181,17 @@ void EvaluateRequestHandler::operator()(
expression = dap.last_nonempty_var_expression;
else
dap.last_nonempty_var_expression = expression;
+ } else {
+ // If this isn't a REPL context, trim leading pointer/reference characters
+ // to ensure we return the actual value of the expression.
+ // This can come up if you hover over a pointer or reference declaration
+ // like 'MyType *foo;' or `void fn(std::string &arg)`, which results in
+ // the hover request sending '*foo' or `&arg`. When we're not in the REPL,
+ // we should trim these characters to get to the actual variable, which
+ // should have the proper type encoded by the compiler.
+ expression = llvm::StringRef(expression).ltrim("*&").str();
}
+
// Always try to get the answer from the local variables if possible. If
// this fails, then if the context is not "hover", actually evaluate an
// expression using the expression parser.
|
What's the reason not to implement that more complete solution? |
It would be pretty language dependent. For example, if we want to break a .cpp file up into resolvable expressions we'd need to maybe make something in clangd for extracting that from the ast or make a helper clang-frontend tool for this. We might be able to cheat slightly and if the language has semantic token support for its lsp (clangd/sourcekit-lsp both support this), we might be able to use that to determine how to break up terms. I suppose could also approach this from the other side and look and see if we can get this from lldb somehow. Maybe there is enough information that we could determine from the debugger itself which parts of the file are expressions and we could make a custom request in lldb-dap for that and call that from our implementation of EvaluatableExpressionProvider. |
LLDB (kind of by design) doesn't parse the source code of a program. In fact, the only reason it touches it at all is to display it to the user. If you delete the source code, every part of the debugger will work just fine, except you won't actually see the code you're debugging. So, no, I don't think there's anything in lldb that would help with that... |
We could search starting from the back for the first occurrence of either "&" or "*". I think We should limit this to just the hover context because we can deference a variable in the watch window. We also need test for it. |
|
I don't know how you are doing this, but if you are treating "hover over" requests as expressions and presenting the result of some real expression evaluation, you have to be quite careful about what you evaluate. For instance, you want to make sure that hovering over
|
The corresponding DAP request is called "expression" which is somewhat confusing. The request includes a "context" (such as "repl" or "hover") which allows us to do something different. Except for "repl", we always try |
Excellent! |
| } else { | ||
| // If this isn't a REPL context, trim leading pointer/reference characters | ||
| // to ensure we return the actual value of the expression. |
There was a problem hiding this comment.
doesn't this also trigger in way too many other contexts? E.g., also in the watch panel? I think in the watch panel a *x should actually dereference x
There was a problem hiding this comment.
Since people have to voluntarily put something in a watch panel, then in that case we should even run it as an expression. If somebody actually puts i++ in a watch window, presumably they meant for it to increment every time they stepped.
We had a silly bug in gdbtk way back in the day where we if you moused over a selection we would run the selection as an expression. But lots of people seem to select expressions so that they can focus on just the part they selected. As you can imagine, that went poorly.
The main thing - for me - is to always have running expressions be a distinct gesture.
That's rather specific to C. In Swift for instance, the variable name comes before the type ( |
This partially fixes #146559.
When hovering over variables while debugging with lldb-dap we are receiving hover requests that include symbols.
For example, if you have:
and hover over 'foo', the request will contain the expression
expression="*foo"and we're evaluating the dereference, so you end up with different hover results vs the variables view.A more complete solution would be to implement an
EvaluatableExpressionProvider in the VS Code extension.
For example, if you have a declaration without any whitespace like
And try to hover over 'foo', the request will be
expression="char*foo", which won't evaluate correctly in lldb.