jingham created this revision. jingham added reviewers: JDevlieghere, jasonmolenda, labath, clayborg. Herald added subscribers: atanasyan, kristof.beyls, arichardson, sdardis. Herald added a project: All. jingham requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits.
On some architectures - e.g. aarch64 - the watchpoint exception is raised before the instruction that would have triggered it gets run. We want to report old/new values, however, so we need to have executed the instruction before reporting the stop. That means before reporting the stop, we need to do one stepi on the thread that got the watchpoint exception. The original version of this code implemented this incorrectly. It did the step over synchronously in the StopInfoWatchpoint::ShouldStopSynchronous. You should NEVER run the target in ShouldStopSynchronous. If you do that, then any other threads that had interesting pending stop info actions won't be able to perform their action because the target will have moved on and invalidated their StopInfo's. You should always do this kind of thing by pushing the action you want to perform onto the ThreadPlanStack for the thread in question. That way all the threads get to register what they need to do, and then the ThreadList runner can arbitrate among them and do the right thing. For instance, in the case of stepping over watchpoints, each thread stopped on the watchpoint pushes a "step instruction" thread plan that re-instates the original watchpoint StopInfo when complete. If there is more than one thread in this state, lldb notices each of them has a "stop others" plan, and one by one lets each thread step over the watchpoint. When all this work is done, we report all the threads stopped at their watchpoints. I also fixed a bug in the same code I noticed while reworking this. We were not decrementing the HitCount for the watchpoint if the condition fails. We consider a failed condition to mean "the stop point wasn't hit". It's weird to see a hit count of 5 when the target only told you about one stop... But that wasn't being enforced in the StopInfoWatchpoint::PerformAction. There was a test that was expecting the now-incorrect value of the hit count in this case, so I fixed that, and also all the tests that were failing because of concurrent access were turned on again and run successfully for me. There's a bit of funniness for MIPS in the watchpoint support. I am pretty sure I got that right, but I don't have anything MIPS to test that on. The same concurrent watchpoint tests that were marked failing for arm64 Darwin are also marked failed for MIPS, but I am not sure if that's for the same reason. Anyway, if someone has access to a MIPS system, it would be good to try this out and see if it works there as well. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D129814 Files: lldb/include/lldb/Breakpoint/Watchpoint.h lldb/include/lldb/Target/StopInfo.h lldb/source/Target/StopInfo.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/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,40 +691,129 @@ } protected: + // 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, StopInfoSP 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(); + } + + ~ThreadPlanStepOverWatchpoint() { ResetWatchpoint(); } + bool DoWillResume(lldb::StateType resume_state, + bool current_plan) override { + if (m_reset_watchpoint) { + GetThread().GetProcess()->DisableWatchpoint(m_watch_sp.get(), false); + m_reset_watchpoint = false; + } + return true; + } + + 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) { + GetThread().SetStopInfo(m_stop_info_sp); + ResetWatchpoint(); + } + return should_stop; + } + + protected: + void ResetWatchpoint() { + if (m_reset_watchpoint) + return; + m_reset_watchpoint = true; + GetThread().GetProcess()->EnableWatchpoint(m_watch_sp.get(), false); + m_watch_sp->SetHardwareIndex(m_watch_index); + } + + private: + StopInfoSP m_stop_info_sp; + WatchpointSP m_watch_sp; + uint32_t m_watch_index = LLDB_INVALID_INDEX32; + bool m_reset_watchpoint = 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 (m_should_stop_is_valid) - return m_should_stop; + // If we are running our step-over the watchpoint plan, stop if it's done + // and continue if it's not: + if (m_step_over_wp_sp) { + if (m_step_over_wp_sp->IsPlanComplete()) + return true; + else + return false; + } 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); + WatchpointSP wp_sp( + thread_sp->CalculateTarget()->GetWatchpointList().FindByID(GetValue())); + if (!wp_sp) { + Log *log = GetLog(LLDBLog::Process); - 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 = true; + m_should_stop_is_valid = true; + return true; } - m_should_stop_is_valid = true; - return m_should_stop; + + 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()) { + if (wp_triggers_after) + return true; + + m_step_over_wp_sp.reset(new ThreadPlanStepOverWatchpoint( + *(thread_sp.get()), shared_from_this(), wp_sp)); + Status error; + error = thread_sp->QueueThreadPlan(m_step_over_wp_sp, false); + if (error.Success()) + return false; + else + return true; + } + // If we don't have to step over the watchpoint, just let the PerformAction + // determine what we should do. + return true; } bool ShouldStop(Event *event_ptr) override { @@ -749,57 +839,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 +870,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 +896,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; @@ -949,6 +985,7 @@ bool m_should_stop = false; bool m_should_stop_is_valid = false; lldb::addr_t m_watch_hit_addr; + ThreadPlanSP m_step_over_wp_sp; }; // StopInfoUnixSignal 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 @@ -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