[Lldb-commits] [PATCH] Add code to exit the NativeProcessLinux Monitor thread on android
Vince Harron
vharron at google.com
Tue Mar 10 18:10:48 PDT 2015
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/
More information about the lldb-commits
mailing list