[lldb-dap] Add feature to remember last non-empty expression. #107485

cmtice · 2024-09-05T22:55:31Z

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.

llvmbot · 2024-09-05T22:56:02Z

@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) {

jeffreytan81 · 2024-09-05T23:41:42Z

lldb/tools/lldb-dap/lldb-dap.cpp

  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;


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

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

labath

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).

labath · 2024-09-06T08:12:45Z

lldb/tools/lldb-dap/lldb-dap.cpp

+  else
+    expression = last_nonempty_expression;

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


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).

ashgti · 2024-09-09T16:21:51Z

lldb/tools/lldb-dap/lldb-dap.cpp

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


Should this be done using

llvm-project/lldb/include/lldb/API/SBCommandInterpreterRunOptions.h

Lines 77 to 81 in ea2da57

/// By default, RunCommandInterpreter will discard repeats if the

/// IOHandler being used is not interactive. Setting AllowRepeats to true

/// will override this behavior and always process empty lines in the input

/// as a repeat command.

void SetAllowRepeats(bool);

on the https://github.com/llvm/llvm-project/blob/ea2da571c761066542f8d2273933d2523279e631/lldb/tools/lldb-dap/LLDBUtils.cpp#L48C14-L48C27 call?

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

walter-erquinigo

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

walter-erquinigo · 2024-09-10T20:48:18Z

lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py


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


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

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.

cmtice · 2024-09-12T17:56:57Z

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.

cmtice · 2024-09-12T17:58:32Z

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

github-actions · 2024-09-12T18:00:00Z

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

github-actions · 2024-09-12T18:00:00Z

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

walter-erquinigo · 2024-09-12T18:18:55Z

lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py

+        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")


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.

walter-erquinigo · 2024-09-12T18:20:22Z

lldb/tools/lldb-dap/lldb-dap.cpp

+       (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


Suggested change

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

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

walter-erquinigo · 2024-09-12T18:28:18Z

lldb/tools/lldb-dap/lldb-dap.cpp

+    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();


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

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.

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

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.

labath · 2024-09-13T09:49:36Z

lldb/tools/lldb-dap/LLDBUtils.cpp

      std::lock_guard<std::mutex> locker(handle_command_mutex);
-      interp.HandleCommand(command.str().c_str(), result);
+      interp.HandleCommand(command.str().c_str(), result,
+                           /* add_to_history */ true);


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?

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.

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

labath · 2024-09-13T09:54:26Z

lldb/tools/lldb-dap/lldb-dap.cpp

    EmplaceSafeString(body, "result", result);
    body.try_emplace("variablesReference", (int64_t)0);
  } else {
+    if (context != "hover") {


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

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

labath · 2024-09-13T10:00:56Z

lldb/tools/lldb-dap/lldb-dap.cpp

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


Suggested change

((!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)) {

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.

cmtice · 2024-09-13T16:22:44Z

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

walter-erquinigo · 2024-09-14T17:21:37Z

lldb/tools/lldb-dap/lldb-dap.cpp

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


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

.. 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").

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.

walter-erquinigo · 2024-09-14T17:25:05Z

lldb/tools/lldb-dap/lldb-dap.cpp

+      // 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;


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.

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.

Fix formatting issues.

labath

This works for me. Walter?

labath · 2024-09-19T13:11:58Z

lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py

        self.assertEvaluate("var1", "20")
+        # Empty expression should equate to the previous expression.
+        if context == "repl":
+            self.assertEvaluate("", "20")


Suggested change

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.

labath · 2024-09-19T13:13:55Z

lldb/tools/lldb-dap/LLDBUtils.cpp

      std::lock_guard<std::mutex> locker(handle_command_mutex);
-      interp.HandleCommand(command.str().c_str(), result);
+      interp.HandleCommand(command.str().c_str(), result,
+                           /* add_to_history */ true);


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).

walter-erquinigo

lgtm!

…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)

[lldb-dap] Add feature to remember last non-empty expression.

15541f3

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.

cmtice requested a review from JDevlieghere as a code owner September 5, 2024 22:55

llvmbot added the lldb label Sep 5, 2024

cmtice requested review from walter-erquinigo, clayborg, labath and jeffreytan81 September 5, 2024 22:56

jeffreytan81 reviewed Sep 5, 2024

View reviewed changes

[lldb] Add feature to remember last non-empty expression

e35928e

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

labath reviewed Sep 9, 2024

View reviewed changes

ashgti reviewed Sep 9, 2024

View reviewed changes

walter-erquinigo requested changes Sep 10, 2024

View reviewed changes

walter-erquinigo reviewed Sep 12, 2024

View reviewed changes

cmtice added 2 commits September 12, 2024 13:24

[lldb-dap] Add feature to remember last non-empty expression.

ebcf2bd

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

[lldb-dap] Add feature to remember last non-empty expression.

09bd19d

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

labath reviewed Sep 13, 2024

View reviewed changes

[lldb-dap] Add feature to remember last non-empty expression

8222384

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

walter-erquinigo reviewed Sep 14, 2024

View reviewed changes

cmtice added 2 commits September 16, 2024 08:42

[lldb-dap] Add feature to remember last non-empty expression

790dd9a

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

[lldb-dap] Add feature to remember last non-empty expression.

231f1ec

Fix formatting issues.

labath reviewed Sep 19, 2024

View reviewed changes

[lldb-dap] Add feature to remember last non-empty expression

13bf7d6

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

walter-erquinigo approved these changes Sep 20, 2024

View reviewed changes

cmtice merged commit 2011cbc into llvm:main Sep 20, 2024
7 checks passed

cmtice deleted the lldb-dap-fix branch December 11, 2024 21:47

	/// By default, RunCommandInterpreter will discard repeats if the
	/// IOHandler being used is not interactive. Setting AllowRepeats to true
	/// will override this behavior and always process empty lines in the input
	/// as a repeat command.
	void SetAllowRepeats(bool);

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

[lldb-dap] Add feature to remember last non-empty expression. #107485

[lldb-dap] Add feature to remember last non-empty expression. #107485

Uh oh!

Conversation

cmtice commented Sep 5, 2024

Uh oh!

llvmbot commented Sep 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walter-erquinigo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmtice commented Sep 12, 2024

Uh oh!

cmtice commented Sep 12, 2024

Uh oh!

github-actions bot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmtice commented Sep 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading