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

[via lldb-commits]

dlav-sc wrote:

> I think the separation of "the low level Process code says why we stopped, and sets an appropriate StopInfo" and then the StopInfo controls how the system responds to that stop has been a really useful division of labors.

Yes, this separation of concerns is quite useful here. However, when it comes to watchpoint/breakpoint checks, I'm not sure they should be part of the "system response" layer.

While I agree with your point that "the low level Process code says why we stopped, and sets an appropriate StopInfo", my key point is that the current implementation can't always correctly derive `StopInfo`. Specifically, in my example, the correct `StopInfo` should have been `StopInfoTrace`, not `StopInfoWatchpoint`.

In my view, the low level code failed to determine the right `StopInfo` due to the lack of the information about whether we will actually report a watchpoint hit or not. If the low-level code could obtain this information, it could discard `StopInfoWatchpoint` when check fails and look for an alternative `StopInfo`. This fact suggests that watchpoint/breakpoint checks should be performed within the low-level code rather than in the `StopInfo`.

> but rather a fairly simple bug in the ThreadPlanStepOver logic

I have some thoughts about the solution that only adjusts `StepOverThreadPlan` logic to fix the behavior in my example. However, I'm not sure this would be a proper approach and I still believe the low level code should be able to perform watchpoint/breakpoint checks to determine the correct `StopInfo`. 

We could try to make `StepOverThreadPlan::DoPlanExplainsStop` accept `watchpoint stop reason` and create `StopInfoTrace` when execution has reached the target address of a step. While such changes would correct the behavior in my example, they would break the implicit priority rule that makes `StopInfoWatchpoint` more important than `StopInfoTrace`. This could be an issue because when both conditions fire simultaneously, we always want to report the watchpoint hit to the user (not the step stop reason).

For example, let's consider my example without any ignore count. After the `next` command, we would expect to see a watchpoint hit. However with only these changes, I suspect we would end up reporting the step stop reason after the `next` and silently skipping the watchpoint hit.

To address this, we could additionally configure `StepOverThreadPlan` as `non-Controlling` and `OkayToDiscard` thread plan, which would allow thread plans lower in the stack to override the `StopInfoTrace`. However, I'm not sure this approach wouldn't break something, because currently, as far as I know, `StepOverThreadPlan` is always `Controlling` thread plan.

That said, I should mention that I don't know all the details of how thread plans work, so I might be missing something here. So there indeed could be a better approach, which I haven't considered, that could fix the issue by only adjusting the thread plan logic.

To summarize, I still have two main concerns:
* Should watchpoint/breakpoint checks really be part of the "system response"? Intuitively, it seems like they should inform the initial `StopInfo` determination.
* I'm not convinced this is purely a `StepOverThreadPlan` issue that can be fixed by adjusting only this thread plan.

Could you share your thoughts on the matter, please?

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