[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)

[via lldb-commits]

================
@@ -1880,60 +1849,40 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
               StopInfo::CreateStopReasonVForkDone(*thread_sp));
           handled = true;
         }
-      } else if (!signo) {
-        addr_t pc = thread_sp->GetRegisterContext()->GetPC();
-        lldb::BreakpointSiteSP bp_site_sp =
-            thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
-
-        // If a thread is stopped at a breakpoint site, set that as the stop
-        // reason even if it hasn't executed the breakpoint instruction yet.
-        // We will silently step over the breakpoint when we resume execution
-        // and miss the fact that this thread hit the breakpoint.
-        if (bp_site_sp && bp_site_sp->ValidForThisThread(*thread_sp)) {
-          thread_sp->SetStopInfo(StopInfo::CreateStopReasonWithBreakpointSiteID(
-              *thread_sp, bp_site_sp->GetID()));
-          handled = true;
-        }
       }
 
       if (!handled && signo && !did_exec) {
         if (signo == SIGTRAP) {
           // Currently we are going to assume SIGTRAP means we are either
           // hitting a breakpoint or hardware single stepping.
-          handled = true;
-          addr_t pc =
-              thread_sp->GetRegisterContext()->GetPC() + m_breakpoint_pc_offset;
-          lldb::BreakpointSiteSP bp_site_sp =
-              thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(
-                  pc);
 
-          if (bp_site_sp) {
-            // 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 (bp_site_sp->ValidForThisThread(*thread_sp)) {
-              if (m_breakpoint_pc_offset != 0)
-                thread_sp->GetRegisterContext()->SetPC(pc);
-              thread_sp->SetStopInfo(
-                  StopInfo::CreateStopReasonWithBreakpointSiteID(
-                      *thread_sp, bp_site_sp->GetID()));
-            } else {
-              StopInfoSP invalid_stop_info_sp;
-              thread_sp->SetStopInfo(invalid_stop_info_sp);
-            }
+          // If we were stepping then assume the stop was the result of the
----------------
jimingham wrote:

The previous algorithm reversed this assumption.  It first checked for the breakpoint, and if a breakpoint was at the site, then returned either a breakpoint hit (if it was for this thread) or no reason, and only if there wasn't a breakpoint there did it return the trace result.

There was also another case that I don't see handled here anymore, if we weren't at a breakpoint, and we weren't stepping, then we made a "StopReasonSignal.

I don't understand why it was right to switch the order of attributing causes here, or why we no longer need the Signal fallback.

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