[Lldb-commits] [lldb] [lldb] Add pc check for thread-step-by-bp algorithms (PR #108504)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda created https://github.com/llvm/llvm-project/pull/108504

lldb-server built with NativeProcessLinux.cpp and NativeProcessFreeBSD.cpp can use breakpoints to implement instruction stepping on cores where there is no native instruction-step primitive.  Currently these set a breakpoint, continue, and if we hit the breakpoint with the original thread, set the stop reason to be "trace".

I am wrapping up a change to lldb's breakpoint algorithm where I change its current behavior of

"if a thread stops at a breakpoint site, we set
the thread's stop reason to breakpoint-hit, even if the breakpoint hasn't been executed" +
"when resuming any thread at a breakpoint site, instruction-step past the breakpoint before resuming"

to a behavior of

"when a thread executes a breakpoint, set the stop reason to breakpoint-hit" +
"when a thread has hit a breakpoint, when the thread resumes, we silently step past the breakpoint and then resume the thread".

For these lldb-server targets doing breakpoint stepping, this means that if we are sitting on a breakpoint that has not yet executed, and instruction-step the thread, we will execute the breakpoint instruction at $pc (instead of $next-pc where it meant to go), and stop again -- at the same pc value.  Then we will rewrite the stop reason to 'trace'.  The higher level logic will see that we haven't hit the breakpoint instruction again, so it will try to instruction step again, hitting the breakpoint again forever.

To fix this, I'm checking that the thread matches the one we are instruction-stepping-by-breakpoint AND that we've stopped at the breakpoint address we are stepping to.  Only in that case will the stop reason be rewritten to "trace" hiding the implementation detail that the step was done by breakpoints.

>From 4d676434b6951b817bed17f73f3842414aabb52a Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Thu, 12 Sep 2024 23:14:08 -0700
Subject: [PATCH] [lldb] Add pc check for thread-step-by-bp algorithms

lldb-server built with NativeProcessLinux.cpp and NativeProcessFreeBSD.cpp
can use breakpoints to implement instruction stepping on cores where
there is no native instruction-step primitive.  Currently these set a
breakpoint, continue, and if we hit the breakpoint with the original
thread, set the stop reason to be "trace".

I am wrapping up a change to lldb's breakpoint algorithm where I change
its current behavior of

"if a thread stops at a breakpoint site, we set
the thread's stop reason to breakpoint-hit, even if the breakpoint
hasn't been executed" +
"when resuming any thread at a breakpoint site, instruction-step past
the breakpoint before resuming"

to a behavior of

"when a thread executes a breakpoint, set the stop reason to
breakpoint-hit" +
"when a thread has hit a breakpoint, when the thread resumes,
we silently step past the breakpoint and then resume the thread".

For these lldb-server targets doing breakpoint stepping, this means
that if we are sitting on a breakpoint that has not yet executed,
and instruction-step the thread, we will execute the breakpoint
instruction at $pc (instead of $next-pc where it meant to go), and
stop again -- at the same pc value.  Then we will rewrite the stop
reason to 'trace'.  The higher level logic will see that we haven't
hit the breakpoint instruction again, so it will try to instruction
step again, hitting the breakpoint again forever.

To fix this, I'm checking that the thread matches the one we are
instruction-stepping-by-breakpoint AND that we've stopped at the
breakpoint address we are stepping to.  Only in that case will the
stop reason be rewritten to "trace" hiding the implementation detail
that the step was done by breakpoints.
---
 .../Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp       | 5 ++++-
 lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp   | 7 +++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp
index 97fff4b9f65a82..80b27571f43d55 100644
--- a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp
+++ b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp
@@ -319,9 +319,12 @@ void NativeProcessFreeBSD::MonitorSIGTRAP(lldb::pid_t pid) {
                info.pl_siginfo.si_addr);
 
       if (thread) {
+        auto &regctx = static_cast<NativeRegisterContextFreeBSD &>(
+            thread->GetRegisterContext());
         auto thread_info =
             m_threads_stepping_with_breakpoint.find(thread->GetID());
-        if (thread_info != m_threads_stepping_with_breakpoint.end()) {
+        if (thread_info != m_threads_stepping_with_breakpoint.end() &&
+            threads_info->second == regctx.GetPC()) {
           thread->SetStoppedByTrace();
           Status brkpt_error = RemoveBreakpoint(thread_info->second);
           if (brkpt_error.Fail())
diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
index 5c262db8db7fde..38b7092682873b 100644
--- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -829,8 +829,11 @@ void NativeProcessLinux::MonitorBreakpoint(NativeThreadLinux &thread) {
   thread.SetStoppedByBreakpoint();
   FixupBreakpointPCAsNeeded(thread);
 
-  if (m_threads_stepping_with_breakpoint.find(thread.GetID()) !=
-      m_threads_stepping_with_breakpoint.end())
+  NativeRegisterContextLinux &reg_ctx = thread.GetRegisterContext();
+  auto stepping_with_bp_it =
+      m_threads_stepping_with_breakpoint.find(thread.GetID());
+  if (stepping_with_bp_it != m_threads_stepping_with_breakpoint.end() &&
+      stepping_with_bp_it->second == reg_ctx.GetPC())
     thread.SetStoppedByTrace();
 
   StopRunningThreads(thread.GetID());