diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 1ab2ad0d34f2b..00d30070c8c9f 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -275,7 +275,13 @@ class StopInfoBreakpoint : public StopInfo { BreakpointSiteSP bp_site_sp( thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value)); std::unordered_set 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, @@ -285,6 +291,8 @@ class StopInfoBreakpoint : public StopInfo { 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 @@ -440,12 +448,17 @@ class StopInfoBreakpoint : public StopInfo { // 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(""); LLDB_LOGF(log, "Error evaluating condition: \"%s\"\n", err_str); @@ -467,7 +480,9 @@ class StopInfoBreakpoint : public StopInfo { loc_desc.GetData(), static_cast(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: @@ -559,6 +574,7 @@ class StopInfoBreakpoint : public StopInfo { } 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, @@ -578,6 +594,12 @@ class StopInfoBreakpoint : public StopInfo { // 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, diff --git a/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/Makefile b/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/Makefile new file mode 100644 index 0000000000000..99998b20bcb05 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp + +include Makefile.rules diff --git a/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/TestTwoHitsOneActual.py b/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/TestTwoHitsOneActual.py new file mode 100644 index 0000000000000..530e715d3c3a9 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/TestTwoHitsOneActual.py @@ -0,0 +1,62 @@ +""" +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) + + diff --git a/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/main.cpp b/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/main.cpp new file mode 100644 index 0000000000000..5873e29458b05 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/two_hits_one_actual/main.cpp @@ -0,0 +1,22 @@ +#include +#include + +void usleep_helper(unsigned int usec) { + // Break here in the helper + std::this_thread::sleep_for(std::chrono::duration(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); + } +}