From 8d024a79ea783ed3fbb5691aeaf186ad3f0a4ae9 Mon Sep 17 00:00:00 2001 From: Jim Ingham Date: Mon, 20 Mar 2023 15:12:38 -0700 Subject: [PATCH] Fix a problem with "watchpoint triggers before" watchpoint handling. We need to step the watchpoint instruction in these cases, but the when we queued the ThreadPlanStepOverWatchpoint to do this, we didn't make it a Controlling plan. So if you are stepping, this plan returns as though it were a utility plan, and the stepping plan keeps going. This only partially fixes the problem on Darwin; there's another bug with reporting a watchpoint when we're instruction single stepping over an instruction that triggers a watchpoint. The kernel reports the "single step completed" but not the watchpoint hit. So this commit also refactors the test into a part that works (at least on Darwin) and a part that still fails. We may have to adjust the test result expectations for other systems after this fix. Differential Revision: https://reviews.llvm.org/D146337 --- lldb/include/lldb/Target/Process.h | 19 +++++- .../Process/Utility/StopInfoMachException.cpp | 13 +++++ lldb/source/Target/StopInfo.cpp | 5 ++ lldb/source/Target/Thread.cpp | 3 + .../step_over_watchpoint/TestStepOverWatchpoint.py | 68 ++++++++++++---------- .../watchpoints/step_over_watchpoint/main.c | 4 +- 6 files changed, 78 insertions(+), 34 deletions(-) diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index 3ffacb5..6ce38f6 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -311,6 +311,14 @@ public: return m_last_natural_stop_event; return lldb::EventSP(); } + + void SetSafeToCallFunctions(bool safe) { + m_safe = safe; + } + + bool GetSafeToCallFunctions() { + return m_safe; + } private: uint32_t m_stop_id = 0; @@ -321,6 +329,7 @@ private: uint32_t m_running_user_expression = false; uint32_t m_running_utility_function = 0; lldb::EventSP m_last_natural_stop_event; + std::atomic m_safe = true; }; inline bool operator==(const ProcessModID &lhs, const ProcessModID &rhs) { @@ -459,7 +468,7 @@ public: void SetRestarted(bool new_value) { m_restarted = new_value; } void SetInterrupted(bool new_value) { m_interrupted = new_value; } - + void AddRestartedReason(const char *reason) { m_restarted_reasons.push_back(reason); } @@ -1250,6 +1259,14 @@ public: DiagnosticManager &diagnostic_manager); static const char *ExecutionResultAsCString(lldb::ExpressionResults result); + + void SetSafeToCallFunctions(bool safe) { + GetModID().SetSafeToCallFunctions(safe); + } + + bool GetSafeToCallFunctions() { + return GetModID().GetSafeToCallFunctions(); + } void GetStatus(Stream &ostrm); diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp index aae15b2..458d44f 100644 --- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp +++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp @@ -795,6 +795,19 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException( case 9: // EXC_RPC_ALERT case 10: // EXC_CRASH break; + case 12: // EXC_GUARD + { + // Some EXC_GUARD exceptions are fatal, and the process will go away + // the next time you allow it to run. When we get one of those + // exceptions we have to make sure SafeToCallFunctions returns false to + // prevent us or other agents running the process. This has to be set + // on the process because even the threads that didn't get the exception + // can't run. + ProcessSP process_sp(thread.GetProcess()); + if (process_sp) + process_sp->SetSafeToCallFunctions(false); + + } } return StopInfoSP(new StopInfoMachException(thread, exc_type, exc_data_count, diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 9fdb29f..ebc355c 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -831,6 +831,11 @@ protected: = std::static_pointer_cast(shared_from_this()); ThreadPlanSP step_over_wp_sp(new ThreadPlanStepOverWatchpoint( *(thread_sp.get()), me_as_siwp_sp, wp_sp)); + // When this plan is done we want to stop, so set this as a Controlling + // plan. + step_over_wp_sp->SetIsControllingPlan(true); + step_over_wp_sp->SetOkayToDiscard(false); + Status error; error = thread_sp->QueueThreadPlan(step_over_wp_sp, false); // If we couldn't push the thread plan, just stop here: diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp index d620f74..df8bff5 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -1664,6 +1664,9 @@ addr_t Thread::GetThreadLocalData(const ModuleSP module, bool Thread::SafeToCallFunctions() { Process *process = GetProcess().get(); if (process) { + if (!process->SafeToCallFunctions()) + return false; + DynamicLoader *loader = GetProcess()->GetDynamicLoader(); if (loader && loader->IsFullyInitialized() == false) return false; diff --git a/lldb/test/API/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py b/lldb/test/API/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py index 7d54156a..52fc899 100644 --- a/lldb/test/API/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py +++ b/lldb/test/API/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py @@ -11,36 +11,11 @@ from lldbsuite.test import lldbutil class TestStepOverWatchpoint(TestBase): NO_DEBUG_INFO_TESTCASE = True - @expectedFailureAll( - oslist=["freebsd", "linux"], - archs=[ - 'aarch64', - 'arm'], - bugnumber="llvm.org/pr26031") - # Read-write watchpoints not supported on SystemZ - @expectedFailureAll(archs=['s390x']) - @expectedFailureAll( - oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"], - archs=['aarch64', 'arm'], - bugnumber="") - @add_test_categories(["basic_process"]) - def test(self): + def get_to_start(self, bkpt_text): """Test stepping over watchpoints.""" self.build() - target = self.createTestTarget() - - lldbutil.run_break_set_by_symbol(self, 'main') - - process = target.LaunchSimple(None, None, - self.get_process_working_directory()) - self.assertTrue(process.IsValid(), PROCESS_IS_VALID) - self.assertState(process.GetState(), lldb.eStateStopped, - PROCESS_STOPPED) - - thread = lldbutil.get_stopped_thread(process, - lldb.eStopReasonBreakpoint) - self.assertTrue(thread.IsValid(), "Failed to get thread.") - + target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, bkpt_text, + lldb.SBFileSpec("main.c")) frame = thread.GetFrameAtIndex(0) self.assertTrue(frame.IsValid(), "Failed to get frame.") @@ -55,14 +30,45 @@ class TestStepOverWatchpoint(TestBase): self.assertSuccess(error, "Error while setting watchpoint") self.assertTrue(read_watchpoint, "Failed to set read watchpoint.") + # Disable the breakpoint we hit so we don't muddy the waters with + # stepping off from the breakpoint: + bkpt.SetEnabled(False) + + return (target, process, thread, read_watchpoint) + + @expectedFailureAll( + oslist=["freebsd", "linux"], + archs=[ + 'aarch64', + 'arm'], + bugnumber="llvm.org/pr26031") + # Read-write watchpoints not supported on SystemZ + @expectedFailureAll(archs=['s390x']) + @add_test_categories(["basic_process"]) + def test_step_over(self): + target, process, thread, wp = self.get_to_start("Set a breakpoint here") + thread.StepOver() self.assertStopReason(thread.GetStopReason(), lldb.eStopReasonWatchpoint, STOPPED_DUE_TO_WATCHPOINT) self.assertEquals(thread.GetStopDescription(20), 'watchpoint 1') - process.Continue() - self.assertState(process.GetState(), lldb.eStateStopped, - PROCESS_STOPPED) + @expectedFailureAll( + oslist=["freebsd", "linux"], + archs=[ + 'aarch64', + 'arm'], + bugnumber="llvm.org/pr26031") + # Read-write watchpoints not supported on SystemZ + @expectedFailureAll(archs=['s390x']) + @expectedFailureAll( + oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"], + archs=['aarch64', 'arm'], + bugnumber="") + @add_test_categories(["basic_process"]) + def test_step_instruction(self): + target, process, thread, wp = self.get_to_start("Set breakpoint after call") + self.assertEquals(thread.GetStopDescription(20), 'step over') self.step_inst_for_watchpoint(1) diff --git a/lldb/test/API/commands/watchpoints/step_over_watchpoint/main.c b/lldb/test/API/commands/watchpoints/step_over_watchpoint/main.c index 2d87d9a..e48d43c 100644 --- a/lldb/test/API/commands/watchpoints/step_over_watchpoint/main.c +++ b/lldb/test/API/commands/watchpoints/step_over_watchpoint/main.c @@ -11,8 +11,8 @@ void watch_write() { } int main() { - watch_read(); - g_temp = g_watch_me_read; + watch_read(); // Set a breakpoint here + g_temp = g_watch_me_read; // Set breakpoint after call watch_write(); g_watch_me_write = g_temp; return 0; -- 2.7.4