[Lldb-commits] [PATCH] D129814: Fix stepping over watchpoints in architectures that raise the exception before executing the instruction

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/source/Target/StopInfo.cpp:805
+
+      m_step_over_wp_sp.reset(new ThreadPlanStepOverWatchpoint(
+          *(thread_sp.get()), shared_from_this(), wp_sp));
----------------
labath wrote:
> This creates a shared pointer loop, does it not?
> 
> IIUC, this `m_step_over_wp_sp` is used only to check whether the plan is complete. Maybe, instead of the pointer, we could have a flag that the thread plan sets when it finishes?
Ack, yes.  I added a "I'm complete" callback to the StopInfoWatchpoint, and the ThreadPlan calls that.


================
Comment at: lldb/source/Target/StopInfo.cpp:809
+      error = thread_sp->QueueThreadPlan(m_step_over_wp_sp, false);
+      if (error.Success())
+        return false;
----------------
DavidSpickett wrote:
> `return error.Success()` ?
Yeah, sometimes it's useful to break on one or other condition w/o having to add conditional breakpoints, but that doesn't seem to be the prevailing style.


================
Comment at: lldb/source/Target/StopInfo.cpp:898-901
                 if (scalar_value.ULongLong(1) == 0) {
-                  // We have been vetoed.  This takes precedence over querying
-                  // the watchpoint whether it should stop (aka ignore count
-                  // and friends).  See also StopInfoWatchpoint::ShouldStop()
-                  // as well as Process::ProcessEventData::DoOnRemoval().
+                  // The condition failed, which we consider "not having hit
+                  // the watchpoint" so undo the hit count here.
+                  wp_sp->UndoHitCount();
----------------
labath wrote:
> How does this relate to the thread plan part of the patch?
This is a drive-by fix.  The point of this effort was to make the hit counts correct for Watchpoints, and as I was reading through the code I noticed that we were actually failing to do that in two ways: mishandling the case of multiple simultaneous watchpoint hits, and mishandling failed condition hits.

I mentioned in the initial comments that I fixed them both in one patch, but I can separate them into two patches if that bugs you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129814/new/

https://reviews.llvm.org/D129814