dlav-sc wrote: > Could you explain in broad strokes the theme of the refactoring here? This is > a big PR to go in without a summary of what to expect.
**I ==== StopInfo.cpp/Watchpoint.cpp ======================================** Prior to this patch, all logic for evaluating a watchpoint hit on the host side was implemented in the `StopInfoWatchpoint::PerformAction` function. Specifically, checks for the watchpoint condition https://github.com/llvm/llvm-project/pull/159807/files#diff-08d3a818bf87a1dc1d1558dec9570f8b4f27fb0a1bd0a8d37c74b29d43a8b92aL979-L1026, its ignore count https://github.com/llvm/llvm-project/pull/159807/files#diff-08d3a818bf87a1dc1d1558dec9570f8b4f27fb0a1bd0a8d37c74b29d43a8b92aL1056-L1058, and an additional check for watchpoints of the modify type https://github.com/llvm/llvm-project/pull/159807/files#diff-08d3a818bf87a1dc1d1558dec9570f8b4f27fb0a1bd0a8d37c74b29d43a8b92aL1056-L1058 were implemented within this function. These checks are also necessary for software watchpoints. However, these checks are not performed within the `StopInfoWatchpoint::PerformAction` function but are instead handled by a special `ThreadPlan`, which is not part of this patch. The reason is the difference in how hardware and software watchpoints are handled. In the case of a hardware watchpoint, we enter `StopInfoWatchpoint::PerformAction` after the LLDB client sets the `WatchpointStopReason`, which itself happens after the `lldb-server` reports a watchpoint hit. In contrast, for a software watchpoint, all the logic is implemented on the client side; the `lldb-server` knows nothing about them. A special `ThreadPlan` is responsible for software watchpoints checks. When this `ThreadPlan` detects a hit, it sets the `WatchpointStopReason`, and only after that do we enter the `StopInfoWatchpoint::PerformAction` function. Furthermore, there were software design issues. For example, the `Watchpoint` class had `StopInfoWatchpoint` as a `friend` https://github.com/llvm/llvm-project/pull/159807/files#diff-19e75401810e5613449c129f4525becae9b6ea148f9487bf8f68aa657b6277aeL194 because `StopInfoWatchpoint::PerformAction` needed to modify the private `m_hit_counter` field of the `Watchpoint` class. These reasons led me to the idea that the watchpoint evaluation checks on the client side should be implemented directly within the `Watchpoint` class. Please note the `Watchpoint::WatchedValueReportable` function, into which a large part of the logic from `StopInfoWatchpoint::PerformAction` was moved. This includes the watchpoint condition check (which was refactored into a separate function `CheckWatchpointCondition` https://github.com/llvm/llvm-project/pull/159807/files#diff-5496aa2a96cec82fa4af70a90383248937efbf2cea6d19f768ecf8163b1f45c8R268 ), the ignore count check https://github.com/llvm/llvm-project/pull/159807/files#diff-5496aa2a96cec82fa4af70a90383248937efbf2cea6d19f768ecf8163b1f45c8R268, the software watchpoint check https://github.com/llvm/llvm-project/pull/159807/files#diff-5496aa2a96cec82fa4af70a90383248937efbf2cea6d19f768ecf8163b1f45c8R332-R340, and an additional check for hardware modify watchpoints https://github.com/llvm/llvm-project/pull/159807/files#diff-5496aa2a96cec82fa4af70a90383248937efbf2cea6d19f768ecf8163b1f45c8R332-R340. This allowed me to eliminate the need for `StopInfoWatchpoint` to be a `friend` and to reuse the logic for checking software watchpoints in the corresponding `ThreadPlan`. Note: I simply moved the `WatchpointSentry` https://github.com/llvm/llvm-project/pull/159807/files#diff-5496aa2a96cec82fa4af70a90383248937efbf2cea6d19f768ecf8163b1f45c8R332-R340 class entirely from `StopInfo.cpp` to `Watchpoint.h`, as I also need this class for software watchpoint checks. **P.S.** Furthermore, I believe that checking the condition of a hardware watchpoint and its ignore count within `StopInfoWatchpoint::PerformAction` is, in general, a bad idea. Conceptually, setting the `WatchpointStopReason` should occur **after** all checks have been completed, specifically the checks for the watchpoint condition and its ignore count. I don't remember all the intricacies anymore, but I encountered some very rare and exceptional bugs where a watchpoint with an ignore count would break the operation of `step/next`, leading to a situation where the debugger wouldn't regain control after the `step/next` command. These are very subtle issues; the stars really have to align to catch something like that. https://github.com/llvm/llvm-project/pull/159807 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
