Automerge: NFC: SBThread should not be the one to compute StopReasonData. (#157577)

This is something the StopInfo class manages, so it should be allowed to
compute this rather than having SBThread do so. This code just moves the
computation to methods in StopInfo. It is mostly NFC. The one change
that I actually had to adjust the tests for was a couple of tests that
were asking for the UnixSignal stop info data by asking for the data at
index 1. GetStopInfoDataCount returns 1 and we don't do 1 based indexing
so the test code was clearly wrong. But I don't think it makes sense to
perpetuate handing out the value regardless of what index you pass us.

Author: jimingham

lldb/include/lldb/Target/StopInfo.h

@@ -97,6 +97,12 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> {
   /// and silently continue again one more time.
   virtual bool WasContinueInterrupted(Thread &thread) { return false; }
 
+  virtual uint32_t GetStopReasonDataCount() const { return 0; }
+  virtual uint64_t GetStopReasonDataAtIndex(uint32_t idx) {
+    // Handle all the common cases that have no data.
+    return 0;
+  }
+
   // 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/source/API/SBThread.cpp

@@ -157,52 +157,8 @@ size_t SBThread::GetStopReasonDataCount() {
   if (exe_ctx) {
     if (exe_ctx->HasThreadScope()) {
       StopInfoSP stop_info_sp = exe_ctx->GetThreadPtr()->GetStopInfo();
-      if (stop_info_sp) {
-        StopReason reason = stop_info_sp->GetStopReason();
-        switch (reason) {
-        case eStopReasonInvalid:
-        case eStopReasonNone:
-        case eStopReasonTrace:
-        case eStopReasonExec:
-        case eStopReasonPlanComplete:
-        case eStopReasonThreadExiting:
-        case eStopReasonInstrumentation:
-        case eStopReasonProcessorTrace:
-        case eStopReasonVForkDone:
-        case eStopReasonHistoryBoundary:
-          // There is no data for these stop reasons.
-          return 0;
-
-        case eStopReasonBreakpoint: {
-          break_id_t site_id = stop_info_sp->GetValue();
-          lldb::BreakpointSiteSP bp_site_sp(
-              exe_ctx->GetProcessPtr()->GetBreakpointSiteList().FindByID(
-                  site_id));
-          if (bp_site_sp)
-            return bp_site_sp->GetNumberOfConstituents() * 2;
-          else
-            return 0; // Breakpoint must have cleared itself...
-        } break;
-
-        case eStopReasonWatchpoint:
-          return 1;
-
-        case eStopReasonSignal:
-          return 1;
-
-        case eStopReasonInterrupt:
-          return 1;
-
-        case eStopReasonException:
-          return 1;
-
-        case eStopReasonFork:
-          return 1;
-
-        case eStopReasonVFork:
-          return 1;
-        }
-      }
+      if (stop_info_sp)
+        return stop_info_sp->GetStopReasonDataCount();
     }
   } else {
     LLDB_LOG_ERROR(GetLog(LLDBLog::API), exe_ctx.takeError(), "{0}");
@@ -220,63 +176,8 @@ uint64_t SBThread::GetStopReasonDataAtIndex(uint32_t idx) {
     if (exe_ctx->HasThreadScope()) {
       Thread *thread = exe_ctx->GetThreadPtr();
       StopInfoSP stop_info_sp = thread->GetStopInfo();
-      if (stop_info_sp) {
-        StopReason reason = stop_info_sp->GetStopReason();
-        switch (reason) {
-        case eStopReasonInvalid:
-        case eStopReasonNone:
-        case eStopReasonTrace:
-        case eStopReasonExec:
-        case eStopReasonPlanComplete:
-        case eStopReasonThreadExiting:
-        case eStopReasonInstrumentation:
-        case eStopReasonProcessorTrace:
-        case eStopReasonVForkDone:
-        case eStopReasonHistoryBoundary:
-          // There is no data for these stop reasons.
-          return 0;
-
-        case eStopReasonBreakpoint: {
-          break_id_t site_id = stop_info_sp->GetValue();
-          lldb::BreakpointSiteSP bp_site_sp(
-              exe_ctx->GetProcessPtr()->GetBreakpointSiteList().FindByID(
-                  site_id));
-          if (bp_site_sp) {
-            uint32_t bp_index = idx / 2;
-            BreakpointLocationSP bp_loc_sp(
-                bp_site_sp->GetConstituentAtIndex(bp_index));
-            if (bp_loc_sp) {
-              if (idx & 1) {
-                // Odd idx, return the breakpoint location ID
-                return bp_loc_sp->GetID();
-              } else {
-                // Even idx, return the breakpoint ID
-                return bp_loc_sp->GetBreakpoint().GetID();
-              }
-            }
-          }
-          return LLDB_INVALID_BREAK_ID;
-        } break;
-
-        case eStopReasonWatchpoint:
-          return stop_info_sp->GetValue();
-
-        case eStopReasonSignal:
-          return stop_info_sp->GetValue();
-
-        case eStopReasonInterrupt:
-          return stop_info_sp->GetValue();
-
-        case eStopReasonException:
-          return stop_info_sp->GetValue();
-
-        case eStopReasonFork:
-          return stop_info_sp->GetValue();
-
-        case eStopReasonVFork:
-          return stop_info_sp->GetValue();
-        }
-      }
+      if (stop_info_sp)
+        return stop_info_sp->GetStopReasonDataAtIndex(idx);
     }
   } else {
     LLDB_LOG_ERROR(GetLog(LLDBLog::API), exe_ctx.takeError(), "{0}");

lldb/source/Target/StopInfo.cpp

@@ -108,8 +108,7 @@ class StopInfoBreakpoint : public StopInfo {
   void StoreBPInfo() {
     ThreadSP thread_sp(m_thread_wp.lock());
     if (thread_sp) {
-      BreakpointSiteSP bp_site_sp(
-          thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value));
+      BreakpointSiteSP bp_site_sp = GetBreakpointSiteSP();
       if (bp_site_sp) {
         uint32_t num_constituents = bp_site_sp->GetNumberOfConstituents();
         if (num_constituents == 1) {
@@ -139,8 +138,7 @@ class StopInfoBreakpoint : public StopInfo {
   bool IsValidForOperatingSystemThread(Thread &thread) override {
     ProcessSP process_sp(thread.GetProcess());
     if (process_sp) {
-      BreakpointSiteSP bp_site_sp(
-          process_sp->GetBreakpointSiteList().FindByID(m_value));
+      BreakpointSiteSP bp_site_sp = GetBreakpointSiteSP();
       if (bp_site_sp)
         return bp_site_sp->ValidForThisThread(thread);
     }
@@ -154,8 +152,7 @@ class StopInfoBreakpoint : public StopInfo {
     if (thread_sp) {
       if (!m_should_stop_is_valid) {
         // Only check once if we should stop at a breakpoint
-        BreakpointSiteSP bp_site_sp(
-            thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value));
+        BreakpointSiteSP bp_site_sp = GetBreakpointSiteSP();
         if (bp_site_sp) {
           ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
           StoppointCallbackContext context(event_ptr, exe_ctx, true);
@@ -186,8 +183,7 @@ class StopInfoBreakpoint : public StopInfo {
     if (m_description.empty()) {
       ThreadSP thread_sp(m_thread_wp.lock());
       if (thread_sp) {
-        BreakpointSiteSP bp_site_sp(
-            thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value));
+        BreakpointSiteSP bp_site_sp = GetBreakpointSiteSP();
         if (bp_site_sp) {
           StreamString strm;
           // If we have just hit an internal breakpoint, and it has a kind
@@ -247,6 +243,35 @@ class StopInfoBreakpoint : public StopInfo {
     return m_description.c_str();
   }
 
+  uint32_t GetStopReasonDataCount() const override {
+    lldb::BreakpointSiteSP bp_site_sp = GetBreakpointSiteSP();
+    if (bp_site_sp)
+      return bp_site_sp->GetNumberOfConstituents() * 2;
+    return 0; // Breakpoint must have cleared itself...
+  }
+
+  uint64_t GetStopReasonDataAtIndex(uint32_t idx) override {
+    lldb::BreakpointSiteSP bp_site_sp = GetBreakpointSiteSP();
+    if (bp_site_sp) {
+      uint32_t bp_index = idx / 2;
+      BreakpointLocationSP bp_loc_sp(
+          bp_site_sp->GetConstituentAtIndex(bp_index));
+      if (bp_loc_sp) {
+        if (idx & 1) {
+          // FIXME: This might be a Facade breakpoint, so we need to fetch
+          // the one that the thread actually hit, not the native loc ID.
+
+          // Odd idx, return the breakpoint location ID
+          return bp_loc_sp->GetID();
+        } else {
+          // Even idx, return the breakpoint ID
+          return bp_loc_sp->GetBreakpoint().GetID();
+        }
+      }
+    }
+    return LLDB_INVALID_BREAK_ID;
+  }
+
   std::optional<uint32_t>
   GetSuggestedStackFrameIndex(bool inlined_stack) override {
     if (!inlined_stack)
@@ -255,8 +280,7 @@ class StopInfoBreakpoint : public StopInfo {
     ThreadSP thread_sp(m_thread_wp.lock());
     if (!thread_sp)
       return {};
-    BreakpointSiteSP bp_site_sp(
-        thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value));
+    BreakpointSiteSP bp_site_sp = GetBreakpointSiteSP();
     if (!bp_site_sp)
       return {};
 
@@ -297,8 +321,7 @@ class StopInfoBreakpoint : public StopInfo {
         return;
       }
 
-      BreakpointSiteSP bp_site_sp(
-          thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value));
+      BreakpointSiteSP bp_site_sp = GetBreakpointSiteSP();
       std::unordered_set<break_id_t> precondition_breakpoints;
       // Breakpoints that fail their condition check are not considered to
       // have been hit.  If the only locations at this site have failed their
@@ -629,6 +652,20 @@ class StopInfoBreakpoint : public StopInfo {
   }
 
 private:
+  BreakpointSiteSP GetBreakpointSiteSP() const {
+    if (m_value == LLDB_INVALID_BREAK_ID)
+      return {};
+
+    ThreadSP thread_sp = GetThread();
+    if (!thread_sp)
+      return {};
+    ProcessSP process_sp = thread_sp->GetProcess();
+    if (!process_sp)
+      return {};
+
+    return process_sp->GetBreakpointSiteList().FindByID(m_value);
+  }
+
   bool m_should_stop;
   bool m_should_stop_is_valid;
   bool m_should_perform_action; // Since we are trying to preserve the "state"
@@ -699,6 +736,13 @@ class StopInfoWatchpoint : public StopInfo {
 
   StopReason GetStopReason() const override { return eStopReasonWatchpoint; }
 
+  uint32_t GetStopReasonDataCount() const override { return 1; }
+  uint64_t GetStopReasonDataAtIndex(uint32_t idx) override {
+    if (idx == 0)
+      return GetValue();
+    return 0;
+  }
+
   const char *GetDescription() override {
     if (m_description.empty()) {
       StreamString strm;
@@ -1139,6 +1183,13 @@ class StopInfoUnixSignal : public StopInfo {
 
   bool ShouldSelect() const override { return IsShouldStopSignal(); }
 
+  uint32_t GetStopReasonDataCount() const override { return 1; }
+  uint64_t GetStopReasonDataAtIndex(uint32_t idx) override {
+    if (idx == 0)
+      return GetValue();
+    return 0;
+  }
+
 private:
   // In siginfo_t terms, if m_value is si_signo, m_code is si_code.
   std::optional<int> m_code;
@@ -1171,6 +1222,14 @@ class StopInfoInterrupt : public StopInfo {
     }
     return m_description.c_str();
   }
+
+  uint32_t GetStopReasonDataCount() const override { return 1; }
+  uint64_t GetStopReasonDataAtIndex(uint32_t idx) override {
+    if (idx == 0)
+      return GetValue();
+    else
+      return 0;
+  }
 };
 
 // StopInfoTrace
@@ -1249,6 +1308,13 @@ class StopInfoException : public StopInfo {
     else
       return m_description.c_str();
   }
+  uint32_t GetStopReasonDataCount() const override { return 1; }
+  uint64_t GetStopReasonDataAtIndex(uint32_t idx) override {
+    if (idx == 0)
+      return GetValue();
+    else
+      return 0;
+  }
 };
 
 // StopInfoProcessorTrace
@@ -1390,6 +1456,14 @@ class StopInfoFork : public StopInfo {
 
   const char *GetDescription() override { return "fork"; }
 
+  uint32_t GetStopReasonDataCount() const override { return 1; }
+  uint64_t GetStopReasonDataAtIndex(uint32_t idx) override {
+    if (idx == 0)
+      return GetValue();
+    else
+      return 0;
+  }
+
 protected:
   void PerformAction(Event *event_ptr) override {
     // Only perform the action once
@@ -1424,6 +1498,13 @@ class StopInfoVFork : public StopInfo {
 
   const char *GetDescription() override { return "vfork"; }
 
+  uint32_t GetStopReasonDataCount() const override { return 1; }
+  uint64_t GetStopReasonDataAtIndex(uint32_t idx) override {
+    if (idx == 0)
+      return GetValue();
+    return 0;
+  }
+
 protected:
   void PerformAction(Event *event_ptr) override {
     // Only perform the action once

lldb/test/API/functionalities/postmortem/elf-core/gcore/TestGCore.py

@@ -37,7 +37,7 @@ def do_test(self, filename, pid):
         for thread in process:
             reason = thread.GetStopReason()
             self.assertStopReason(reason, lldb.eStopReasonSignal)
-            signal = thread.GetStopReasonDataAtIndex(1)
+            signal = thread.GetStopReasonDataAtIndex(0)
             # Check we got signal 19 (SIGSTOP)
             self.assertEqual(signal, 19)
 

lldb/test/API/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py

@@ -91,7 +91,7 @@ def do_test(self, filename, pid, tid):
             reason = thread.GetStopReason()
             if thread.GetThreadID() == tid:
                 self.assertStopReason(reason, lldb.eStopReasonSignal)
-                signal = thread.GetStopReasonDataAtIndex(1)
+                signal = thread.GetStopReasonDataAtIndex(0)
                 # Check we got signal 4 (SIGILL)
                 self.assertEqual(signal, 4)
             else: