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

[Pavel 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?


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

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

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

http://reviews.llvm.org/D8205

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