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

[Vince Harron]

Thanks for the patch!  Looking forward to v2!


================
Comment at: source/Host/common/Host.cpp:146
@@ +145,3 @@
+#ifdef __ANDROID__
+static void SigChldHandler(int) {}
+#endif // __ANDROID__
----------------
Rename to "emptySignalHandler"?

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


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

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

================
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
----------------
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



================
Comment at: source/Plugins/Process/Android/NativeProcessAndroid.cpp:95
@@ +94,3 @@
+
+	    return !process->m_monitor_thread_running;
+	}
----------------
ovyalov wrote:
> 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.
I don't think that his current usage is a problem but it might cause noise in ThreadSanitizer and an atomic can't hurt.

http://reviews.llvm.org/D8205

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