Fix r284795 for the !wp_triggers_after case

[jingham]

Summary

The single-step dance needed in the wp_triggers_after case was using the WatchpointSentry to disable the watchpoint, and enable it again after. That wasn't right, this should be an operation that happens unknown to the machinery that handles the watchpoint actions. So I changed it to by-hand disable/enable in the case where the step is needed, and then go on to set up the sentry.

Pavel, can you see if this works for you?

[jingham]

Actually, that should be the !wp_triggers_after case...

[jingham]

Tiny cleanup. I moved wp_triggers_after out of the process_sp check 'cause I though I might need to know it once the step was over, but then I didn't. So I moved it back.

[labath]

Unfortunately, this does not seem to do the trick, Upon hitting the watchpoint lldb enters a loop, where it continually tries to single-step, but never actually disables the watchpoint.

source/Target/StopInfo.cpp

696

This does not seem to cause the watchpoint to be disabled. It just ends up calling StoppointLocation::SetHardwareIndex(INVALID_INDEX), but no packet is sent that actually disables the watchpoint server-side.

[jingham]

I swapped back into my brain how to run the testsuite on an arm64 iOS device. I verified (by the advanced means of putting a printf there) that we were doing the !wp_triggers_after single-step over watchpoint two-step in StopInfoWatchpoint::PerformAction.

Then I tried this patch using both wp_sp->SetEnabled({true, false}, false), and process_sp->{Enable,Disable}Watchpoint(wp_sp.get(), false) in the single-step over bit. In both cases it failed one testcase: TestStepOverWatchpoint.TestStepOverWatchpoint - which is an instruction single step over watchpoint failure. But all the other watchpoint tests passed, and I saw my printf a whole bunch of times, so that little dance was done successfully many times. And the one failing test didn't go into an infinite loop, it just got the wrong stop reason.

Maybe the Watchpoint::SetEnabled failure is something on your end? Anyway, I uploaded a new version of the patch that uses the Process method - which apparently does work for you or this never would have worked at all... Can you see if that works any better? If it does, then I'll check in this version, and we can file a PR about the Watchpoint::SetEnabled failure.

[labath]

It works now. Thanks for fixing it.

I am not sure how the previous version could have worked for you. If you look at ProcessGDBRemote::DisableWatchpoint, you see that it calls wp->SetEnabled(false) after it sends the "z" packet. It would be strange if SetEnabled() ended up calling back into DisableWatchpoint() (and I don't see any indications of that in the code).

source/Target/StopInfo.cpp

598

this variable seems to be unused.

[jingham]

Thanks, Pavel.

Closed by 285114.

packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py

def test_watchpoint_command(self):
         self.setTearDownCleanup(dictionary=self.d)
 
         exe = os.path.join(os.getcwd(), self.exe_name)
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # 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)
 
         # We should be stopped again due to the breakpoint.
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stopped',
def test_continue_in_watchpoint_command(self):
         self.setTearDownCleanup(dictionary=self.d)
 
         exe = os.path.join(os.getcwd(), self.exe_name)
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # 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)
 
         # We should be stopped again due to the breakpoint.
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stopped',
def test_continue_in_watchpoint_command(self):
         # The stop reason of the thread should be watchpoint.
         self.expect("thread backtrace", STOPPED_DUE_TO_WATCHPOINT,
                     substrs=['stop reason = watchpoint'])
 
         # We should have hit the watchpoint once, set cookie to 888, then continued to the
         # second hit and set it to 999
         self.expect("frame variable --show-globals cookie",
                     substrs=['(int32_t)', 'cookie = 999'])
+

source/Breakpoint/Watchpoint.cpp

 
 void Watchpoint::TurnOffEphemeralMode() {
   m_is_ephemeral = false;
   // Leaving ephemeral mode, reset the m_disabled_count!
   m_disabled_count = 0;
 }
 
 bool Watchpoint::IsDisabledDuringEphemeralMode() {
-  return m_disabled_count > 1;
+  return m_disabled_count > 1 && m_is_ephemeral;
 }
 
 void Watchpoint::SetEnabled(bool enabled, bool notify) {
   if (!enabled) {
     if (!m_is_ephemeral)
       SetHardwareIndex(LLDB_INVALID_INDEX32);
     else
       ++m_disabled_count;

source/Target/StopInfo.cpp

 //----------------------------------------------------------------------
 
 class StopInfoWatchpoint : public StopInfo {
 public:
   // Make sure watchpoint is properly disabled and subsequently enabled while
   // 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);
-        }
-        watchpoint->TurnOffEphemeralMode();
-      }
+    void DoReenable() {
+      if (process_sp && watchpoint_sp) {
+        bool was_disabled = watchpoint_sp->IsDisabledDuringEphemeralMode();
+        watchpoint_sp->TurnOffEphemeralMode();
+        if (was_disabled) {
+          process_sp->DisableWatchpoint(watchpoint_sp.get(), notify);
+        } else {
+          process_sp->EnableWatchpoint(watchpoint_sp.get(), notify);
+        }
+    
+    ~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;
+    bool sentry_triggered = false;
   };
 
   StopInfoWatchpoint(Thread &thread, break_id_t watch_id,
                      lldb::addr_t watch_hit_addr)
       : StopInfo(thread, watch_id), m_should_stop(false),
         m_should_stop_is_valid(false), m_watch_hit_addr(watch_hit_addr) {}
 
   ~StopInfoWatchpoint() override = default;
protected:
   }
 
   void PerformAction(Event *event_ptr) override {
     Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_WATCHPOINTS);
     // We're going to calculate if we should stop or not in some way during the
     // 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
           // watchpoints trigger
           // 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) {
+                wp_sp->SetEnabled(false, false);
                 StopInfoSP stored_stop_info_sp = thread_sp->GetStopInfo();
                 assert(stored_stop_info_sp.get() == this);
 
                 ThreadPlanSP new_plan_sp(
                     thread_sp->QueueThreadPlanForStepSingleInstruction(
                         false,  // step-over
                         false,  // abort_other_plans
                         true)); // stop_other_threads
                 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);
+                wp_sp->SetEnabled(true, 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:
          * 'n' is at 0x120010d00 and 'm' is 0x120010d04. When a watchpoint is
          * set at 'm', then
          * watch exception is generated even when 'n' is read/written. To handle
          * this case,
          * server emulates the instruction at PC and finds the base address of
if (thread_sp) {
         // 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())
           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
           // constructor errors up to the debugger's Async I/O.
           ExpressionResults result_code;
           EvaluateExpressionOptions expr_options;
           expr_options.SetUnwindOnError(true);
           expr_options.SetIgnoreBreakpoints(true);
if (thread_sp) {
               } else {
                 m_should_stop = true;
                 if (log)
                   log->Printf(
                       "Failed to get an integer result from the expression.");
               }
             }
           } else {
-            Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
             StreamSP error_sp = debugger.GetAsyncErrorStream();
             error_sp->Printf(
                 "Stopped due to an error evaluating condition of watchpoint ");
             wp_sp->GetDescription(error_sp.get(), eDescriptionLevelBrief);
             error_sp->Printf(": \"%s\"", wp_sp->GetConditionText());
             error_sp->EOL();
             const char *err_str = error.AsCString("<Unknown Error>");
             if (log)
               log->Printf("Error evaluating condition: \"%s\"\n", err_str);
 
             error_sp->PutCString(err_str);
             error_sp->EOL();
             error_sp->Flush();
             // If the condition fails to be parsed or run, we should stop.
             m_should_stop = true;
           }
         }
 
         // 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.
           if (HasTargetRunSinceMe())
             m_should_stop = false;
 
           if (m_should_stop && !stop_requested) {
             // We have been vetoed by the callback mechanism.