[lldb] Make repeat commands work for regex commands

[kastiglione]

Summary

Fix logic for repeat commands, so that regex commands (specificially bt) are
given the opportunity to provide a repeat command.

rdar://104562616

[jingham]

This change didn't immediately make sense to me, which means it's a bit tricky and tricky changes should get comments or they will confuse us later on. Plus, I wasn't sure how it worked, so I'm looking forward to reading the comment as part of the review...

lldb/source/Interpreter/CommandInterpreter.cpp

2005

You at least need to fix this comment, since you aren't checking empty_command anymore.

It would also be good to explain the logic here, since I'm not entirely sure why this patch fixes the problem. It should just mean that we are setting the "previous repeat command" twice, once above and once here, in the case where we didn't set "empty_command", so it's not clear why that helps.

I also think it's wrong to set a repeat command when add_to_history is false. That generally happens if some code, like a breakpoint command, uses HandleCommand, i.e. that's a command the user didn't directly execute. So it doesn't make sense that that non-user command should set the user repeat command.

[kastiglione]

Introduce new allow_repeat_command parameter

[kastiglione]

lldb/source/Interpreter/CommandInterpreter.cpp

2005

I've updated the logic here, keeping add_to_history || empty_command and adding a third check, which is the caller can specific an argument for allow_repeat_command. This let's CommandObjectRegexCommand opt-in to the repeat behavior without changing the semantics of add_to_history.

[jingham]

So the problem is that regex commands don't want to add the resolved command to the history, and so they need to force the creation of the repeat command even though they aren't adding to the history. But allow_repeat_command is a misleading name for that. It makes is sound like if I don't pass that flag I won't get a repeat command. force_repeat_command would be a better name - you aren't allowing this you're requiring it... And some explanation of why it's necessary would help make this patch understandable.

[kastiglione]

But allow_repeat_command is a misleading name for that. It makes is sound like if I don't pass that flag I won't get a repeat command. force_repeat_command would be a better name

I agree. I did have some loose thoughts matching this, when writing it, yet somehow ended up with allow_.

[kastiglione]

Rename variable to force_repeat_command and add comment block

[jingham]

LGTM

[kastiglione]

thanks for the feedback

lldb/source/Interpreter/CommandInterpreter.cpp

bool CommandInterpreter::HandleCommand(const char *command_line,
   // Phase 2.
   // Take care of things like setting up the history command & calling the
   // appropriate Execute method on the CommandObject, with the appropriate
   // arguments.
 
   if (cmd_obj != nullptr) {
     // If we got here when empty_command was true, then this command is a
     // stored "repeat command" which we should give a chance to produce it's
     // repeat command, even though we don't add repeat commands to the history.
-    if (add_to_history || empty_command) {
-      Args command_args(command_string);
-      std::optional<std::string> repeat_command =
-          cmd_obj->GetRepeatCommand(command_args, 0);
-      if (repeat_command)
-        m_repeat_command.assign(*repeat_command);
-      else
-        m_repeat_command.assign(original_command_string);
+    if (repeat_command) {
+      LLDB_LOGF(log, "Repeat command: %s", repeat_command->data());
+    } else {
     }
 
     if (add_to_history)
       m_command_history.AppendString(original_command_string);
 
     std::string remainder;
     const std::size_t actual_cmd_name_len = cmd_obj->GetCommandName().size();
     if (actual_cmd_name_len < command_string.length())

lldb/test/API/functionalities/alias/TestBtAliasRepeat.py

+import lldb
+from lldbsuite.test.lldbtest import TestBase
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "return", lldb.SBFileSpec("main.c"))
+
+        # Expect "frame #0" but not "frame #1".
+        self.expect("bt 1", inHistory=True, patterns=["frame #0", "^(?!.*frame #1)"])
+
+        # Run an empty command to run the repeat command for `bt`.
+        # The repeat command for `bt N` lists the subsequent N frames.
+        #
+        # In this case, after printing the frame 0 with `bt 1`, the repeat
+        # command will print "frame #1" (and won't print "frame #0").
+        self.expect("", patterns=["^(?!.*frame #0)", "frame #1"])