RunThreadPlan: Fix halting logic in IgnoreBreakpoints = false

[labath]

Summary

The function had logic to handle the case when the expression terminated
while we were trying to halt the process, but it failed to take into
account the possibility that the expression stopped because it hit a
breakpoint. This was caused by the fact that the handling of the stopped
events was duplicated for the "halting" and regular cases (the regular
case handled this situation correctly). I've tried to merge these two
cases into one to make sure they stay in sync.

I should call out that the two cases were checking whether the thread
plan has completed in slightly different ways. I am not sure what is the
difference between them, but I think the check should be the same in
both cases, whatever it is, so I just took the one from the regular
case, as that is probably more tested.

For the test, I modified TestUnwindExpression to run the expression with
a smaller timeout (this is how I found this bug originally). With a 1ms
one thread timeout, the test failed consistently without this patch.

[jingham]

The first code site did: checking for thread plan success, then check for hit breakpoint, then for anything else (thread plan failed or other stop reason.)

The code in the first of your substitution sites retains the anything else case outside the bits you factored out, in the if clause you've retained after your function (5281 in the new version.)

But ThreadPlanDone returns true regardless of the success or failure of the plan (it gets used in a bunch of places, so it's no less well tested, just had different semantics...) So if you use your new function in the second site, the code will no longer handle the case where the plan was completed but failed. Be good to keep a check for that.

Other than that, this looks good. Thanks for catching this.

[labath]

I'm not sure I understand what you're saying. Did you mean to say that I should add the "thread plan didn't successfully complete." (line 5281) block to the "Halt" branch as well ? (possibly by including it into the factored out function)

[labath]

Ok, I've missed the distinction between plan completing (aka being "done") and completing sucessfully. Things make a bit more sense after that.

With that in mind, let me try to explain how I understand it the code now, and then you can tell me if it's correct :)

For our purposes we can get a Stopped event due to one of these things happening:

• plan_success
• breakpoint_hit
• interrupt
• plan_failure (I'm not really sure when can that happen)
• other (this can include the process stopping due to a signal)

What the old code did in the non-halt case was:

if(plan_success) handle_and_stop()
else if(breakpoint_hit) handle_and_stop()
else {
  // "interrupt" (which we can still get even though we haven't sent it) , "plan_failure" or "other"
  handle_and_stop()
}

The old code in the halt case did:

if (plan_failure || plan_success) handle_and_stop()
else {
  // "interrupt", "breakpoint_hit" or "other"
  resume_with_all_threads()
}

Here, the else part is wrong because we treat two other events the same way as the interrupt.

In my mind, the two versions of the code should behave the same way for all cases except for the "interrupt" case. In the first one, we should stop because the interrupt must have been initiated externally, while in the second one, we should resume because it was (probably) our halt event.

I'll upload a new version which attempts to to just that. The logic in the first case should remain unchanged, while the "halt" logic becomes:

if(plan_success) handle_and_stop()
else if(breakpoint_hit) handle_and_stop()
else if(interrupt) resume_with_all_threads()
else {
  // "plan_failure" or "other"
  handle_and_stop()
}

Let me know what you think.

PS: I'm not in a hurry with this, so I can wait a couple of days if you're busy right now.

[labath]

New version

[jingham]

That looks right to me, and is much nicer to read.

I think "plan failure" once you've actually kicked off the execution of a function calling thread plan is theoretical, there are plenty of ways the plan can fail, though at present all the ways I can think of would happen either before or after you got here. But just because we haven't found a way to make that stage fail yet...

[jingham]

Oh, yeah, check the checkbox, Jim...

[labath]

thank you

[labath]

Reopening for a re-review of a fix.

[labath]

Fixed version.

The original patch caused a regression in TestLoadUnload, which has only showed
up when running the remote test suite. The problem there was that we interrupted
the target just as it has hit the rendezvous breakpoint in the dlopen call. This
meant that the stop reason was set to "breakpoint" even though the event would
not have been broadcast if we had not stopped the process. I fix this by
checking StopInfo->ShouldNotify() before stopping.

I was hoping I would be able to create a more reliable test for this bug by
calling an expression which will hit a conditional breakpoint, whose condition
will evaluate to false. However, I have found that this does not work at all --
we always stop at the breakpoint, regardless of the expression. So, I add a
(disabled) test for that instead.

[labath]

Let me know what you think of the fix, and please confirm whether the ignoring of the breakpoint condition is a bug.

thanks.

[jingham]

The fix seems good.

The fact that a breakpoint hit while evaluating an expression doesn't check the condition is a known limitation.

You've got to protect against artificial recursions in handling breakpoints. For instance, you could have a breakpoint condition in a function where the condition calls back into the same function, hitting the breakpoint again, calling the condition again, etc... Sounds a bit far-fetched, but it actually happened in the field, which is why I added this protection. The code that does this is in StopInfoBreakpoint.cpp::PerformAction.

We could be smarter about condition evaluation, since nesting expression evaluation does work. Maybe tracking how deeply nested the condition evaluation is and having a cutoff at some (settable?) depth. Or have a "I promise I'm not going to do anything stupid" setting - though that seems hacky... I don't think there is a bug about this, but it would be great to file one.

Note, we also prohibit breakpoint command execution in expressions. We have to do that because the current implementation of the command evaluator isn't re-entrant. We really should fix that some day, but that's a decent chunk of work. Anyway, so we have to be draconian about command evaluation, but with some care we could relax the condition evaluation prohibition.

[labath]

Thanks for the explanation. I'll continue the discussion about the conditional breakpoint thingy on bug #33164, which I've just filed.

lldb/trunk/packages/Python/lldbsuite/test/expression_command/unwind_expression/TestUnwindExpression.py

 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class UnwindFromExpressionTest(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
+    main_spec = lldb.SBFileSpec("main.cpp", False)
 
-    def setUp(self):
-        # Call super's setUp().
-        TestBase.setUp(self)
-
-    @add_test_categories(['pyapi'])
-    @expectedFailureAll(oslist=["windows"])
-    def test_unwind_expression(self):
-        """Test unwinding from an expression."""
+    def build_and_run_to_bkpt(self):
         self.build()
 
         exe = os.path.join(os.getcwd(), "a.out")
 
         target = self.dbg.CreateTarget(exe)
         self.assertTrue(target, VALID_TARGET)
 
         # Create the breakpoint.
-        main_spec = lldb.SBFileSpec("main.cpp", False)
         breakpoint = target.BreakpointCreateBySourceRegex(
-            "// Set a breakpoint here to get started", main_spec)
+            "// Set a breakpoint here to get started", self.main_spec)
         self.assertTrue(breakpoint, VALID_BREAKPOINT)
 
         # Launch the process, and do not stop at the entry point.
         process = target.LaunchSimple(
             None, None, self.get_process_working_directory())
 
         if not process:
             self.fail("SBTarget.LaunchProcess() failed")
 
         if process.GetState() != lldb.eStateStopped:
             self.fail("Process should be in the 'stopped' state, "
                       "instead the actual state is: '%s'" %
                       lldbutil.state_type_to_str(process.GetState()))
 
-        thread = lldbutil.get_one_thread_stopped_at_breakpoint(
+        self.thread = lldbutil.get_one_thread_stopped_at_breakpoint(
             process, breakpoint)
         self.assertIsNotNone(
-            thread, "Expected one thread to be stopped at the breakpoint")
+            self.thread, "Expected one thread to be stopped at the breakpoint")
+
+        # Next set a breakpoint in this function, set up Expression options to stop on
+        # breakpoint hits, and call the function.
+        self.fun_bkpt = self.target().BreakpointCreateBySourceRegex(
+            "// Stop inside the function here.", self.main_spec)
+        self.assertTrue(self.fun_bkpt, VALID_BREAKPOINT)
+
+
+    @no_debug_info_test
+    @expectedFailureAll(bugnumber="llvm.org/pr33164")
+    def test_conditional_bktp(self):
+        """
+        Test conditional breakpoint handling in the IgnoreBreakpoints = False case
+        """
+        self.build_and_run_to_bkpt()
 
+        self.fun_bkpt.SetCondition("0") # Should not get hit
+        options = lldb.SBExpressionOptions()
+        options.SetIgnoreBreakpoints(False)
+        options.SetUnwindOnError(False)
+
+        main_frame = self.thread.GetFrameAtIndex(0)
+        val = main_frame.EvaluateExpression("second_function(47)", options)
+        self.assertTrue(
+            val.GetError().Success(),
+            "We did complete the execution.")
+        self.assertEquals(47, val.GetValueAsSigned())
+
+
+    @add_test_categories(['pyapi'])
+    @expectedFailureAll(oslist=["windows"])
+    def test_unwind_expression(self):
+        """Test unwinding from an expression."""
+        self.build_and_run_to_bkpt()
+
+        # Run test with varying one thread timeouts to also test the halting
+        # logic in the IgnoreBreakpoints = False case
+        self.do_unwind_test(self.thread, self.fun_bkpt, 1000)
+        self.do_unwind_test(self.thread, self.fun_bkpt, 100000)
+
+    def do_unwind_test(self, thread, bkpt, timeout):
         #
         # Use Python API to evaluate expressions while stopped in a stack frame.
         #
         main_frame = thread.GetFrameAtIndex(0)
 
-        # Next set a breakpoint in this function, set up Expression options to stop on
-        # breakpoint hits, and call the function.
-        fun_bkpt = target.BreakpointCreateBySourceRegex(
-            "// Stop inside the function here.", main_spec)
-        self.assertTrue(fun_bkpt, VALID_BREAKPOINT)
         options = lldb.SBExpressionOptions()
         options.SetIgnoreBreakpoints(False)
         options.SetUnwindOnError(False)
+        options.SetOneThreadTimeoutInMicroSeconds(timeout)
 
         val = main_frame.EvaluateExpression("a_function_to_call()", options)
 
         self.assertTrue(
             val.GetError().Fail(),
             "We did not complete the execution.")
         error_str = val.GetError().GetCString()
         self.assertTrue(
             "Execution was interrupted, reason: breakpoint" in error_str,
             "And the reason was right.")
 
         thread = lldbutil.get_one_thread_stopped_at_breakpoint(
-            process, fun_bkpt)
+            self.process(), bkpt)
         self.assertTrue(
             thread.IsValid(),
             "We are indeed stopped at our breakpoint")
 
         # Now unwind the expression, and make sure we got back to where we
         # started.
         error = thread.UnwindInnermostExpression()
         self.assertTrue(error.Success(), "We succeeded in unwinding")
 
         cur_frame = thread.GetFrameAtIndex(0)
         self.assertTrue(
             cur_frame.IsEqual(main_frame),
             "We got back to the main frame.")

lldb/trunk/packages/Python/lldbsuite/test/expression_command/unwind_expression/main.cpp

 static int static_value = 0;
 
 int
 a_function_to_call()
 {
     static_value++; // Stop inside the function here.
     return static_value;
 }
 
+int second_function(int x){
+  for(int i=0; i<10; ++i) {
+    a_function_to_call();
+  }
+  return x;
+}
+
 int main (int argc, char const *argv[])
 {
     a_function_to_call();  // Set a breakpoint here to get started 
+    second_function(1);
     return 0;
 }

lldb/trunk/source/Target/Process.cpp

if (before_first_timeout)
     return GetOneThreadExpressionTimeout(options);
 
   if (!options.GetTimeout())
     return llvm::None;
   else
     return *options.GetTimeout() - GetOneThreadExpressionTimeout(options);
 }
 
+static llvm::Optional<ExpressionResults>
+HandleStoppedEvent(Thread &thread, const ThreadPlanSP &thread_plan_sp,
+                   RestorePlanState &restorer, const EventSP &event_sp,
+                   EventSP &event_to_broadcast_sp,
+                   const EvaluateExpressionOptions &options, bool handle_interrupts) {
+  Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STEP | LIBLLDB_LOG_PROCESS);
+
+  ThreadPlanSP plan = thread.GetCompletedPlan();
+  if (plan == thread_plan_sp && plan->PlanSucceeded()) {
+    LLDB_LOG(log, "execution completed successfully");
+
+    // Restore the plan state so it will get reported as intended when we are
+    // done.
+    restorer.Clean();
+    return eExpressionCompleted;
+  }
+
+  StopInfoSP stop_info_sp = thread.GetStopInfo();
+  if (stop_info_sp && stop_info_sp->GetStopReason() == eStopReasonBreakpoint &&
+      stop_info_sp->ShouldNotify(event_sp.get())) {
+    LLDB_LOG(log, "stopped for breakpoint: {0}.", stop_info_sp->GetDescription());
+    if (!options.DoesIgnoreBreakpoints()) {
+      // Restore the plan state and then force Private to false.  We are going
+      // to stop because of this plan so we need it to become a public plan or
+      // it won't report correctly when we continue to its termination later on.
+      restorer.Clean();
+      thread_plan_sp->SetPrivate(false);
+      event_to_broadcast_sp = event_sp;
+    }
+    return eExpressionHitBreakpoint;
+  }
+
+  if (!handle_interrupts &&
+      Process::ProcessEventData::GetInterruptedFromEvent(event_sp.get()))
+    return llvm::None;
+
+  LLDB_LOG(log, "thread plan did not successfully complete");
+  if (!options.DoesUnwindOnError())
+    event_to_broadcast_sp = event_sp;
+  return eExpressionInterrupted;
+}
+
 ExpressionResults
 Process::RunThreadPlan(ExecutionContext &exe_ctx,
                        lldb::ThreadPlanSP &thread_plan_sp,
                        const EvaluateExpressionOptions &options,
                        DiagnosticManager &diagnostic_manager) {
   ExpressionResults return_value = eExpressionSetupError;
 
   std::lock_guard<std::mutex> run_thread_plan_locker(m_run_thread_plan_lock);
#endif
               if (!thread_sp) {
                 // Ooh, our thread has vanished.  Unlikely that this was
                 // successful execution...
                 if (log)
                   log->Printf("Process::RunThreadPlan(): execution completed "
                               "but our thread (index-id=%u) has vanished.",
                               thread_idx_id);
                 return_value = eExpressionInterrupted;
-              } else {
+              } else if (Process::ProcessEventData::GetRestartedFromEvent(
+                             event_sp.get())) {
                 // If we were restarted, we just need to go back up to fetch
                 // another event.
-                if (Process::ProcessEventData::GetRestartedFromEvent(
-                        event_sp.get())) {
-                  if (log) {
-                    log->Printf("Process::RunThreadPlan(): Got a stop and "
-                                "restart, so we'll continue waiting.");
-                  }
-                  keep_going = true;
-                  do_resume = false;
-                  handle_running_event = true;
-                } else {
-                  ThreadPlanSP plan = thread->GetCompletedPlan();
-                  if (plan == thread_plan_sp && plan->PlanSucceeded()) {
-
-                    if (log)
-                      log->PutCString("Process::RunThreadPlan(): execution "
-                                      "completed successfully.");
-
-                    // Restore the plan state so it will get reported as
-                    // intended when we are done.
-                    thread_plan_restorer.Clean();
-
-                    return_value = eExpressionCompleted;
-                  } else {
-                    StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
-                    // Something restarted the target, so just wait for it to
-                    // stop for real.
-                    if (stop_info_sp &&
-                        stop_info_sp->GetStopReason() == eStopReasonBreakpoint) {
-                      if (log)
-                        log->Printf("Process::RunThreadPlan() stopped for "
-                                    "breakpoint: %s.",
-                                    stop_info_sp->GetDescription());
-                      return_value = eExpressionHitBreakpoint;
-                      if (!options.DoesIgnoreBreakpoints()) {
-                        // Restore the plan state and then force Private to
-                        // false.  We are
-                        // going to stop because of this plan so we need it to
-                        // become a public
-                        // plan or it won't report correctly when we continue to
-                        // its termination
-                        // later on.
-                        thread_plan_restorer.Clean();
-                        if (thread_plan_sp)
-                          thread_plan_sp->SetPrivate(false);
-                        event_to_broadcast_sp = event_sp;
-                      }
-                    } else {
-                      if (log)
-                        log->PutCString("Process::RunThreadPlan(): thread plan "
-                                        "didn't successfully complete.");
-                      if (!options.DoesUnwindOnError())
-                        event_to_broadcast_sp = event_sp;
-                      return_value = eExpressionInterrupted;
-                    }
-                  }
-                }
+                const bool handle_interrupts = true;
+                return_value = *HandleStoppedEvent(
+                    *thread, thread_plan_sp, thread_plan_restorer, event_sp,
+                    event_to_broadcast_sp, options, handle_interrupts);
               }
             } break;
 
             case lldb::eStateRunning:
               // This shouldn't really happen, but sometimes we do get two
               // running events without an
               // intervening stop, and in that case we should just go back to
               // waiting for the stop.
#endif
                             StateAsCString(stop_state));
                 if (stop_state == lldb::eStateStopped &&
                     Process::ProcessEventData::GetInterruptedFromEvent(
                         event_sp.get()))
                   log->PutCString("    Event was the Halt interruption event.");
               }
 
               if (stop_state == lldb::eStateStopped) {
-                // Between the time we initiated the Halt and the time we
-                // delivered it, the process could have
-                // already finished its job.  Check that here:
-
-                if (thread->IsThreadPlanDone(thread_plan_sp.get())) {
-                  if (log)
-                    log->PutCString("Process::RunThreadPlan(): Even though we "
-                                    "timed out, the call plan was done.  "
-                                    "Exiting wait loop.");
-                  return_value = eExpressionCompleted;
-                  back_to_top = false;
-                  break;
-                }
-
                 if (Process::ProcessEventData::GetRestartedFromEvent(
                         event_sp.get())) {
                   if (log)
                     log->PutCString("Process::RunThreadPlan(): Went to halt "
                                     "but got a restarted event, there must be "
                                     "an un-restarted stopped event so try "
                                     "again...  "
                                     "Exiting wait loop.");
                   try_halt_again++;
                   do_halt = false;
                   continue;
                 }
 
+                // Between the time we initiated the Halt and the time we
+                // delivered it, the process could have
+                // already finished its job.  Check that here:
+                const bool handle_interrupts = false;
+                if (auto result = HandleStoppedEvent(
+                        *thread, thread_plan_sp, thread_plan_restorer, event_sp,
+                        event_to_broadcast_sp, options, handle_interrupts)) {
+                  return_value = *result;
+                  back_to_top = false;
+                  break;
+                }
+
                 if (!options.GetTryAllThreads()) {
                   if (log)
                     log->PutCString("Process::RunThreadPlan(): try_all_threads "
                                     "was false, we stopped so now we're "
                                     "quitting.");
                   return_value = eExpressionInterrupted;
                   back_to_top = false;
                   break;