[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint detection behavior [WIP] (PR #105594)

via lldb-commits lldb-commits at lists.llvm.org
Wed Aug 21 15:56:51 PDT 2024


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

<details>
<summary>Changes</summary>

This is a PR to track the changes I need to make to
https://github.com/llvm/llvm-project/pull/96260

to address the CI bot failures when I merged that PR, as well as investigating the windows problem Martin Storsjö saw.  There's likely going to be 4-5 additional changes to this PR before it's ready to be merged again, I'll create a new PR with the patches I originally landed and fixing those separate issues in additional commits.

Issues:

1. lldb-server armv7/aarch32 stepping-by-breakpoint doesn't verify the pc changed as expected, when reporting a `trace` stop-reason, so it will fail to recognize stepping over a breakpoint correctly.
2. ProcessGDBRemote needs to add a `reason = "breakpoint"` when we have a `swbreak` or `hwbreak` from the remote stub, like it does today for `watch`/`awatch`.  https://github.com/llvm/llvm-project/pull/102873
3. debugserver on armv7k watches is doesn't correctly report a "step by breakpoint" as a `trace` event, need to verify that we can step / step past breakpoints correctly on old watches running that old debugserver.
4. fix debuginfo dexter fails because stepping behavior was different.
5. Look into the Martin Storsjö `finish` failure on windows.

---

Patch is 35.61 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105594.diff


9 Files Affected:

- (modified) lldb/include/lldb/Target/Thread.h (+24) 
- (modified) lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp (+148-174) 
- (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp (+12-20) 
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+31-62) 
- (modified) lldb/source/Plugins/Process/scripted/ScriptedThread.cpp (+11) 
- (modified) lldb/source/Target/StopInfo.cpp (+2) 
- (modified) lldb/source/Target/Thread.cpp (+14-1) 
- (modified) lldb/test/API/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py (+19-7) 
- (modified) lldb/test/API/functionalities/breakpoint/step_over_breakpoint/TestStepOverBreakpoint.py (+5-1) 


``````````diff
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 38b65b2bc58490..22d452c7b4b8a3 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -131,6 +131,7 @@ class Thread : public std::enable_shared_from_this<Thread>,
         register_backup_sp; // You need to restore the registers, of course...
     uint32_t current_inlined_depth;
     lldb::addr_t current_inlined_pc;
+    lldb::addr_t stopped_at_unexecuted_bp;
   };
 
   /// Constructor
@@ -380,6 +381,26 @@ class Thread : public std::enable_shared_from_this<Thread>,
 
   virtual void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t) {}
 
+  /// When a thread stops at an enabled BreakpointSite that has not executed,
+  /// the Process plugin should call SetThreadStoppedAtUnexecutedBP(pc).
+  /// If that BreakpointSite was actually triggered (the instruction was
+  /// executed, for a software breakpoint), regardless of whether the
+  /// breakpoint is valid for this thread, SetThreadHitBreakpointSite()
+  /// should be called to record that fact.
+  ///
+  /// Depending on the structure of the Process plugin, it may be easiest
+  /// to call SetThreadStoppedAtUnexecutedBP(pc) unconditionally when at
+  /// a BreakpointSite, and later when it is known that it was triggered,
+  /// SetThreadHitBreakpointSite() can be called.  These two methods
+  /// overwrite the same piece of state in the Thread, the last one
+  /// called on a Thread wins.
+  void SetThreadStoppedAtUnexecutedBP(lldb::addr_t pc) {
+    m_stopped_at_unexecuted_bp = pc;
+  }
+  void SetThreadHitBreakpointSite() {
+    m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
+  }
+
   /// Whether this Thread already has all the Queue information cached or not
   ///
   /// A Thread may be associated with a libdispatch work Queue at a given
@@ -1314,6 +1335,9 @@ class Thread : public std::enable_shared_from_this<Thread>,
   bool m_should_run_before_public_stop;  // If this thread has "stop others" 
                                          // private work to do, then it will
                                          // set this.
+  lldb::addr_t m_stopped_at_unexecuted_bp; // Set to the address of a breakpoint
+                                           // instruction that we have not yet
+                                           // hit, but will hit when we resume.
   const uint32_t m_index_id; ///< A unique 1 based index assigned to each thread
                              /// for easy UI/command line access.
   lldb::RegisterContextSP m_reg_context_sp; ///< The register context for this
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index 25cee369d7ee3d..f1853be12638ea 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -488,38 +488,6 @@ const char *StopInfoMachException::GetDescription() {
   return m_description.c_str();
 }
 
-static StopInfoSP GetStopInfoForHardwareBP(Thread &thread, Target *target,
-                                           uint32_t exc_data_count,
-                                           uint64_t exc_sub_code,
-                                           uint64_t exc_sub_sub_code) {
-  // Try hardware watchpoint.
-  if (target) {
-    // The exc_sub_code indicates the data break address.
-    WatchpointResourceSP wp_rsrc_sp =
-        target->GetProcessSP()->GetWatchpointResourceList().FindByAddress(
-            (addr_t)exc_sub_code);
-    if (wp_rsrc_sp && wp_rsrc_sp->GetNumberOfConstituents() > 0) {
-      return StopInfo::CreateStopReasonWithWatchpointID(
-          thread, wp_rsrc_sp->GetConstituentAtIndex(0)->GetID());
-    }
-  }
-
-  // Try hardware breakpoint.
-  ProcessSP process_sp(thread.GetProcess());
-  if (process_sp) {
-    // The exc_sub_code indicates the data break address.
-    lldb::BreakpointSiteSP bp_sp =
-        process_sp->GetBreakpointSiteList().FindByAddress(
-            (lldb::addr_t)exc_sub_code);
-    if (bp_sp && bp_sp->IsEnabled()) {
-      return StopInfo::CreateStopReasonWithBreakpointSiteID(thread,
-                                                            bp_sp->GetID());
-    }
-  }
-
-  return nullptr;
-}
-
 #if defined(__APPLE__)
 const char *
 StopInfoMachException::MachException::Name(exception_type_t exc_type) {
@@ -607,6 +575,25 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
       target ? target->GetArchitecture().GetMachine()
              : llvm::Triple::UnknownArch;
 
+  ProcessSP process_sp(thread.GetProcess());
+  RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
+  // Caveat: with x86 KDP if we've hit a breakpoint, the pc we
+  // receive is past the breakpoint instruction.
+  // If we have a breakpoints at 0x100 and 0x101, we hit the
+  // 0x100 breakpoint and the pc is reported at 0x101.
+  // We will initially mark this thread as being stopped at an
+  // unexecuted breakpoint at 0x101. Later when we see that
+  // we stopped for a Breakpoint reason, we will decrement the
+  // pc, and update the thread to record that we hit the
+  // breakpoint at 0x100.
+  // The fact that the pc may be off by one at this point
+  // (for an x86 KDP breakpoint hit) is not a problem.
+  addr_t pc = reg_ctx_sp->GetPC();
+  BreakpointSiteSP bp_site_sp =
+      process_sp->GetBreakpointSiteList().FindByAddress(pc);
+  if (bp_site_sp && bp_site_sp->IsEnabled())
+    thread.SetThreadStoppedAtUnexecutedBP(pc);
+
   switch (exc_type) {
   case 1: // EXC_BAD_ACCESS
   case 2: // EXC_BAD_INSTRUCTION
@@ -633,171 +620,158 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
     }
     break;
 
+    // A mach exception comes with 2-4 pieces of data.
+    // The sub-codes are only provided for certain types
+    // of mach exceptions.
+    // [exc_type, exc_code, exc_sub_code, exc_sub_sub_code]
+    //
+    // Here are all of the EXC_BREAKPOINT, exc_type==6,
+    // exceptions we can receive.
+    //
+    // Instruction step:
+    //   [6, 1, 0]
+    //   Intel KDP [6, 3, ??]
+    //   armv7 [6, 0x102, <stop-pc>]  Same as software breakpoint!
+    //
+    // Software breakpoint:
+    //   x86 [6, 2, 0]
+    //   Intel KDP [6, 2, <bp-addr + 1>]
+    //   arm64 [6, 1, <bp-addr>]
+    //   armv7 [6, 0x102, <bp-addr>]  Same as instruction step!
+    //
+    // Hardware breakpoint:
+    //   x86 [6, 1, <bp-addr>, 0]
+    //   x86/Rosetta not implemented, see software breakpoint
+    //   arm64 [6, 1, <bp-addr>]
+    //   armv7 not implemented, see software breakpoint
+    //
+    // Hardware watchpoint:
+    //   x86 [6, 1, <accessed-addr>, 0] (both Intel hw and Rosetta)
+    //   arm64 [6, 0x102, <accessed-addr>, 0]
+    //   armv7 [6, 0x102, <accessed-addr>, 0]
+    //
+    // arm64 BRK instruction (imm arg not reflected in the ME)
+    //   [ 6, 1, <addr-of-BRK-insn>]
+    //
+    // In order of codes mach exceptions:
+    //   [6, 1, 0] - instruction step
+    //   [6, 1, <bp-addr>] - hardware breakpoint or watchpoint
+    //
+    //   [6, 2, 0] - software breakpoint
+    //   [6, 2, <bp-addr + 1>] - software breakpoint
+    //
+    //   [6, 3] - instruction step
+    //
+    //   [6, 0x102, <stop-pc>] armv7 instruction step
+    //   [6, 0x102, <bp-addr>] armv7 software breakpoint
+    //   [6, 0x102, <accessed-addr>, 0] arm64/armv7 watchpoint
+
   case 6: // EXC_BREAKPOINT
   {
-    bool is_actual_breakpoint = false;
-    bool is_trace_if_actual_breakpoint_missing = false;
-    switch (cpu) {
-    case llvm::Triple::x86:
-    case llvm::Triple::x86_64:
-      if (exc_code == 1) // EXC_I386_SGL
-      {
-        if (!exc_sub_code) {
-          // This looks like a plain trap.
-          // Have to check if there is a breakpoint here as well.  When you
-          // single-step onto a trap, the single step stops you not to trap.
-          // Since we also do that check below, let's just use that logic.
-          is_actual_breakpoint = true;
-          is_trace_if_actual_breakpoint_missing = true;
-        } else {
-          if (StopInfoSP stop_info =
-                  GetStopInfoForHardwareBP(thread, target, exc_data_count,
-                                           exc_sub_code, exc_sub_sub_code))
-            return stop_info;
-        }
-      } else if (exc_code == 2 || // EXC_I386_BPT
-                 exc_code == 3)   // EXC_I386_BPTFLT
-      {
-        // KDP returns EXC_I386_BPTFLT for trace breakpoints
-        if (exc_code == 3)
-          is_trace_if_actual_breakpoint_missing = true;
-
-        is_actual_breakpoint = true;
+    bool stopped_by_hitting_breakpoint = false;
+    bool stopped_by_completing_stepi = false;
+    bool stopped_watchpoint = false;
+    std::optional<addr_t> address;
+
+    // exc_code 1
+    if (exc_code == 1) {
+      if (exc_sub_code == 0) {
+        stopped_by_completing_stepi = true;
+      } else {
+        // Ambiguous: could be signalling a
+        // breakpoint or watchpoint hit.
+        stopped_by_hitting_breakpoint = true;
+        stopped_watchpoint = true;
+        address = exc_sub_code;
+      }
+    }
+
+    // exc_code 2
+    if (exc_code == 2) {
+      if (exc_sub_code == 0)
+        stopped_by_hitting_breakpoint = true;
+      else {
+        stopped_by_hitting_breakpoint = true;
+        // Intel KDP software breakpoint
         if (!pc_already_adjusted)
           pc_decrement = 1;
       }
-      break;
+    }
 
-    case llvm::Triple::arm:
-    case llvm::Triple::thumb:
-      if (exc_code == 0x102) // EXC_ARM_DA_DEBUG
-      {
-        // LWP_TODO: We need to find the WatchpointResource that matches
-        // the address, and evaluate its Watchpoints.
-
-        // It's a watchpoint, then, if the exc_sub_code indicates a
-        // known/enabled data break address from our watchpoint list.
-        lldb::WatchpointSP wp_sp;
-        if (target)
-          wp_sp = target->GetWatchpointList().FindByAddress(
-              (lldb::addr_t)exc_sub_code);
-        if (wp_sp && wp_sp->IsEnabled()) {
-          return StopInfo::CreateStopReasonWithWatchpointID(thread,
-                                                            wp_sp->GetID());
-        } else {
-          is_actual_breakpoint = true;
-          is_trace_if_actual_breakpoint_missing = true;
-        }
-      } else if (exc_code == 1) // EXC_ARM_BREAKPOINT
-      {
-        is_actual_breakpoint = true;
-        is_trace_if_actual_breakpoint_missing = true;
-      } else if (exc_code == 0) // FIXME not EXC_ARM_BREAKPOINT but a kernel
-                                // is currently returning this so accept it
-                                // as indicating a breakpoint until the
-                                // kernel is fixed
-      {
-        is_actual_breakpoint = true;
-        is_trace_if_actual_breakpoint_missing = true;
-      }
-      break;
+    // exc_code 3
+    if (exc_code == 3)
+      stopped_by_completing_stepi = true;
 
-    case llvm::Triple::aarch64_32:
-    case llvm::Triple::aarch64: {
-      // xnu describes three things with type EXC_BREAKPOINT:
-      //
-      //   exc_code 0x102 [EXC_ARM_DA_DEBUG], exc_sub_code addr-of-insn
-      //      Watchpoint access.  exc_sub_code is the address of the
-      //      instruction which trigged the watchpoint trap.
-      //      debugserver may add the watchpoint number that was triggered
-      //      in exc_sub_sub_code.
-      //
-      //   exc_code 1 [EXC_ARM_BREAKPOINT], exc_sub_code 0
-      //      Instruction step has completed.
-      //
-      //   exc_code 1 [EXC_ARM_BREAKPOINT], exc_sub_code address-of-instruction
-      //      Software breakpoint instruction executed.
-
-      if (exc_code == 1 && exc_sub_code == 0) // EXC_ARM_BREAKPOINT
-      {
-        // This is hit when we single instruction step aka MDSCR_EL1 SS bit 0
-        // is set
-        is_actual_breakpoint = true;
-        is_trace_if_actual_breakpoint_missing = true;
-        if (thread.GetTemporaryResumeState() != eStateStepping)
-          not_stepping_but_got_singlestep_exception = true;
-      }
-      if (exc_code == 0x102) // EXC_ARM_DA_DEBUG
-      {
-        // LWP_TODO: We need to find the WatchpointResource that matches
-        // the address, and evaluate its Watchpoints.
-
-        // It's a watchpoint, then, if the exc_sub_code indicates a
-        // known/enabled data break address from our watchpoint list.
-        lldb::WatchpointSP wp_sp;
-        if (target)
-          wp_sp = target->GetWatchpointList().FindByAddress(
-              (lldb::addr_t)exc_sub_code);
-        if (wp_sp && wp_sp->IsEnabled()) {
-          return StopInfo::CreateStopReasonWithWatchpointID(thread,
-                                                            wp_sp->GetID());
-        }
-        // EXC_ARM_DA_DEBUG seems to be reused for EXC_BREAKPOINT as well as
-        // EXC_BAD_ACCESS
-        if (thread.GetTemporaryResumeState() == eStateStepping)
-          return StopInfo::CreateStopReasonToTrace(thread);
+    // exc_code 0x102
+    if (exc_code == 0x102 && exc_sub_code != 0) {
+      if (cpu == llvm::Triple::arm || cpu == llvm::Triple::thumb) {
+        stopped_by_hitting_breakpoint = true;
+        stopped_by_completing_stepi = true;
       }
-      // It looks like exc_sub_code has the 4 bytes of the instruction that
-      // triggered the exception, i.e. our breakpoint opcode
-      is_actual_breakpoint = exc_code == 1;
-      break;
+      stopped_watchpoint = true;
+      address = exc_sub_code;
     }
 
-    default:
-      break;
-    }
+    // The Mach Exception may have been ambiguous --
+    // e.g. we stopped either because of a breakpoint
+    // or a watchpoint.  We'll disambiguate which it
+    // really was.
 
-    if (is_actual_breakpoint) {
-      RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
+    if (stopped_by_hitting_breakpoint) {
       addr_t pc = reg_ctx_sp->GetPC() - pc_decrement;
 
-      ProcessSP process_sp(thread.CalculateProcess());
-
-      lldb::BreakpointSiteSP bp_site_sp;
-      if (process_sp)
+      if (address)
+        bp_site_sp =
+            process_sp->GetBreakpointSiteList().FindByAddress(*address);
+      if (!bp_site_sp && reg_ctx_sp) {
         bp_site_sp = process_sp->GetBreakpointSiteList().FindByAddress(pc);
+      }
       if (bp_site_sp && bp_site_sp->IsEnabled()) {
-        // Update the PC if we were asked to do so, but only do so if we find
-        // a breakpoint that we know about cause this could be a trap
-        // instruction in the code
-        if (pc_decrement > 0 && adjust_pc_if_needed)
-          reg_ctx_sp->SetPC(pc);
-
-        // If the breakpoint is for this thread, then we'll report the hit,
-        // but if it is for another thread, we can just report no reason.  We
-        // don't need to worry about stepping over the breakpoint here, that
-        // will be taken care of when the thread resumes and notices that
-        // there's a breakpoint under the pc. If we have an operating system
-        // plug-in, we might have set a thread specific breakpoint using the
-        // operating system thread ID, so we can't make any assumptions about
-        // the thread ID so we must always report the breakpoint regardless
-        // of the thread.
+        // We've hit this breakpoint, whether it was intended for this thread
+        // or not.  Clear this in the Tread object so we step past it on resume.
+        thread.SetThreadHitBreakpointSite();
+
+        // If we have an operating system plug-in, we might have set a thread
+        // specific breakpoint using the operating system thread ID, so we
+        // can't make any assumptions about the thread ID so we must always
+        // report the breakpoint regardless of the thread.
         if (bp_site_sp->ValidForThisThread(thread) ||
-            thread.GetProcess()->GetOperatingSystem() != nullptr)
+            thread.GetProcess()->GetOperatingSystem() != nullptr) {
+          // Update the PC if we were asked to do so, but only do so if we find
+          // a breakpoint that we know about cause this could be a trap
+          // instruction in the code
+          if (pc_decrement > 0 && adjust_pc_if_needed && reg_ctx_sp)
+            reg_ctx_sp->SetPC(pc);
           return StopInfo::CreateStopReasonWithBreakpointSiteID(
               thread, bp_site_sp->GetID());
-        else if (is_trace_if_actual_breakpoint_missing)
-          return StopInfo::CreateStopReasonToTrace(thread);
-        else
+        } else {
           return StopInfoSP();
+        }
       }
+    }
 
-      // Don't call this a trace if we weren't single stepping this thread.
-      if (is_trace_if_actual_breakpoint_missing &&
-          thread.GetTemporaryResumeState() == eStateStepping) {
-        return StopInfo::CreateStopReasonToTrace(thread);
+    // Breakpoint-hit events are handled.
+    // Now handle watchpoints.
+
+    if (stopped_watchpoint && address) {
+      WatchpointResourceSP wp_rsrc_sp =
+          target->GetProcessSP()->GetWatchpointResourceList().FindByAddress(
+              *address);
+      if (wp_rsrc_sp && wp_rsrc_sp->GetNumberOfConstituents() > 0) {
+        return StopInfo::CreateStopReasonWithWatchpointID(
+            thread, wp_rsrc_sp->GetConstituentAtIndex(0)->GetID());
       }
     }
+
+    // Finally, handle instruction step.
+
+    if (stopped_by_completing_stepi) {
+      if (thread.GetTemporaryResumeState() != eStateStepping)
+        not_stepping_but_got_singlestep_exception = true;
+      else
+        return StopInfo::CreateStopReasonToTrace(thread);
+    }
+
   } break;
 
   case 7:  // EXC_SYSCALL
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index f383b3d40a4f3a..8b12b87eacbc61 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -377,24 +377,17 @@ void ProcessWindows::RefreshStateAfterStop() {
   if (!stop_thread)
     return;
 
-  switch (active_exception->GetExceptionCode()) {
-  case EXCEPTION_SINGLE_STEP: {
-    RegisterContextSP register_context = stop_thread->GetRegisterContext();
-    const uint64_t pc = register_context->GetPC();
-    BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
-    if (site && site->ValidForThisThread(*stop_thread)) {
-      LLDB_LOG(log,
-               "Single-stepped onto a breakpoint in process {0} at "
-               "address {1:x} with breakpoint site {2}",
-               m_session_data->m_debugger->GetProcess().GetProcessId(), pc,
-               site->GetID());
-      stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(*stop_thread,
-                                                                 site->GetID());
-      stop_thread->SetStopInfo(stop_info);
+  RegisterContextSP register_context = stop_thread->GetRegisterContext();
+  uint64_t pc = register_context->GetPC();
 
-      return;
-    }
+  // If we're at a BreakpointSite, mark this as an Unexecuted Breakpoint.
+  // We'll clear that state if we've actually executed the breakpoint.
+  BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
+  if (site && site->IsEnabled())
+    stop_thread->SetThreadStoppedAtUnexecutedBP(pc);
 
+  switch (active_exception->GetExceptionCode()) {
+  case EXCEPTION_SINGLE_STEP: {
     auto *reg_ctx = static_cast<RegisterContextWindows *>(
         stop_thread->GetRegisterContext().get());
     uint32_t slot_id = reg_ctx->GetTriggeredHardwareBreakpointSlotId();
@@ -419,8 +412,6 @@ void ProcessWindows::RefreshStateAfterStop() {
   }
 
   case EXCEPTION_BREAKPOINT: {
-    RegisterContextSP register_context = stop_thread->GetRegisterContext();
-
     int breakpoint_size = 1;
     switch (GetTarget().GetArchitecture().GetMachine()) {
     case llvm::Triple::aarch64:
@@ -443,9 +434,9 @@ void ProcessWindows::RefreshStateAfterStop() {
     }
 
     // The current PC is AFTER the BP opcode, on all architectures.
-    uint64_t pc = register_context->GetPC() - breakpoint_size;
+    pc = register_context->GetPC() - breakpoint_size;
 
-    BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
+    site = GetBreakpointSiteList().FindByAddress(pc);
     if (site) {
       LLDB_LOG(log,
                "detected breakp...
[truncated]

``````````

</details>


https://github.com/llvm/llvm-project/pull/105594


More information about the lldb-commits mailing list