https://github.com/ashgti updated
https://github.com/llvm/llvm-project/pull/78005
>From c3a4cd38b41e332342aa7042d3a9c2f75416bfc3 Mon Sep 17 00:00:00 2001
From: John Harrison <harj...@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.
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
```
---
.../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);
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
