The breakpoint location hit counts were getting incremented in

BreakpointLocation::ShouldStop.  That worked but wasn't really right,
since there's nothing to guarantee that won't get called more than
once.  So this change moves that responsibility to the StopInfoBreakpoint
directly, and then it uses the BreakpointSite to actually do the bumping.

Also fix a test case that was assuming if you had many threads running some
code with a breakpoint in it, the hit count when you stopped would always be
1.  Many of the threads could have hit it at the same time...

<rdar://problem/18577603>

llvm-svn: 220358

diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index 5097150d682be77e315ed5f246baa2ccec26305f..8d5ebce411dfee21171a9b6e5ae271213cea262d 100644 (file)
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -387,6 +387,7 @@ public:
     bool EquivalentToLocation(BreakpointLocation &location);
     
 protected:
+    friend class BreakpointSite;
     friend class BreakpointLocationList;
     friend class Process;
 
@@ -413,6 +414,9 @@ private:
     void
     SwapLocation (lldb::BreakpointLocationSP swap_from);
 
+    void
+    BumpHitCount();
+
 
     //------------------------------------------------------------------
     // Constructors and Destructors

diff --git a/lldb/include/lldb/Breakpoint/BreakpointSite.h b/lldb/include/lldb/Breakpoint/BreakpointSite.h
index 1d2cbea18f9ffe659c841a12704038d7c07a8f98..c6dbef781f22b6156430f990d8eeaecabe0606a0 100644 (file)
--- a/lldb/include/lldb/Breakpoint/BreakpointSite.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointSite.h
@@ -259,6 +259,12 @@ public:
 private:
     friend class Process;
     friend class BreakpointLocation;
+    // The StopInfoBreakpoint knows when it is processing a hit for a thread for a site, so let it be the
+    // one to manage setting the location hit count once and only once.
+    friend class StopInfoBreakpoint;
+
+    void
+    BumpHitCounts();
 
     //------------------------------------------------------------------
     /// The method removes the owner at \a break_loc_id from this breakpoint list.

diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index 02d9609e24844406c66f1b2a69831cb62bc8c3de..11ecfecc5bc777102337893d41cf847e38554aaf 100644 (file)
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -449,8 +449,7 @@ BreakpointLocation::ShouldStop (StoppointCallbackContext *context)
     bool should_stop = true;
     Log *log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_BREAKPOINTS);
 
-    IncrementHitCount();
-
+    // Do this first, if a location is disabled, it shouldn't increment its hit count.
     if (!IsEnabled())
         return false;
 
@@ -474,6 +473,13 @@ BreakpointLocation::ShouldStop (StoppointCallbackContext *context)
     return should_stop;
 }
 
+void
+BreakpointLocation::BumpHitCount()
+{
+    if (IsEnabled())
+        IncrementHitCount();
+}
+
 bool
 BreakpointLocation::IsResolved () const
 {

diff --git a/lldb/source/Breakpoint/BreakpointLocationCollection.cpp b/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
index ee3f56f928d58fdf42a37d3d95102d9471e58e53..5756ccedfaa4af05984d9ff219190f16c812a72f 100644 (file)
--- a/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
@@ -196,3 +196,4 @@ BreakpointLocationCollection::GetDescription (Stream *s, lldb::DescriptionLevel
         (*pos)->GetDescription(s, level);
     }
 }
+

diff --git a/lldb/source/Breakpoint/BreakpointSite.cpp b/lldb/source/Breakpoint/BreakpointSite.cpp
index 3cf6d37af37981f3440c1f73367dea69c2d5cad3..469514b03f8a37182af978b9bc48ba236a4b0bf4 100644 (file)
--- a/lldb/source/Breakpoint/BreakpointSite.cpp
+++ b/lldb/source/Breakpoint/BreakpointSite.cpp
@@ -199,6 +199,15 @@ BreakpointSite::ValidForThisThread (Thread *thread)
     return m_owners.ValidForThisThread(thread);
 }
 
+void
+BreakpointSite::BumpHitCounts()
+{
+    for (BreakpointLocationSP loc_sp : m_owners.BreakpointLocations())
+    {
+        loc_sp->BumpHitCount();
+    }
+}
+
 bool
 BreakpointSite::IntersectsRange(lldb::addr_t addr, size_t size, lldb::addr_t *intersect_addr, size_t *intersect_size, size_t *opcode_offset) const
 {

diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index 99b1ac1f3e38b01e09ca8aa97ed59e88c365ab86..4b57ca65a2dff4790c1ed31876f61365117d3f64 100644 (file)
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -182,6 +182,7 @@ public:
                 {
                     ExecutionContext exe_ctx (thread_sp->GetStackFrameAtIndex(0));
                     StoppointCallbackContext context (event_ptr, exe_ctx, true);
+                    bp_site_sp->BumpHitCounts();
                     m_should_stop = bp_site_sp->ShouldStop (&context);
                 }
                 else
@@ -403,10 +404,21 @@ protected:
 
                     // Let's copy the breakpoint locations out of the site and store them in a local list.  That way if
                     // one of the breakpoint actions changes the site, then we won't be operating on a bad list.
+                    // For safety's sake let's also grab an extra reference to the breakpoint owners of the locations we're
+                    // going to examine, since the locations are going to have to get back to their breakpoints, and the
+                    // locations don't keep their owners alive.  I'm just sticking the BreakpointSP's in a vector since
+                    // I'm only really using it to locally increment their retain counts.
 
                     BreakpointLocationCollection site_locations;
+                    std::vector<lldb::BreakpointSP> location_owners;
+
                     for (size_t j = 0; j < num_owners; j++)
-                        site_locations.Add(bp_site_sp->GetOwnerAtIndex(j));
+                    {
+                        BreakpointLocationSP loc(bp_site_sp->GetOwnerAtIndex(j));
+                        site_locations.Add(loc);
+                        location_owners.push_back(loc->GetBreakpoint().shared_from_this());
+
+                    }
 
                     for (size_t j = 0; j < num_owners; j++)
                     {

diff --git a/lldb/test/functionalities/attach_resume/TestAttachResume.py b/lldb/test/functionalities/attach_resume/TestAttachResume.py
index 2cdfba1a8b440fd78a747cb712e8cd0aea9b740d..ef01fdacca75a1cb5bb64e5c6b31342377d96ac7 100644 (file)
--- a/lldb/test/functionalities/attach_resume/TestAttachResume.py
+++ b/lldb/test/functionalities/attach_resume/TestAttachResume.py
@@ -80,8 +80,12 @@ class AttachResumeTestCase(TestBase):
 
         self.assertTrue(wait_for_state(lldb.eStateStopped),
             'Process not stopped after breakpoint')
+        # This test runs a bunch of threads in the same little function with this
+        # breakpoint.  We want to make sure the breakpoint got hit at least once,
+        # but more than one thread may hit it at a time.  So we really only care
+        # that is isn't 0 times, not how many times it is.
         self.expect('br list', 'Breakpoint not hit',
-            patterns = ['hit count = 1'])
+            patterns = ['hit count = [1-9]'])
 
         self.runCmd("c")
         self.assertTrue(wait_for_state(lldb.eStateRunning),