Handle a stop when another thread has hit a breakpoint with a failed condition

[jingham]

Summary

Threads which hit a breakpoint but fail the condition are considered
not to be hit. But another thread might be hit at the same time and
actually stop. So we have to be sure to switch the first thread's
stop info to eStopReasonNone or we'll report a hit when the condition
failed, which is confusing.

I have a test for this, but it only tests the actual fix on systems that report
multiple threads hitting breakpoints on a given stop. Darwin
behaves this way, and w/o this fix the test fails on the first stop. But I give it
20 tries so this is likely to test the condition on systems that report multiple
hits less frequently. And for a system that serializes all stops, all this code is
moot anyway, and this is just testing breakpoint conditions.

[labath]

(Other than the inline comment, this seems fine to me)

lldb/test/API/functionalities/breakpoint/two_hits_one_actual/main.cpp

7

usleep doesn't exist on windows. Use std::this_thread::sleep_for.

[jingham]

Use sleep_for instead of usleep. Man, that API isn't winning any beauty contests!

[labath]

In D128776#3620886, @jingham wrote:

Use sleep_for instead of usleep. Man, that API isn't winning any beauty contests!

well.. normally one would write that as sleep_for(std::chrono:microseconds(47)), or (if he's into that sort of thing) using namespace std::chrono_literals; sleep_for(47us);

lldb/source/Target/StopInfo.cpp

if (thread_sp) {
         m_should_stop = true;
         m_should_stop_is_valid = true;
         return;
       }
 
       BreakpointSiteSP bp_site_sp(
           thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value));
       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
+      // conditions, we should change the stop-info to none.  Otherwise, if we
+      // hit another breakpoint on a different thread which does stop, users
+      // will see a breakpont hit with a failed condition, which is wrong.
+      // Use this variable to tell us if that is true.
+      bool actually_hit_any_locations = false;
       if (bp_site_sp) {
         // Let's copy the owners list 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.
         BreakpointLocationCollection site_locations;
         size_t num_owners = bp_site_sp->CopyOwnersList(site_locations);
 
         if (num_owners == 0) {
           m_should_stop = true;
+          actually_hit_any_locations = true;  // We're going to stop, don't 
+                                              // change the stop info.
         } else {
           // We go through each location, and test first its precondition -
           // this overrides everything.  Note, we only do this once per
           // breakpoint - not once per location... Then check the condition.
           // If the condition says to stop, then we run the callback for that
           // location.  If that callback says to stop as well, then we set
           // m_should_stop to true; we are going to stop. But we still want to
           // give all the breakpoints whose conditions say we are going to stop
if (thread_sp) {
             if (!precondition_result) {
               actually_said_continue = true;
               continue;
             }
             // Next run the condition for the breakpoint.  If that says we
             // should stop, then we'll run the callback for the breakpoint.  If
             // the callback says we shouldn't stop that will win.
 
-            if (bp_loc_sp->GetConditionText() != nullptr) {
+            if (bp_loc_sp->GetConditionText() == nullptr)
+              actually_hit_any_locations = true;
+            else {
               Status condition_error;
               bool condition_says_stop =
                   bp_loc_sp->ConditionSaysStop(exe_ctx, condition_error);
 
               if (!condition_error.Success()) {
+                // If the condition fails to evaluate, we are going to stop 
+                // at it, so the location was hit.
+                actually_hit_any_locations = true;
                 const char *err_str =
                     condition_error.AsCString("<unknown error>");
                 LLDB_LOGF(log, "Error evaluating condition: \"%s\"\n", err_str);
 
                 StreamString strm;
                 strm << "stopped due to an error evaluating condition of "
                         "breakpoint ";
                 bp_loc_sp->GetDescription(&strm, eDescriptionLevelBrief);
                 strm << ": \"" << bp_loc_sp->GetConditionText() << "\"\n";
                 strm << err_str;
 
                 Debugger::ReportError(
                     strm.GetString().str(),
                     exe_ctx.GetTargetRef().GetDebugger().GetID());
               } else {
                 LLDB_LOGF(log,
                           "Condition evaluated for breakpoint %s on thread "
                           "0x%llx condition_says_stop: %i.",
                           loc_desc.GetData(),
                           static_cast<unsigned long long>(thread_sp->GetID()),
                           condition_says_stop);
-                if (!condition_says_stop) {
+                if (condition_says_stop) 
+                  actually_hit_any_locations = true;
+                else {
                   // We don't want to increment the hit count of breakpoints if
                   // the condition fails. We've already bumped it by the time
                   // we get here, so undo the bump:
                   bp_loc_sp->UndoBumpHitCount();
                   actually_said_continue = true;
                   continue;
                 }
               }
if (thread_sp) {
           }
         }
         // We've figured out what this stop wants to do, so mark it as valid so
         // we don't compute it again.
         m_should_stop_is_valid = true;
       } else {
         m_should_stop = true;
         m_should_stop_is_valid = true;
+        actually_hit_any_locations = true;
         Log *log_process(GetLog(LLDBLog::Process));
 
         LLDB_LOGF(log_process,
                   "Process::%s could not find breakpoint site id: %" PRId64
                   "...",
                   __FUNCTION__, m_value);
       }
 
       if ((!m_should_stop || internal_breakpoint) &&
           thread_sp->CompletedPlanOverridesBreakpoint()) {
 
         // Override should_stop decision when we have completed step plan
         // additionally to the breakpoint
         m_should_stop = true;
 
         // We know we're stopping for a completed plan and we don't want to
         // show the breakpoint stop, so compute the public stop info immediately
         // here.
         thread_sp->CalculatePublicStopInfo();
+      } else if (!actually_hit_any_locations) {
+        // In the end, we didn't actually have any locations that passed their
+        // "was I hit" checks.  So say we aren't stopped.
+        GetThread()->ResetStopInfo();
+        LLDB_LOGF(log, "Process::%s all locations failed condition checks.",
+          __FUNCTION__);
       }
 
       LLDB_LOGF(log,
                 "Process::%s returning from action with m_should_stop: %d.",
                 __FUNCTION__, m_should_stop);
     }
   }
 

lldb/test/API/functionalities/breakpoint/two_hits_one_actual/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/functionalities/breakpoint/two_hits_one_actual/TestTwoHitsOneActual.py

+"""
+Test that if we hit a breakpoint on two threads at the 
+same time, one of which passes the condition, one not,
+we only have a breakpoint stop reason for the one that
+passed the condition.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestTwoHitsOneActual(TestBase):
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_two_hits_one_actual(self):
+        """There can be many tests in a test case - describe this test here."""
+        self.build()
+        self.main_source_file = lldb.SBFileSpec("main.cpp")
+        self.sample_test()
+
+    def sample_test(self):
+        """You might use the test implementation in several ways, say so here."""
+
+        (target, process, main_thread, _) = lldbutil.run_to_source_breakpoint(self,
+                                   "Set bkpt here to get started", self.main_source_file)
+        # This is working around a separate bug.  If you hit a breakpoint and
+        # run an expression and it is the first expression you've ever run, on
+        # Darwin that will involve running the ObjC runtime parsing code, and we'll
+        # be in the middle of that when we do PerformAction on the other thread,
+        # which will cause the condition expression to fail.  Calling another
+        # expression first works around this.
+        val_obj = main_thread.frame[0].EvaluateExpression("main_usec==1")
+        self.assertSuccess(val_obj.GetError(), "Ran our expression successfully")
+        self.assertEqual(val_obj.value, "true", "Value was true.")
+        # Set two breakpoints just to test the multiple location logic:
+        bkpt1 = target.BreakpointCreateBySourceRegex("Break here in the helper", self.main_source_file);
+        bkpt2 = target.BreakpointCreateBySourceRegex("Break here in the helper", self.main_source_file);
+
+        # This one will never be hit:
+        bkpt1.SetCondition("usec == 100")
+        # This one will only be hit on the main thread:
+        bkpt2.SetCondition("usec == 1")
+
+        # This is hard to test definitively, becuase it requires hitting
+        # a breakpoint on multiple threads at the same time.  On Darwin, this
+        # will happen pretty much ever time we continue.  What we are really
+        # asserting is that we only ever stop on one thread, so we approximate that
+        # by continuing 20 times and assert we only ever hit the first thread.  Either
+        # this is a platform that only reports one hit at a time, in which case all
+        # this code is unused, or we actually didn't hit the other thread.
+
+        for idx in range(0, 20):
+            process.Continue()
+            for thread in process.threads:
+                if thread.id == main_thread.id:
+                    self.assertEqual(thread.stop_reason, lldb.eStopReasonBreakpoint)
+                else:
+                    self.assertEqual(thread.stop_reason, lldb.eStopReasonNone)
+
+                

lldb/test/API/functionalities/breakpoint/two_hits_one_actual/main.cpp

+#include <thread>
+#include <chrono>
+
+void usleep_helper(unsigned int usec) {
+  // Break here in the helper
+  std::this_thread::sleep_for(std::chrono::duration<unsigned int, std::milli>(usec));
+}
+
+void *background_thread(void *arg) {
+    (void) arg;
+    for (;;) {
+        usleep_helper(2);
+    }
+}
+
+int main(void) {
+  unsigned int main_usec = 1;
+  std::thread main_thread(background_thread, nullptr); // Set bkpt here to get started
+  for (;;) {
+    usleep_helper(main_usec);
+  }
+}