[lldb-dap] Add feature to remember last non-empty expression. by cmtice · Pull Request #107485 · llvm/llvm-project (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation42 Commits9 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Update lldb-dap so if the user just presses return, which sends an empty expression, it re-evaluates the most recent non-empty expression/command. Also udpated test to test this case.

@llvm/pr-subscribers-lldb

Author: None (cmtice)

Changes

Update lldb-dap so if the user just presses return, which sends an empty expression, it re-evaluates the most recent non-empty expression/command. Also udpated test to test this case.

Full diff: https://github.com/llvm/llvm-project/pull/107485.diff

2 Files Affected:

(modified) lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py (+3)
(modified) lldb/tools/lldb-dap/lldb-dap.cpp (+8)

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 29548a835c6919..9ed0fc564268a7 100644 --- a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py +++ b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py @@ -60,7 +60,10 @@ def run_test_evaluate_expressions(

     # Expressions at breakpoint 1, which is in main
     self.assertEvaluate("var1", "20")

   # Empty expression should equate to the previous expression.

   self.assertEvaluate("", "20")
   self.assertEvaluate("var2", "21")

   self.assertEvaluate("", "21")
   self.assertEvaluate("static_int", "42")
   self.assertEvaluate("non_static_int", "43")
   self.assertEvaluate("struct1.foo", "15")

diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp index c5c4b09f15622b..a6a701dc2219fa 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -1363,6 +1363,14 @@ void request_evaluate(const llvm::json::Object &request) { lldb::SBFrame frame = g_dap.GetLLDBFrame(*arguments); std::string expression = GetString(arguments, "expression").str(); llvm::StringRef context = GetString(arguments, "context");

static std::string last_nonempty_expression;
// Remember the last non-empty expression from the user, and use that if
// the current expression is empty (i.e. the user hit plain 'return').
if (!expression.empty())
last_nonempty_expression = expression;
else
expression = last_nonempty_expression;

if (context == "repl" && g_dap.DetectExpressionContext(frame, expression) == ExpressionContext::Command) {

@@ -1363,6 +1363,14 @@ void request_evaluate(const llvm::json::Object &request) {
lldb::SBFrame frame = g_dap.GetLLDBFrame(*arguments);
std::string expression = GetString(arguments, "expression").str();
llvm::StringRef context = GetString(arguments, "context");
static std::string last_nonempty_expression;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use static object. Move to DAP object instead.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Make last_nonempty_spression part of DAP struct rather than a static variable.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a fairly long comment on Friday, but I don't see it anymore so, it looks like github has swallowed it. Here's my reconstruction of it:

LLDB commands have the notion of a "repeat command", which can sometimes be more complicated than just running the same string over and over again. This can be e.g. seen with the memory read command, which doesn't just print the same memory again -- it actually prints the memory that comes after it (a pretty nifty feature actually):

(lldb) memory read argv
0x7fffffffd8a8: 49 dc ff ff ff 7f 00 00 00 00 00 00 00 00 00 00  I...............
0x7fffffffd8b8: 6c dc ff ff ff 7f 00 00 ae dc ff ff ff 7f 00 00  l...............
(lldb) 
0x7fffffffd8c8: c4 dc ff ff ff 7f 00 00 cf dc ff ff ff 7f 00 00  ................
0x7fffffffd8d8: 09 dd ff ff ff 7f 00 00 18 dd ff ff ff 7f 00 00  ................
(lldb) 
0x7fffffffd8e8: 40 dd ff ff ff 7f 00 00 7d dd ff ff ff 7f 00 00  @.......}.......
0x7fffffffd8f8: 81 e5 ff ff ff 7f 00 00 9d e5 ff ff ff 7f 00 00  ................
(lldb) memory read argv
0x7fffffffd8a8: 49 dc ff ff ff 7f 00 00 00 00 00 00 00 00 00 00  I...............
0x7fffffffd8b8: 6c dc ff ff ff 7f 00 00 ae dc ff ff ff 7f 00 00  l...............

Storing (and repeating) the command string in lldb-dap would break this behavior. What we'd ideally want is to actually take the empty string and pass it to lldb's command interpreter so that the proper repeat logic kicks in.

The thing which makes this tricky (but not too complicated I think) is that lldb-dap multiplexes expression commands and CLI commands into the same string (the DetectExpressionContext does the demultiplexing).

I think the proper repeat handling could be two things about each command:

the type ("expression context") of the command
the command string itself, if the command was not a CLI command (so we can repeat the expression)

Then, when we get an empty string, we check the type of the previous command:

if it was a CLI command (ExpressionContext::Command), we change send the empty string to lldb command interpreter, so that it does the right thing
otherwise, we take the expression string and re-evaluate it (like you do here).

if (!expression.empty())
last_nonempty_expression = expression;
else
expression = last_nonempty_expression;

if (context == "repl" && g_dap.DetectExpressionContext(frame, expression) ==

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably ought to be doing this only for commands in the "repl" context. I don't think want to repeat random expressions automatically evaluated by the IDE (e.g. to fill the "watch" window).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// the current expression is empty (i.e. the user hit plain 'return').
if (!expression.empty())
g_dap.last_nonempty_expression = expression;
else

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SBCommandIntepreterRunOptions don't seem to be used on this execution path, so I couldn't make use of this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!
I second all that Pavel said.

@@ -60,7 +60,10 @@ def run_test_evaluate_expressions(

# Expressions at breakpoint 1, which is in main
self.assertEvaluate("var1", "20")
# Empty expression should equate to the previous expression.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test that uses memory read to make sure that two different memory read invocations give different valid results

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Update to handle commands & variables separately: empty command expressions are passed to the CommandIntepreter to handle as it normally does; empty variable expressions are updated to use the last non-empty variable expression, if the last expression was a variable (not a command). Also updated the test case to test these cases properly, and added a 'memory read' followed by an empty expression, to make sure it handles that sequence correctly.

I wrote a fairly long comment on Friday, but I don't see it anymore so, it looks like github has swallowed it. Here's my reconstruction of it:

LLDB commands have the notion of a "repeat command", which can sometimes be more complicated than just running the same string over and over again. This can be e.g. seen with the memory read command, which doesn't just print the same memory again -- it actually prints the memory that comes after it (a pretty nifty feature actually):
(lldb) memory read argv
0x7fffffffd8a8: 49 dc ff ff ff 7f 00 00 00 00 00 00 00 00 00 00  I...............
0x7fffffffd8b8: 6c dc ff ff ff 7f 00 00 ae dc ff ff ff 7f 00 00  l...............
(lldb) 
0x7fffffffd8c8: c4 dc ff ff ff 7f 00 00 cf dc ff ff ff 7f 00 00  ................
0x7fffffffd8d8: 09 dd ff ff ff 7f 00 00 18 dd ff ff ff 7f 00 00  ................
(lldb) 
0x7fffffffd8e8: 40 dd ff ff ff 7f 00 00 7d dd ff ff ff 7f 00 00  @.......}.......
0x7fffffffd8f8: 81 e5 ff ff ff 7f 00 00 9d e5 ff ff ff 7f 00 00  ................
(lldb) memory read argv
0x7fffffffd8a8: 49 dc ff ff ff 7f 00 00 00 00 00 00 00 00 00 00  I...............
0x7fffffffd8b8: 6c dc ff ff ff 7f 00 00 ae dc ff ff ff 7f 00 00  l...............
Storing (and repeating) the command string in lldb-dap would break this behavior. What we'd ideally want is to actually take the empty string and pass it to lldb's command interpreter so that the proper repeat logic kicks in.

The thing which makes this tricky (but not too complicated I think) is that lldb-dap multiplexes expression commands and CLI commands into the same string (the DetectExpressionContext does the demultiplexing).

I think the proper repeat handling could be two things about each command:

the type ("expression context") of the command

the command string itself, if the command was not a CLI command (so we can repeat the expression)

Then, when we get an empty string, we check the type of the previous command:

if it was a CLI command (ExpressionContext::Command), we change send the empty string to lldb command interpreter, so that it does the right thing

otherwise, we take the expression string and re-evaluate it (like you do here).

I think I have done what you requested now.

I think I have addressed all the review comments; please take another look.

✅ With the latest revision this PR passed the Python code formatter.

✅ With the latest revision this PR passed the C/C++ code formatter.

Comment on lines 202 to 207

if context == "repl":
self.continue_to_next_stop()
self.assertEvaluate("memory read &my_ints",
".00 00 00 00 02 00 00 00 04 00 00 00 06 00 00 00.\n.08 00 00 00 0a 00 00 00 0c 00 00 00 0e 00 00 00.\n")
self.assertEvaluate("",
".10 00 00 00 12 00 00 00 14 00 00 00 16 00 00 00.\n.18 00 00 00 1a 00 00 00 1c 00 00 00.\n")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this will reliably be the same across systems. Something that would be better would be to read from an uint8_t array and just read one byte at a time. That should work anywhere.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

(expression.empty() &&
g_dap.last_expression_context == ExpressionContext::Command))) {
// If the current expression is empty, and the last expression context was
// for a command, pass the empty expression along to the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// for a command, pass the empty expression along to the
// for a command, pass the empty expression along to the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 1377 to 1380

g_dap.last_expression_context = ExpressionContext::Command;
// Since the current expression context is not for a variable, clear the
// last_nonempty_var_expression field.
g_dap.last_nonempty_var_expression.clear();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cleaner if this gets stored in a sort of discriminated union:

LastExpressionInfo:

Case Command (no metadata)
Case Var (expression)

this way it's easier to maintain these two variables correctly in sync

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a union will work because last_expression_context is needed and used in all empty expression cases, while for the variable case we ALSO need the last_nonempty_var_expression.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I have an idea for a way to make this work with only one variable; let me try that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea worked; I've updated the code now.

Update test to use single-byte ints for memory reads, and to read one byte at a time. Also fix space in comment.

Update to need only one new additional field in DAP structure, instead of two.

@@ -45,7 +45,8 @@ bool RunLLDBCommands(llvm::StringRef prefix,
// RunTerminateCommands.
static std::mutex handle_command_mutex;
std::lock_guardstd::mutex locker(handle_command_mutex);
interp.HandleCommand(command.str().c_str(), result);
interp.HandleCommand(command.str().c_str(), result,
/* add_to_history */ true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recommended (but not very strictly enforced) formatting is /*add_to_history=*/ (equal sign, no spaces).

That said, how does this relate to the rest of the patch?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add_to_history defaults to false. If the commands are not added to the history, then the CommandInterpreter can't process an empty expression because there's no record of what the previous non-empty expression was.

I will update the comment as you request.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see now. The comment threw me off, as I though this code handles commands which run during termination.

@@ -1376,6 +1382,16 @@ void request_evaluate(const llvm::json::Object &request) {
EmplaceSafeString(body, "result", result);
body.try_emplace("variablesReference", (int64_t)0);
} else {
if (context != "hover") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the !=hover be ==repl instead? I don't think we want to repeat the expression s from the watch window..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this feature should be limited to the repl Debug Console

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 1368 to 1371

((!expression.empty() &&
g_dap.DetectExpressionContext(frame, expression) ==
ExpressionContext::Command) \|
(expression.empty() && g_dap.last_nonempty_var_expression.empty()))) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

((!expression.empty() &&
g_dap.DetectExpressionContext(frame, expression) ==
ExpressionContext::Command) \|
(expression.empty() && g_dap.last_nonempty_var_expression.empty()))) {
(expression.empty() ? g_dap.last_nonempty_var_expression.empty() : g_dap.DetectExpressionContext(frame, expression) ==
ExpressionContext::Command)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion! Done.

Clean up formmatting for parameter comment; simplify conditional as suggested by reviewer; only handle empty variable expressions in 'repl' context.

All reviewer comments have been addressed. Please take another look. Thank you!

Comment on lines 1368 to 1370

(expression.empty() ?
g_dap.last_nonempty_var_expression.empty() :
g_dap.DetectExpressionContext(frame, expression) ==

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's hard to read this. Please move this ternary check into another variable with a good name.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. but note that the work that DetectExpressionContext does (looking up local variables) is relatively expensive, so we probably don't want to do it when it's not needed (when context!="repl").

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done the best compromise I can -- I've created a new variable, 'repeat_last_command' to use in the if-condition, and tried to set up the if-condition to avoid unnecessary calls to DetectExpressionContext.

Comment on lines +1386 to +1391

// If the expression is empty and the last expression was for a
// variable, set the expression to the previous expression (repeat the
// evaluation); otherwise save the current non-empty expression for the
// next (possibly empty) variable expression.
if (expression.empty())
expression = g_dap.last_nonempty_var_expression;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this has the following flow in the debug console:

You send a command (non-var expression) FOO
You send a var expression VAR
You send empty text -> then FOO repeats

If that understanding is correct, that would lead to a confusing behavior. I think it's better to repeat only if the previous input was exactly a command.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your description of the behavior is not correct. last_nonempty_var_expression is now being used for two purposes: It both holds the last non-empty var expression IF-AND-ONLY-IF the last expression was a var expression; or it indicates that the last expression was a command, by being empty itself. Note the very first thing we do, unconditionally, when processing a command is to clear this variable.

When we receive an empty expression, we check to see if the most recent non-empty expression was a command (test to see if last_nonempty_var_expression is empty). If it was a command, then we pass the empty expression to the command interpreter. If the most recent non-empty expression was a variable expression (i.e. last_nonempty_var_expression is NOT empty), then we treat the empty expression as a variable expression, re-evalulating the contents of last_nonempty_var_expression.

Move ternary expression out of conditional, and use new boolean variable instead.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me. Walter?

],
)
self.continue_to_next_stop()

# Expressions at breakpoint 1, which is in main
self.assertEvaluate("var1", "20")
# Empty expression should equate to the previous expression.
if context == "repl":
self.assertEvaluate("", "20")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.assertEvaluate("", "20")
self.assertEvaluate("", "20")
else:
self.assertEvaluateFailure("")

.. or something along those lines (I haven't checked whether this works) -- basically to check that the empty string does not repeat in non-repl contexts.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -45,7 +45,8 @@ bool RunLLDBCommands(llvm::StringRef prefix,
// RunTerminateCommands.
static std::mutex handle_command_mutex;
std::lock_guardstd::mutex locker(handle_command_mutex);
interp.HandleCommand(command.str().c_str(), result);
interp.HandleCommand(command.str().c_str(), result,
/* add_to_history */ true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see now. The comment threw me off, as I though this code handles commands which run during termination.

Update test slightly (verfiy empty variable expression fails in non-repl mode).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

JDevlieghere pushed a commit to swiftlang/llvm-project that referenced this pull request

Oct 16, 2024

…07485)

Update lldb-dap so if the user just presses return, which sends an empty expression, it re-evaluates the most recent non-empty expression/command. Also udpated test to test this case.

(cherry picked from commit 2011cbc)

Reviewers

ashgti ashgti left review comments

labath labath left review comments

jeffreytan81 jeffreytan81 left review comments

walter-erquinigo walter-erquinigo approved these changes

JDevlieghere Awaiting requested review from JDevlieghere JDevlieghere is a code owner

clayborg Awaiting requested review from clayborg