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

[Tamas Berghammer]

================
Comment at: source/Host/common/Host.cpp:146
@@ +145,3 @@
+#ifdef __linux__
+// Change it to a std::set with a mutex if multiple thread can be cancelled at the same time
+static pthread_t g_usr1_signal_thread = 0;
----------------
labath wrote:
> labath wrote:
> > Please remove the std::set reference, as it is not a valid solution. There are very few things that can be done safely in a signal handler and std::set manipulation is not one of them. If we ever need to cancel multiple threads, we will need to do something more clever, probably set a flag on NativeProcessAndroid (outside of the signal handler), as @vharron suggseted.
> It just occurred to me that this would be a good use case for a thread_local variable. I'm not sure what is the official policy on thread local variables, as I can't find any occurrences in the codebase, but if that's ok then the killer solution would be
>   static thread_local volatile sig_atomic_t g_usr1_called;
> If not then you can at least make your existing variable volatile for some added safety.
Done

================
Comment at: source/Host/common/Host.cpp:194
@@ -176,2 +193,3 @@
         // Wait for all child processes
-#if !defined(__ANDROID__) && !defined(__ANDROID_NDK__)
+#ifdef __linux__
+        if (g_usr1_signal_thread == ::pthread_self())
----------------
ovyalov wrote:
> I'd rather put this snippet into a separate function since it's called twice:
> 
> 
> ```
> bool CheckForCancellation()
> {
> #ifdef __linux__
>         if (g_usr1_signal_thread == ::pthread_self())
>         {
>             g_usr1_signal_thread = 0;
>             return true;
>         }
>         return false;
> #else
>         ::pthread_testcancel ();
> #endif
> }
> ```
> 
> 
Done

================
Comment at: source/Host/common/Host.cpp:262
@@ -232,3 +261,3 @@
 
                 log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS);
                 if (log)
----------------
ovyalov wrote:
> Unrelated but looks weird - could you remove this log re-initialization? It looks unnecessary. 
It is necessary to let the user enable the logging after starting the monitor thread.

http://reviews.llvm.org/D8205

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