Fix race condition in Target::Launch

[zturner]

Summary

Based on the message at http://lists.cs.uiuc.edu/pipermail/lldb-dev/2015-March/007046.html, it looks like the global event listener is racing with the event thread to pull the process stop event in the case of synchronous execution. This patch addresses this by *always* hijacking process events, and not restoring them until after *both* waits are complete.

With this patch, I can successfully use .lldbinit file to run a process. It still does not fix the issue with the stack trace not displaying as discussed in http://lists.cs.uiuc.edu/pipermail/lldb-dev/2015-March/007033.html, but it seems to be a different but related issue, so I don't attempt to address it here.

[zturner]

Rebase this against ToT. Ilia K committed another patch recently which fixed a race condition for the stop at entry case, but it did nothing for the case of not stopping at entry.

[ki.stfu]

source/Target/Target.cpp

2605–2606 ↗

(On Diff #22524)

Can we use a mixed code (for sync and async execution) to avoid copy-paste?

2627 ↗

(On Diff #22524)

it's really needed?

[zturner]

source/Target/Target.cpp

2605–2606 ↗

(On Diff #22524)

I'm not crazy about the copy/paste either, but I thought this separation made the logic easier to follow. Let's see what Jim or Greg says, I don't feel very strongly, so if there's a consensus one way or the other I'll just go with the consensus.

2627 ↗

(On Diff #22524)

I'm still learning this section of the code, but if we hijack the events, surely we need to restore them?

[ki.stfu]

source/Target/Target.cpp

2605–2606 ↗

(On Diff #22524)

Ok, I don't mind but I'd prefer to keep as is.

2627 ↗

(On Diff #22524)

Right, but in this case we do that on line #2688

2688 ↗

(On Diff #22524)

here

[zturner]

ping, Greg or Jim would you mind taking a look?

source/Target/Target.cpp

2688 ↗

(On Diff #22524)

Thanks, I'll fix this.

[clayborg]

Looks good to me, but Jim also give the OK just to make sure.

[zturner]

Jim, thoughts?

[jingham]

I prefer treating the synchronous & async cases separately. It seems a little easier to read to me, and we're more likely to have to modify one or the other branch, so keeping them separate is slightly preferable from that standpoint.

Is that SyncIOHandler hack needed in the synchronous case?

Jim

[zturner]

In D8562#147401, @jingham wrote:

I prefer treating the synchronous & async cases separately. It seems a little easier to read to me, and we're more likely to have to modify one or the other branch, so keeping them separate is slightly preferable from that standpoint.

Is that SyncIOHandler hack needed in the synchronous case?

Jim

Not sure, you tell me? :) I guess probably not though, so I can remove it (which also makes the argument for keeping the sync and async cases separate a no-brainer) since at that point there's very little duplication left. Is this good to go after removing the SyncIOHandler hack?

[jingham]

Todd originally put that in, but only in the async case. It is just some dopey little delay in the hope of avoiding a race. The synchronous case is going to the trouble of waiting for a real stop to happen and get reported, that's going to take longer than this little delay.

Of course, it would be better to see how to avoid whatever this delay is papering over, but I don't have the time to open that can of worms right now...

Jim

lldb/trunk/source/Target/Target.cpp

Target::Launch (ProcessLaunchInfo &launch_info, Stream *stream)
     }
 
     if (error.Success())
     {
         if (synchronous_execution || launch_info.GetFlags().Test(eLaunchFlagStopAtEntry) == false)
         {
             EventSP event_sp;
             ListenerSP hijack_listener_sp (launch_info.GetHijackListener());
+            if (!hijack_listener_sp)
+            {
+                hijack_listener_sp.reset(new Listener("lldb.Target.Launch.hijack"));
+                launch_info.SetHijackListener(hijack_listener_sp);
+                m_process_sp->HijackProcessEvents(hijack_listener_sp.get());
+            }
 
             StateType state = m_process_sp->WaitForProcessToStop (NULL, &event_sp, false, hijack_listener_sp.get(), NULL);
             
             if (state == eStateStopped)
             {
-                if (launch_info.GetFlags().Test(eLaunchFlagStopAtEntry) == false)
+                if (!launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
+                {
+                    if (synchronous_execution)
                     {
-                    if (!synchronous_execution)
-                        m_process_sp->RestoreProcessEvents ();
-
                         error = m_process_sp->PrivateResume();
-
                         if (error.Success())
                         {
-                        // there is a race condition where this thread will return up the call stack to the main command
-                        // handler and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
-                        // a chance to call PushProcessIOHandler()
-                        m_process_sp->SyncIOHandler(2000);
-
-                        if (synchronous_execution)
-                        {
                             state = m_process_sp->WaitForProcessToStop (NULL, NULL, true, hijack_listener_sp.get(), stream);
                             const bool must_be_alive = false; // eStateExited is ok, so this must be false
                             if (!StateIsStoppedState(state, must_be_alive))
                             {
                                 error.SetErrorStringWithFormat("process isn't stopped: %s", StateAsCString(state));
                             }
                         }
                     }
                     else
                     {
+                        m_process_sp->RestoreProcessEvents();
+                        error = m_process_sp->PrivateResume();
+                        if (error.Success())
+                        {
+                            // there is a race condition where this thread will return up the call stack to the main command
+                            // handler and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
+                            // a chance to call PushProcessIOHandler()
+                            m_process_sp->SyncIOHandler(2000);
+                        }
+                    }
+                    if (!error.Success())
+                    {
                         Error error2;
                         error2.SetErrorStringWithFormat("process resume at entry point failed: %s", error.AsCString());
                         error = error2;
                     }
                 }
                 else
                 {
-                    assert(synchronous_execution && launch_info.GetFlags().Test(eLaunchFlagStopAtEntry) == true);
+                    assert(synchronous_execution && launch_info.GetFlags().Test(eLaunchFlagStopAtEntry));
 
                     // Target was stopped at entry as was intended. Need to notify the listeners about it.
                     m_process_sp->RestoreProcessEvents();
                     m_process_sp->HandlePrivateEvent(event_sp);
 
                     // there is a race condition where this thread will return up the call stack to the main command
                     // handler and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
                     // a chance to call PushProcessIOHandler()