jingham created this revision.
jingham added a reviewer: JDevlieghere.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D128776
Files:
lldb/source/Target/StopInfo.cpp
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
Index: lldb/test/API/functionalities/breakpoint/two_hits_one_actual/main.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/two_hits_one_actual/main.cpp
@@ -0,0 +1,22 @@
+//#include <stdlib.h>
+#include <unistd.h>
+#include <thread>
+
+void usleep_helper(useconds_t usec) {
+ usleep(usec); // Break here in the helper
+}
+
+void *background_thread(void *arg) {
+ (void) arg;
+ for (;;) {
+ usleep_helper(2);
+ }
+}
+
+int main(void) {
+ useconds_t main_usec = 1;
+ std::thread main_thread(background_thread, nullptr); // Set bkpt here to get started
+ for (;;) {
+ usleep_helper(main_usec);
+ }
+}
Index: lldb/test/API/functionalities/breakpoint/two_hits_one_actual/TestTwoHitsOneActual.py
===================================================================
--- /dev/null
+++ 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)
+
+
Index: lldb/test/API/functionalities/breakpoint/two_hits_one_actual/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/two_hits_one_actual/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Target/StopInfo.cpp
===================================================================
--- lldb/source/Target/StopInfo.cpp
+++ lldb/source/Target/StopInfo.cpp
@@ -275,7 +275,13 @@
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,
@@ -285,6 +291,8 @@
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 @@
// 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 @@
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 @@
} 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 @@
// 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,
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
