Make sure the "synchronous breakpoint callbacks" get called before the thread plan logic gets invoked, and if they

ask to continue that should short-circuit the thread plans for that thread.  Also add a bit more explanation for
how this machinery is supposed to work.  
Also pass eExecutionPolicyOnlyWhenNeeded, not eExecutionPolicyAlways when evaluating the expression for breakpoint
conditions.

llvm-svn: 155236

6 files changed, 143 insertions, 4 deletions

diff --git a/lldb/include/lldb/Breakpoint/BreakpointOptions.h b/lldb/include/lldb/Breakpoint/BreakpointOptions.h
index 7a6100b05ff..5d590c5afa2 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointOptions.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointOptions.h
@@ -79,15 +79,91 @@ public:
 
     //------------------------------------------------------------------
     // Callbacks
+    //
+    // Breakpoint callbacks come in two forms, synchronous and asynchronous.  Synchronous callbacks will get
+    // run before any of the thread plans are consulted, and if they return false the target will continue
+    // "under the radar" of the thread plans.  There are a couple of restrictions to synchronous callbacks:
+    // 1) They should NOT resume the target themselves.  Just return false if you want the target to restart.
+    // 2) Breakpoints with synchronous callbacks can't have conditions (or rather, they can have them, but they
+    //    won't do anything.  Ditto with ignore counts, etc...  You are supposed to control that all through the
+    //    callback.
+    // Asynchronous callbacks get run as part of the "ShouldStop" logic in the thread plan.  The logic there is:
+    //   a) If the breakpoint is thread specific and not for this thread, continue w/o running the callback.
+    //   b) If the ignore count says we shouldn't stop, then ditto.
+    //   c) If the condition says we shouldn't stop, then ditto.
+    //   d) Otherwise, the callback will get run, and if it returns true we will stop, and if false we won't.
+    //  The asynchronous callback can run the target itself, but at present that should be the last action the
+    //  callback does.  We will relax this condition at some point, but it will take a bit of plumbing to get
+    //  that to work.
+    // 
+    //------------------------------------------------------------------
+    
+    //------------------------------------------------------------------
+    /// Adds a callback to the breakpoint option set.
+    ///
+    /// @param[in] callback
+    ///    The function to be called when the breakpoint gets hit.
+    ///
+    /// @param[in] baton_sp
+    ///    A baton which will get passed back to the callback when it is invoked.
+    ///
+    /// @param[in] synchronous
+    ///    Whether this is a synchronous or asynchronous callback.  See discussion above.
     //------------------------------------------------------------------
     void SetCallback (BreakpointHitCallback callback, const lldb::BatonSP &baton_sp, bool synchronous = false);
+    
+    
+    //------------------------------------------------------------------
+    /// Remove the callback from this option set.
+    //------------------------------------------------------------------
+    void ClearCallback ();
+
+    // The rest of these functions are meant to be used only within the breakpoint handling mechanism.
+    
+    //------------------------------------------------------------------
+    /// Use this function to invoke the callback for a specific stop.
+    ///
+    /// @param[in] context
+    ///    The context in which the callback is to be invoked.  This includes the stop event, the
+    ///    execution context of the stop (since you might hit the same breakpoint on multiple threads) and
+    ///    whether we are currently executing synchronous or asynchronous callbacks.
+    /// 
+    /// @param[in] break_id
+    ///    The breakpoint ID that owns this option set.
+    ///
+    /// @param[in] break_loc_id
+    ///    The breakpoint location ID that owns this option set.
+    ///
+    /// @return
+    ///     The callback return value.
+    //------------------------------------------------------------------
     bool InvokeCallback (StoppointCallbackContext *context, lldb::user_id_t break_id, lldb::user_id_t break_loc_id);
+    
+    //------------------------------------------------------------------
+    /// Used in InvokeCallback to tell whether it is the right time to run this kind of callback.
+    ///
+    /// @param[in] condition
+    ///    The condition expression to evaluate when the breakpoint is hit.
+    //------------------------------------------------------------------
     bool IsCallbackSynchronous () {
         return m_callback_is_synchronous;
     }
+    
+    //------------------------------------------------------------------
+    /// Fetch the baton from the callback.
+    ///
+    /// @return
+    ///     The baton.
+    //------------------------------------------------------------------
     Baton *GetBaton ();
+    
+    //------------------------------------------------------------------
+    /// Fetch  a const version of the baton from the callback.
+    ///
+    /// @return
+    ///     The baton.
+    //------------------------------------------------------------------
     const Baton *GetBaton () const;
-    void ClearCallback ();
     
     //------------------------------------------------------------------
     // Condition
diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h
index 6711e2329d8..3ed6d804c24 100644
--- a/lldb/include/lldb/Target/StopInfo.h
+++ b/lldb/include/lldb/Target/StopInfo.h
@@ -74,12 +74,25 @@ public:
     }
 
     // Stop the thread by default. Subclasses can override this to allow
-    // the thread to continue if desired.
+    // the thread to continue if desired.  The ShouldStop method should not do anything
+    // that might run code.  If you need to run code when deciding whether to stop
+    // at this StopInfo, that must be done in the PerformAction.  The PerformAction will
+    // always get called before the ShouldStop.
     virtual bool
     ShouldStop (Event *event_ptr)
     {
         return true;
     }
+    
+    // ShouldStopSynchronous will get called before any thread plans are consulted, and if it says we should
+    // resume the target, then we will just immediately resume.  This should not run any code in or resume the
+    // target.
+    
+    virtual bool
+    ShouldStopSynchronous (Event *event_ptr)
+    {
+        return true;
+    }
 
     // If should stop returns false, check if we should notify of this event
     virtual bool
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index d5ef53b46cb..113aa57aacb 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -134,7 +134,7 @@ public:
     }
 
     virtual bool
-    ShouldStop (Event *event_ptr)
+    ShouldStopSynchronous (Event *event_ptr)
     {
         if (!m_should_stop_is_valid)
         {
@@ -160,6 +160,15 @@ public:
         return m_should_stop;
     }
     
+    bool
+    ShouldStop (Event *event_ptr)
+    {
+        // This just reports the work done by PerformAction or the synchronous stop.  It should
+        // only ever get called after they have had a chance to run.
+        assert (m_should_stop_is_valid);
+        return m_should_stop;
+    }
+    
     virtual void
     PerformAction (Event *event_ptr)
     {
@@ -216,7 +225,7 @@ public:
                         const bool discard_on_error = true;
                         Error error;
                         result_code = ClangUserExpression::EvaluateWithError (exe_ctx,
-                                                                              eExecutionPolicyAlways,
+                                                                              eExecutionPolicyOnlyWhenNeeded,
                                                                               lldb::eLanguageTypeUnknown,
                                                                               ClangUserExpression::eResultTypeAny,
                                                                               discard_on_error,
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index c483e5ae3da..7445ffc06f2 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -333,6 +333,17 @@ Thread::ShouldStop (Event* event_ptr)
     
     // The top most plan always gets to do the trace log...
     current_plan->DoTraceLog ();
+    
+    // First query the stop info's ShouldStopSynchronous.  This handles "synchronous" stop reasons, for example the breakpoint
+    // command on internal breakpoints.  If a synchronous stop reason says we should not stop, then we don't have to
+    // do any more work on this stop.
+    StopInfoSP private_stop_info (GetPrivateStopReason());
+    if (private_stop_info && private_stop_info->ShouldStopSynchronous(event_ptr) == false)
+    {
+        if (log)
+            log->Printf ("StopInfo::ShouldStop async callback says we should not stop, returning ShouldStop of false.");
+        return false;
+    }
 
     // If the base plan doesn't understand why we stopped, then we have to find a plan that does.
     // If that plan is still working, then we don't need to do any more work.  If the plan that explains 
diff --git a/lldb/source/Target/ThreadPlanStepRange.cpp b/lldb/source/Target/ThreadPlanStepRange.cpp
index 59f760c9e6b..113e45963ea 100644
--- a/lldb/source/Target/ThreadPlanStepRange.cpp
+++ b/lldb/source/Target/ThreadPlanStepRange.cpp
@@ -368,6 +368,9 @@ ThreadPlanStepRange::PlanExplainsStop ()
         case eStopReasonBreakpoint:
             if (NextRangeBreakpointExplainsStop(stop_info_sp))
                 return true;
+            else
+                return false;
+            break;
         case eStopReasonWatchpoint:
         case eStopReasonSignal:
         case eStopReasonException:
diff --git a/lldb/test/functionalities/load_unload/TestLoadUnload.py b/lldb/test/functionalities/load_unload/TestLoadUnload.py
index 129aac24244..2f58f0338e0 100644
--- a/lldb/test/functionalities/load_unload/TestLoadUnload.py
+++ b/lldb/test/functionalities/load_unload/TestLoadUnload.py
@@ -222,6 +222,33 @@ class LoadUnloadTestCase(TestBase):
         self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
             substrs = [' resolved, hit count = 2'])
 
+    def test_step_over_load (self):
+        """Test stepping over code that loads a shared library works correctly."""
+
+        # Invoke the default build rule.
+        self.buildDefault()
+
+        exe = os.path.join(os.getcwd(), "a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+        # Break by function name a_function (not yet loaded).
+        self.expect("breakpoint set -f main.c -l %d"%(self.line), BREAKPOINT_CREATED,
+            substrs = ['Breakpoint created:',
+                      "file ='main.c', line = %d, locations = 1"%(self.line)])
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint and at a_function.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+            substrs = ['stopped',
+                       'stop reason = breakpoint'])
+
+        self.runCmd("thread step-over", "Stepping over function that loads library")
+        
+        # The stop reason should be step end.
+        self.expect("thread list", "step over succeeded.", 
+            substrs = ['stopped',
+                      'stop reason = step over'])
 
 if __name__ == '__main__':
     import atexit