Add completed_plan_stack to LLDB ThreadStateCheckpoint

[boris.ulasevich]

Summary

Here is a fix for Ubuntu tests fails after the recent change D26497 (https://reviews.llvm.org/D26497). The cause of those fails is in internal call to mmap fuction which destroys completed_plan_stack content which in its turn leads to incorrect stop reason discovering.

The solution is to save/restore completed_plan_stack along with existing ThreadStateCheckpoint functionality.

[labath]

Jim should probably review this change.

That said, I don't see any failures on linux currently. Can you elaborate on which tests are failing and why? It sounds like there is an opportunity here to write a more robust test...

[boris.ulasevich]

Jim should probably review this change.

Yes, thank you. We are in contact with Jim. And I see now I had put Jim to Subscribers list instead of Revievers list :)

That said, I don't see any failures on linux currently. Can you elaborate on which tests are failing and why?

Let me explain it. A month ago I got ready-to-land resolution for D26497 and committed it. Very soon I got tests fail notification from build robot: several tests (including my new test) failed on Ubuntu. The decision was to revert the change temporary, and study the issue without a rush. And now I am back with the fix for this issue. I am going to commit D26497 back after this fix.

[jingham]

The restoration of the stop reason works for the most part. For instance, when we call a user expression the stop reason is correctly restored. This was done by saving and restoring the stop info. There's one other little two-step you have to do to produce the "cooked" stop reason. For instance, if you stop because of a breakpoint hit that completed a "step-out" plan, you have to check the completed plan stack to figure out what the user visible stop reason was. But before Boris' change the state of the completed plan stack wasn't important once this was done. Boris' change started using the completed plan stack for more than just the initial determination of the cooked stop reason, so you do need to preserve it as well.

I don't think that letting the completed plans live a little longer should pose any problems. The plans should do whatever cleanup they are going to do when they get popped, and should not rely on the destructors to do this work. But it would be worth a quick audit to make sure I didn't get sloppy about that before we make this change, and we should document that requirement in the ThreadPlan dissertation at the beginning of ThreadPlan.h.

Can you do that, and then we should be fine to go in?

[jingham]

BTW, this shows one important difference between OS X and Linux that pops up from time to time. On OS X, we can allocate memory in the target process without having to call functions in the target. We don't do that by hand in lldb (that wouldn't work for remote debugging) rather we have a packet we send to debugserver, and it does the allocation. But the Linux lldb-server doesn't have this ability so on Linux, lldb ends up having to make and run a ThreadPlanCallFunction from awkward places in processing stops. This works for the most part, but it sometimes requires tweaks like this.

This functionality gets tested pretty rigorously just by using lldb on Linux, since this gets done as a side effect of many operations. So it is tested indirectly by the test suite. I'm not sure how you would test this directly, however.

[labath]

Thank you both for explaining the situation. The "test failure on ubuntu" part threw me off -- I did not realize you were talking about hypothetical failure should a different change go in. I was worried we have some very environment-dependent tests.

Looks good from my side then.

[boris.ulasevich]

In D28945#651806, @jingham wrote:

The plans should do whatever cleanup they are going to do when they get popped, and should not rely on the destructors to do this work.

Yes, you already made a note a time ago:

r123869 | jingham | 2011-01-20

Back up both the register AND the stop state when calling functions.

123869 jingham N.B. Don't wait to do clean up target state till the destructor, since that will usually get called when
123869 jingham the target resumes, and you want to leave the target state correct for new plans in the time between when
123869 jingham // your plan gets unshipped and the next resume.

I don't think that letting the completed plans live a little longer should pose any problems

It is not something extraordinary, but just a life frame the plans intended to live without unexpected internall call.

By the way, I fixed my issue by restoring specific Thread's property - don't you think it is a potential bug for somebody who will use another property (destroyed plans?) without proper treatment in Thread State Checkpoint?

But it would be worth a quick audit

As I see, actually destructors clears breakpoints - it is important, but not urgent job.

we should document that requirement in the ThreadPlan dissertation at the beginning of ThreadPlan.h

For me the ThreadPlan dissertation is like the Bible: I feel it is very old and complicated, a lot on wisdom is hidden there, but nobody around have ever read it attentively :)

I would add words about Thread State there and define stack direction to make words 'below' and 'higher' more definite. Please see new diff.

[boris.ulasevich]

description update

[jingham]

I can't think of a case where you would need the discarded plans to stick around across a function call, I don't think you need to preserve that stack.

I do worry a bit about how the completed plans go away, mostly as you say because of breakpoints which could be left without their ThreadPlan handlers. The simplest proceeding is to convert all the Destructor cleanups to WillPop and then just call that in the destructor. That way the real cleanup goes on when the plan is popped and the destructor is just a safety backup. Leaving the plans around longer increases the risk of this: especially when we know we are changing behavior to leave completed plans around while running a function call thread plan.

I did that conversion for the Function call thread plans a while back, but must have gotten interrupted on the way to doing them all. Anyway, I doesn't make sense to gate this change on completing that piece of work.

lldb/include/lldb/Target/Thread.h

private:
     DISALLOW_COPY_AND_ASSIGN(ThreadEventData);
   };
 
   struct ThreadStateCheckpoint {
     uint32_t orig_stop_id; // Dunno if I need this yet but it is an interesting
                            // bit of data.
     lldb::StopInfoSP stop_info_sp; // You have to restore the stop info or you
                                    // might continue with the wrong signals.
+    std::vector<lldb::ThreadPlanSP> m_completed_plan_stack;
     lldb::RegisterCheckpointSP
         register_backup_sp; // You need to restore the registers, of course...
     uint32_t current_inlined_depth;
     lldb::addr_t current_inlined_pc;
   };
 
   //------------------------------------------------------------------
   /// Constructor

lldb/include/lldb/Target/ThreadPlan.h

 //  all the logical process control, either directly from commands or through
 //  more complex composite plans will rely on.
 //
 //  Plan Stack:
 //
 //  The thread maintaining a thread plan stack, and you program the actions of a
 //  particular thread
 //  by pushing plans onto the plan stack.
-//  There is always a "Current" plan, which is the head of the plan stack,
+//  There is always a "Current" plan, which is the top of the plan stack,
 //  though in some cases
-//  a plan may defer to plans higher in the stack for some piece of information.
+//  a plan may defer to plans higher in the stack for some piece of information
+//  (let us define that the plan stack grows downwards).
 //
 //  The plan stack is never empty, there is always a Base Plan which persists
 //  through the life
 //  of the running process.
 //
 //
 //  Creating Plans:
 //

 //  leave the plan able to correctly
 //  fill the StopInfo, however.
 //  N.B. Don't wait to do clean up target state till the destructor, since that
 //  will usually get called when
 //  the target resumes, and you want to leave the target state correct for new
 //  plans in the time between when
 //  your plan gets unshipped and the next resume.
 //
+//  Thread State Checkpoint:
+//
+//  Note that calling functions on target process (ThreadPlanCallFunction) changes
+//  current thread state. The function can be called either by direct user demand or
+//  internally, for example lldb allocates memory on device to calculate breakpoint
+//  condition expression - on Linux it is performed by calling mmap on device.
+//  ThreadStateCheckpoint saves Thread state (stop info and completed
+//  plan stack) to restore it after completing function call.
+//
 //  Over the lifetime of the plan, various methods of the ThreadPlan are then
 //  called in response to changes of state in
 //  the process we are debugging as follows:
 //
 //  Resuming:
 //
 //  When the target process is about to be restarted, the plan's WillResume
 //  method is called,

 //
 //  When the target process stops, the plan is called in the following stages:
 //
 //  First the thread asks the Current Plan if it can handle this stop by calling
 //  PlanExplainsStop.
 //  If the Current plan answers "true" then it is asked if the stop should
 //  percolate all the way to the
 //  user by calling the ShouldStop method.  If the current plan doesn't explain
-//  the stop, then we query down
+//  the stop, then we query up
 //  the plan stack for a plan that does explain the stop.  The plan that does
 //  explain the stop then needs to
 //  figure out what to do about the plans below it in the stack.  If the stop is
 //  recoverable, then the plan that
 //  understands it can just do what it needs to set up to restart, and then
 //  continue.
 //  Otherwise, the plan that understood the stop should call DiscardPlanStack to
 //  clean up the stack below it.

 //  all threads will be called, the stop event is placed on the Process's public
 //  broadcaster, and
 //  control returns to the upper layers of the debugger.
 //
 //  Reporting the stop:
 //
 //  When the process stops, the thread is given a StopReason, in the form of a
 //  StopInfo object.  If there is a completed
-//  plan corresponding to the stop, then the "actual" stop reason will be
+//  plan corresponding to the stop, then the "actual" stop reason can be
 //  suppressed, and instead a StopInfoThreadPlan
-//  object will be cons'ed up from the highest completed plan in the stack.
+//  object will be cons'ed up from the top completed plan in the stack.
 //  However, if the plan doesn't want to be
 //  the stop reason, then it can call SetPlanComplete and pass in "false" for
 //  the "success" parameter.  In that case,
 //  the real stop reason will be used instead.  One exapmle of this is the
 //  "StepRangeStepIn" thread plan.  If it stops
 //  because of a crash or breakpoint hit, it wants to unship itself, because it
 //  isn't so useful to have step in keep going
 //  after a breakpoint hit.  But it can't be the reason for the stop or no-one

lldb/source/Target/Thread.cpp

bool Thread::CheckpointThreadState(ThreadStateCheckpoint &saved_state) {
   if (!saved_state.register_backup_sp)
     return false;
 
   saved_state.stop_info_sp = GetStopInfo();
   ProcessSP process_sp(GetProcess());
   if (process_sp)
     saved_state.orig_stop_id = process_sp->GetStopID();
   saved_state.current_inlined_depth = GetCurrentInlinedDepth();
+  saved_state.m_completed_plan_stack = m_completed_plan_stack;
   
   return true;
 }
 
 bool Thread::RestoreRegisterStateFromCheckpoint(
     ThreadStateCheckpoint &saved_state) {
   if (saved_state.register_backup_sp) {
     lldb::StackFrameSP frame_sp(GetStackFrameAtIndex(0));
     if (frame_sp) {

 
 bool Thread::RestoreThreadStateFromCheckpoint(
     ThreadStateCheckpoint &saved_state) {
   if (saved_state.stop_info_sp)
     saved_state.stop_info_sp->MakeStopInfoValid();
   SetStopInfo(saved_state.stop_info_sp);
   GetStackFrameList()->SetCurrentInlinedDepth(
       saved_state.current_inlined_depth);
+  m_completed_plan_stack = saved_state.m_completed_plan_stack;
   return true;
 }
 
 StateType Thread::GetState() const {
   // If any other threads access this we will need a mutex for it
   std::lock_guard<std::recursive_mutex> guard(m_state_mutex);
   return m_state;
 }