Fix deadlock in operation thread in NativeProcessLinux

[tberghammer]

Summary

Fix deadlock in operation thread in NativeProcessLinux

The deadlock occurred when the Attach or the Launch operation failed for any reason.

[labath]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

3591 ↗

(On Diff #21094)

Enumerating the states when the thread could have exited on its own seems quite fragile. I would propose a more principled treatment. For example we could make DoOperation not wait on the acknowledgement semaphore in case of an exit operation (it is supposed to be the last operation, and the fact that it was processed is indicated by the fact that Join() returns).

PS: instead of accompanying every use of nullptr with a comment "nullptr is a code for the exit operation", I would recommend declaring a constant "static void * const EXIT_OPERATION = nullptr" (or something along those lines) and using it.

[tberghammer]

Update based on suggestion in comment

source/Plugins/Process/Linux/NativeProcessLinux.cpp

3591 ↗

(On Diff #21094)

I don't like my solution either, but haven't managed to find a better way. Thank you for your suggestion, I updated the CL based on it.

[labath]

Looks much better to me now. I'll leave the final signoff to ovyalov.

[ovyalov]

Looks good - please see my comments.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

3400 ↗

(On Diff #21098)

You may have early return if m_operation == EXIT_OPERATION

3563 ↗

(On Diff #21098)

Monitoring thread uses Cancel as well - should we redesign it for Android?

[tberghammer]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

3563 ↗

(On Diff #21098)

In long run possibly yes, but it already have a lot of logic to stop it in different cases and I haven't seen a case when the cancel is actually have effect yet. I have a design to replace it on android but it is a bit nasty so I hold it back until it isn't cause any issue with the hope that we will find a better solution.

[ovyalov]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

3563 ↗

(On Diff #21098)

Monitoring thread cancellation definitely affects Detach request on Android - NativeProcessLinux::Detach after issuing PTRACE_DETACH calls StopMonitor and eventually becomes deadlocked on m_monitor_thread.Join(nullptr);
We should take care of this case since lldb on host side hangs in such situation - it's another issue why host lldb hangs forever in this case because detach seems to be timeout-based operation:

(lldb) exit
Quitting LLDB will detach from one or more processes. Do you really want to proceed: [Y/n] Y
Process::SetExitStatus (status=6 (0x00000006), description="failed to send the k packet")
Process::ControlPrivateStateThread (signal = 1)
Sending control event of type: 1.
Timed out responding to the control event, cancel got error: "(null)".

[tberghammer]

Created Cl to stop the monitor thread on android as D8205

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

 #endif
 
 // Private bits we only need internally.
 namespace
 {
     using namespace lldb;
     using namespace lldb_private;
 
+    static void * const EXIT_OPERATION = nullptr;
+
     const UnixSignals&
     GetUnixSignals ()
     {
         static process_linux::LinuxSignals signals;
         return signals;
     }
 
     ThreadStateCoordinator::LogFunction
for(;;)
         // wait for next pending operation
         if (sem_wait(&monitor->m_operation_pending))
         {
             if (errno == EINTR)
                 continue;
             assert(false && "Unexpected errno from sem_wait");
         }
 
-        // nullptr as operation means the operation thread should exit. Cancel() can't be used
-        // because it is not supported on android.
-        if (!monitor->m_operation)
-        {
-            // notify calling thread that operation is complete
-            sem_post(&monitor->m_operation_done);
+        // EXIT_OPERATION used to stop the operation thread because Cancel() isn't supported on
+        // android. We don't have to send a post to the m_operation_done semaphore because in this
+        // case the synchronization is achieved by a Join() call
+        if (monitor->m_operation == EXIT_OPERATION)
             break;
-        }
 
         reinterpret_cast<Operation*>(monitor->m_operation)->Execute(monitor);
 
         // notify calling thread that operation is complete
         sem_post(&monitor->m_operation_done);
     }
 }
 
 void
 NativeProcessLinux::DoOperation(void *op)
 {
     Mutex::Locker lock(m_operation_mutex);
 
     m_operation = op;
 
     // notify operation thread that an operation is ready to be processed
     sem_post(&m_operation_pending);
 
+    // Don't wait for the operation to complete in case of an exit operation. The operation thread
+    // will exit without posting to the semaphore
+    if (m_operation == EXIT_OPERATION)
+        return;
+
     // wait for operation to complete
     while (sem_wait(&m_operation_done))
     {
         if (errno == EINTR)
             continue;
         assert(false && "Unexpected errno from sem_wait");
     }
 }
if (m_monitor_thread.IsJoinable())
         m_monitor_thread.Cancel();
         m_monitor_thread.Join(nullptr);
     }
 }
 
 void
 NativeProcessLinux::StopMonitor()
 {
-    StopOpThread();
     StopMonitoringChildProcess();
     StopCoordinatorThread ();
+    StopOpThread();
     sem_destroy(&m_operation_pending);
     sem_destroy(&m_operation_done);
 
     // TODO: validate whether this still holds, fix up comment.
     // Note: ProcessPOSIX passes the m_terminal_fd file descriptor to
     // Process::SetSTDIOFileDescriptor, which in turn transfers ownership of
     // the descriptor to a ConnectionFileDescriptor object.  Consequently
     // even though still has the file descriptor, we shouldn't close it here.
 }
 
 void
 NativeProcessLinux::StopOpThread()
 {
     if (!m_operation_thread.IsJoinable())
         return;
 
-    DoOperation(nullptr); // nullptr as operation ask the operation thread to exit
+    DoOperation(EXIT_OPERATION);
     m_operation_thread.Join(nullptr);
 }
 
 Error
 NativeProcessLinux::StartCoordinatorThread ()
 {
     Error error;
     static const char *g_thread_name = "lldb.process.linux.ts_coordinator";