From 4ea1994a0307b09532d519292d34dad7555598ca Mon Sep 17 00:00:00 2001 From: John Harrison Date: Wed, 17 Jan 2024 09:00:22 -0800 Subject: [PATCH] [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 ``` --- .../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 5f6d63392f4d5..2b3ec656c107a 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 &", - } - ], - [ + }, { "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 7651a67b64309..0192746f1277b 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 4b72c13f9215a..b254ddfef0d5f 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 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 20817194de2d8..8015dec9ba8fe 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 8c8e92146e63c..bb3500c21e745 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -37,6 +37,7 @@ #endif #include +#include #include #include #include @@ -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, 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);