commit 984efd1b91921a6e04634a6d131b7257e3c5af82
author Brett Wilson <brettw@google.com> Mon Apr 12 20:30:18 2021 +0000
committer CQ Bot <commit-bot@chromium.org> Mon Apr 12 20:30:18 2021 +0000
tree c5758a8e34ad30def8e2b82c56adb2441cf0664a
parent 6d800b71570b5fec429e0b3f218c8055170a86f6

[debugger] Implement stop_mode = none

Previously we never checked for this condition and the stopped thread
reported a normal breakpoint hit. This auto-resumes from such
breakpoints.

Bug: 73511
Change-Id: I88e633e4d7214d8c07f2bbb8239e30477a660e06
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/514485
Reviewed-by: Dangyi Liu <dangyi@google.com>
Commit-Queue: Brett Wilson <brettw@google.com>

src/developer/debug/zxdb/client/session.cc

diff --git a/src/developer/debug/zxdb/client/session.cc b/src/developer/debug/zxdb/client/session.cc
index e0d7556..65a9710 100644
--- a/src/developer/debug/zxdb/client/session.cc
+++ b/src/developer/debug/zxdb/client/session.cc
@@ -49,10 +49,10 @@
 // buffer size if the stream is corrupt.
 constexpr uint32_t kMaxMessageSize = 16777216;
 
-// Remove conditional breakpoints from stop_info; return whether we'll need to skip this stop_info
-// and continue execution, which happens when the exception is a breakpoint one and all breakpoints
-// in it are conditional.
-bool FilterConditionalBreakpoints(StopInfo* info) {
+// Remove conditional and no-sop breakpoints from stop_info; return whether we'll need to skip this
+// stop_info and continue execution, which happens when the exception is a breakpoint one and all
+// breakpoints in it are conditional.
+bool FilterApplicableBreakpoints(StopInfo* info) {
   bool skip = false;
 
   if (info->exception_type == debug_ipc::ExceptionType::kHardwareBreakpoint ||
@@ -65,15 +65,23 @@
     }
   }
 
+  // TODO(dangyi): Consider whether to move this logic to the Breakpoint class.
   auto breakpoint_iter = info->hit_breakpoints.begin();
   while (breakpoint_iter != info->hit_breakpoints.end()) {
     Breakpoint* breakpoint = breakpoint_iter->get();
-    // TODO(dangyi): Consider whether to move the condition to Breakpoint class.
-    if (breakpoint->GetStats().hit_count % breakpoint->GetSettings().hit_mult == 0) {
+    BreakpointSettings settings = breakpoint->GetSettings();
+
+    if (settings.stop_mode == BreakpointSettings::StopMode::kNone) {
+      // This breakpoint should be auto-resumed always. This could be done automatically by the
+      // debug agent which will give better performance, but in the future we likely want to
+      // add some kind of logging features that will require evaluation in the client.
+      info->hit_breakpoints.erase(breakpoint_iter);
+    } else if (breakpoint->GetStats().hit_count % settings.hit_mult != 0) {
+      // Hit-count mismatch, auto-resume.
+      info->hit_breakpoints.erase(breakpoint_iter);
+    } else {
       skip = false;
       breakpoint_iter++;
-    } else {
-      info->hit_breakpoints.erase(breakpoint_iter);
     }
   }
 
@@ -590,7 +598,7 @@
   }
 
   // Continue if it's a conditional breakpoint.
-  if (FilterConditionalBreakpoints(&info)) {
+  if (FilterApplicableBreakpoints(&info)) {
     // For simplicity, we're resuming all threads right now.
     // TODO(dangyi): It's better to continue only the affected threads.
     system_.Continue(false);

src/developer/debug/zxdb/client/session_unittest.cc

diff --git a/src/developer/debug/zxdb/client/session_unittest.cc b/src/developer/debug/zxdb/client/session_unittest.cc
index 205ad6f..65919cf 100644
--- a/src/developer/debug/zxdb/client/session_unittest.cc
+++ b/src/developer/debug/zxdb/client/session_unittest.cc
@@ -278,6 +278,44 @@
   EXPECT_FALSE(weak_bp);
 }
 
+// Tests breakpoints with stop_mode == kNone. These are auto-resumed by the Session.
+TEST_F(SessionTest, BreakpointStopNone) {
+  // Make a process and thread for notifying about.
+  constexpr uint64_t kProcessKoid = 1234;
+  InjectProcess(kProcessKoid);
+  constexpr uint64_t kThreadKoid = 5678;
+  InjectThread(kProcessKoid, kThreadKoid);
+  SessionThreadObserver thread_observer;
+  session().thread_observers().AddObserver(&thread_observer);
+
+  // Create a breakpoint.
+  Breakpoint* bp = session().system().CreateNewBreakpoint();
+  constexpr uint64_t kAddress = 0x12345678;
+  BreakpointSettings settings;
+  settings.enabled = true;
+  settings.stop_mode = BreakpointSettings::StopMode::kNone;
+  settings.locations.emplace_back(kAddress);
+  bp->SetSettings(settings);
+
+  debug_ipc::NotifyException notify;
+  notify.type = debug_ipc::ExceptionType::kSoftwareBreakpoint;
+  notify.thread.process_koid = kProcessKoid;
+  notify.thread.thread_koid = kThreadKoid;
+  notify.thread.state = debug_ipc::ThreadRecord::State::kBlocked;
+  // Don't need stack pointers for this test.
+  notify.thread.frames.emplace_back(kAddress, 0);
+  sink()->PopulateNotificationWithBreakpoints(&notify);
+
+  // Notify of the breakpoint hit, it should continue the execution without notifying the thread.
+  notify.hit_breakpoints[0].hit_count = 1;
+  InjectException(notify);
+  EXPECT_EQ(1u, sink()->resume_count());
+  EXPECT_EQ(0u, thread_observer.stop_count());
+
+  // Cleanup.
+  session().thread_observers().RemoveObserver(&thread_observer);
+}
+
 // Tests a breakpoint with a hit_mult other than 1
 TEST_F(SessionTest, HitMultBreakpoint) {
   // Make a process and thread for notifying about.