[Lldb-commits] [lldb] [lldb-dap] Adjusting how repl-mode auto determines commands vs variable expressions. (PR #78005)
John Harrison via lldb-commits
lldb-commits at lists.llvm.org
Tue Jan 16 09:53:32 PST 2024
https://github.com/ashgti updated https://github.com/llvm/llvm-project/pull/78005
>From f01c82be09e28c1eead4d80f4ceea67dee0318d6 Mon Sep 17 00:00:00 2001
From: John Harrison <harjohn at google.com>
Date: Fri, 12 Jan 2024 16:39:47 -0800
Subject: [PATCH] [lldb-dap] Adjusting how repl-mode auto determines commands
vs variable expressions.
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, evaluating the expression as a variable. To evaluate the lldb command, use '`' as a prefix.
note: This error message is only displayed once. Use '`' as a prefix ensure expressions are evaluated as lldb commands.
To change the repl behavior use:
'`lldb-dap repl-mode variable' to evaluate all expressions as variables.
'`lldb-dap repl-mode command' to evaluate all expressions as commands.
'`lldb-dap repl-mode auto' to use a heuristic to detect the mode based on the input and local variables.
45
lldb-dap> var + 1
warning: Expression 'var' is both an lldb command and variable, evaluating the expression as a variable. To evaluate the lldb command, use '`' as a prefix.
43
```
---
.../completions/TestDAP_completions.py | 6 +-
.../lldb-dap/evaluate/TestDAP_evaluate.py | 8 +-
lldb/tools/lldb-dap/DAP.cpp | 74 +++++++++----------
lldb/tools/lldb-dap/DAP.h | 9 ++-
lldb/tools/lldb-dap/lldb-dap.cpp | 35 ++++++---
5 files changed, 75 insertions(+), 57 deletions(-)
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 e91b4115156b73..2b2efbe7a7a19d 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>
@@ -1125,21 +1126,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