LLDB: fix for TestCallThatThrows.py test fail

[boris.ulasevich]

Summary

Additional change for D26497, D28945 - fix for TestCallThatThrows test fail (works for Mac only).

Recent change of stop reason priorities affected function call mechanism: it is important that CallFunctionThreadPlan complete event StopInfo should not be overridden by other simultaneous events (internal breakpoint?), otherwise caller do not get proper return value.

This change is a kind of fast fix for the test broken by my yesterday's change. I should admit, I do not like the code around, but any further change leads to tests fails with tedious LIBLLDB_LOG_STEP logs investigation.

[jingham]

Can you explain in more detail the scenario this is addressing? You seem to be handling a case where some plan has been completed, but it is not the function call plan we are waiting for. What was the completed thread plan which WAS done but wasn't the function call thread plan? Calling nested functions is tricky, and I'd like to understand what is going on when this goes wrong.

BTW, if you only have a Mac available to you, you may be able to reproduce the Linux behavior locally by going into ProcessGDBRemote::DoAllocateMemory and turning off the code that checks SupportsAllocDeallocMemory, forcing us to use the InferiorMmap call instead. That might make development easier for you.

[boris.ulasevich]

I work on the case when we have two plans complete: internal breakpoint plus our function call plan:

Active plan stack:
  Element 0: Base thread plan.
Completed Plan Stack:
  Element 0: Run to address: 0x000000010e78be00 using breakpoint: 0 - but the breakpoint has been deleted.
  Element 1: Thread plan to call 0x10e7c4bf0

[jingham]

Something else is going on then. The run to break point plan is just the subsidiary plan that implements the call function plan. The way the function call plan works is that finds some code which we know isn't going to get executed (_start or something like it), writes a stack frame that returns there, pushes the call frame for the function under that, pushes a run to _start thread plan and continues. Then when the function call is done, we hit the breakpoint we've set at _start. Before we report that event (and get back to the code in RunThreadPlan) the thread plan machinery should decide that the run to address plan is successfully completed, it will get popped, then the call function plan will resume control and notice it also is done. Since the call function plan is a "master plan" it will get popped but then the completion will get reported directly rather than continuing to unwind the plan stack. When you go to pick the plan to report for the stop it should be the top of the completed plan stack, the "thread plan to call function". Why in this particular case is the bottom of the completed stack getting handed out? That's what seems wrong.

[boris.ulasevich]

In D29144#657798, @jingham wrote:

When you go to pick the plan to report for the stop it should be the top of the completed plan stack, the "thread plan to call function". Why in this particular case is the bottom of the completed stack getting handed out?

"thread plan to call function" do stay on the top of the completed plan stack, but GetStopInfo returns another value because ProcessGDBRemote::SetThreadStopInfo have put StopInfoBreakpoint to Thread::m_stop_info_sp, and preset value have priority over completed plan.

[boris.ulasevich]

I made another diff with using GetCompletedPlan call. Hope it makes the code clear.

[jingham]

If things were working generally the way your explanation states, then I don't understand why this doesn't cause all expression evaluations to fail. We have lots of tests that do expression evaluation. Why is only this test failing. Your change looks reasonable to me, but I'm a bit worried that we don't understand why the code as it was isn't failing all the time. What's special about the TestCallThatThrows test?

[boris.ulasevich]

In D29144#659052, @jingham wrote:

What's special about the TestCallThatThrows test?

Good question! It must be related with SetIgnoreBreakpoints(True) option used in the test: with this option StopInfoBreakpoint::PerformAction is not performed and preset StopInfoBreakpoint value is not cleared :)

[boris.ulasevich]

ping!

[jingham]

If the problem is related to the handling of "don't stop at breakpoints" expressions, can we fix it closer to there?

[boris.ulasevich]

Yes, it is quite old story. Original issue was that breakpoint with false condition resumes execution in spite of completed step plan. There was two local fix proposals, but finally the issue was fixed by StopInfoBreakpoint::PerformAction and Thread::GetStopInfo reworking.

That change had side effect on Ubuntu platform, and now we have another side effect here: function call processing does not expect that in case of both (1) unprocessed breakpoint hit and (2) plan complete event GetStopInfo() can return breakpoint's StopInfo. For me the fix is correct: do not ask GetStopInfo, but simply check plan for complete.

[jingham]

Okay, I buy that.

[labath]

The test added by this batch is failing on windows http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/2499, for a very prosaic reason - we cannot run a "make clean" as something is holding the executable file open. Based on my debugging, it is not a problem in the test itself, but rather in the liblldb code (e.g. when I change all the breakpoint conditions to true, and adjust expectations accordingly, make clean suceeds). My feeling is something is creating a shared_ptr loop which prevents things from going away when the test is over.

I am going to debug this further, but I writing this here, in case you have any ideas where I should start looking.

lldb/source/Target/Process.cpp

#endif
                   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 {
-                  StopInfoSP stop_info_sp(thread_sp->GetStopInfo());
-                  StopReason stop_reason = eStopReasonInvalid;
-                  if (stop_info_sp)
-                    stop_reason = stop_info_sp->GetStopReason();
-
-                  // FIXME: We only check if the stop reason is plan complete,
-                  // should we make sure that
-                  // it is OUR plan that is complete?
-                  if (stop_reason == eStopReasonPlanComplete) {
+                  ThreadPlanSP plan_sp = thread->GetCompletedPlan();
+                  if (plan_sp == thread_plan_sp && plan_sp->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_reason == eStopReasonBreakpoint) {
+                    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