Partially fix stepping over watchpoints on "watch triggers before" systems

[jingham]

Summary

There were (at least) two problems that were causing TestStepOverWatchpoints.py to fail on arm64 Darwin systems. The first was that the "step over the watch instruction" plan that we pushed so we could report the watch stop when we knew the "old and new" values was a utility plan that wouldn't cause a user stop when it was done, and so instead of reporting the watchpoint we just continued on with the step.

The second one may be specific to Darwin systems, but when you single-step the processor over an instruction that raises the watchpoint, when we stop the stop reason is Trace and not Watchpoint. That not lldb's interpretation, that what the exception we get from the kernel says.

I don't have a good way to work around the second one yet, but I this patch fixes the first problem, and reworks the test so that we have a case that doesn't end up single-stepping over the instruction that triggers the watchpoint - which now succeeds on Darwin - and one that tests single-stepping over that instruction which still fails.

[JDevlieghere]

LGTM

lldb/include/lldb/Target/Process.h

void SetStopEventForLastNaturalStopID(lldb::EventSP event_sp) {
     m_last_natural_stop_event = std::move(event_sp);
   }
 
   lldb::EventSP GetStopEventForStopID(uint32_t stop_id) const {
     if (stop_id == m_last_natural_stop_id)
       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;
   uint32_t m_last_natural_stop_id = 0;
   uint32_t m_resume_id = 0;
   uint32_t m_memory_id = 0;
   uint32_t m_last_user_expression_resume = 0;
   uint32_t m_running_user_expression = false;
   uint32_t m_running_utility_function = 0;
   lldb::EventSP m_last_natural_stop_event;
+  std::atomic<bool> m_safe = true;
 };
 
 inline bool operator==(const ProcessModID &lhs, const ProcessModID &rhs) {
   if (lhs.StopIDEqual(rhs) && lhs.MemoryIDEqual(rhs))
     return true;
   else
     return false;
 }
public:
     static bool SetUpdateStateOnRemoval(Event *event_ptr);
 
   private:
     void SetUpdateStateOnRemoval() { m_update_state++; }
 
     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);
     }
 
     lldb::ProcessWP m_process_wp;
     lldb::StateType m_state = lldb::eStateInvalid;
     std::vector<std::string> m_restarted_reasons;
     bool m_restarted = false; // For "eStateStopped" events, this is true if the
/// \see Process::Notifications
   lldb::StateType GetState();
 
   lldb::ExpressionResults
   RunThreadPlan(ExecutionContext &exe_ctx, lldb::ThreadPlanSP &thread_plan_sp,
                 const EvaluateExpressionOptions &options,
                 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);
 
   size_t GetThreadStatus(Stream &ostrm, bool only_threads_with_stop_reason,
                          uint32_t start_frame, uint32_t num_frames,
                          uint32_t num_frames_with_source,
                          bool stop_format);
 
   void SendAsyncInterrupt();

lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp

case 6: // EXC_BREAKPOINT
     }
   } break;
 
   case 7:  // EXC_SYSCALL
   case 8:  // EXC_MACH_SYSCALL
   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,
                                               exc_code, exc_sub_code));
 }

lldb/source/Target/StopInfo.cpp

bool ShouldStopSynchronous(Event *event_ptr) override {
     bool wp_triggers_after = process_sp->GetWatchpointReportedAfter();
 
     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));
+      // 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:
       if (!error.Success()) {
         LLDB_LOGF(log, "Could not push our step over watchpoint plan: %s", 
             error.AsCString());
 
         m_should_stop = true;

lldb/source/Target/Thread.cpp

return loader->GetThreadLocalData(module, shared_from_this(),
                                       tls_file_addr);
   else
     return LLDB_INVALID_ADDRESS;
 }
 
 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;
 
     SystemRuntime *runtime = process->GetSystemRuntime();
     if (runtime) {
       return runtime->SafeToCallFunctionsOnThisThread(shared_from_this());
     }

lldb/test/API/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py

 """Test stepping over watchpoints."""
 
 
 
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 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="<rdar://problem/34027183>")
-    @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.")
 
         read_value = frame.FindValue('g_watch_me_read',
                                      lldb.eValueTypeVariableGlobal)
         self.assertTrue(read_value.IsValid(), "Failed to find read value.")
 
         error = lldb.SBError()
 
         # resolve_location=True, read=True, write=False
         read_watchpoint = read_value.Watch(True, True, False, error)
         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="<rdar://problem/34027183>")
+    @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)
 
         write_value = frame.FindValue('g_watch_me_write',
                                       lldb.eValueTypeVariableGlobal)
         self.assertTrue(write_value, "Failed to find write value.")
 

lldb/test/API/commands/watchpoints/step_over_watchpoint/main.c

 char g_watch_me_read;
 char g_watch_me_write;
 char g_temp;
 
 void watch_read() {
     g_temp = g_watch_me_read;
 }
 
 void watch_write() {
     g_watch_me_write = g_temp;
 }
 
 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;
 }