[Lldb-commits] [lldb] b8e0b5a - Threads which hit a breakpoint but fail the condition are considered
Jim Ingham via lldb-commits
lldb-commits at lists.llvm.org
Thu Jun 30 11:44:07 PDT 2022
Author: Jim Ingham
Date: 2022-06-30T11:43:59-07:00
New Revision: b8e0b5a071600cb39d938470bd20d845013384ce
URL: https://github.com/llvm/llvm-project/commit/b8e0b5a071600cb39d938470bd20d845013384ce
DIFF: https://github.com/llvm/llvm-project/commit/b8e0b5a071600cb39d938470bd20d845013384ce.diff
LOG: 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.
Differential Revision: https://reviews.llvm.org/D128776
Added:
lldb/test/API/functionalities/breakpoint/two_hits_one_actual/Makefile
lldb/test/API/functionalities/breakpoint/two_hits_one_actual/TestTwoHitsOneActual.py
lldb/test/API/functionalities/breakpoint/two_hits_one_actual/main.cpp
Modified:
lldb/source/Target/StopInfo.cpp
Removed:
################################################################################
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<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
diff erent 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("<unknown error>");
LLDB_LOGF(log, "Error evaluating condition: \"%s\"\n", err_str);
@@ -467,7 +480,9 @@ class StopInfoBreakpoint : public StopInfo {
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:
@@ -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 <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);
+ }
+}
More information about the lldb-commits
mailing list