NFC: SBThread should not be the one to compute StopReasonData.

[jimingham]

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/157577.diff

5 Files Affected:

• (modified) lldb/include/lldb/Target/StopInfo.h (+7)
• (modified) lldb/source/API/SBThread.cpp (+4-103)
• (modified) lldb/source/Target/StopInfo.cpp (+97-12)
• (modified) lldb/test/API/functionalities/postmortem/elf-core/gcore/TestGCore.py (+1-1)
• (modified) lldb/test/API/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py (+1-1)

diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h
index 368ec51d81891..0c18351a561a5 100644
--- a/lldb/include/lldb/Target/StopInfo.h
+++ b/lldb/include/lldb/Target/StopInfo.h
@@ -96,6 +96,13 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> {
   /// before any execution has happened. We need to detect this
   /// 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
diff --git a/lldb/source/API/SBThread.cpp b/lldb/source/API/SBThread.cpp
index ec68b2a4b6f31..4e4aa48bc9a2e 100644
--- a/lldb/source/API/SBThread.cpp
+++ b/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}");
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index ddf8c62e969ed..e1d24d24709b3 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/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,36 @@ 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;
+    else
+      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 +281,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 +322,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 +653,20 @@ class StopInfoBreakpoint : public StopInfo {
   }
 
 private:
+  BreakpointSiteSP GetBreakpointSiteSP() const {
+    if (m_break_id == 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"
@@ -698,6 +736,14 @@ class StopInfoWatchpoint : public StopInfo {
   ~StopInfoWatchpoint() override = default;
 
   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();
+    else
+      return 0;
+  }
 
   const char *GetDescription() override {
     if (m_description.empty()) {
@@ -1138,6 +1184,14 @@ 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();
+    else
+      return 0;
+  }
 
 private:
   // In siginfo_t terms, if m_value is si_signo, m_code is si_code.
@@ -1171,6 +1225,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 +1311,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 +1459,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 +1501,14 @@ 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();
+    else
+      return 0;
+  }
+
 protected:
   void PerformAction(Event *event_ptr) override {
     // Only perform the action once
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/gcore/TestGCore.py b/lldb/test/API/functionalities/postmortem/elf-core/gcore/TestGCore.py
index 020a226924ea6..497b8e8a19a86 100644
--- a/lldb/test/API/functionalities/postmortem/elf-core/gcore/TestGCore.py
+++ b/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)
 
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py b/lldb/test/API/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py
index 4a848d1c2eb9e..6d9aef286a796 100644
--- a/lldb/test/API/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py
+++ b/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:

[jimingham]

I need to have the StopInfo be the one computing the stop reason data for some future work.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[JDevlieghere]

LGTM modulo formatting and no-else-after-return.

[JDevlieghere]

Thanks!