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

[Oleksiy Vyalov]

Please see my comments.


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

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

================
Comment at: source/Plugins/Process/Android/NativeProcessAndroid.cpp:11
@@ +10,3 @@
+#include "NativeProcessAndroid.h"
+#include "NativeProcessLinux.h"
+#include "ThreadStateCoordinator.h"
----------------
Please check whether you do need this include.

================
Comment at: source/Plugins/Process/Android/NativeProcessAndroid.cpp:64
@@ +63,3 @@
+    if (error.Success ())
+        m_monitor_thread_running = true;
+}
----------------
Could you make it unconditional -  m_monitor_thread_running = error.Success () ?

================
Comment at: source/Plugins/Process/Android/NativeProcessAndroid.cpp:72
@@ +71,3 @@
+    if (error.Success ())
+        m_monitor_thread_running = true;
+}
----------------
ditto.

================
Comment at: source/Plugins/Process/Android/NativeProcessAndroid.cpp:95
@@ +94,3 @@
+
+	    return !process->m_monitor_thread_running;
+	}
----------------
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.

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

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

http://reviews.llvm.org/D8205

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