[Lldb-commits] [lldb] 4ea1994 - [lldb-dap] Adjusting how repl-mode auto determines commands vs variable expressions. (#78005)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Jan 17 09:00:27 PST 2024
Author: John Harrison
Date: 2024-01-17T09:00:22-08:00
New Revision: 4ea1994a0307b09532d519292d34dad7555598ca
URL: https://github.com/llvm/llvm-project/commit/4ea1994a0307b09532d519292d34dad7555598ca
DIFF: https://github.com/llvm/llvm-project/commit/4ea1994a0307b09532d519292d34dad7555598ca.diff
LOG: [lldb-dap] Adjusting how repl-mode auto determines commands vs variable expressions. (#78005)
The previous logic for determining if an expression was a command or
variable expression in the repl would incorrectly identify the context
in many common cases where a local variable name partially overlaps with
the repl input.
For example:
```
int foo() {
int var = 1; // break point, evaluating "p var", previously emitted a warning
}
```
Instead of checking potentially multiple conflicting values against the
expression input, I updated the heuristic to only consider the first
term. This is much more reliable at eliminating false positives when the
input does not actually hide a local variable.
Additionally, I updated the warning on conflicts to occur anytime the
conflict is detected since the specific conflict can change based on the
current input. This also includes additional details on how users can
change the behavior.
Example Debug Console output from
lldb/test/API/tools/lldb-dap/evaluate/main.cpp:11 breakpoint 3.
```
lldb-dap> var + 3
Warning: Expression 'var' is both an LLDB command and variable. It will be evaluated as a variable. To evaluate the expression as an LLDB command, use '`' as a prefix.
45
lldb-dap> var + 1
Warning: Expression 'var' is both an LLDB command and variable. It will be evaluated as a variable. To evaluate the expression as an LLDB command, use '`' as a prefix.
43
```
Added:
Modified:
lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py
lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py
lldb/tools/lldb-dap/DAP.cpp
lldb/tools/lldb-dap/DAP.h
lldb/tools/lldb-dap/lldb-dap.cpp
Removed:
################################################################################
diff --git a/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py b/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py
index 5f6d63392f4d5f..2b3ec656c107a5 100644
--- a/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py
+++ b/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py
@@ -41,13 +41,13 @@ def test_completions(self):
{
"text": "var",
"label": "var -- vector<baz> &",
- }
- ],
- [
+ },
{
"text": "var",
"label": "var -- Show variables for the current stack frame. Defaults to all arguments and local variables in scope. Names of argument, local, file static and file global variables can be specified.",
},
+ ],
+ [
{"text": "var1", "label": "var1 -- int &"},
],
)
diff --git a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py
index 7651a67b643094..0192746f1277b5 100644
--- a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py
+++ b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py
@@ -87,7 +87,13 @@ def run_test_evaluate_expressions(
)
self.assertEvaluate("struct3", "0x.*0")
- self.assertEvaluateFailure("var") # local variable of a_function
+ if context == "repl":
+ # In the repl context expressions may be interpreted as lldb
+ # commands since no variables have the same name as the command.
+ self.assertEvaluate("var", r"\(lldb\) var\n.*")
+ else:
+ self.assertEvaluateFailure("var") # local variable of a_function
+
self.assertEvaluateFailure("my_struct") # type name
self.assertEvaluateFailure("int") # type name
self.assertEvaluateFailure("foo") # member of my_struct
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 4b72c13f9215a8..b254ddfef0d5ff 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -47,8 +47,7 @@ DAP::DAP()
configuration_done_sent(false), waiting_for_run_in_terminal(false),
progress_event_reporter(
[&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
- reverse_request_seq(0), repl_mode(ReplMode::Auto),
- auto_repl_mode_collision_warning(false) {
+ reverse_request_seq(0), repl_mode(ReplMode::Auto) {
const char *log_file_path = getenv("LLDBDAP_LOG");
#if defined(_WIN32)
// Windows opens stdout and stdin in text mode which converts \n to 13,10
@@ -380,12 +379,12 @@ llvm::json::Value DAP::CreateTopLevelScopes() {
return llvm::json::Value(std::move(scopes));
}
-ExpressionContext DAP::DetectExpressionContext(lldb::SBFrame &frame,
- std::string &text) {
+ExpressionContext DAP::DetectExpressionContext(lldb::SBFrame frame,
+ std::string &expression) {
// Include the escape hatch prefix.
- if (!text.empty() &&
- llvm::StringRef(text).starts_with(g_dap.command_escape_prefix)) {
- text = text.substr(g_dap.command_escape_prefix.size());
+ if (!expression.empty() &&
+ llvm::StringRef(expression).starts_with(g_dap.command_escape_prefix)) {
+ expression = expression.substr(g_dap.command_escape_prefix.size());
return ExpressionContext::Command;
}
@@ -395,43 +394,40 @@ ExpressionContext DAP::DetectExpressionContext(lldb::SBFrame &frame,
case ReplMode::Command:
return ExpressionContext::Command;
case ReplMode::Auto:
- // If the frame is invalid then there is no variables to complete, assume
- // this is an lldb command instead.
- if (!frame.IsValid()) {
- return ExpressionContext::Command;
- }
-
+ // To determine if the expression is a command or not, check if the first
+ // term is a variable or command. If it's a variable in scope we will prefer
+ // that behavior and give a warning to the user if they meant to invoke the
+ // operation as a command.
+ //
+ // Example use case:
+ // int p and expression "p + 1" > variable
+ // int i and expression "i" > variable
+ // int var and expression "va" > command
+ std::pair<llvm::StringRef, llvm::StringRef> token =
+ llvm::getToken(expression);
+ std::string term = token.first.str();
lldb::SBCommandReturnObject result;
- debugger.GetCommandInterpreter().ResolveCommand(text.data(), result);
-
- // If this command is a simple expression like `var + 1` check if there is
- // a local variable name that is in the current expression. If so, ensure
- // the expression runs in the variable context.
- lldb::SBValueList variables = frame.GetVariables(true, true, true, true);
- llvm::StringRef input = text;
- for (uint32_t i = 0; i < variables.GetSize(); i++) {
- llvm::StringRef name = variables.GetValueAtIndex(i).GetName();
- // Check both directions in case the input is a partial of a variable
- // (e.g. input = `va` and local variable = `var1`).
- if (input.contains(name) || name.contains(input)) {
- if (!auto_repl_mode_collision_warning) {
- llvm::errs() << "Variable expression '" << text
- << "' is hiding an lldb command, prefix an expression "
- "with '"
- << g_dap.command_escape_prefix
- << "' to ensure it runs as a lldb command.\n";
- auto_repl_mode_collision_warning = true;
- }
- return ExpressionContext::Variable;
- }
+ debugger.GetCommandInterpreter().ResolveCommand(term.c_str(), result);
+ bool term_is_command = result.Succeeded();
+ bool term_is_variable = frame.FindVariable(term.c_str()).IsValid();
+
+ // If we have both a variable and command, warn the user about the conflict.
+ if (term_is_command && term_is_variable) {
+ llvm::errs()
+ << "Warning: Expression '" << term
+ << "' is both an LLDB command and variable. It will be evaluated as "
+ "a variable. To evaluate the expression as an LLDB command, use '"
+ << g_dap.command_escape_prefix << "' as a prefix.\n";
}
- if (result.Succeeded()) {
- return ExpressionContext::Command;
- }
+ // Variables take preference to commands in auto, since commands can always
+ // be called using the command_escape_prefix
+ return term_is_variable ? ExpressionContext::Variable
+ : term_is_command ? ExpressionContext::Command
+ : ExpressionContext::Variable;
}
- return ExpressionContext::Variable;
+ llvm_unreachable("enum cases exhausted.");
}
bool DAP::RunLLDBCommands(llvm::StringRef prefix,
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 20817194de2d8d..8015dec9ba8fe6 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -189,7 +189,6 @@ struct DAP {
StartDebuggingRequestHandler start_debugging_request_handler;
ReplModeRequestHandler repl_mode_request_handler;
ReplMode repl_mode;
- bool auto_repl_mode_collision_warning;
std::string command_escape_prefix = "`";
lldb::SBFormat frame_format;
lldb::SBFormat thread_format;
@@ -225,8 +224,12 @@ struct DAP {
llvm::json::Value CreateTopLevelScopes();
- ExpressionContext DetectExpressionContext(lldb::SBFrame &frame,
- std::string &text);
+ /// \return
+ /// Attempt to determine if an expression is a variable expression or
+ /// lldb command using a hueristic based on the first term of the
+ /// expression.
+ ExpressionContext DetectExpressionContext(lldb::SBFrame frame,
+ std::string &expression);
/// \return
/// \b false if a fatal error was found while executing these commands,
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 8c8e92146e63c0..bb3500c21e7452 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -37,6 +37,7 @@
#endif
#include <algorithm>
+#include <array>
#include <chrono>
#include <fstream>
#include <map>
@@ -1126,21 +1127,33 @@ void request_completions(const llvm::json::Object &request) {
}
llvm::json::Array targets;
- if (g_dap.DetectExpressionContext(frame, text) ==
- ExpressionContext::Variable) {
- char command[] = "expression -- ";
- text = command + text;
- offset += strlen(command);
- }
- lldb::SBStringList matches;
- lldb::SBStringList descriptions;
+ if (!text.empty() &&
+ llvm::StringRef(text).starts_with(g_dap.command_escape_prefix)) {
+ text = text.substr(g_dap.command_escape_prefix.size());
+ }
+
+ // While the user is typing then we likely have an incomplete input and cannot
+ // reliably determine the precise intent (command vs variable), try completing
+ // the text as both a command and variable expression, if applicable.
+ const std::string expr_prefix = "expression -- ";
+ std::array<std::tuple<ReplMode, std::string, uint64_t>, 2> exprs = {
+ {std::make_tuple(ReplMode::Command, text, offset),
+ std::make_tuple(ReplMode::Variable, expr_prefix + text,
+ offset + expr_prefix.size())}};
+ for (const auto &[mode, line, cursor] : exprs) {
+ if (g_dap.repl_mode != ReplMode::Auto && g_dap.repl_mode != mode)
+ continue;
+
+ lldb::SBStringList matches;
+ lldb::SBStringList descriptions;
+ if (!g_dap.debugger.GetCommandInterpreter()
+ .HandleCompletionWithDescriptions(line.c_str(), cursor, 0, 100,
+ matches, descriptions))
+ continue;
- if (g_dap.debugger.GetCommandInterpreter().HandleCompletionWithDescriptions(
- text.c_str(), offset, 0, 100, matches, descriptions)) {
// The first element is the common substring after the cursor position for
// all the matches. The rest of the elements are the matches so ignore the
// first result.
- targets.reserve(matches.GetSize() - 1);
for (size_t i = 1; i < matches.GetSize(); i++) {
std::string match = matches.GetStringAtIndex(i);
std::string description = descriptions.GetStringAtIndex(i);
More information about the lldb-commits
mailing list