[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

2010–2024

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

2010–2024

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/include/lldb/Interpreter/CommandInterpreter.h

CommandObject *BuildAliasResult(llvm::StringRef alias_name,
                                   std::string &alias_result,
                                   CommandReturnObject &result);
 
   bool HandleCommand(const char *command_line, LazyBool add_to_history,
                      const ExecutionContext &override_context,
                      CommandReturnObject &result);
 
   bool HandleCommand(const char *command_line, LazyBool add_to_history,
-                     CommandReturnObject &result);
+                     CommandReturnObject &result,
+                     bool force_repeat_command = false);
 
   bool WasInterrupted() const;
 
   /// Execute a list of commands in sequence.
   ///
   /// \param[in] commands
   ///    The list of commands to execute.
   /// \param[in,out] context

lldb/source/Commands/CommandObjectRegexCommand.cpp

if (pos->regex.Execute(command, &matches)) {
         return false;
       }
 
       // Interpret the new command and return this as the result!
       if (m_interpreter.GetExpandRegexAliases())
         result.GetOutputStream().Printf("%s\n", new_command->c_str());
       // We don't have to pass an override_context here, as the command that 
       // called us should have set up the context appropriately.
-      return m_interpreter.HandleCommand(new_command->c_str(),
-                                         eLazyBoolNo, result);
+      bool force_repeat_command = true;
+      return m_interpreter.HandleCommand(new_command->c_str(), eLazyBoolNo,
+                                         result, force_repeat_command);
     }
   }
   result.SetStatus(eReturnStatusFailed);
   if (!GetSyntax().empty())
     result.AppendError(GetSyntax());
   else
     result.GetErrorStream() << "Command contents '" << command
                             << "' failed to match any "

lldb/source/Interpreter/CommandInterpreter.cpp

bool CommandInterpreter::HandleCommand(const char *command_line,
   OverrideExecutionContext(override_context);
   bool status = HandleCommand(command_line, lazy_add_to_history, result);
   RestoreExecutionContext();
   return status;
 }
 
 bool CommandInterpreter::HandleCommand(const char *command_line,
                                        LazyBool lazy_add_to_history,
-                                       CommandReturnObject &result) {
-
+                                       CommandReturnObject &result,
+                                       bool force_repeat_command) {
   std::string command_string(command_line);
   std::string original_command_string(command_line);
 
   Log *log = GetLog(LLDBLog::Commands);
   llvm::PrettyStackTraceFormat stack_trace("HandleCommand(command = \"%s\")",
                                    command_line);
 
   LLDB_LOGF(log, "Processing command: %s", command_line);
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) {
+    bool generate_repeat_command = add_to_history;
     // 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) {
+    generate_repeat_command |= empty_command;
+    // For `command regex`, the regex command (ex `bt`) is added to history, but
+    // the resolved command (ex `thread backtrace`) is _not_ added to history.
+    // However, the resolved command must be given the opportunity to provide a
+    // repeat command. `force_repeat_command` supports this case.
+    generate_repeat_command |= force_repeat_command;
+    if (generate_repeat_command) {
       Args command_args(command_string);
       std::optional<std::string> repeat_command =
           cmd_obj->GetRepeatCommand(command_args, 0);
-      if (repeat_command)
+      if (repeat_command) {
+        LLDB_LOGF(log, "Repeat command: %s", repeat_command->data());
         m_repeat_command.assign(*repeat_command);
-      else
+      } else {
         m_repeat_command.assign(original_command_string);
       }
+    }
 
     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())
       remainder = command_string.substr(actual_cmd_name_len);

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