[Lldb-commits] [lldb] [lldb][NFC] Refactor Watchpoint class (PR #163695)

[via lldb-commits]

dlav-sc wrote:

I'd like to share some concerns regarding `StopInfo{Watchpoint, Breakpoint}`. You mentioned that you want to unify the logic of these classes, so this information might be relevant. Or perhaps you are already aware of the issue I want to demonstrate.

>From my point of view, performing any checks regarding whether there was a watchpoint hit or not in `StopInfo` is quite questionable. I'll try to illustrate my concerns with an example that you can reproduce by yourself using `ToT lldb`. Let's consider a program:
```
int main() {
  int watch_value = 10;
  int a = 5;
  watch_value += a;
  int b = 5;
  watch_value = 42;
}
```
Consider this scenario:

1. I set a watchpoint on `watch_value`
2. I configure a large ignore count for this watchpoint
3. I attempt to step to the next instruction using `next`

```
(lldb) b main
(lldb) r
-> int watch_value = 10;
(lldb) wa s v watch_value // set hw wp on watch_value
(lldb) watchpoint ignore -i 100 1 // set ignore_count=100
(lldb) next
Process 3706163 exited with status = 0 (0x00000000)
```
I expected to obtain a control at the next line, however as we can see, lldb doesn't return control to the user.
\
\
\
To understand the source of the issue, I decided to examine the `lldb step` logs. Here's what I saw after the `next` command:
```
intern-state     ThreadList::ShouldStop: 1 threads, 1 unsuspended threads                                                                                                               
intern-state     Thread::ShouldStop(0x5652ead54180) for tid = 0x3a7483 0x3a7483, pc = 0x000055555555513b                                                                                
intern-state     ^^^^^^^^ Thread::ShouldStop Begin ^^^^^^^^                                                                                                                             
intern-state     Plan stack initial state:                                                                                                                                              
  thread #1: tid = 0x3a7483:                                                                                                                                                            
    Active plan stack:                                                                                                                                                                  
      Element 0: Base thread plan.                                                                                                                                                      
      Element 1: Stepping over line ig_c.c:2:7 using ranges: [0x0000555555555134-0x000055555555513b).                                                                                   
      Element 2: Single stepping past breakpoint site 2 at 0x555555555134

intern-state     Step over breakpoint stopped for reason: watchpoint.
intern-state     ThreadPlanStepOverRange got asked if it explains the stop for some reason other than step.
intern-state     Plan base plan explains stop. 
intern-state     Base plan discarding thread plans for thread tid = 0x3a7483 (breakpoint hit.)
intern-state     Discarding thread plans for thread (tid = 0x3a7483, force 0)
intern-state     Plan Step over breakpoint trap being discarded in cleanup, it says it is already done.
intern-state     Discarding plan: "Step over breakpoint trap", tid = 0x3a7483.
intern-state     Step range plan out of range to 0x55555555513b
intern-state     Plan Step range stepping over being discarded in cleanup, it says it is already done.
intern-state     Popping plan: "Step range stepping over", tid = 0x3a7483.
intern-state     Plan stack final state:
  thread #1: tid = 0x3a7483:
    Active plan stack:
      Element 0: Base thread plan.
    Completed plan stack:
      Element 0: Stepping over line ig_c.c:2:7 using ranges: [0x0000555555555134-0x000055555555513b).
    Discarded plan stack:
      Element 0: Single stepping past breakpoint site 2 at 0x555555555134
```
As we can see, `lldb-server` sends a `watchpoint stop reason` to us. This is expected because before the first instruction of the `main()`, `watch_value` was uninitialized and contained garbage, and after the instruction it was initialized with 10. `lldb-server` detected the memory access and stopped execution with a `watchpoint stop reason`.

`lldb-client` starts its usual `ShouldStop` procedure, which involves walking through the thread plan stack. `StepOverThreadPlan` doesn't handle `watchpoint stop reason` (it expects `trace stop reason`), therefore we falls through to `ThreadPlanBase`.

After that, we initialize `StopInfoWatchpoint` and only in `PerformAction` do we discover that the ignore count condition is not satisfied. In that case, we don't report the watchpoint hit to the user and continue execution instead. Meanwhile `StepOverThreadPlan` has already been marked complete and removed from the stack. Therefore, before resuming, we have only the `ThreadPlanBase` plan on the stack:

```
intern-state     Plan stack initial state:
  thread #1: tid = 0x3a7483:
    Active plan stack:
      Element 0: Base thread plan.
```
As a result, with only `ThreadPlanBase` remaining, execution resumes without stopping.
\
\
\
The fundamental problem for me is that watchpoint checks are performed too late.

This behavior led me to believe that we should perform **all** watchpoint/breakpoint checks elsewhere, **before** creating `StopInfo{Watchpoint, Breakpoint}`.

I'm curious to hear your thoughts about the issue. Do you see this as a valid concern?

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