[lldb-server] Reset stop reason of all threads when resuming

[jarin]

Summary

This patch makes the stop reason reset logic similar to MacOS' debugserver, where exceptions are reset for all threads when resuming process for stepping or continuing (see MachThreadList::ProcessWillResume and MachThread::ThreadWillResume).

Resetting stop reasons on resume fixes problems where LLDB spuriously reports SIGTRAP signal stop reason for deleted breakpoints (both internal and public) and where LLDB stops on an internal breakpoint while stepping over while a breakpoint is hit in another thread. See PR45642 for details.

[labath]

The test setup here seems unnecessarily complex. Wouldn't an inferior like this work better?

void thread1() {
  pseudo_barrier_wait(g_barrier); // See other tests how this works.
  g_foo = 0; // break_here
}
int main() {
  pseudo_barrier_init(g_barrier1, 2);
  std::thread t1(thread1);
  pseudo_barrier_wait(g_barrier);
  for (int i = 0; i<10000; ++i) g_bar = i; // empty loop to have something to step over
  t1.join();
}

That way you always know only one thread will hit a breakpoint, and and you can just pick the "other" thread as the target for stepping.

[jarin]

In D79308#2017348, @labath wrote:

The test setup here seems unnecessarily complex. Wouldn't an inferior like this work better?

void thread1() {
  pseudo_barrier_wait(g_barrier); // See other tests how this works.
  g_foo = 0; // break_here
}
int main() {
  pseudo_barrier_init(g_barrier1, 2);
  std::thread t1(thread1);
  pseudo_barrier_wait(g_barrier);
  for (int i = 0; i<10000; ++i) g_bar = i; // empty loop to have something to step over
  t1.join();
}

That way you always know only one thread will hit a breakpoint, and and you can just pick the "other" thread as the target for stepping.

Yeah, I considered something like that, but then I thought it would be better if the "other" thread only runs code that we completely control. In my patch, the "other" thread is guaranteed to be in thread_func after we hit the breakpoint. In your suggested inferior, it could be still in pseudo_barrier_wait. If you feel stepping in external code is safe, I am happy to rewrite the test to the simpler version.

[jarin]

Simplify the test based on the suggestion from labath@.

[labath]

In D79308#2017377, @jarin wrote:

Yeah, I considered something like that, but then I thought it would be better if the "other" thread only runs code that we completely control. In my patch, the "other" thread is guaranteed to be in thread_func after we hit the breakpoint. In your suggested inferior, it could be still in pseudo_barrier_wait. If you feel stepping in external code is safe, I am happy to rewrite the test to the simpler version.

The main reason that "pseudo_barrier_wait" even exists is so that we can drive multiple test threads to very precise points in the code, so I wouldn't really call it "external" code. If we were doing something more complicated to the thread (like playing with the line numbers for instance, then I might get worried, but given that all we need is to do an instruction step, I think this is perfectly safe, and a lot more understandable than the previous version.

lldb/test/API/functionalities/thread/break_step_other/TestThreadBreakStepOther.py

22

You can add NO_DEBUG_INFO_TESTCASE = True here.

lldb/test/API/functionalities/thread/break_step_other/main.cpp

24–26

Just a small tweak to ensure the inferior exits if anything happens to the debugger (e.g., it crashes)

volatile int i = 0;
while (g_foo == 0)
  ++i;
t.join();

(and change g_foo = 0 to g_foo = 1 in thread_func above).

[jarin]

Addressed reviewer comments.

[jarin]

... and remove the extra braces.

[jarin]

... now also fixed the 'volatile'. It took only three patches to copy four lines of code by hand. Not bad, huh?

[jarin]

lldb/test/API/functionalities/thread/break_step_other/TestThreadBreakStepOther.py

22

Out of curiosity, what does that do?

[labath]

lldb/test/API/functionalities/thread/break_step_other/TestThreadBreakStepOther.py

22

It prevents the test suite from forking the test for different debug info "formats" ("regular dwarf", split dwarf, dsym, etc.).
That does not seem relevant/useful here as this test is mainly about the stop reason machinery and debug info is only used incidentally.

[jarin]

Jim, do you think this is good to go?

[jingham]

LGTM too!

[jarin]

Pavel, could you possibly land this for us?

[labath]

Sure.

Btw, at this point, I think one of you guys should get commit-after-aproval access to the repository, so you can handle these things yourselves.

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

for (const auto &thread : m_threads) {
     assert(thread && "thread list should not contain NULL threads");
 
     const ResumeAction *const action =
         resume_actions.GetActionForThread(thread->GetID(), true);
 
     if (action == nullptr) {
       LLDB_LOG(log, "no action specified for pid {0} tid {1}", GetID(),
                thread->GetID());
+      // Make sure we reset the stop reason for all the threads.
+      static_cast<NativeThreadLinux &>(*thread).ResetStopReason();
       continue;
     }
 
     LLDB_LOG(log, "processing resume action state {0} for pid {1} tid {2}",
              action->state, GetID(), thread->GetID());
 
     switch (action->state) {
     case eStateRunning:

lldb/source/Plugins/Process/Linux/NativeThreadLinux.h

private:
 
   // Private interface
   void MaybeLogStateChange(lldb::StateType new_state);
 
   NativeProcessLinux &GetProcess();
 
   void SetStopped();
 
+  void ResetStopReason();
+
   // Member Variables
   lldb::StateType m_state;
   ThreadStopInfo m_stop_info;
   std::unique_ptr<NativeRegisterContextLinux> m_reg_context_up;
   std::string m_stop_description;
   using WatchpointIndexMap = std::map<lldb::addr_t, uint32_t>;
   WatchpointIndexMap m_watchpoint_index_map;
   WatchpointIndexMap m_hw_break_index_map;
   std::unique_ptr<SingleStepWorkaround> m_step_workaround;
 };
 } // namespace process_linux
 } // namespace lldb_private
 
 #endif // #ifndef liblldb_NativeThreadLinux_H_

lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp

void NativeThreadLinux::SetStoppedByTrace() {
   SetStopped();
 
   m_stop_info.reason = StopReason::eStopReasonTrace;
   m_stop_info.details.signal.signo = SIGTRAP;
 }
 
 void NativeThreadLinux::SetStoppedWithNoReason() {
   SetStopped();
+  ResetStopReason();
+}
 
+void NativeThreadLinux::ResetStopReason() {
   m_stop_info.reason = StopReason::eStopReasonNone;
   m_stop_info.details.signal.signo = 0;
 }
 
 void NativeThreadLinux::SetExited() {
   const StateType new_state = StateType::eStateExited;
   MaybeLogStateChange(new_state);
   m_state = new_state;

lldb/test/API/functionalities/thread/break_step_other/Makefile

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

lldb/test/API/functionalities/thread/break_step_other/TestThreadBreakStepOther.py

+"""
+Test stop reasons after hitting and deleting a breakpoint and
+stepping another thread. Scenario:
+  - run a thread
+  - stop the thread at a breakpoint
+  - delete the breakpoint
+  - single step on the main thread
+The thread stopped at the deleted breakpoint should have stop reason
+'none'.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ThreadBreakStepOtherTestCase(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_hit_breakpoint_delete_step_other_thread(self):
+        main_source_file = lldb.SBFileSpec("main.cpp")
+        self.build()
+        (target, process, main_thread, _) = lldbutil.run_to_source_breakpoint(
+            self, "// main break here", main_source_file, only_one_thread = False)
+
+        # Run until the breakpoint in the thread.
+        thread_breakpoint = target.BreakpointCreateBySourceRegex(
+            "// thread break here", main_source_file)
+        self.assertGreater(
+            thread_breakpoint.GetNumLocations(),
+            0,
+            "thread breakpoint has no locations associated with it.")
+        process.Continue()
+        stopped_threads = lldbutil.get_threads_stopped_at_breakpoint(
+            process, thread_breakpoint)
+        self.assertEquals(
+            1,
+            len(stopped_threads),
+            "only one thread expected stopped at the thread breakpoint")
+        breakpoint_thread = stopped_threads[0]
+
+        # Delete the breakpint in the thread and do a step in the main thread.
+        target.BreakpointDelete(thread_breakpoint.GetID())
+        main_thread.StepInstruction(False)
+
+        # Check the stop reasons.
+        reason = main_thread.GetStopReason()
+        self.assertEqual(
+            lldb.eStopReasonPlanComplete,
+            reason,
+            "Expected thread stop reason 'plancomplete', but got '%s'" %
+            lldbutil.stop_reason_to_str(reason))
+
+        reason = breakpoint_thread.GetStopReason()
+        self.assertEqual(
+            lldb.eStopReasonNone,
+            reason,
+            "Expected thread stop reason 'none', but got '%s'" %
+            lldbutil.stop_reason_to_str(reason))

lldb/test/API/functionalities/thread/break_step_other/main.cpp

+#include <thread>
+#include "pseudo_barrier.h"
+
+// Barrier for starting the thread and reaching the loop in main.
+pseudo_barrier_t g_barrier;
+volatile int g_foo = 0;
+
+void thread_func() {
+  // Wait until all the threads are running
+  pseudo_barrier_wait(g_barrier);
+  g_foo = 1; // thread break here
+}
+
+int main() {
+  g_foo = 0; // main break here
+
+  pseudo_barrier_init(g_barrier, 2);
+  std::thread t(thread_func);
+  pseudo_barrier_wait(g_barrier);
+
+  // A dummy loop to have something to step through.
+  volatile int i = 0;
+  while (g_foo == 0)
+    ++i;
+  t.join();
+  return 0;
+}