stop-hooks don't fire on "step-out"

[jingham]

Summary

Step hooks were not firing when you did a "step out".

This was because, when a breakpoint's PerformAction observes that the breakpoint has completed a plan, it resets the thread StopInfo so that the completed plan will be reported as the stop reason, not the trap that is the private stop reason. But for no good reason that I can remember, it doesn't immediately reset the StopInfo to the completed plan one, but lets that happen the next time somebody gets the stop reason. That didn't happen by the time stop-hooks checked if the thread had a stop reason.

We could make the stop-hooks force the StopInfo to get recomputed, but that might happen somewhere else as well. It's better to just do it right away.

I also added a test for this case.

[davide]

packages/Python/lldbsuite/test/functionalities/stop-hooks/Makefile

4 ↗

(On Diff #215201)

Forgive my ignorance, but why you need this?

[jingham]

packages/Python/lldbsuite/test/functionalities/stop-hooks/Makefile

4 ↗

(On Diff #215201)

That's how the dotest tests know what to build.

[davide]

packages/Python/lldbsuite/test/functionalities/stop-hooks/Makefile

4 ↗

(On Diff #215201)

I understand C_SOURCES, but do you need to specify also the standard? That seems redundant, but I might be missing something [mainly asking for my education].

[clayborg]

source/Target/StopInfo.cpp

550 ↗

(On Diff #215201)

Can you clarify what is going on here? Does this force a recalculation of the stop info? This looks really goofy from a code perspective.

[jingham]

Do you think I should put more verbiage in the comment, or would this have been clear if you were actually working on the code?

source/Target/StopInfo.cpp

550 ↗

(On Diff #215201)

This is replacing the raw stop info (a StopInfoBreakpoint) with the public - cooked - stop info (StopInfoThreadPlan).

I think the handling of "public and private" stop info has gotten a bit muddled up. I swear when I first did this there were distinct Public and Private stop infos. But (I think when the StopInfo's acquired the Stop Actions many years back) we went to having just one stop info, and when you wanted to produce a different StopInfo from the one that you got from the Process plugin you overwrite it.

I think it would be worth playing around with keeping these two separate. It would make this sort of code much clearer, since then you wouldn't muck with the truth you got from the Process plugin, but just layer the public facing Info over top of it. But that's a much bigger change.

[clayborg]

source/Target/StopInfo.cpp

550 ↗

(On Diff #215201)

ok, it does look quite goofy from a API usage scenario. Seems like you are getting and then setting to the same value. If we had an API that returned an integer, it would be like:

foo->SetInt(foo->GetInt());

Can we just do this in ResetStopInfo()? Or rename it to "UpdateStopInfo" or something clearer? I would like to see this:

thread_sp->ResetStopInfo();
thread_sp->SetStopInfo(thread_sp->GetStopInfo());

become:

thread_sp->UpdateStopInfo();

[jingham]

Does this seem clearer? We use the language of Private and Public StopInfo's in a bunch of places, even though at present that's more about "How" you get them than actual separate entities. But still, this seems like it follows the language we are using elsewhere.

[clayborg]

Better. Might be good to put a comment inside CalculatePublicStopInfo() regarding why the "SetStopInfo(GetStopInfo());" is needed. Seems like you could just remove the code from the sight of it. I will leave that up to you though.

[jingham]

In D66241#1631426, @clayborg wrote:

Better. Might be good to put a comment inside CalculatePublicStopInfo() regarding why the "SetStopInfo(GetStopInfo());" is needed. Seems like you could just remove the code from the sight of it. I will leave that up to you though.

The thing you have to understand for this to make sense is that what you get from GetStopInfo depends on when you ask the question. If you have just stopped, but haven't gone through the ShouldStop mechanism, then there are no completed plans, and you get the raw stop info. But if you ask again after we've done "ShouldStop", then we're in a position to attribute the stop to the plan that was managing it. That's why I said explicitly you had to do CalculatePublicStopInfo after ShouldStop.

I can refine the comment a bit to make this clearer.

I think this is going to be a little opaque so long as we aren't making a real distinction between the private (what I used to call the actual stop info - there's even a few comments that still reference this...) and the public stop info.

There's the added complication that sometimes you need to rewind to an old stop reason. For instance, you finish a step out, then run a function. Now the actual stop info is the breakpoint stop at the entry point that caught the end of the expression evaluation. The public stop info is "Completed Function Thread Plan". But showing that would be confusing, users don't care that a function was run, particularly if it was one they didn't directly cause. The stop info you actually want to show users is still "Completed step out plan".

These complexities are overlaid on one stop info, and a bunch of stop_id variables. We also rely on the assumption that you can always re-fetch the private stop info with the plugin-implemented CalculateStopInfo call. But you have to be careful, because though you can rewind to an old Public StopInfo, you can't get back to the actual StopInfo that caused it - since the process has moved on. This requires other bits of fancy footwork.

This all needs to be teased apart and better represented. But that's a task for another day.

lldb/trunk/include/lldb/Target/Thread.h

   // moved to the completed plan stack (in Thread::ShouldStop()).
   //
   // NOTE: If subclasses override this function, ensure they do not overwrite
   // the m_actual_stop_info if it is valid.  The stop info may be a
   // "checkpointed and restored" stop info, so if it is still around it is
   // right even if you have not calculated this yourself, or if it disagrees
   // with what you might have calculated.
   virtual lldb::StopInfoSP GetPrivateStopInfo();
   
+  // Calculate the stop info that will be shown to lldb clients.  For instance,
+  // a "step out" is implemented by running to a breakpoint on the function 
+  // return PC, so the process plugin initially sets the stop info to a
+  // StopInfoBreakpoint. But once we've run the ShouldStop machinery, we
+  // discover that there's a completed ThreadPlanStepOut, and that's really
+  // the StopInfo we want to show.  That will happen naturally the next
+  // time GetStopInfo is called, but if you want to force the replacement,
+  // you can call this.
+
+  void CalculatePublicStopInfo();
+
   // Ask the thread subclass to set its stop info.
   //
   // Thread subclasses should call Thread::SetStopInfo(...) with the reason the
   // thread stopped.
   //
   // \return
   //      True if Thread::SetStopInfo(...) was called, false otherwise.
   virtual bool CalculateStopInfo() = 0;

lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hooks/Makefile

+LEVEL = ../../make
+
+C_SOURCES := main.c
+CFLAGS_EXTRAS += -std=c99
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hooks/TestStopHooks.py

+"""
+Test that stop hooks trigger on "step-out"
+"""
+
+from __future__ import print_function
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestStopHooks(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    # If your test case doesn't stress debug info, the
+    # set this to true.  That way it won't be run once for
+    # each debug info format.
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_stop_hooks_step_out(self):
+        """Test that stop hooks fire on step-out."""
+        self.build()
+        self.main_source_file = lldb.SBFileSpec("main.c")
+        self.step_out_test()
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+
+    def step_out_test(self):
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                   "Set a breakpoint here", self.main_source_file)
+
+        interp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+        interp.HandleCommand("target stop-hook add -o 'expr g_var++'", result)
+        self.assertTrue(result.Succeeded, "Set the target stop hook")
+        thread.StepOut()
+        var = target.FindFirstGlobalVariable("g_var")
+        self.assertTrue(var.IsValid())
+        self.assertEqual(var.GetValueAsUnsigned(), 1, "Updated g_var")
+
+

lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hooks/main.c

+#include <stdio.h>
+
+static int g_var = 0;
+
+int step_out_of_me()
+{
+  return g_var; // Set a breakpoint here and step out.
+}
+
+int
+main()
+{
+  return step_out_of_me();
+}

lldb/trunk/source/Target/StopInfo.cpp

if (thread_sp) {
 
       if ((!m_should_stop || internal_breakpoint) &&
           thread_sp->CompletedPlanOverridesBreakpoint()) {
 
         // Override should_stop decision when we have completed step plan
         // additionally to the breakpoint
         m_should_stop = true;
         
-        // Here we clean the preset stop info so the next GetStopInfo call will
-        // find the appropriate stop info, which should be the stop info
-        // related to the completed plan
-        thread_sp->ResetStopInfo();
+        // We know we're stopping for a completed plan and we don't want to
+        // show the breakpoint stop, so compute the public stop info immediately
+        // here.
+        thread_sp->CalculatePublicStopInfo();
       }
 
       LLDB_LOGF(log,
                 "Process::%s returning from action with m_should_stop: %d.",
                 __FUNCTION__, m_should_stop);
     }
   }
 

lldb/trunk/source/Target/Thread.cpp

if (have_valid_stop_info && !plan_overrides_trace && !plan_failed) {
     return StopInfo::CreateStopReasonWithPlan(
         completed_plan_sp, GetReturnValueObject(), GetExpressionVariable());
   } else {
     GetPrivateStopInfo();
     return m_stop_info_sp;
   }
 }
 
+void Thread::CalculatePublicStopInfo() {
+  ResetStopInfo();
+  SetStopInfo(GetStopInfo());
+}
+
 lldb::StopInfoSP Thread::GetPrivateStopInfo() {
   if (m_destroy_called)
     return m_stop_info_sp;
 
   ProcessSP process_sp(GetProcess());
   if (process_sp) {
     const uint32_t process_stop_id = process_sp->GetStopID();
     if (m_stop_info_stop_id != process_stop_id) {