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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
