Fix race condition during process detach

[labath]

Summary

The following situation occured in TestAttachResume:

The inferior was stoped at a breakpoint and we did a continue, immediately followed by a detach.
Since there was a trap instruction under the IP, the continue did a step-over-breakpoint before
resuming the inferior for real. In some cases, the detach command was executed between these two
events (after the step-over stop, but before continue). Here, public state was running, but
private state was stopped. This caused a problem because HaltForDestroyOrDetach was checking the
public state to see whether it needs to stop the process (call Halt()), but Halt() was checking
the private state and concluded that there is nothing for it to do.

Solution: Instead of Halt() call SendAsyncInterrupt(), which will then cause Halt() to be
executed in the context of the private state thread. I also rename HaltForDestroyOrDetach to
reflect it does not call halt directly.

[labath]

remove some binaries that managed to sneak in

[KLapshin]

See http://reviews.llvm.org/D12968 also - fix for missed hijacked listener set in Process::HaltForDestroyOrDetach()

[ovyalov]

It looks like this CL is introducing additional latency when destroying process - I noticed 10 seconds delay when destroying Android process and got logs for code with this CL and without:

1443231085.125153000 Process::StopForDestroyOrDetach() About to stop.
1443231085.125225000 Process::WaitForProcessToStop (timeout = 0x7fa9fb5fab28)
1443231085.125243000 Process::WaitForStateChangedEvents (timeout = 0x7fa9fb5fab28, event_sp)...
1443231085.125284000 Process::RunPrivateStateThread (arg = 0x7fa9ec38d470, pid = 20579) woke up with an interrupt - Halting.
1443231085.137527000 Process::SetPrivateState (stopped)
1443231085.137569000 Process::SetPrivateState (stopped) stop_id = 13
1443231085.137646000 Process::WaitForEventsPrivate (timeout = (nil), event_sp)...
1443231085.139090000 Process::ShouldBroadcastEvent (0x7fa9e4003a10) stopped due to an interrupt, state: stopped
1443231085.139159000 Process::ShouldBroadcastEvent (0x7fa9e4003a10) => new state: stopped, last broadcast state: stopped - YES
1443231085.139166000 Process::HandlePrivateEvent (pid = 20579) broadcasting new state stopped (old state running) to public
1443231085.139184000 Process::WaitForEventsPrivate (timeout = (nil), event_sp)...
1443231085.139267000 Process::SetPublicState (state = stopped, restarted = 0)
1443231085.139336000 Process::SetPublicState (stopped) -- unlocking run lock
1443231095.125328000 Process::WaitForStateChangedEvents (timeout = 0x7fa9fb5fab28, event_sp) => invalid
1443231095.125507000 Process::StopForDestroyOrDetach() failed to stop, state is: invalid

1443231549.351362000 Process::HaltForDestroyOrDetach() About to halt.
1443231549.364460000 Process::SetPrivateState (stopped)
1443231549.364475000 Process::SetPrivateState (stopped) stop_id = 13
1443231549.364499000 Process::WaitForProcessToStop (timeout = 0x7f4f34ff6b18)
1443231549.364508000 Process::WaitForStateChangedEvents (timeout = 0x7f4f34ff6b18, event_sp)...
1443231549.364973000 Process::ShouldBroadcastEvent (0x7f4f1c002150) stopped due to an interrupt, state: stopped
1443231549.365040000 Process::ShouldBroadcastEvent (0x7f4f1c002150) => new state: stopped, last broadcast state: stopped - YES
1443231549.365046000 Process::HandlePrivateEvent (pid = 20768) broadcasting new state stopped (old state running) to public
1443231549.365062000 Process::WaitForEventsPrivate (timeout = (nil), event_sp)...
1443231549.365095000 Process::SetPublicState (state = stopped, restarted = 0)
1443231549.365117000 Process::SetPublicState (stopped) -- unlocking run lock
1443231550.364602000 Process::WaitForStateChangedEvents (timeout = 0x7f4f34ff6b18, event_sp) => invalid
1443231550.364682000 Process::HaltForDestroyOrDetach() Halt failed to stop, state is: invalid

So, I believe there is still a problem with stopped state delivery within WaitForProcessToStop which should be fixed in long-term. As quick workaround we can either revert this CL or reduce timeout to avoid 10 second delay upon process termination.

[labath]

I have reverted this for now. This fix is not complete without D12968.

lldb/trunk/include/lldb/Target/Process.h

protected:
 
     bool
     ProcessIOHandlerExists () const
     {
         return static_cast<bool>(m_process_input_reader);
     }
     
     Error
-    HaltForDestroyOrDetach(lldb::EventSP &exit_event_sp);
+    StopForDestroyOrDetach(lldb::EventSP &exit_event_sp);
 
     bool
     StateChangedIsExternallyHijacked();
 
     void
     LoadOperatingSystemPlugin(bool flush);
 private:
 

lldb/trunk/source/Target/Process.cpp

Process::Halt (bool clear_thread_plans)
     // correctly setting the state.
     if (event_sp)
         m_private_state_broadcaster.BroadcastEvent(event_sp);
 
     return error;
 }
 
 Error
-Process::HaltForDestroyOrDetach(lldb::EventSP &exit_event_sp)
+Process::StopForDestroyOrDetach(lldb::EventSP &exit_event_sp)
 {
     Error error;
     if (m_public_state.GetValue() == eStateRunning)
     {
         Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
         if (log)
-            log->Printf("Process::%s() About to halt.", __FUNCTION__);
-        error = Halt();
-        if (error.Success())
-        {
-            // Consume the halt event.
-            TimeValue timeout (TimeValue::Now());
-            timeout.OffsetWithSeconds(1);
-            StateType state = WaitForProcessToStop (&timeout, &exit_event_sp);
-            
-            // If the process exited while we were waiting for it to stop, put the exited event into
-            // the shared pointer passed in and return.  Our caller doesn't need to do anything else, since
-            // they don't have a process anymore...
-            
-            if (state == eStateExited || m_private_state.GetValue() == eStateExited)
-            {
-                if (log)
-                    log->Printf("Process::HaltForDestroyOrDetach() Process exited while waiting to Halt.");
-                return error;
-            }
-            else
-                exit_event_sp.reset(); // It is ok to consume any non-exit stop events
-    
-            if (state != eStateStopped)
-            {
-                if (log)
-                    log->Printf("Process::HaltForDestroyOrDetach() Halt failed to stop, state is: %s", StateAsCString(state));
-                // If we really couldn't stop the process then we should just error out here, but if the
-                // lower levels just bobbled sending the event and we really are stopped, then continue on.
-                StateType private_state = m_private_state.GetValue();
-                if (private_state != eStateStopped)
-                {
-                    return error;
-                }
-            }
-        }
-        else
-        {
-            if (log)
-                log->Printf("Process::HaltForDestroyOrDetach() Halt got error: %s", error.AsCString());
-        }
-    }
+            log->Printf("Process::%s() About to stop.", __FUNCTION__);
+
+        SendAsyncInterrupt();
+
+        // Consume the interrupt event.
+        timeout.OffsetWithSeconds(10);
+                log->Printf("Process::%s() Process exited while waiting to stop.", __FUNCTION__);
+                log->Printf("Process::%s() failed to stop, state is: %s", __FUNCTION__, StateAsCString(state));
     return error;
 }
 
 Error
 Process::Detach (bool keep_stopped)
 {
     EventSP exit_event_sp;
     Error error;
     m_destroy_in_process = true;
     
     error = WillDetach();
 
     if (error.Success())
     {
         if (DetachRequiresHalt())
         {
-            error = HaltForDestroyOrDetach (exit_event_sp);
+            error = StopForDestroyOrDetach (exit_event_sp);
             if (!error.Success())
             {
                 m_destroy_in_process = false;
                 return error;
             }
             else if (exit_event_sp)
             {
                 // We shouldn't need to do anything else here.  There's no process left to detach from...
Process::Destroy (bool force_kill)
     m_destroy_in_process = true;
 
     Error error (WillDestroy());
     if (error.Success())
     {
         EventSP exit_event_sp;
         if (DestroyRequiresHalt())
         {
-            error = HaltForDestroyOrDetach(exit_event_sp);
+            error = StopForDestroyOrDetach(exit_event_sp);
         }
         
         if (m_public_state.GetValue() != eStateRunning)
         {
             // Ditch all thread plans, and remove all our breakpoints: in case we have to restart the target to
             // kill it, we don't want it hitting a breakpoint...
             // Only do this if we've stopped, however, since if we didn't manage to halt it above, then
             // we're not going to have much luck doing this now.
#ifndef _WIN32
                     {
                         switch (ch)
                         {
                             case 'q':
                                 SetIsDone(true);
                                 break;
                             case 'i':
                                 if (StateIsRunningState(m_process->GetState()))
-                                    m_process->Halt();
+                                    m_process->SendAsyncInterrupt();
                                 break;
                         }
                     }
                 }
             }
         }
 #endif
         terminal_state.Restore();
     }
     
     void
     Cancel () override
     {
         char ch = 'q';  // Send 'q' for quit
         size_t bytes_written = 0;
         m_pipe.Write(&ch, 1, bytes_written);
     }
 
     bool
     Interrupt () override
     {
         // Do only things that are safe to do in an interrupt context (like in
         // a SIGINT handler), like write 1 byte to a file descriptor. This will
         // interrupt the IOHandlerProcessSTDIO::Run() and we can look at the byte
-        // that was written to the pipe and then call m_process->Halt() from a
-        // much safer location in code.
+        // that was written to the pipe and then call m_process->SendAsyncInterrupt()
+        // from a much safer location in code.
         if (m_active)
         {
             char ch = 'i'; // Send 'i' for interrupt
             size_t bytes_written = 0;
             Error result = m_pipe.Write(&ch, 1, bytes_written);
             return result.Success();
         }
         else

lldb/trunk/test/functionalities/attach_resume/TestAttachResume.py

 
 exe_name = "AttachResume"  # Must match Makefile
 
 class AttachResumeTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     @expectedFailureFreeBSD('llvm.org/pr19310')
-    @expectedFlakeyLinux('llvm.org/pr19310')
     @expectedFailureWindows("llvm.org/pr24778")
     @skipIfRemote
     @dwarf_test
     def test_attach_continue_interrupt_detach(self):
         """Test attach/continue/interrupt/detach"""
         self.buildDwarf()
         self.process_attach_continue_interrupt_detach()