Add a repeat command option for "thread backtrace --count N".

[jingham]

Summary

This way if you have a long stack, you can issue "thread backtrace --count 10"
and then subsequent <Return>-s will page you through the stack 10 frames at a time.
Previously the repeat just redid the previous command. "thread backtrace" without a
--count will continue to just dump the complete stack.

This took a little more effort than just adding the repeat command, since
the GetRepeatCommand API was returning a "const char *". That meant the command
had to keep the repeat string alive, which is inconvenient. The original
API returned either a nullptr, or a const char *, so I changed the private API to
return an llvm::Optional<std::string>. Most of the patch is propagating that change.

Also, there was a little thinko in fetching the repeat command. We don't
fetch repeat commands for commands that aren't being added to history, which
is in general reasonable. And we don't add repeat commands to the history -
also reasonable. But we do want the repeat command to be able to generate
the NEXT repeat command. So I adjusted the logic in HandleCommand to work
that way.

[JDevlieghere]

API and code change make sense, but I think we should be able to simplify the test.

lldb/test/API/commands/thread/backtrace/TestThreadBacktraceRepeat.py

19–29

I assume you copied this from an existing tests. Can we make the test simpler (by not having two threads) and therefore not having to skip it in so many configurations?

[jingham]

lldb/test/API/commands/thread/backtrace/TestThreadBacktraceRepeat.py

19–29

No. One of the things I want to test is that the repeat command sticks to backtracking the thread that you started on, which I can only test with more than one thread. I also need to test that when you list TWO threads, the repeat command continues listing both threads, rather than arbitrarily picking one. Again for that purpose I need two threads. I could make two tests one with one thread and one with two, but that seems a bit silly.

BTW, the test I copied this from tries to do some "step out's" after stopping where this test stops. I don't know (yet) whether the expected failures are in the initial rendezvous or in the stepping. I'll only be able to figure that out once the test is running on those systems... For instance, I bet this won't fail on FreeBSD because I don't run the process to exit, I just kill it at the breakpoint.

[jingham]

Ran clang-format over the diff

Added a simpler test that doesn't try to rendezvous the two threads so they are in known states, but just does a repeated backtrace on the first thread that hits the breakpoint. Hopefully the simpler test will run on more platforms.

[JDevlieghere]

Thanks Jim. LGTM.

[labath]

I've fixed the synchronization code in f8d42c55ec6e9. Based on my experiments, lldb reports still reports a stop reason for suspended threads (regardless of the platform), so the code should expect continue_to_breakpoint to return two threads when it is synchronizing them.

lldb/include/lldb/Interpreter/CommandObject.h

   ///    The command arguments.
   ///
   /// \return
-  ///     nullptr if there is no special repeat command - it will use the
+  ///     llvm::None if there is no special repeat command - it will use the
   ///     current command line.
-  ///     Otherwise a pointer to the command to be repeated.
-  ///     If the returned string is the empty string, the command won't be
-  ///     repeated.
-  virtual const char *GetRepeatCommand(Args &current_command_args,
-                                       uint32_t index) {
-    return nullptr;
+  ///     Otherwise a std::string containing the command to be repeated.
+  ///     If the string is empty, the command won't be allow repeating.
+  virtual llvm::Optional<std::string>
+  GetRepeatCommand(Args &current_command_args, uint32_t index) {
+    return llvm::None;
   }
 
   bool HasOverrideCallback() const {

lldb/include/lldb/Interpreter/CommandObjectMultiword.h

 
   void HandleCompletion(CompletionRequest &request) override;
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override;
 
   bool Execute(const char *args_string, CommandReturnObject &result) override;
 

   HandleArgumentCompletion(CompletionRequest &request,
                            OptionElementVector &opt_element_vector) override;
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override;
 
   /// \return
   ///     An error message to be displayed when the command is executed (i.e.

lldb/source/API/SBCommandInterpreter.cpp

   bool IsRemovable() const override { return true; }
 
   /// More documentation is available in lldb::CommandObject::GetRepeatCommand,
-  /// but in short, if nullptr is returned, the previous command will be
+  /// but in short, if llvm::None is returned, the previous command will be
   /// repeated, and if an empty string is returned, no commands will be
   /// executed.
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override {
     if (!m_auto_repeat_command)
-      return nullptr;
+      return llvm::None;
     else
-      return m_auto_repeat_command->c_str();
+      return m_auto_repeat_command;
   }
 
 protected:

lldb/source/Breakpoint/BreakpointLocationCollection.cpp

   size_t i = 0;
   size_t prev_size = GetSize();
   while (i < prev_size) {
-    // ShouldStop can remove the breakpoint from the list
-    if (GetByIndex(i)->ShouldStop(context))
+    // ShouldStop can remove the breakpoint from the list, or even delete
+    // it, so we should 
+    BreakpointLocationSP cur_loc_sp = GetByIndex(i);
+    BreakpointSP keep_bkpt_alive_sp = cur_loc_sp->GetBreakpoint().shared_from_this();
+    if (cur_loc_sp->ShouldStop(context))
       shouldStop = true;
 
     if (prev_size == GetSize())

lldb/source/Commands/CommandObjectCommands.cpp

 
   ~CommandObjectCommandsSource() override = default;
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override {
-    return "";
+    return std::string("");
   }
 
   void

lldb/source/Commands/CommandObjectMemory.cpp

 
   Options *GetOptions() override { return &m_option_group; }
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override {
-    return m_cmd_name.c_str();
+    return m_cmd_name;
   }
 
 protected:

 
   ~CommandObjectMemoryHistory() override = default;
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override {
-    return m_cmd_name.c_str();
+    return m_cmd_name;
   }
 
 protected:

     return false;
   }
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override {
     // If we repeat this command, repeat it without any arguments so we can
     // show the next memory range
-    return m_cmd_name.c_str();
+    return m_cmd_name;
   }
 
   lldb::addr_t m_prev_end_addr;

lldb/source/Commands/CommandObjectMultiword.cpp

   sub_command_object->HandleCompletion(request);
 }
 
-const char *CommandObjectMultiword::GetRepeatCommand(Args &current_command_args,
-                                                     uint32_t index) {
+llvm::Optional<std::string>
+CommandObjectMultiword::GetRepeatCommand(Args &current_command_args,
+                                         uint32_t index) {
   index++;
   if (current_command_args.GetArgumentCount() <= index)
-    return nullptr;
+    return llvm::None;
   CommandObject *sub_command_object =
       GetSubcommandObject(current_command_args[index].ref());
   if (sub_command_object == nullptr)
-    return nullptr;
+    return llvm::None;
   return sub_command_object->GetRepeatCommand(current_command_args, index);
 }
 

     proxy_command->HandleArgumentCompletion(request, opt_element_vector);
 }
 
-const char *CommandObjectProxy::GetRepeatCommand(Args &current_command_args,
-                                                 uint32_t index) {
+llvm::Optional<std::string>
+CommandObjectProxy::GetRepeatCommand(Args &current_command_args,
+                                     uint32_t index) {
   CommandObject *proxy_command = GetProxyCommandObject();
   if (proxy_command)
     return proxy_command->GetRepeatCommand(current_command_args, index);
-  return nullptr;
+  return llvm::None;
 }
 
 llvm::StringRef CommandObjectProxy::GetUnsupportedError() {

lldb/source/Commands/CommandObjectProcess.cpp

 
   Options *GetOptions() override { return &m_all_options; }
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override {
     // No repeat for "process launch"...
-    return "";
+    return std::string("");
   }
 
 protected:

lldb/source/Commands/CommandObjectSource.cpp

 
   Options *GetOptions() override { return &m_options; }
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override {
     // This is kind of gross, but the command hasn't been parsed yet so we
     // can't look at the option values for this invocation...  I have to scan
     // the arguments directly.

           return e.ref() == "-r" || e.ref() == "--reverse";
         });
     if (iter == current_command_args.end())
-      return m_cmd_name.c_str();
+      return m_cmd_name;
 
     if (m_reverse_name.empty()) {
       m_reverse_name = m_cmd_name;
       m_reverse_name.append(" -r");
     }
-    return m_reverse_name.c_str();
+    return m_reverse_name;
   }
 
 protected:

lldb/source/Commands/CommandObjectThread.cpp

 
   Options *GetOptions() override { return &m_options; }
 
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_args,
+                                               uint32_t idx) override {
+    llvm::StringRef count_opt("--count");
+    llvm::StringRef start_opt("--start");
+
+    // If no "count" was provided, we are dumping the entire backtrace, so
+    // there isn't a repeat command.  So we search for the count option in
+    // the args, and if we find it, we make a copy and insert or modify the
+    // start option's value to start count indices greater.
+
+    Args copy_args(current_args);
+    size_t num_entries = copy_args.GetArgumentCount();
+    // These two point at the index of the option value if found.
+    size_t count_idx = 0;
+    size_t start_idx = 0;
+    size_t count_val = 0;
+    size_t start_val = 0;
+
+    for (size_t idx = 0; idx < num_entries; idx++) {
+      llvm::StringRef arg_string = copy_args[idx].ref();
+      if (arg_string.equals("-c") || count_opt.startswith(arg_string)) {
+        idx++;
+        if (idx == num_entries)
+          return llvm::None;
+        count_idx = idx;
+        if (copy_args[idx].ref().getAsInteger(0, count_val))
+          return llvm::None;
+      } else if (arg_string.equals("-s") || start_opt.startswith(arg_string)) {
+        idx++;
+        if (idx == num_entries)
+          return llvm::None;
+        start_idx = idx;
+        if (copy_args[idx].ref().getAsInteger(0, start_val))
+          return llvm::None;
+      }
+    }
+    if (count_idx == 0)
+      return llvm::None;
+
+    std::string new_start_val = llvm::formatv("{0}", start_val + count_val);
+    if (start_idx == 0) {
+      copy_args.AppendArgument(start_opt);
+      copy_args.AppendArgument(new_start_val);
+    } else {
+      copy_args.ReplaceArgumentAtIndex(start_idx, new_start_val);
+    }
+    std::string repeat_command;
+    if (!copy_args.GetQuotedCommandString(repeat_command))
+      return llvm::None;
+    return repeat_command;
+  }
+
 protected:
   void DoExtendedBacktrace(Thread *thread, CommandReturnObject &result) {
     SystemRuntime *runtime = thread->GetProcess()->GetSystemRuntime();

         uint32_t index_ptr = 0, end_ptr;
         std::vector<addr_t> address_list;
 
-        // Find the beginning & end index of the
+        // Find the beginning & end index of the        
         AddressRange fun_addr_range = sc.function->GetAddressRange();
         Address fun_start_addr = fun_addr_range.GetBaseAddress();
         line_table->FindLineEntryByAddress(fun_start_addr, function_start,

 
   Options *GetOptions() override { return &m_options; }
 
-  const char *GetRepeatCommand(Args &current_command_args,
+  llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
                                                uint32_t index) override {
     current_command_args.GetCommandString(m_repeat_command);
     m_create_repeat_command_just_invoked = true;
-    return m_repeat_command.c_str();
+    return m_repeat_command;
   }
 
 protected:

lldb/source/Interpreter/CommandInterpreter.cpp

   // arguments.
 
   if (cmd_obj != nullptr) {
-    if (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) {
       Args command_args(command_string);
-      const char *repeat_command = cmd_obj->GetRepeatCommand(command_args, 0);
-      if (repeat_command != nullptr)
-        m_repeat_command.assign(repeat_command);
+      llvm::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 (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();

lldb/test/API/commands/thread/backtrace/Makefile

+CXX_SOURCES := main.cpp
+ENABLE_THREADS := YES
+
+include Makefile.rules

lldb/test/API/commands/thread/backtrace/TestThreadBacktraceRepeat.py

+"""
+Test that we page getting a long backtrace on more than one thread
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestThreadBacktracePage(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_thread_backtrace_one_thread(self):
+        """Run a simplified version of the test that just hits one breakpoint and
+           doesn't care about synchronizing the two threads - hopefully this will
+           run on more systems."""
+
+    def test_thread_backtrace_one_thread(self):
+        self.build()
+        (self.inferior_target, self.process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+            self, self.bkpt_string, lldb.SBFileSpec('main.cpp'), only_one_thread = False)
+
+        # We hit the breakpoint on at least one thread.  If we hit it on both threads
+        # simultaneously, we are ready to run our tests.  Otherwise, suspend the thread
+        # that hit the breakpoint, and continue till the second thread hits
+        # the breakpoint:
+
+        (breakpoint_threads, other_threads) = ([], [])
+        lldbutil.sort_stopped_threads(self.process,
+                                      breakpoint_threads=breakpoint_threads,
+                                      other_threads=other_threads)
+        self.assertGreater(len(breakpoint_threads), 0, "We hit at least one breakpoint thread")
+        self.assertGreater(len(breakpoint_threads[0].frames), 2, "I can go up")
+        thread_id = breakpoint_threads[0].idx
+        name = breakpoint_threads[0].frame[1].name.split("(")[0]
+        self.check_one_thread(thread_id, name)
+        
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+        # Find the line number for our breakpoint.
+        self.bkpt_string = '// Set breakpoint here'
+
+    def check_one_thread(self, thread_id, func_name):
+        # Now issue some thread backtrace commands and make sure they
+        # get the right answer back.
+        interp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+
+        # Run the real backtrace, remember to pass True for add_to_history since
+        # we don't generate repeat commands for commands that aren't going into the history.
+        interp.HandleCommand("thread backtrace --count 10 {0}".format(thread_id), result, True)
+        self.assertTrue(result.Succeeded(), "bt with count succeeded")
+        # There should be 11 lines:
+        lines = result.GetOutput().splitlines()
+        self.assertEqual(len(lines), 11, "Got the right number of lines")
+        # First frame is stop_here:
+        self.assertNotEqual(lines[1].find("stop_here"), -1, "Found Stop Here")
+        for line in lines[2:10]:
+            self.assertNotEqual(line.find(func_name), -1, "Name {0} not found in line: {1}".format(func_name, line))
+        # The last entry should be 43:
+        self.assertNotEqual(lines[10].find("count=43"), -1, "First show ends at 43")
+            
+        # Now try a repeat, and make sure we get 10 more on this thread:
+        #import pdb; pdb.set_trace()
+        interp.HandleCommand("", result, True)
+        self.assertTrue(result.Succeeded(), "repeat command failed: {0}".format(result.GetError()))
+        lines = result.GetOutput().splitlines()
+        self.assertEqual(len(lines), 11, "Repeat got 11 lines")
+        # Every line should now be the recurse function:
+        for line in lines[1:10]:
+            self.assertNotEqual(line.find(func_name), -1, "Name in every line")
+        self.assertNotEqual(lines[10].find("count=33"), -1, "Last one is now 33")
+
+    def check_two_threads(self, result_str, thread_id_1, name_1, thread_id_2, name_2, start_idx, end_idx):
+        # We should have 2 occurrences ot the thread header:
+        self.assertEqual(result_str.count("thread #{0}".format(thread_id_1)), 1, "One for thread 1")
+        self.assertEqual(result_str.count("thread #{0}".format(thread_id_2)), 1, "One for thread 2")
+        # We should have 10 occurrences of each name:
+        self.assertEqual(result_str.count(name_1), 10, "Found 10 of {0}".format(name_1))
+        self.assertEqual(result_str.count(name_2), 10, "Found 10 of {0}".format(name_1))
+        # There should be two instances of count=<start_idx> and none of count=<start-1>:
+        self.assertEqual(result_str.count("count={0}".format(start_idx)), 2, "Two instances of start_idx")
+        self.assertEqual(result_str.count("count={0}".format(start_idx-1)), 0, "No instances of start_idx - 1")
+        # There should be two instances of count=<end_idx> and none of count=<end_idx+1>:
+        self.assertEqual(result_str.count("count={0}".format(end_idx)), 2, "Two instances of end_idx")
+        self.assertEqual(result_str.count("count={0}".format(end_idx+1)), 0, "No instances after end idx")
+
+    # The setup of this test was copied from the step-out test, and I can't tell from
+    # the comments whether it was getting two threads to the same breakpoint that was
+    # problematic, or the step-out part.  This test stops at the rendevous point so I'm
+    # removing the skipIfLinux to see if we see any flakiness in just this part of the test.
+    @expectedFailureAll(
+        oslist=["linux"],
+        bugnumber="llvm.org/pr23477 Test occasionally times out on the Linux build bot")
+    @expectedFailureAll(
+        oslist=["freebsd"],
+        bugnumber="llvm.org/pr18066 inferior does not exit")
+    @skipIfWindows # This test will hang on windows llvm.org/pr21753
+    @expectedFailureAll(oslist=["windows"])
+    @expectedFailureNetBSD
+    def test_thread_backtrace_two_threads(self):
+        """Test that repeat works even when backtracing on more than one thread."""
+        self.build()
+        (self.inferior_target, self.process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+            self, self.bkpt_string, lldb.SBFileSpec('main.cpp'), only_one_thread = False)
+
+        # We hit the breakpoint on at least one thread.  If we hit it on both threads
+        # simultaneously, we are ready to run our tests.  Otherwise, suspend the thread
+        # that hit the breakpoint, and continue till the second thread hits
+        # the breakpoint:
+
+        (breakpoint_threads, other_threads) = ([], [])
+        lldbutil.sort_stopped_threads(self.process,
+                                      breakpoint_threads=breakpoint_threads,
+                                      other_threads=other_threads)
+        if len(breakpoint_threads) == 1:
+            success = thread.Suspend()
+            self.assertTrue(success, "Couldn't suspend a thread")
+            bkpt_threads = lldbutil.continue_to_breakpoint(self.process,
+                                                           bkpt)
+            self.assertEqual(len(bkpt_threads), 1, "Second thread stopped")
+            breakpoint_threads.append(bkpt_threads[0])
+
+        # Figure out which thread is which:
+        thread_id_1 = breakpoint_threads[0].idx
+        self.assertGreater(len(breakpoint_threads[0].frames), 2, "I can go up")
+        name_1 = breakpoint_threads[0].frame[1].name.split("(")[0]
+
+        thread_id_2 = breakpoint_threads[1].idx
+        self.assertGreater(len(breakpoint_threads[1].frames), 2, "I can go up")
+        name_2 = breakpoint_threads[1].frame[1].name.split("(")[0]
+
+        # Check that backtrace and repeat works on one thread, then works on the second
+        # when we switch to it:
+        self.check_one_thread(thread_id_1, name_1)
+        self.check_one_thread(thread_id_2, name_2)
+
+        # The output is looking right at this point, let's just do a couple more quick checks
+        # to see we handle two threads and a start count:
+        interp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+
+        interp.HandleCommand("thread backtrace --count 10 --start 10 {0} {1}".format(thread_id_1, thread_id_2), result, True)
+        self.assertTrue(result.Succeeded(), "command succeeded for two threads")
+
+        result.Clear()
+        interp.HandleCommand("", result, True)
+        self.assertTrue(result.Succeeded(), "repeat command succeeded for two threads")
+        result_str = result.GetOutput()
+        self.check_two_threads(result_str, thread_id_1, name_1, thread_id_2, name_2, 23, 32)
+
+        # Finally make sure the repeat repeats:
+        result.Clear()
+        interp.HandleCommand("", result, True)
+        self.assertTrue(result.Succeeded(), "repeat command succeeded for two threads")
+        result_str = result.GetOutput()
+        self.check_two_threads(result_str, thread_id_1, name_1, thread_id_2, name_2, 13, 22)
+
+        
+
+        

lldb/test/API/commands/thread/backtrace/main.cpp

+// This test is intended to create a situation in which two threads are stopped
+// at a breakpoint and the debugger issues a step-out command.
+
+#include "pseudo_barrier.h"
+#include <thread>
+
+pseudo_barrier_t g_barrier;
+
+volatile int g_test = 0;
+
+void stop_here() {
+  g_test += 5; // Set breakpoint here
+}
+
+void recurse_a_bit_1(int count) {
+  if (count == 50)
+    stop_here();
+  else
+    recurse_a_bit_1(++count);
+}
+
+void recurse_a_bit_2(int count) {
+  if (count == 50)
+    stop_here();
+  else
+    recurse_a_bit_2(++count);
+}
+
+void *thread_func_1() {
+  // Wait until both threads are running
+  pseudo_barrier_wait(g_barrier);
+
+  // Start the recursion:
+  recurse_a_bit_1(0);
+
+  // Return
+  return NULL;
+}
+
+void *thread_func_2() {
+  // Wait until both threads are running
+  pseudo_barrier_wait(g_barrier);
+
+  // Start the recursion:
+  recurse_a_bit_2(0);
+
+  // Return
+  return NULL;
+}
+
+int main() {
+  // Don't let either thread do anything until they're both ready.
+  pseudo_barrier_init(g_barrier, 2);
+
+  // Create two threads
+  std::thread thread_1(thread_func_1);
+  std::thread thread_2(thread_func_2);
+
+  // Wait for the threads to finish
+  thread_1.join();
+  thread_2.join();
+
+  return 0;
+}