[Lldb-commits] [PATCH] D127997: Remember whether all owners of the site we hit were internal in StopInfoBreakpoint

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, jasonmolenda.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127997

Files:
  lldb/source/Target/StopInfo.cpp


Index: lldb/source/Target/StopInfo.cpp
===================================================================
--- lldb/source/Target/StopInfo.cpp
+++ lldb/source/Target/StopInfo.cpp
@@ -88,7 +88,7 @@
       : 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();
   }
 
@@ -96,7 +96,7 @@
       : 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();
   }
 
@@ -108,11 +108,22 @@
       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();
@@ -163,23 +174,7 @@
   }
 
   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 {
@@ -603,6 +598,7 @@
   // 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;
 };
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D127997.437630.patch
Type: text/x-patch
Size: 2913 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220616/2018ee9b/attachment.bin>