Remember whether all owners of the site we hit were internal in StopInfoBreakpoint

[jingham]

Summary

We noticed several test suite failures (e.g. TestStopAtEntry.py) with the new MacOS dyld support that was recently added when run on the macOS Ventura beta. The reason was that he was an unexpected breakpoint stop event coming from the new three-step dance needed to follow dyld's loading into the process. At the first two stops, we delete the instrumentation breakpoint we just hit in the callback, and add another. Since these were internal breakpoints the ThreadPlanBase::ShouldNotify should have returned eVoteNo. But that logic requires knowing that all the owners of the breakpoint site were internal, and by the time we got to asking the thread plan the breakpoint site had been removed, so we no longer knew that.

The only time you know no one could have deleted the site you hit is when you make the StopInfoBreakpoint for the stop. So this change adds an ivar to record whether all the breakpoints at this site were internal at that point, and uses that in the "ShouldNotify" calculation.

[JDevlieghere]

Neat!

[jasonmolenda]

Thanks for getting to the bottom of this one!

lldb/source/Target/StopInfo.cpp

 
 namespace lldb_private {
 class StopInfoBreakpoint : public StopInfo {
 public:
   StopInfoBreakpoint(Thread &thread, break_id_t break_id)
       : StopInfo(thread, break_id), m_should_stop(false),
         m_should_stop_is_valid(false), m_should_perform_action(true),
         m_address(LLDB_INVALID_ADDRESS), m_break_id(LLDB_INVALID_BREAK_ID),
-        m_was_one_shot(false) {
+        m_was_all_internal(false), m_was_one_shot(false) {
     StoreBPInfo();
   }
 
   StopInfoBreakpoint(Thread &thread, break_id_t break_id, bool should_stop)
       : StopInfo(thread, break_id), m_should_stop(should_stop),
         m_should_stop_is_valid(true), m_should_perform_action(true),
         m_address(LLDB_INVALID_ADDRESS), m_break_id(LLDB_INVALID_BREAK_ID),
-        m_was_one_shot(false) {
+        m_was_all_internal(false), m_was_one_shot(false) {
     StoreBPInfo();
   }
 
   ~StopInfoBreakpoint() override = default;
 
   void StoreBPInfo() {
     ThreadSP thread_sp(m_thread_wp.lock());
     if (thread_sp) {
       BreakpointSiteSP bp_site_sp(
           thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value));
       if (bp_site_sp) {
-        if (bp_site_sp->GetNumberOfOwners() == 1) {
+        uint32_t num_owners = bp_site_sp->GetNumberOfOwners();
+        if (num_owners == 1) {
           BreakpointLocationSP bp_loc_sp = bp_site_sp->GetOwnerAtIndex(0);
           if (bp_loc_sp) {
-            m_break_id = bp_loc_sp->GetBreakpoint().GetID();
-            m_was_one_shot = bp_loc_sp->GetBreakpoint().IsOneShot();
+            Breakpoint & bkpt = bp_loc_sp->GetBreakpoint();
+            m_break_id = bkpt.GetID();
+            m_was_one_shot = bkpt.IsOneShot();
+            m_was_all_internal = bkpt.IsInternal();
+          }
+        } else {
+          m_was_all_internal = true;
+          for (uint32_t i = 0; i < num_owners; i++) {
+            if (!bp_site_sp->GetOwnerAtIndex(i)->GetBreakpoint().IsInternal()) {
+              m_was_all_internal = false;
+              break;
+            }
           }
         }
         m_address = bp_site_sp->GetLoadAddress();
       }
     }
   }
 
   bool IsValidForOperatingSystemThread(Thread &thread) override {
if (thread_sp) {
         m_should_stop_is_valid = true;
       }
       return m_should_stop;
     }
     return false;
   }
 
   bool DoShouldNotify(Event *event_ptr) override {
-    ThreadSP thread_sp(m_thread_wp.lock());
-    if (thread_sp) {
-      BreakpointSiteSP bp_site_sp(
-          thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value));
-      if (bp_site_sp) {
-        bool all_internal = true;
-
-        for (uint32_t i = 0; i < bp_site_sp->GetNumberOfOwners(); i++) {
-          if (!bp_site_sp->GetOwnerAtIndex(i)->GetBreakpoint().IsInternal()) {
-            all_internal = false;
-            break;
-          }
-        }
-        return !all_internal;
-      }
-    }
-    return true;
+    return !m_was_all_internal;
   }
 
   const char *GetDescription() override {
     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));
bool m_should_perform_action; // Since we are trying to preserve the "state"
                                 // of the system even if we run functions
   // etc. behind the users backs, we need to make sure we only REALLY perform
   // the action once.
   lldb::addr_t m_address; // We use this to capture the breakpoint site address
                           // when we create the StopInfo,
   // in case somebody deletes it between the time the StopInfo is made and the
   // description is asked for.
   lldb::break_id_t m_break_id;
+  bool m_was_all_internal;
   bool m_was_one_shot;
 };
 
 // StopInfoWatchpoint
 
 class StopInfoWatchpoint : public StopInfo {
 public:
   // Make sure watchpoint is properly disabled and subsequently enabled while