Add code to exit the NativeProcessLinux Monitor thread on android

[tberghammer]

Summary

Add code to exit the NativeProcessLinux Monitor thread on android

This CL change the logic used to terminate the monitor thread of NativeProcessLinux to use a signal instead of pthread_cancel as pthread_cancel is not supported on android.

[labath]

Please see my inline comments on the signal handler.

A thought for general discussion: I am wondering whether we could avoid having two methods for shutting down a thread. If the code in question is not likely to be used on windows (which I think it's not as it is just a glorified waitpid() wrapper and waitpid is very posix-y), then all other platforms should support the pthread_kill method and we could use a single approach everywhere. Thoughts?

source/Host/common/Host.cpp

176 ↗

(On Diff #21583)

Be bold and specify the signal behavior that you expect, don't rely on them being what you expect by default.
If the previous signal handler has SA_RESTART set, you would achieve nothing, as the waitpid call would get restarted. If the previous handler had SA_RESETHAND set, your trick would only work once.

178 ↗

(On Diff #21583)

There is nothing special about SIGCHLD - any signal can interrupt a system call. Please use a signal which is not overloaded with other meanings, e.g. SIGUSR1.

198 ↗

(On Diff #21583)

Be careful about race conditions here. What if your signal is not received in the waitpid call, but slightly before? The signal will be handled, but you will still end up blocked forever.
I don't think there is a completely race-free solution to this problem (apart from resending the signal until it works), but we should at least try to decrease the race window by testing the exit condition before and after the call.

[ovyalov]

Please see my comments.

source/Host/common/Host.cpp

178 ↗

(On Diff #21583)

+1 to use SIGUSR1

198 ↗

(On Diff #21583)

We may use sigtimedwait instead of waitpid on Android with some conservative timeout (to avoid a possibility to be blocked forever in case of thread race) and if the function timeouts -(callback (callback_baton, -1,..).

source/Plugins/Process/Android/NativeProcessAndroid.cpp

11 ↗

(On Diff #21583)

Please check whether you do need this include.

64 ↗

(On Diff #21583)

Could you make it unconditional - m_monitor_thread_running = error.Success () ?

72 ↗

(On Diff #21583)

ditto.

95 ↗

(On Diff #21583)

There is a chance that m_monitor_thread_running will updated and read concurrently from different threads - please consider an option to use atomic type here.

source/Plugins/Process/Linux/NativeProcessLinux.h

224 ↗

(On Diff #21583)

s/DoLaunch/DoAttach

233 ↗

(On Diff #21583)

I'd rather named this method StopMonitorThread in order to make it complement to StartMonitorThread.

[vharron]

Thanks for the patch! Looking forward to v2!

source/Host/common/Host.cpp

146 ↗

(On Diff #21583)

Rename to "emptySignalHandler"?

Comment that the signal is only to interrupt the thread from waitpid

178 ↗

(On Diff #21583)

+1 to use SIGUSR1

194 ↗

(On Diff #21583)

Please remove all pthread_cancel code paths. Linux and Android should follow the same code paths.

201 ↗

(On Diff #21583)

Why are we calling a function? It seems like we should just check m_monitor_thread_running? If you want to log something, do it after the check, before the break. Reduces complexity

source/Plugins/Process/Android/NativeProcessAndroid.cpp

95 ↗

(On Diff #21583)

I don't think that his current usage is a problem but it might cause noise in ThreadSanitizer and an atomic can't hurt.

[tberghammer]

Thanks for the comments. Based on them I created a new patch which use the same (signal based) logic for Linux and Android. I still left the pthread_cancel logic in place in MonitorChildProcessThreadFunction because it is used in several other case also and I don't have the environment set up to update and test the effect of the change in those cases.

source/Host/common/Host.cpp

178 ↗

(On Diff #21583)

Done

194 ↗

(On Diff #21583)

Changed Linux to use the same logic used by Android, but other architectures (e.g.: FreeBSD) and Local debugging also use this function, so I still have to leave the pthread_cancel there.

198 ↗

(On Diff #21583)

We can't specify the pid of the child we waiting for in sigtimedwait what is required in this case. Especially as sometimes we use group pid instead of a process pid what is handled by waitpid, but handling it manually with sigtimedwait would be tricky.

201 ↗

(On Diff #21583)

We don't have explicit access to the instance of NativeProcessAndroid (we are in a static function in an other module). I can cast callback_baton into NativeProcessAndroid and then check for the flag, but I think that one is a bit nasty solution.

source/Plugins/Process/Linux/NativeProcessLinux.h

224 ↗

(On Diff #21583)

Done

233 ↗

(On Diff #21583)

Done

[ovyalov]

source/Host/common/Host.cpp

194 ↗

(On Diff #21701)

I'd rather put this snippet into a separate function since it's called twice:

bool CheckForCancellation()
{
#ifdef __linux__
        if (g_usr1_signal_thread == ::pthread_self())
        {
            g_usr1_signal_thread = 0;
            return true;
        }
        return false;
#else
        ::pthread_testcancel ();
#endif
}

262 ↗

(On Diff #21701)

Unrelated but looks weird - could you remove this log re-initialization? It looks unnecessary.

[labath]

I have an issue with one of the comments, but otherwise looks fine.

source/Host/common/Host.cpp

146 ↗

(On Diff #21701)

Please remove the std::set reference, as it is not a valid solution. There are very few things that can be done safely in a signal handler and std::set manipulation is not one of them. If we ever need to cancel multiple threads, we will need to do something more clever, probably set a flag on NativeProcessAndroid (outside of the signal handler), as @vharron suggseted.

[labath]

source/Host/common/Host.cpp

146 ↗

(On Diff #21701)

It just occurred to me that this would be a good use case for a thread_local variable. I'm not sure what is the official policy on thread local variables, as I can't find any occurrences in the codebase, but if that's ok then the killer solution would be

static thread_local volatile sig_atomic_t g_usr1_called;

If not then you can at least make your existing variable volatile for some added safety.

[tberghammer]

source/Host/common/Host.cpp

146 ↗

(On Diff #21701)

Done

194 ↗

(On Diff #21701)

Done

262 ↗

(On Diff #21701)

It is necessary to let the user enable the logging after starting the monitor thread.

[labath]

lgtm

[ovyalov]

LGTM

lldb/trunk/source/Host/common/Host.cpp

Host::StartMonitoringChildProcess(Host::MonitorChildProcessCallback callback, void *callback_baton, lldb::pid_t pid, bool monitor_signals)
     info_ptr->callback_baton = callback_baton;
     info_ptr->monitor_signals = monitor_signals;
     
     char thread_name[256];
     ::snprintf(thread_name, sizeof(thread_name), "<lldb.host.wait4(pid=%" PRIu64 ")>", pid);
     return ThreadLauncher::LaunchThread(thread_name, MonitorChildProcessThreadFunction, info_ptr, NULL);
 }
 
-#if !defined(__ANDROID__) && !defined(__ANDROID_NDK__)
+#ifndef __linux__
 //------------------------------------------------------------------
 // Scoped class that will disable thread canceling when it is
 // constructed, and exception safely restore the previous value it
 // when it goes out of scope.
 //------------------------------------------------------------------
 class ScopedPThreadCancelDisabler
 {
 public:
~ScopedPThreadCancelDisabler()
         // Restore the ability for this thread to be cancelled to what it
         // previously was.
         if (m_old_state != -1)
             ::pthread_setcancelstate (m_old_state, 0);
     }
 private:
     int m_old_state;    // Save the old cancelability state.
 };
-#endif // __ANDROID_NDK__
+#endif // __linux__
+
+#ifdef __linux__
+static thread_local volatile sig_atomic_t g_usr1_called;
+
+static void
+SigUsr1Handler (int)
+{
+    g_usr1_called = 1;
+}
+#endif // __linux__
+
+static bool
+CheckForMonitorCancellation()
+{
+#ifdef __linux__
+    if (g_usr1_called)
+    {
+        g_usr1_called = 0;
+        return true;
+    }
+#else
+    ::pthread_testcancel ();
+#endif
+    return false;
+}
 
 static thread_result_t
 MonitorChildProcessThreadFunction (void *arg)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
     const char *function = __FUNCTION__;
     if (log)
         log->Printf ("%s (arg = %p) thread starting...", function, arg);
MonitorChildProcessThreadFunction (void *arg)
     delete info;
 
     int status = -1;
 #if defined (__FreeBSD__) || defined (__FreeBSD_kernel__)
     #define __WALL 0
 #endif
     const int options = __WALL;
 
+#ifdef __linux__
+    // This signal is only used to interrupt the thread from waitpid
+    struct sigaction sigUsr1Action;
+    memset(&sigUsr1Action, 0, sizeof(sigUsr1Action));
+    sigUsr1Action.sa_handler = SigUsr1Handler;
+    ::sigaction(SIGUSR1, &sigUsr1Action, nullptr);
+#endif // __linux__    
+
     while (1)
     {
         log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS);
         if (log)
             log->Printf("%s ::waitpid (pid = %" PRIi32 ", &status, options = %i)...", function, pid, options);
 
-        // Wait for all child processes
-#if !defined(__ANDROID__) && !defined(__ANDROID_NDK__)
-        ::pthread_testcancel ();
-#endif
+        if (CheckForMonitorCancellation ())
+            break;
+
         // Get signals from all children with same process group of pid
         const ::pid_t wait_pid = ::waitpid (pid, &status, options);
-#if !defined(__ANDROID__) && !defined(__ANDROID_NDK__)
-        ::pthread_testcancel ();
-#endif
+
+        if (CheckForMonitorCancellation ())
+            break;
+
         if (wait_pid == -1)
         {
             if (errno == EINTR)
                 continue;
             else
             {
                 if (log)
                     log->Printf ("%s (arg = %p) thread exiting because waitpid failed (%s)...", __FUNCTION__, arg, strerror(errno));
while (1)
             }
             else
             {
                 status_cstr = "(\?\?\?)";
             }
 
             // Scope for pthread_cancel_disabler
             {
-#if !defined(__ANDROID__) && !defined(__ANDROID_NDK__)
+#ifndef __linux__
                 ScopedPThreadCancelDisabler pthread_cancel_disabler;
 #endif
 
                 log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS);
                 if (log)
                     log->Printf ("%s ::waitpid (pid = %" PRIi32 ", &status, options = %i) => pid = %" PRIi32 ", status = 0x%8.8x (%s), signal = %i, exit_state = %i",
                                  function,
                                  pid,

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

private:
             const std::string &stdin_path,
             const std::string &stdout_path,
             const std::string &stderr_path,
             const char *working_dir,
             const lldb_private::ProcessLaunchInfo &launch_info,
             Error &error);
 
         /// Attaches to an existing process.  Forms the
-        /// implementation of Process::DoLaunch.
+        /// implementation of Process::DoAttach
         void
         AttachToInferior (lldb::pid_t pid, Error &error);
 
         void
         StartLaunchOpThread(LaunchArgs *args, lldb_private::Error &error);
 
         static void *
         LaunchOpThread(void *arg);
#if 0
         GetCrashReasonForSIGBUS(const siginfo_t *info);
 #endif
 
         void
         DoOperation(void *op);
 
         /// Stops the child monitor thread.
         void
-        StopMonitoringChildProcess();
+        StopMonitorThread();
 
         /// Stops the operation thread used to attach/launch a process.
         void
         StopOpThread();
 
         Error
         StartCoordinatorThread ();
 

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

 // Other libraries and framework includes
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/RegisterValue.h"
 #include "lldb/Core/Scalar.h"
 #include "lldb/Core/State.h"
+#include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Host/HostNativeThread.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Symbol/ObjectFile.h"
-#include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/ProcessLaunchInfo.h"
 #include "lldb/Utility/PseudoTerminal.h"
 
 #include "lldb/Host/common/NativeBreakpoint.h"
 #include "Utility/StringExtractor.h"
 
 #include "Plugins/Process/Utility/LinuxSignals.h"
NativeProcessLinux::DupDescriptor(const char *path, int fd, int flags)
 
     if (dup2(target_fd, fd) == -1)
         return false;
 
     return (close(target_fd) == -1) ? false : true;
 }
 
 void
-NativeProcessLinux::StopMonitoringChildProcess()
+NativeProcessLinux::StopMonitorThread()
 {
     if (m_monitor_thread.IsJoinable())
     {
-        m_monitor_thread.Cancel();
+        ::pthread_kill(m_monitor_thread.GetNativeThread().GetSystemHandle(), SIGUSR1);
         m_monitor_thread.Join(nullptr);
     }
 }
 
 void
 NativeProcessLinux::StopMonitor()
 {
-    StopMonitoringChildProcess();
+    StopMonitorThread();
     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

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

 
 // C++ Includes
 // Other libraries and framework includes
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Core/RegisterValue.h"
 #include "lldb/Core/Scalar.h"
 #include "lldb/Host/Host.h"
+#include "lldb/Host/HostNativeThread.h"
 #include "lldb/Host/HostThread.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/PseudoTerminal.h"
 
 #include "Plugins/Process/POSIX/CrashReason.h"

 
 // Try to define a macro to encapsulate the tgkill syscall
 // fall back on kill() if tgkill isn't available
 #define tgkill(pid, tid, sig) \
     syscall(SYS_tgkill, static_cast<::pid_t>(pid), static_cast<::pid_t>(tid), sig)
 
 using namespace lldb_private;
 
+static Operation* EXIT_OPERATION = nullptr;
+
 // FIXME: this code is host-dependent with respect to types and
 // endianness and needs to be fixed.  For example, lldb::addr_t is
 // hard-coded to uint64_t, but on a 32-bit Linux host, ptrace requires
 // 32-bit pointer arguments.  This code uses casts to work around the
 // problem.
 
 // We disable the tracing of ptrace calls for integration builds to
 // avoid the additional indirection and checks.
ProcessMonitor::DupDescriptor(const char *path, int fd, int flags)
     return (close(target_fd) == -1) ? false : true;
 }
 
 void
 ProcessMonitor::StopMonitoringChildProcess()
 {
     if (m_monitor_thread.IsJoinable())
     {
-        m_monitor_thread.Cancel();
+        ::pthread_kill(m_monitor_thread.GetNativeThread().GetSystemHandle(), SIGUSR1);
         m_monitor_thread.Join(nullptr);
     }
 }
 
 void
 ProcessMonitor::StopMonitor()
 {
     StopMonitoringChildProcess();
     StopOpThread();
     sem_destroy(&m_operation_pending);
     sem_destroy(&m_operation_done);
     if (m_terminal_fd >= 0) {
         close(m_terminal_fd);
         m_terminal_fd = -1;
     }
 }
 
 void
 ProcessMonitor::StopOpThread()
 {
     if (!m_operation_thread.IsJoinable())
         return;
 
-    m_operation_thread.Cancel();
+    DoOperation(EXIT_OPERATION);
     m_operation_thread.Join(nullptr);
 }