Merge pull request #8274 from jasonmolenda/cp/fix-flakey-TestConcurrent-tests

Cp/fix flakey test concurrent tests

Author: jasonmolenda

lldb/include/lldb/Target/StopInfo.h

@@ -79,6 +79,11 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> {
 
   virtual bool IsValidForOperatingSystemThread(Thread &thread) { return true; }
 
+  /// A Continue operation can result in a false stop event
+  /// before any execution has happened. We need to detect this
+  /// and silently continue again one more time.
+  virtual bool WasContinueInterrupted(Thread &thread) { return false; }
+
   // Sometimes the thread plan logic will know that it wants a given stop to
   // stop or not, regardless of what the ordinary logic for that StopInfo would
   // dictate.  The main example of this is the ThreadPlanCallFunction, which

lldb/include/lldb/Target/Thread.h

@@ -11,6 +11,7 @@
 
 #include <memory>
 #include <mutex>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -1187,13 +1188,20 @@ class Thread : public std::enable_shared_from_this<Thread>,
 
   void CalculatePublicStopInfo();
 
-  // Ask the thread subclass to set its stop info.
-  //
-  // Thread subclasses should call Thread::SetStopInfo(...) with the reason the
-  // thread stopped.
-  //
-  // \return
-  //      True if Thread::SetStopInfo(...) was called, false otherwise.
+  /// Ask the thread subclass to set its stop info.
+  ///
+  /// Thread subclasses should call Thread::SetStopInfo(...) with the reason the
+  /// thread stopped.
+  ///
+  /// A thread that is sitting at a breakpoint site, but has not yet executed
+  /// the breakpoint instruction, should have a breakpoint-hit StopInfo set.
+  /// When execution is resumed, any thread sitting at a breakpoint site will
+  /// instruction-step over the breakpoint instruction silently, and we will
+  /// never record this breakpoint as being hit, updating the hit count,
+  /// possibly executing breakpoint commands or conditions.
+  ///
+  /// \return
+  ///      True if Thread::SetStopInfo(...) was called, false otherwise.
   virtual bool CalculateStopInfo() = 0;
 
   // Gets the temporary resume state for a thread.
@@ -1251,6 +1259,16 @@ class Thread : public std::enable_shared_from_this<Thread>,
 
   lldb::ValueObjectSP GetSiginfoValue();
 
+  /// Request the pc value the thread had when previously stopped.
+  ///
+  /// When the thread performs execution, it copies the current RegisterContext
+  /// GetPC() value.  This method returns that value, if it is available.
+  ///
+  /// \return
+  ///     The PC value before execution was resumed.  May not be available;
+  ///     an empty std::optional is returned in that case.
+  std::optional<lldb::addr_t> GetPreviousFrameZeroPC();
+
 protected:
   friend class ThreadPlan;
   friend class ThreadList;
@@ -1331,6 +1349,9 @@ class Thread : public std::enable_shared_from_this<Thread>,
                                            ///populated after a thread stops.
   lldb::StackFrameListSP m_prev_frames_sp; ///< The previous stack frames from
                                            ///the last time this thread stopped.
+  std::optional<lldb::addr_t>
+      m_prev_framezero_pc; ///< Frame 0's PC the last
+                           /// time this thread was stopped.
   int m_resume_signal; ///< The signal that should be used when continuing this
                        ///thread.
   lldb::StateType m_resume_state; ///< This state is used to force a thread to

lldb/packages/Python/lldbsuite/test/concurrent_base.py

@@ -264,12 +264,40 @@ def do_thread_actions(
                 "Expected main thread (finish) breakpoint to be hit once",
             )
 
-            num_threads = self.inferior_process.GetNumThreads()
+            # There should be a single active thread (the main one) which hit
+            # the breakpoint after joining.  Depending on the pthread
+            # implementation we may have a worker thread finishing the pthread_join()
+            # after it has returned.  Filter the threads to only count those
+            # with user functions on them from our test case file,
+            # lldb/test/API/functionalities/thread/concurrent_events/main.cpp
+            user_code_funcnames = [
+                "breakpoint_func",
+                "crash_func",
+                "do_action_args",
+                "dotest",
+                "main",
+                "register_signal_handler",
+                "signal_func",
+                "sigusr1_handler",
+                "start_threads",
+                "watchpoint_func",
+            ]
+            num_threads_with_usercode = 0
+            for t in self.inferior_process.threads:
+                thread_has_user_code = False
+                for f in t.frames:
+                    for funcname in user_code_funcnames:
+                        if funcname in f.GetDisplayFunctionName():
+                            thread_has_user_code = True
+                            break
+                if thread_has_user_code:
+                    num_threads_with_usercode += 1
+
             self.assertEqual(
                 1,
-                num_threads,
+                num_threads_with_usercode,
                 "Expecting 1 thread but seeing %d. Details:%s"
-                % (num_threads, "\n\t".join(self.describe_threads())),
+                % (num_threads_with_usercode, "\n\t".join(self.describe_threads())),
             )
             self.runCmd("continue")
 

lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp

@@ -26,6 +26,8 @@
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadPlan.h"
 #include "lldb/Target/UnixSignals.h"
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 #include <optional>
 
@@ -600,6 +602,7 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
   if (exc_type == 0)
     return StopInfoSP();
 
+  bool not_stepping_but_got_singlestep_exception = false;
   uint32_t pc_decrement = 0;
   ExecutionContext exe_ctx(thread.shared_from_this());
   Target *target = exe_ctx.GetTargetPtr();
@@ -729,30 +732,8 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
         // is set
         is_actual_breakpoint = true;
         is_trace_if_actual_breakpoint_missing = true;
-#ifndef NDEBUG
-        if (thread.GetTemporaryResumeState() != eStateStepping) {
-          StreamString s;
-          s.Printf("CreateStopReasonWithMachException got EXC_BREAKPOINT [1,0] "
-                   "indicating trace event, but thread is not tracing, it has "
-                   "ResumeState %d",
-                   thread.GetTemporaryResumeState());
-          if (RegisterContextSP regctx = thread.GetRegisterContext()) {
-            if (const RegisterInfo *ri = regctx->GetRegisterInfoByName("esr")) {
-              uint32_t esr =
-                  (uint32_t)regctx->ReadRegisterAsUnsigned(ri, UINT32_MAX);
-              if (esr != UINT32_MAX) {
-                s.Printf(" esr value: 0x%" PRIx32, esr);
-              }
-            }
-          }
-          thread.GetProcess()->DumpPluginHistory(s);
-          llvm::report_fatal_error(s.GetData());
-          lldbassert(
-              false &&
-              "CreateStopReasonWithMachException got EXC_BREAKPOINT [1,0] "
-              "indicating trace event, but thread was not doing a step.");
-        }
-#endif
+        if (thread.GetTemporaryResumeState() != eStateStepping)
+          not_stepping_but_got_singlestep_exception = true;
       }
       if (exc_code == 0x102) // EXC_ARM_DA_DEBUG
       {
@@ -839,6 +820,56 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
     break;
   }
 
-  return StopInfoSP(new StopInfoMachException(thread, exc_type, exc_data_count,
-                                              exc_code, exc_sub_code));
+  return std::make_shared<StopInfoMachException>(
+      thread, exc_type, exc_data_count, exc_code, exc_sub_code,
+      not_stepping_but_got_singlestep_exception);
+}
+
+// Detect an unusual situation on Darwin where:
+//
+//   0. We did an instruction-step before this.
+//   1. We have a hardware breakpoint or watchpoint set.
+//   2. We resumed the process, but not with an instruction-step.
+//   3. The thread gets an "instruction-step completed" mach exception.
+//   4. The pc has not advanced - it is the same as before.
+//
+// This method returns true for that combination of events.
+bool StopInfoMachException::WasContinueInterrupted(Thread &thread) {
+  Log *log = GetLog(LLDBLog::Step);
+
+  // We got an instruction-step completed mach exception but we were not
+  // doing an instruction step on this thread.
+  if (!m_not_stepping_but_got_singlestep_exception)
+    return false;
+
+  RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
+  std::optional<addr_t> prev_pc = thread.GetPreviousFrameZeroPC();
+  if (!reg_ctx_sp || !prev_pc)
+    return false;
+
+  // The previous pc value and current pc value are the same.
+  if (*prev_pc != reg_ctx_sp->GetPC())
+    return false;
+
+  // We have a watchpoint -- this is the kernel bug.
+  ProcessSP process_sp = thread.GetProcess();
+  if (process_sp->GetWatchpointResourceList().GetSize()) {
+    LLDB_LOGF(log,
+              "Thread stopped with insn-step completed mach exception but "
+              "thread was not stepping; there is a hardware watchpoint set.");
+    return true;
+  }
+
+  // We have a hardware breakpoint -- this is the kernel bug.
+  auto &bp_site_list = process_sp->GetBreakpointSiteList();
+  for (auto &site : bp_site_list.Sites()) {
+    if (site->IsHardware() && site->IsEnabled()) {
+      LLDB_LOGF(log,
+                "Thread stopped with insn-step completed mach exception but "
+                "thread was not stepping; there is a hardware breakpoint set.");
+      return true;
+    }
+  }
+
+  return false;
 }

lldb/source/Plugins/Process/Utility/StopInfoMachException.h

@@ -31,9 +31,12 @@ class StopInfoMachException : public StopInfo {
   // Constructors and Destructors
   StopInfoMachException(Thread &thread, uint32_t exc_type,
                         uint32_t exc_data_count, uint64_t exc_code,
-                        uint64_t exc_subcode)
+                        uint64_t exc_subcode,
+                        bool not_stepping_but_got_singlestep_exception)
       : StopInfo(thread, exc_type), m_exc_data_count(exc_data_count),
-        m_exc_code(exc_code), m_exc_subcode(exc_subcode) {}
+        m_exc_code(exc_code), m_exc_subcode(exc_subcode),
+        m_not_stepping_but_got_singlestep_exception(
+            not_stepping_but_got_singlestep_exception) {}
 
   ~StopInfoMachException() override = default;
 
@@ -58,10 +61,14 @@ class StopInfoMachException : public StopInfo {
       uint64_t exc_code, uint64_t exc_sub_code, uint64_t exc_sub_sub_code,
       bool pc_already_adjusted = true, bool adjust_pc_if_needed = false);
 
+  bool WasContinueInterrupted(Thread &thread) override;
+
 protected:
   uint32_t m_exc_data_count;
   uint64_t m_exc_code;
   uint64_t m_exc_subcode;
+
+  bool m_not_stepping_but_got_singlestep_exception;
 };
 
 } // namespace lldb_private

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

@@ -1601,6 +1601,26 @@ bool ProcessGDBRemote::CalculateThreadStopInfo(ThreadGDBRemote *thread) {
     // has no stop reason.
     thread->GetRegisterContext()->InvalidateIfNeeded(true);
     if (!GetThreadStopInfoFromJSON(thread, m_jstopinfo_sp)) {
+      // If a thread is stopped at a breakpoint site, set that as the stop
+      // reason even if it hasn't executed the breakpoint instruction yet.
+      // We will silently step over the breakpoint when we resume execution
+      // and miss the fact that this thread hit the breakpoint.
+      const size_t num_thread_ids = m_thread_ids.size();
+      for (size_t i = 0; i < num_thread_ids; i++) {
+        if (m_thread_ids[i] == thread->GetID() && m_thread_pcs.size() > i) {
+          addr_t pc = m_thread_pcs[i];
+          lldb::BreakpointSiteSP bp_site_sp =
+              thread->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
+          if (bp_site_sp) {
+            if (bp_site_sp->ValidForThisThread(*thread)) {
+              thread->SetStopInfo(
+                  StopInfo::CreateStopReasonWithBreakpointSiteID(
+                      *thread, bp_site_sp->GetID()));
+              return true;
+            }
+          }
+        }
+      }
       thread->SetStopInfo(StopInfoSP());
     }
     return true;
@@ -1715,7 +1735,9 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
     } else {
       bool handled = false;
       bool did_exec = false;
-      if (!reason.empty()) {
+      // debugserver can send reason = "none" which is equivalent
+      // to no reason.
+      if (!reason.empty() && reason != "none") {
         if (reason == "trace") {
           addr_t pc = thread_sp->GetRegisterContext()->GetPC();
           lldb::BreakpointSiteSP bp_site_sp =
@@ -1852,11 +1874,10 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
         lldb::BreakpointSiteSP bp_site_sp =
             thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
 
-        // If the current pc is a breakpoint site then the StopInfo should be
-        // set to Breakpoint even though the remote stub did not set it as such.
-        // This can happen when the thread is involuntarily interrupted (e.g.
-        // due to stops on other threads) just as it is about to execute the
-        // breakpoint instruction.
+        // If a thread is stopped at a breakpoint site, set that as the stop
+        // reason even if it hasn't executed the breakpoint instruction yet.
+        // We will silently step over the breakpoint when we resume execution
+        // and miss the fact that this thread hit the breakpoint.
         if (bp_site_sp && bp_site_sp->ValidForThisThread(*thread_sp)) {
           thread_sp->SetStopInfo(StopInfo::CreateStopReasonWithBreakpointSiteID(
               *thread_sp, bp_site_sp->GetID()));

lldb/source/Target/Thread.cpp

@@ -223,7 +223,7 @@ Thread::Thread(Process &process, lldb::tid_t tid, bool use_invalid_index_id)
                                       : process.GetNextThreadIndexID(tid)),
       m_reg_context_sp(), m_state(eStateUnloaded), m_state_mutex(),
       m_frame_mutex(), m_curr_frames_sp(), m_prev_frames_sp(),
-      m_resume_signal(LLDB_INVALID_SIGNAL_NUMBER),
+      m_prev_framezero_pc(), m_resume_signal(LLDB_INVALID_SIGNAL_NUMBER),
       m_resume_state(eStateRunning), m_temporary_resume_state(eStateRunning),
       m_unwinder_up(), m_destroy_called(false),
       m_override_should_notify(eLazyBoolCalculate),
@@ -252,6 +252,7 @@ void Thread::DestroyThread() {
   std::lock_guard<std::recursive_mutex> guard(m_frame_mutex);
   m_curr_frames_sp.reset();
   m_prev_frames_sp.reset();
+  m_prev_framezero_pc.reset();
 }
 
 void Thread::BroadcastSelectedFrameChange(StackID &new_frame_id) {
@@ -440,6 +441,12 @@ lldb::StopInfoSP Thread::GetPrivateStopInfo(bool calculate) {
       }
     }
   }
+
+  // If we were resuming the process and it was interrupted,
+  // return no stop reason.  This thread would like to resume.
+  if (m_stop_info_sp && m_stop_info_sp->WasContinueInterrupted(*this))
+    return {};
+
   return m_stop_info_sp;
 }
 
@@ -1471,16 +1478,22 @@ StackFrameListSP Thread::GetStackFrameList() {
   return m_curr_frames_sp;
 }
 
+std::optional<addr_t> Thread::GetPreviousFrameZeroPC() {
+  return m_prev_framezero_pc;
+}
+
 void Thread::ClearStackFrames() {
   std::lock_guard<std::recursive_mutex> guard(m_frame_mutex);
 
   GetUnwinder().Clear();
+  m_prev_framezero_pc.reset();
+  if (RegisterContextSP reg_ctx_sp = GetRegisterContext())
+    m_prev_framezero_pc = reg_ctx_sp->GetPC();
 
   // Only store away the old "reference" StackFrameList if we got all its
   // frames:
   // FIXME: At some point we can try to splice in the frames we have fetched
-  // into
-  // the new frame as we make it, but let's not try that now.
+  // into the new frame as we make it, but let's not try that now.
   if (m_curr_frames_sp && m_curr_frames_sp->GetAllFramesFetched())
     m_prev_frames_sp.swap(m_curr_frames_sp);
   m_curr_frames_sp.reset();