[Lldb-commits] [PATCH] Add code to exit the NativeProcessLinux Monitor thread on android

[Tamas Berghammer]

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.


================
Comment at: source/Host/common/Host.cpp:178
@@ +177,3 @@
+    sigChldAction.sa_handler = SigChldHandler;
+    ::sigaction(SIGCHLD, &sigChldAction, NULL);
+#endif // __ANDROID__    
----------------
vharron wrote:
> ovyalov wrote:
> > labath wrote:
> > > 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.
> > +1 to use SIGUSR1
> +1 to use SIGUSR1
Done

================
Comment at: source/Host/common/Host.cpp:194
@@ -182,3 +193,3 @@
 #if !defined(__ANDROID__) && !defined(__ANDROID_NDK__)
         ::pthread_testcancel ();
 #endif
----------------
vharron wrote:
> Please remove all pthread_cancel code paths.  Linux and Android should follow the same code paths.
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.

================
Comment at: source/Host/common/Host.cpp:198
@@ -186,2 +197,3 @@
         {
             if (errno == EINTR)
+            {
----------------
ovyalov wrote:
> labath wrote:
> > 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.
> 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,..).
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.

================
Comment at: source/Host/common/Host.cpp:201
@@ +200,3 @@
+#ifdef __ANDROID__
+                // On Android we have to call the callback on EINTR also because the monitor thread
+                // is stopped with sending a SIGCHLD and then returning true from the callback
----------------
vharron wrote:
> 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
> 
> 
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.

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:224
@@ +223,3 @@
+        /// Attaches to an existing process.  Forms the
+        /// implementation of Process::DoLaunch.
+        virtual void
----------------
ovyalov wrote:
> s/DoLaunch/DoAttach
Done

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:233
@@ +232,3 @@
+        virtual void
+        StopMonitoringChildProcess();
+
----------------
ovyalov wrote:
> I'd rather named this method  StopMonitorThread in order to make it complement to StartMonitorThread.
Done

http://reviews.llvm.org/D8205

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/