jingham created this revision.
jingham added reviewers: labath, clayborg, jasonmolenda, JDevlieghere.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
This is a reworking of the previous patch to get the watchpoint hit counts to
be accurate on systems where you have to walk the PC past the faulting
instruction before reporting the stop. The previous patch worked reliably on
macOS, but failed on the Linux aarch64 bots. As I suspected from the
discussion of the previous patch, the problem was that the threads that don't
get to step over the faulting instruction see a new StopInfoWatchpoint on the
next stop, and so they schedule a new step-over plan, and things go poorly
after that...
I fixed that issue by checking whether the thread had gotten a chance to run in
StopInfoWatchpoint::ShouldStopSynchronous and not doing any work in that case.
In the answer to Pavel's question, I managed to get both modes to work but it
is much easier for lldb to deal with debugserver's clearing of stop info's on
each step, than lldb-server's preserving them. The former is easy for lldb to
reason about, since it is in control of what stop-info's it decides to
preserve, the latter requires a little guessing...
However, even with that fix, the "threads/concurrent_events" Linux test runs
that had several watchpoints and a breakpoint or signal were flaky. The reason
for that is that depending on how events arrived, a signal or breakpoint event
could force a stop before the threads stopped at watchpoints all had a chance
to move past their faulting instructions. Then on Linux, we would get all the
unprocessed watchpoint hits reported anew when the breakpoint stopped so that
it looked like the watchpoint threads had hit the watchpoints too many times.
I fixed that by adding an explicit mechanism for a thread & it's thread plans
to say "I have work that needs to be done before a public stop can be
declared". That way all the threads that stop on the watchpoint can force
their step-instructions before the final stop is reported, and we won't end up
with some having run the faulting instruction and others not, causing us to
overcount the hits. That's the ShouldRunBeforePublicStop machinery.
The only other substantial change over the previous patch is that there were a
handful of places where we weren't being rigorous about our rule that "LLDB
preserves the StopInfo for thread that doesn't get to run". In one place, we
were using GetResumeState not GetTemporaryResumeState to check whether the
thread had run. GetResumeState only returns true if the User suspended the
thread, not if lldb has suspended it for this one resume for its own purposes,
so it wasn't the right test. And in another place, if we get thread stop info
in the JSON form, we would unconditionally reset all the threads w/o checking
if any of them had run or not.
With this I'm getting clean test suite runs on macOS and on a Linux guest
running Ubuntu22.04 on my AS Mac.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D130674
Files:
lldb/include/lldb/Breakpoint/Watchpoint.h
lldb/include/lldb/Target/StopInfo.h
lldb/include/lldb/Target/Thread.h
lldb/include/lldb/Target/ThreadPlan.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Target/StopInfo.cpp
lldb/source/Target/Thread.cpp
lldb/source/Target/ThreadList.cpp
lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py
@@ -12,10 +12,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
@add_test_categories(["watchpoint"])
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
def test(self):
"""Test watchpoint and a breakpoint in multiple threads."""
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
@@ -12,10 +12,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
@expectedFailureNetBSD
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
def test(self):
"""Test two threads that trigger a watchpoint and one signal thread. """
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
@@ -11,10 +11,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
def test(self):
"""Test two threads that trigger a watchpoint and one (1 second delay) breakpoint thread. """
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
@@ -11,10 +11,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
def test(self):
"""Test two threads that trigger a watchpoint and one breakpoint thread. """
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py
@@ -11,10 +11,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
def test(self):
"""Test two threads that trigger a watchpoint. """
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py
@@ -12,10 +12,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
@expectedFailureNetBSD
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
def test(self):
"""Test a signal/watchpoint/breakpoint in multiple threads."""
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py
@@ -11,10 +11,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
def test(self):
"""Test a watchpoint and a signal in multiple threads."""
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py
@@ -14,10 +14,6 @@
@expectedFailureNetBSD
@expectedFailureAll(archs=["aarch64"], oslist=["freebsd"],
bugnumber="llvm.org/pr49433")
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
def test(self):
"""Test one signal thread with 5 watchpoint and breakpoint threads."""
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py
@@ -13,10 +13,6 @@
@skipIf(triple='^mips')
@expectedFailureAll(archs=["aarch64"], oslist=["freebsd"],
bugnumber="llvm.org/pr49433")
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
def test(self):
"""Test with 5 watchpoint and breakpoint threads."""
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py
@@ -10,10 +10,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://93863107")
@add_test_categories(["watchpoint"])
@skipIfOutOfTreeDebugserver
def test(self):
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py
===================================================================
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py
@@ -11,10 +11,6 @@
# Atomic sequences are not supported yet for MIPS in LLDB.
@skipIf(triple='^mips')
- @skipIf(
- oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
- archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
- bugnumber="rdar://81811539")
@add_test_categories(["watchpoint"])
def test(self):
"""Test (1-second delay) watchpoint and a breakpoint in multiple threads."""
Index: lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
===================================================================
--- lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
+++ lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
@@ -82,4 +82,4 @@
# Use the '-v' option to do verbose listing of the watchpoint.
# The hit count should now be 2.
self.expect("watchpoint list -v",
- substrs=['hit_count = 5'])
+ substrs=['hit_count = 1'])
Index: lldb/source/Target/ThreadList.cpp
===================================================================
--- lldb/source/Target/ThreadList.cpp
+++ lldb/source/Target/ThreadList.cpp
@@ -245,14 +245,17 @@
for (lldb::ThreadSP thread_sp : m_threads) {
// This is an optimization... If we didn't let a thread run in between
// the previous stop and this one, we shouldn't have to consult it for
- // ShouldStop. So just leave it off the list we are going to inspect. On
- // Linux, if a thread-specific conditional breakpoint was hit, it won't
+ // ShouldStop. So just leave it off the list we are going to inspect.
+ // If the thread didn't run but had work to do before declaring a public
+ // stop, then also include it.
+ // On Linux, if a thread-specific conditional breakpoint was hit, it won't
// necessarily be the thread that hit the breakpoint itself that
// evaluates the conditional expression, so the thread that hit the
// breakpoint could still be asked to stop, even though it hasn't been
// allowed to run since the previous stop.
if (thread_sp->GetTemporaryResumeState() != eStateSuspended ||
- thread_sp->IsStillAtLastBreakpointHit())
+ thread_sp->IsStillAtLastBreakpointHit()
+ || thread_sp->ShouldRunBeforePublicStop())
threads_copy.push_back(thread_sp);
}
@@ -300,6 +303,10 @@
thread_sp->GetStopInfo();
}
+ // If a thread needs to finish some job that can be done just on this thread
+ // before broadcastion the stop, it will signal that by returning true for
+ // ShouldRunBeforePublicStop. This variable gathers the results from that.
+ bool a_thread_needs_to_run = false;
for (pos = threads_copy.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
@@ -329,11 +336,23 @@
did_anybody_stop_for_a_reason |= thread_sp->ThreadStoppedForAReason();
const bool thread_should_stop = thread_sp->ShouldStop(event_ptr);
+
if (thread_should_stop)
should_stop |= true;
+ else {
+ bool this_thread_forces_run = thread_sp->ShouldRunBeforePublicStop();
+ a_thread_needs_to_run |= this_thread_forces_run;
+ if (this_thread_forces_run)
+ LLDB_LOG(log,
+ "ThreadList::{0} thread: {1:x}, "
+ "says it needs to run before public stop.",
+ __FUNCTION__, thread_sp->GetID());
+ }
}
- if (!should_stop && !did_anybody_stop_for_a_reason) {
+ if (a_thread_needs_to_run) {
+ should_stop = false;
+ } else if (!should_stop && !did_anybody_stop_for_a_reason) {
should_stop = true;
LLDB_LOGF(log,
"ThreadList::%s we stopped but no threads had a stop reason, "
@@ -368,9 +387,17 @@
// Run through the threads and ask whether we should report this event. For
// stopping, a YES vote wins over everything. A NO vote wins over NO
- // opinion.
+ // opinion. The exception is if a thread has work it needs to force before
+ // a public stop, which overrides everyone else's opinion:
for (pos = m_threads.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
+ if (thread_sp->ShouldRunBeforePublicStop()) {
+ LLDB_LOG(log, "Thread {0:x} has private business to complete, overrode "
+ "the should report stop.", thread_sp->GetID());
+ result = eVoteNo;
+ break;
+ }
+
const Vote vote = thread_sp->ShouldReportStop(event_ptr);
switch (vote) {
case eVoteNoOpinion:
@@ -550,7 +577,13 @@
run_me_only_list.SetStopID(m_process->GetStopID());
- bool run_only_current_thread = false;
+ // One or more threads might want to "Stop Others". We want to handle all
+ // those requests first. And if there is a thread that wanted to "resume
+ // before a public stop", let it get the first crack:
+ // There are two special kinds of thread that have priority for "StopOthers":
+ // a "ShouldRunBeforePublicStop thread, or the currently selected thread. If
+ // we find one satisfying that critereon, put it here.
+ ThreadSP stop_others_thread_sp;
for (pos = m_threads.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
@@ -562,17 +595,16 @@
// You can't say "stop others" and also want yourself to be suspended.
assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended);
+ run_me_only_list.AddThread(thread_sp);
- if (thread_sp == GetSelectedThread()) {
- // If the currently selected thread wants to run on its own, always let
- // it.
- run_only_current_thread = true;
- run_me_only_list.Clear();
- run_me_only_list.AddThread(thread_sp);
+ if (thread_sp == GetSelectedThread())
+ stop_others_thread_sp = thread_sp;
+
+ if (thread_sp->ShouldRunBeforePublicStop()) {
+ // This takes precedence, so if we find one of these, service it:
+ stop_others_thread_sp = thread_sp;
break;
}
-
- run_me_only_list.AddThread(thread_sp);
}
}
@@ -593,8 +625,8 @@
} else {
ThreadSP thread_to_run;
- if (run_only_current_thread) {
- thread_to_run = GetSelectedThread();
+ if (stop_others_thread_sp) {
+ thread_to_run = stop_others_thread_sp;
} else if (run_me_only_list.GetSize(false) == 1) {
thread_to_run = run_me_only_list.GetThreadAtIndex(0);
} else {
@@ -607,6 +639,9 @@
for (pos = m_threads.begin(); pos != end; ++pos) {
ThreadSP thread_sp(*pos);
if (thread_sp == thread_to_run) {
+ // Note, a thread might be able to fulfil it's plan w/o actually
+ // resuming. An example of this is a step that changes the current
+ // inlined function depth w/o moving the PC. Check that here:
if (!thread_sp->ShouldResume(thread_sp->GetCurrentPlan()->RunState()))
need_to_resume = false;
} else
@@ -624,7 +659,7 @@
// Don't clear out threads that aren't going to get a chance to run, rather
// leave their state for the next time around.
ThreadSP thread_sp(*pos);
- if (thread_sp->GetResumeState() != eStateSuspended)
+ if (thread_sp->GetTemporaryResumeState() != eStateSuspended)
thread_sp->DidResume();
}
}
Index: lldb/source/Target/Thread.cpp
===================================================================
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -221,6 +221,7 @@
Thread::GetStaticBroadcasterClass().AsCString()),
m_process_wp(process.shared_from_this()), m_stop_info_sp(),
m_stop_info_stop_id(0), m_stop_info_override_stop_id(0),
+ m_should_run_before_public_stop(false),
m_index_id(use_invalid_index_id ? LLDB_INVALID_INDEX32
: process.GetNextThreadIndexID(tid)),
m_reg_context_sp(), m_state(eStateUnloaded), m_state_mutex(),
@@ -369,7 +370,10 @@
SetStopInfo(GetStopInfo());
}
-lldb::StopInfoSP Thread::GetPrivateStopInfo() {
+lldb::StopInfoSP Thread::GetPrivateStopInfo(bool calculate) {
+ if (!calculate)
+ return m_stop_info_sp;
+
if (m_destroy_called)
return m_stop_info_sp;
@@ -377,9 +381,15 @@
if (process_sp) {
const uint32_t process_stop_id = process_sp->GetStopID();
if (m_stop_info_stop_id != process_stop_id) {
+ // We preserve the old stop info for a variety of reasons:
+ // 1) Someone has already updated it by the time we get here
+ // 2) We didn't get to execute the breakpoint instruction we stopped at
+ // 3) This is a virtual step so we didn't actually run
+ // 4) If this thread wasn't allowed to run the last time round.
if (m_stop_info_sp) {
if (m_stop_info_sp->IsValid() || IsStillAtLastBreakpointHit() ||
- GetCurrentPlan()->IsVirtualStep())
+ GetCurrentPlan()->IsVirtualStep()
+ || GetTemporaryResumeState() == eStateSuspended)
SetStopInfo(m_stop_info_sp);
else
m_stop_info_sp.reset();
@@ -723,7 +733,11 @@
return need_to_resume;
}
-void Thread::DidResume() { SetResumeSignal(LLDB_INVALID_SIGNAL_NUMBER); }
+void Thread::DidResume() {
+ SetResumeSignal(LLDB_INVALID_SIGNAL_NUMBER);
+ // This will get recomputed each time when we stop.
+ SetShouldRunBeforePublicStop(false);
+}
void Thread::DidStop() { SetState(eStateStopped); }
@@ -763,6 +777,9 @@
: LLDB_INVALID_ADDRESS);
return false;
}
+
+ // Clear the "must run me before stop" if it was set:
+ SetShouldRunBeforePublicStop(false);
if (log) {
LLDB_LOGF(log,
@@ -845,9 +862,14 @@
// stack below.
done_processing_current_plan =
(plan_ptr->IsControllingPlan() && !plan_ptr->OkayToDiscard());
- } else
+ } else {
+ bool should_force_run = plan_ptr->ShouldRunBeforePublicStop();
+ if (should_force_run) {
+ SetShouldRunBeforePublicStop(true);
+ should_stop = false;
+ }
done_processing_current_plan = true;
-
+ }
break;
}
}
Index: lldb/source/Target/StopInfo.cpp
===================================================================
--- lldb/source/Target/StopInfo.cpp
+++ lldb/source/Target/StopInfo.cpp
@@ -20,6 +20,7 @@
#include "lldb/Target/Target.h"
#include "lldb/Target/Thread.h"
#include "lldb/Target/ThreadPlan.h"
+#include "lldb/Target/ThreadPlanStepInstruction.h"
#include "lldb/Target/UnixSignals.h"
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"
@@ -690,39 +691,184 @@
}
protected:
+ using StopInfoWatchpointSP = std::shared_ptr<StopInfoWatchpoint>;
+ // This plan is used to orchestrate stepping over the watchpoint for
+ // architectures (e.g. ARM) that report the watch before running the watched
+ // access. This is the sort of job you have to defer to the thread plans,
+ // if you try to do it directly in the stop info and there are other threads
+ // that needed to process this stop you will have yanked control away from
+ // them and they won't behave correctly.
+ class ThreadPlanStepOverWatchpoint : public ThreadPlanStepInstruction {
+ public:
+ ThreadPlanStepOverWatchpoint(Thread &thread,
+ StopInfoWatchpointSP stop_info_sp,
+ WatchpointSP watch_sp)
+ : ThreadPlanStepInstruction(thread, false, true, eVoteNoOpinion,
+ eVoteNoOpinion),
+ m_stop_info_sp(stop_info_sp), m_watch_sp(watch_sp) {
+ assert(watch_sp);
+ m_watch_index = watch_sp->GetHardwareIndex();
+ }
+
+ bool DoWillResume(lldb::StateType resume_state,
+ bool current_plan) override {
+ if (resume_state == eStateSuspended)
+ return true;
+
+ if (!m_did_disable_wp) {
+ GetThread().GetProcess()->DisableWatchpoint(m_watch_sp.get(), false);
+ m_did_disable_wp = true;
+ }
+ return true;
+ }
+
+ bool DoPlanExplainsStop(Event *event_ptr) override {
+ if (ThreadPlanStepInstruction::DoPlanExplainsStop(event_ptr))
+ return true;
+ StopInfoSP stop_info_sp = GetThread().GetPrivateStopInfo();
+ // lldb-server resets the stop info for threads that didn't get to run,
+ // so we might have not gotten to run, but still have a watchpoint stop
+ // reason, in which case this will indeed be for us.
+ if (stop_info_sp
+ && stop_info_sp->GetStopReason() == eStopReasonWatchpoint)
+ return true;
+ return false;
+ }
+
+ void DidPop() override {
+ // Don't artifically keep the watchpoint alive.
+ m_watch_sp.reset();
+ }
+
+ bool ShouldStop(Event *event_ptr) override {
+ bool should_stop = ThreadPlanStepInstruction::ShouldStop(event_ptr);
+ bool plan_done = MischiefManaged();
+ if (plan_done) {
+ m_stop_info_sp->SetStepOverPlanComplete();
+ GetThread().SetStopInfo(m_stop_info_sp);
+ ResetWatchpoint();
+ }
+ return should_stop;
+ }
+
+ bool ShouldRunBeforePublicStop() override {
+ return true;
+ }
+
+ protected:
+ void ResetWatchpoint() {
+ if (!m_did_disable_wp)
+ return;
+ m_did_disable_wp = true;
+ GetThread().GetProcess()->EnableWatchpoint(m_watch_sp.get(), true);
+ m_watch_sp->SetHardwareIndex(m_watch_index);
+ }
+
+ private:
+ StopInfoWatchpointSP m_stop_info_sp;
+ WatchpointSP m_watch_sp;
+ uint32_t m_watch_index = LLDB_INVALID_INDEX32;
+ bool m_did_disable_wp = false;
+ };
+
bool ShouldStopSynchronous(Event *event_ptr) override {
- // ShouldStop() method is idempotent and should not affect hit count. See
- // Process::RunPrivateStateThread()->Process()->HandlePrivateEvent()
- // -->Process()::ShouldBroadcastEvent()->ThreadList::ShouldStop()->
- // Thread::ShouldStop()->ThreadPlanBase::ShouldStop()->
- // StopInfoWatchpoint::ShouldStop() and
- // Event::DoOnRemoval()->Process::ProcessEventData::DoOnRemoval()->
- // StopInfoWatchpoint::PerformAction().
+ // If we are running our step-over the watchpoint plan, stop if it's done
+ // and continue if it's not:
if (m_should_stop_is_valid)
return m_should_stop;
+ // If we are running our step over plan, then stop here and let the regular
+ // ShouldStop figure out what we should do: Otherwise, give our plan
+ // more time to get run:
+ if (m_using_step_over_plan)
+ return m_step_over_plan_complete;
+
+ Log *log = GetLog(LLDBLog::Process);
ThreadSP thread_sp(m_thread_wp.lock());
- if (thread_sp) {
- WatchpointSP wp_sp(
- thread_sp->CalculateTarget()->GetWatchpointList().FindByID(
- GetValue()));
- if (wp_sp) {
- // Check if we should stop at a watchpoint.
- ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
- StoppointCallbackContext context(event_ptr, exe_ctx, true);
- m_should_stop = wp_sp->ShouldStop(&context);
- } else {
- Log *log = GetLog(LLDBLog::Process);
+ assert(thread_sp);
+
+ if (thread_sp->GetTemporaryResumeState() == eStateSuspended) {
+ // This is the second firing of a watchpoint so don't process it again.
+ LLDB_LOG(log, "We didn't run but stopped with a StopInfoWatchpoint, we "
+ "have already handled this one, don't do it again.");
+ m_should_stop = false;
+ m_should_stop_is_valid = true;
+ return m_should_stop;
+ }
+
+ WatchpointSP wp_sp(
+ thread_sp->CalculateTarget()->GetWatchpointList().FindByID(GetValue()));
+ // If we can no longer find the watchpoint, we just have to stop:
+ if (!wp_sp) {
- LLDB_LOGF(log,
- "Process::%s could not find watchpoint location id: %" PRId64
- "...",
- __FUNCTION__, GetValue());
+ LLDB_LOGF(log,
+ "Process::%s could not find watchpoint location id: %" PRId64
+ "...",
+ __FUNCTION__, GetValue());
+
+ m_should_stop = true;
+ m_should_stop_is_valid = true;
+ return true;
+ }
+
+ ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
+ StoppointCallbackContext context(event_ptr, exe_ctx, true);
+ m_should_stop = wp_sp->ShouldStop(&context);
+ if (!m_should_stop) {
+ // This won't happen at present because we only allow one watchpoint per
+ // watched range. So we won't stop at a watched address with a disabled
+ // watchpoint. If we start allowing overlapping watchpoints, then we
+ // will have to make watchpoints be real "WatchpointSite" and delegate to
+ // all the watchpoints sharing the site. In that case, the code below
+ // would be the right thing to do.
+ m_should_stop_is_valid = true;
+ return m_should_stop;
+ }
+ // If this is a system where we need to execute the watchpoint by hand
+ // after the hit, queue a thread plan to do that, and then say not to stop.
+ // Otherwise, let the async action figure out whether the watchpoint should
+ // stop
+
+ ProcessSP process_sp = exe_ctx.GetProcessSP();
+ uint32_t num;
+ bool wp_triggers_after;
+
+ if (!process_sp->GetWatchpointSupportInfo(num, wp_triggers_after)
+ .Success()) {
+ m_should_stop_is_valid = true;
+ m_should_stop = true;
+ return m_should_stop;
+ }
+
+ if (!wp_triggers_after) {
+ // We have to step over the watchpoint before we know what to do:
+ StopInfoWatchpointSP me_as_siwp_sp
+ = std::static_pointer_cast<StopInfoWatchpoint>(shared_from_this());
+ ThreadPlanSP step_over_wp_sp(new ThreadPlanStepOverWatchpoint(
+ *(thread_sp.get()), me_as_siwp_sp, wp_sp));
+ Status error;
+ error = thread_sp->QueueThreadPlan(step_over_wp_sp, false);
+ // If we couldn't push the thread plan, just stop here:
+ if (!error.Success()) {
+ LLDB_LOGF(log, "Could not push our step over watchpoint plan: %s",
+ error.AsCString());
m_should_stop = true;
+ m_should_stop_is_valid = true;
+ return true;
+ } else {
+ // Otherwise, don't set m_should_stop, we don't know that yet. Just
+ // say we should continue, and tell the thread we really should do so:
+ thread_sp->SetShouldRunBeforePublicStop(true);
+ m_using_step_over_plan = true;
+ return false;
}
+ } else {
+ // We didn't have to do anything special
+ m_should_stop_is_valid = true;
+ return m_should_stop;
}
- m_should_stop_is_valid = true;
+
return m_should_stop;
}
@@ -749,57 +895,12 @@
thread_sp->CalculateTarget()->GetWatchpointList().FindByID(
GetValue()));
if (wp_sp) {
- ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
- ProcessSP process_sp = exe_ctx.GetProcessSP();
-
- {
- // check if this process is running on an architecture where
- // watchpoints trigger before the associated instruction runs. if so,
- // disable the WP, single-step and then re-enable the watchpoint
- if (process_sp) {
- uint32_t num;
- bool wp_triggers_after;
-
- if (process_sp->GetWatchpointSupportInfo(num, wp_triggers_after)
- .Success()) {
- if (!wp_triggers_after) {
- // We need to preserve the watch_index before watchpoint is
- // disable. Since Watchpoint::SetEnabled will clear the watch
- // index. This will fix TestWatchpointIter failure
- Watchpoint *wp = wp_sp.get();
- uint32_t watch_index = wp->GetHardwareIndex();
- process_sp->DisableWatchpoint(wp, false);
- StopInfoSP stored_stop_info_sp = thread_sp->GetStopInfo();
- assert(stored_stop_info_sp.get() == this);
-
- Status new_plan_status;
- ThreadPlanSP new_plan_sp(
- thread_sp->QueueThreadPlanForStepSingleInstruction(
- false, // step-over
- false, // abort_other_plans
- true, // stop_other_threads
- new_plan_status));
- if (new_plan_sp && new_plan_status.Success()) {
- new_plan_sp->SetIsControllingPlan(true);
- new_plan_sp->SetOkayToDiscard(false);
- new_plan_sp->SetPrivate(true);
- }
- process_sp->GetThreadList().SetSelectedThreadByID(
- thread_sp->GetID());
- process_sp->ResumeSynchronous(nullptr);
- process_sp->GetThreadList().SetSelectedThreadByID(
- thread_sp->GetID());
- thread_sp->SetStopInfo(stored_stop_info_sp);
- process_sp->EnableWatchpoint(wp, false);
- wp->SetHardwareIndex(watch_index);
- }
- }
- }
- }
-
// This sentry object makes sure the current watchpoint is disabled
// while performing watchpoint actions, and it is then enabled after we
// are finished.
+ ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
+ ProcessSP process_sp = exe_ctx.GetProcessSP();
+
WatchpointSentry sentry(process_sp, wp_sp);
/*
@@ -825,18 +926,10 @@
}
}
- // TODO: This condition should be checked in the synchronous part of the
- // watchpoint code
- // (Watchpoint::ShouldStop), so that we avoid pulling an event even if
- // the watchpoint fails the ignore count condition. It is moved here
- // temporarily, because for archs with
- // watchpoint_exceptions_received=before, the code in the previous
- // lines takes care of moving the inferior to next PC. We have to check
- // the ignore count condition after this is done, otherwise we will hit
- // same watchpoint multiple times until we pass ignore condition, but
- // we won't actually be ignoring them.
- if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount())
+ if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount()) {
m_should_stop = false;
+ m_should_stop_is_valid = true;
+ }
Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
@@ -859,10 +952,9 @@
Scalar scalar_value;
if (result_value_sp->ResolveValue(scalar_value)) {
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();
m_should_stop = false;
} else
m_should_stop = true;
@@ -946,9 +1038,16 @@
}
private:
+ void SetStepOverPlanComplete() {
+ assert(m_using_step_over_plan);
+ m_step_over_plan_complete = true;
+ }
+
bool m_should_stop = false;
bool m_should_stop_is_valid = false;
lldb::addr_t m_watch_hit_addr;
+ bool m_step_over_plan_complete = false;
+ bool m_using_step_over_plan = false;
};
// StopInfoUnixSignal
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1724,7 +1724,15 @@
if (dispatch_queue_t != LLDB_INVALID_ADDRESS)
gdb_thread->SetQueueLibdispatchQueueAddress(dispatch_queue_t);
- // Make sure we update our thread stop reason just once
+ // Make sure we update our thread stop reason just once, but don't
+ // overwrite the stop info for threads that haven't moved:
+ StopInfoSP current_stop_info_sp = thread_sp->GetPrivateStopInfo(false);
+ if (thread_sp->GetTemporaryResumeState() == eStateSuspended &&
+ current_stop_info_sp) {
+ thread_sp->SetStopInfo(current_stop_info_sp);
+ return thread_sp;
+ }
+
if (!thread_sp->StopInfoIsUpToDate()) {
thread_sp->SetStopInfo(StopInfoSP());
// If there's a memory thread backed by this thread, we need to use it
Index: lldb/include/lldb/Target/ThreadPlan.h
===================================================================
--- lldb/include/lldb/Target/ThreadPlan.h
+++ lldb/include/lldb/Target/ThreadPlan.h
@@ -384,6 +384,8 @@
virtual void SetStopOthers(bool new_value);
virtual bool StopOthers();
+
+ virtual bool ShouldRunBeforePublicStop() { return false; }
// This is the wrapper for DoWillResume that does generic ThreadPlan logic,
// then calls DoWillResume.
Index: lldb/include/lldb/Target/Thread.h
===================================================================
--- lldb/include/lldb/Target/Thread.h
+++ lldb/include/lldb/Target/Thread.h
@@ -240,6 +240,7 @@
// this just calls through to the ThreadSpec's ThreadPassesBasicTests method.
virtual bool MatchesSpec(const ThreadSpec *spec);
+ // Get the current public stop info, calculating it if necessary.
lldb::StopInfoSP GetStopInfo();
lldb::StopReason GetStopReason();
@@ -1121,7 +1122,7 @@
// "checkpointed and restored" stop info, so if it is still around it is
// right even if you have not calculated this yourself, or if it disagrees
// with what you might have calculated.
- virtual lldb::StopInfoSP GetPrivateStopInfo();
+ virtual lldb::StopInfoSP GetPrivateStopInfo(bool calculate = true);
// Calculate the stop info that will be shown to lldb clients. For instance,
// a "step out" is implemented by running to a breakpoint on the function
@@ -1165,6 +1166,14 @@
void ResetStopInfo();
void SetShouldReportStop(Vote vote);
+
+ void SetShouldRunBeforePublicStop(bool newval) {
+ m_should_run_before_public_stop = newval;
+ }
+
+ bool ShouldRunBeforePublicStop() {
+ return m_should_run_before_public_stop;
+ }
/// Sets the extended backtrace token for this thread
///
@@ -1254,6 +1263,9 @@
uint32_t m_stop_info_override_stop_id; // The stop ID containing the last time
// the stop info was checked against
// the stop info override
+ bool m_should_run_before_public_stop; // If this thread has "stop others"
+ // private work to do, then it will
+ // set this.
const uint32_t m_index_id; ///< A unique 1 based index assigned to each thread
/// for easy UI/command line access.
lldb::RegisterContextSP m_reg_context_sp; ///< The register context for this
Index: lldb/include/lldb/Target/StopInfo.h
===================================================================
--- lldb/include/lldb/Target/StopInfo.h
+++ lldb/include/lldb/Target/StopInfo.h
@@ -17,7 +17,7 @@
namespace lldb_private {
-class StopInfo {
+class StopInfo : public std::enable_shared_from_this<StopInfo> {
friend class Process::ProcessEventData;
friend class ThreadPlanBase;
Index: lldb/include/lldb/Breakpoint/Watchpoint.h
===================================================================
--- lldb/include/lldb/Breakpoint/Watchpoint.h
+++ lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -75,7 +75,7 @@
bool IsHardware() const override;
bool ShouldStop(StoppointCallbackContext *context) override;
-
+
bool WatchpointRead() const;
bool WatchpointWrite() const;
uint32_t GetIgnoreCount() const;
@@ -157,12 +157,15 @@
private:
friend class Target;
friend class WatchpointList;
+ friend class StopInfoWatchpoint; // This needs to call UndoHitCount()
void ResetHistoricValues() {
m_old_value_sp.reset();
m_new_value_sp.reset();
}
+ void UndoHitCount() { m_hit_counter.Decrement(); }
+
Target &m_target;
bool m_enabled; // Is this watchpoint enabled
bool m_is_hardware; // Is this a hardware watchpoint
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
