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

[Jason Molenda via lldb-commits]

jasonmolenda wrote:

> > @AlexK0 if you have a setup to build and run the testsuite on windows, could you try it with this patch some time when you're able? I can't see this being merged for at least another 5-6 days, there's no rush. You can download the unidiff style diff of the patch from https://patch-diff.githubusercontent.com/raw/llvm/llvm-project/pull/96260.diff
> 
> @jasonmolenda I checked the tests with VS2022/x86-64/win11 in a debug build with assertions enabled. Unfortunately, the main branch is a bit unstable, and some tests constantly fail :( . Nonetheless, I noticed one new failure with the applied patch:
> 
> `functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py`
>

These TestConcurrent / TestConsecutiveBreakpoints tests are great for finding corner cases with these changes, thanks so much.  I was a little uncertain about one part of my ProcessWindows change, where the pc is *past* the breakpoint when a software breakpoint instruction is used, on Windows.  For a moment, I thought, "oh, I don't need to record that we hit the breakpoint because we're already past it and we don't need to instruction past it" but of course that was wrong -- ProcessWindows::RefreshStateAfterStop decrements the $pc value by the size of its breakpoint instruction, which is necessary because e.g. the size of an x86 breakpoint instruction is 1 byte (0xcc) but x86_64 instructions can be between 1 to 15 bytes long, so stopping 1 byte after the BreakpointSite means we are possibly in the middle of the real instruction.  We must set the pc to the BreakpointSite address and use lldb's normal logic.

Anyway, tl;dr, I believe this will fix it:

```
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -442,6 +442,7 @@ void ProcessWindows::RefreshStateAfterStop() {
                m_session_data->m_debugger->GetProcess().GetProcessId(), pc,
                site->GetID());
 
+      stop_thread->SetThreadHitBreakpointAtAddr(pc);
       if (site->ValidForThisThread(*stop_thread)) {
         LLDB_LOG(log,
                  "Breakpoint site {0} is valid for this thread ({1:x}), "
```

I'll push it right now.  Sorry for my confusion, and thanks again for looking at it, this was a big help.

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