Fix a race condition between the "ephemeral watchpoint disabling" and commands the...
authorJim Ingham <jingham@apple.com>
Tue, 25 Oct 2016 20:34:32 +0000 (20:34 +0000)
committerJim Ingham <jingham@apple.com>
Tue, 25 Oct 2016 20:34:32 +0000 (20:34 +0000)
This closes https://reviews.llvm.org/D25875.

llvm-svn: 285114

lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py
lldb/source/Breakpoint/Watchpoint.cpp
lldb/source/Target/StopInfo.cpp

index e6726f5..a6cc4f2 100644 (file)
@@ -54,9 +54,6 @@ class WatchpointPythonCommandTestCase(TestBase):
         # Add a breakpoint to set a watchpoint when stopped on the breakpoint.
         lldbutil.run_break_set_by_file_and_line(
             self, None, self.line, num_expected_locations=1)
-#        self.expect("breakpoint set -l %d" % self.line, BREAKPOINT_CREATED,
-#            startstr = "Breakpoint created: 1: file ='%s', line = %d, locations = 1" %
-#                       (self.source, self.line))#
 
         # Run the program.
         self.runCmd("run", RUN_SUCCEEDED)
@@ -131,9 +128,6 @@ class WatchpointPythonCommandTestCase(TestBase):
         # Add a breakpoint to set a watchpoint when stopped on the breakpoint.
         lldbutil.run_break_set_by_file_and_line(
             self, None, self.line, num_expected_locations=1)
-#        self.expect("breakpoint set -l %d" % self.line, BREAKPOINT_CREATED,
-#            startstr = "Breakpoint created: 1: file ='%s', line = %d, locations = 1" %
-#                       (self.source, self.line))#
 
         # Run the program.
         self.runCmd("run", RUN_SUCCEEDED)
@@ -177,3 +171,4 @@ class WatchpointPythonCommandTestCase(TestBase):
         # second hit and set it to 999
         self.expect("frame variable --show-globals cookie",
                     substrs=['(int32_t)', 'cookie = 999'])
+
index db602d5..ce2ce8b 100644 (file)
@@ -232,7 +232,7 @@ void Watchpoint::TurnOffEphemeralMode() {
 }
 
 bool Watchpoint::IsDisabledDuringEphemeralMode() {
-  return m_disabled_count > 1;
+  return m_disabled_count > 1 && m_is_ephemeral;
 }
 
 void Watchpoint::SetEnabled(bool enabled, bool notify) {
index 294ccf3..d796071 100644 (file)
@@ -557,27 +557,44 @@ public:
   // performing watchpoint actions.
   class WatchpointSentry {
   public:
-    WatchpointSentry(Process *p, Watchpoint *w) : process(p), watchpoint(w) {
-      if (process && watchpoint) {
+    WatchpointSentry(ProcessSP p_sp, WatchpointSP w_sp) : process_sp(p_sp), 
+                     watchpoint_sp(w_sp) {
+      if (process_sp && watchpoint_sp) {
         const bool notify = false;
-        watchpoint->TurnOnEphemeralMode();
-        process->DisableWatchpoint(watchpoint, notify);
+        watchpoint_sp->TurnOnEphemeralMode();
+        process_sp->DisableWatchpoint(watchpoint_sp.get(), notify);
+        process_sp->AddPreResumeAction(SentryPreResumeAction, this);
       }
     }
-
-    ~WatchpointSentry() {
-      if (process && watchpoint) {
-        if (!watchpoint->IsDisabledDuringEphemeralMode()) {
-          const bool notify = false;
-          process->EnableWatchpoint(watchpoint, notify);
+    
+    void DoReenable() {
+      if (process_sp && watchpoint_sp) {
+        bool was_disabled = watchpoint_sp->IsDisabledDuringEphemeralMode();
+        watchpoint_sp->TurnOffEphemeralMode();
+        const bool notify = false;
+        if (was_disabled) {
+          process_sp->DisableWatchpoint(watchpoint_sp.get(), notify);
+        } else {
+          process_sp->EnableWatchpoint(watchpoint_sp.get(), notify);
         }
-        watchpoint->TurnOffEphemeralMode();
       }
     }
+    
+    ~WatchpointSentry() {
+        DoReenable();
+        if (process_sp)
+            process_sp->ClearPreResumeAction(SentryPreResumeAction, this);
+    }
+    
+    static bool SentryPreResumeAction(void *sentry_void) {
+        WatchpointSentry *sentry = (WatchpointSentry *) sentry_void;
+        sentry->DoReenable();
+        return true;
+    }
 
   private:
-    Process *process;
-    Watchpoint *watchpoint;
+    ProcessSP process_sp;
+    WatchpointSP watchpoint_sp;
   };
 
   StopInfoWatchpoint(Thread &thread, break_id_t watch_id,
@@ -650,21 +667,17 @@ protected:
     // course of
     // this code.  Also by default we're going to stop, so set that here.
     m_should_stop = true;
+    
 
     ThreadSP thread_sp(m_thread_wp.lock());
     if (thread_sp) {
 
       WatchpointSP wp_sp(
           thread_sp->CalculateTarget()->GetWatchpointList().FindByID(
-              GetValue()));
+              GetValue()));      
       if (wp_sp) {
         ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
-        Process *process = exe_ctx.GetProcessPtr();
-
-        // This sentry object makes sure the current watchpoint is disabled
-        // while performing watchpoint actions,
-        // and it is then enabled after we are finished.
-        WatchpointSentry sentry(process, wp_sp.get());
+        ProcessSP process_sp = exe_ctx.GetProcessSP();
 
         {
           // check if this process is running on an architecture where
@@ -672,12 +685,14 @@ protected:
           // before the associated instruction runs. if so, disable the WP,
           // single-step and then
           // re-enable the watchpoint
-          if (process) {
+          if (process_sp) {
             uint32_t num;
             bool wp_triggers_after;
-            if (process->GetWatchpointSupportInfo(num, wp_triggers_after)
+
+            if (process_sp->GetWatchpointSupportInfo(num, wp_triggers_after)
                     .Success()) {
               if (!wp_triggers_after) {
+                process_sp->DisableWatchpoint(wp_sp.get(), false);
                 StopInfoSP stored_stop_info_sp = thread_sp->GetStopInfo();
                 assert(stored_stop_info_sp.get() == this);
 
@@ -689,17 +704,23 @@ protected:
                 new_plan_sp->SetIsMasterPlan(true);
                 new_plan_sp->SetOkayToDiscard(false);
                 new_plan_sp->SetPrivate(true);
-                process->GetThreadList().SetSelectedThreadByID(
+                process_sp->GetThreadList().SetSelectedThreadByID(
                     thread_sp->GetID());
-                process->ResumeSynchronous(nullptr);
-                process->GetThreadList().SetSelectedThreadByID(
+                process_sp->ResumeSynchronous(nullptr);
+                process_sp->GetThreadList().SetSelectedThreadByID(
                     thread_sp->GetID());
                 thread_sp->SetStopInfo(stored_stop_info_sp);
+                process_sp->EnableWatchpoint(wp_sp.get(), false);
               }
             }
           }
         }
 
+        // This sentry object makes sure the current watchpoint is disabled
+        // while performing watchpoint actions,
+        // and it is then enabled after we are finished.
+        WatchpointSentry sentry(process_sp, wp_sp);
+
         /*
          * MIPS: Last 3bits of the watchpoint address are masked by the kernel.
          * For example:
@@ -739,6 +760,8 @@ protected:
         if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount())
           m_should_stop = false;
 
+        Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
+
         if (m_should_stop && wp_sp->GetConditionText() != nullptr) {
           // We need to make sure the user sees any parse errors in their
           // condition, so we'll hook the
@@ -778,7 +801,6 @@ protected:
               }
             }
           } else {
-            Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
             StreamSP error_sp = debugger.GetAsyncErrorStream();
             error_sp->Printf(
                 "Stopped due to an error evaluating condition of watchpoint ");
@@ -800,8 +822,20 @@ protected:
         // If the condition says to stop, we run the callback to further decide
         // whether to stop.
         if (m_should_stop) {
+            // FIXME: For now the callbacks have to run in async mode - the
+            // first time we restart we need
+            // to get out of there.  So set it here.
+            // When we figure out how to nest watchpoint hits then this will
+            // change.
+
+          bool old_async = debugger.GetAsyncExecution();
+          debugger.SetAsyncExecution(true);
+          
           StoppointCallbackContext context(event_ptr, exe_ctx, false);
           bool stop_requested = wp_sp->InvokeCallback(&context);
+          
+          debugger.SetAsyncExecution(old_async);
+          
           // Also make sure that the callback hasn't continued the target.
           // If it did, when we'll set m_should_stop to false and get out of
           // here.