[NativeProcessLinux] Add back synchronisation of thread create events

[labath]

Summary

Without the synchronisation between the two thread creation events the following case could
happen:

• threads A and B are running. A hits a breakpoint. We note that we want to stop B.
• before we could stop it, B creates a new thread C, we get the stop notification for B, but we don't record C's existence yet.
• we resume B
• before we get the C notification, B stops again (e.g. hits a breakpoint, gets our SIGSTOP, etc.)
• we see all known threads have stopped, and we notify LLDB
• C notification comes, we note it's existence and resume it

> we have an inconsistent state (LLDB thinks we've stopped, but C is running)

I resolve this by doing a blocking wait for for the C notification when we get the creation
notification on the parent (B) thread. This way the two events are synchronised, but we don't
need to introduce the intermediate "launching" state which would complicate handling of thread
states as all code would need to be aware of the third possible state.

[chaoren]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2315 ↗

(On Diff #24289)

I think we should log it anyway.

[labath]

Add more logging.

[labath]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2315 ↗

(On Diff #24289)

Done. Please take another look.

[chaoren]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2274 ↗

(On Diff #24361)

This should probably be at the end. I don't think GetEventMessage(pid, ...) would work if the thread is running. The thread could have also exited by the time you call waitpid(pid, ...).

2301 ↗

(On Diff #24361)

waitpid(tid, ...)?

[labath]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2274 ↗

(On Diff #24361)

Woops, that's embarrasing.
Are you OK if I put it right after the GetEventMessage call? (The waitpid should be waiting on the *tid*, of course, so this should not interfere with it).

2301 ↗

(On Diff #24361)

Yes, thanks for catching that.

[chaoren]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2274 ↗

(On Diff #24361)

That's fine too. I'm just worried that it's easy to forget that there is a thread running, so it's easy to introduce bugs in the future if someone makes some changes to read something from the parent thread. There would need to be two resumes (or a goto, or an ugly giant if), but I think it's worth it.

2301 ↗

(On Diff #24361)

Probably doesn't make much of a difference, but wouldn't __WCLONE be more correct than __WALL here?

2322 ↗

(On Diff #24361)

GetSignalInfo(tid, ...)?

[labath]

Fix silly copy-paste errors and clean things up.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2274 ↗

(On Diff #24361)

I have put the code in a function and placed the resume at the end.

2301 ↗

(On Diff #24361)

I would rather keep WALL. When attaching, we also use WALL even though we are only waiting on a specific thread.

2322 ↗

(On Diff #24361)

done.

[chaoren]

LGTM after this one change:

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2352 ↗

(On Diff #24462)

Please only do this if GetEventMessage succeeded.

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h

private:
 
         static void *
         MonitorThread(void *baton);
 
         void
         MonitorCallback(lldb::pid_t pid, bool exited, int signal, int status);
 
         void
+        WaitForNewThread(::pid_t tid);
+
+        void
         MonitorSIGTRAP(const siginfo_t *info, lldb::pid_t pid);
 
         void
         MonitorTrace(lldb::pid_t pid, NativeThreadProtocolSP thread_sp);
 
         void
         MonitorBreakpoint(lldb::pid_t pid, NativeThreadProtocolSP thread_sp);
 

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

else
                 if (log)
                     log->Printf ("NativeProcessLinux::%s pid %" PRIu64 " tid %" PRIu64 " non-main thread exit occurred, didn't tell delegate anything since thread disappeared out from underneath us", __FUNCTION__, GetID (), pid);
             }
         }
     }
 }
 
 void
+NativeProcessLinux::WaitForNewThread(::pid_t tid)
+{
+    Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
+
+    NativeThreadProtocolSP new_thread_sp = GetThreadByID(tid);
+
+    if (new_thread_sp)
+    {
+        // We are already tracking the thread - we got the event on the new thread (see
+        // MonitorSignal) before this one. We are done.
+        return;
+    }
+
+    // The thread is not tracked yet, let's wait for it to appear.
+    int status = -1;
+    ::pid_t wait_pid;
+    do
+    {
+        if (log)
+            log->Printf ("NativeProcessLinux::%s() received thread creation event for tid %" PRIu32 ". tid not tracked yet, waiting for thread to appear...", __FUNCTION__, tid);
+        wait_pid = waitpid(tid, &status, __WALL);
+    }
+    while (wait_pid == -1 && errno == EINTR);
+    // Since we are waiting on a specific tid, this must be the creation event. But let's do
+    // some checks just in case.
+    if (wait_pid != tid) {
+        if (log)
+            log->Printf ("NativeProcessLinux::%s() waiting for tid %" PRIu32 " failed. Assuming the thread has disappeared in the meantime", __FUNCTION__, tid);
+        // The only way I know of this could happen is if the whole process was
+        // SIGKILLed in the mean time. In any case, we can't do anything about that now.
+        return;
+    }
+    if (WIFEXITED(status))
+    {
+        if (log)
+            log->Printf ("NativeProcessLinux::%s() waiting for tid %" PRIu32 " returned an 'exited' event. Not tracking the thread.", __FUNCTION__, tid);
+        // Also a very improbable event.
+        return;
+    }
+
+    siginfo_t info;
+    Error error = GetSignalInfo(tid, &info);
+    if (error.Fail())
+    {
+        if (log)
+            log->Printf ("NativeProcessLinux::%s() GetSignalInfo for tid %" PRIu32 " failed. Assuming the thread has disappeared in the meantime.", __FUNCTION__, tid);
+        return;
+    }
+
+    if (((info.si_pid != 0) || (info.si_code != SI_USER)) && log)
+    {
+        // We should be getting a thread creation signal here, but we received something
+        // else. There isn't much we can do about it now, so we will just log that. Since the
+        // thread is alive and we are receiving events from it, we shall pretend that it was
+        // created properly.
+        log->Printf ("NativeProcessLinux::%s() GetSignalInfo for tid %" PRIu32 " received unexpected signal with code %d from pid %d.", __FUNCTION__, tid, info.si_code, info.si_pid);
+    }
+
+    if (log)
+        log->Printf ("NativeProcessLinux::%s() pid = %" PRIu64 ": tracking new thread tid %" PRIu32,
+                 __FUNCTION__, GetID (), tid);
+
+    new_thread_sp = AddThread(tid);
+    std::static_pointer_cast<NativeThreadLinux> (new_thread_sp)->SetRunning ();
+    Resume (tid, LLDB_INVALID_SIGNAL_NUMBER);
+    m_coordinator_up->NotifyThreadCreate (tid, false, CoordinatorErrorHandler);
+}
+
+void
 NativeProcessLinux::MonitorSIGTRAP(const siginfo_t *info, lldb::pid_t pid)
 {
     Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
     const bool is_main_thread = (pid == GetID ());
 
     assert(info && info->si_signo == SIGTRAP && "Unexpected child signal!");
     if (!info)
         return;
NativeProcessLinux::MonitorSIGTRAP(const siginfo_t *info, lldb::pid_t pid)
     {
     // TODO: these two cases are required if we want to support tracing of the inferiors' children.  We'd need this to debug a monitor.
     // case (SIGTRAP | (PTRACE_EVENT_FORK << 8)):
     // case (SIGTRAP | (PTRACE_EVENT_VFORK << 8)):
 
     case (SIGTRAP | (PTRACE_EVENT_CLONE << 8)):
     {
         // This is the notification on the parent thread which informs us of new thread
-        // creation. We are not interested in these events at this point (an interesting use
-        // case would be to stop the process upon thread creation), so we just resume the thread.
-        // We will pickup the new thread when we get its SIGSTOP notification.
+        // creation.
+        // We don't want to do anything with the parent thread so we just resume it. In case we
+        // want to implement "break on thread creation" functionality, we would need to stop
+        // here.
 
-        if (log)
-        {
-            unsigned long event_message = 0;
-            if (GetEventMessage (pid, &event_message).Success())
-            {
-                lldb::tid_t tid = static_cast<lldb::tid_t> (event_message);
-                log->Printf ("NativeProcessLinux::%s() pid %" PRIu64 " received thread creation event for tid %" PRIu64, __FUNCTION__, pid, tid);
-
-            }
-            else
+        if (GetEventMessage (pid, &event_message).Fail())
+            if (log)
                 log->Printf ("NativeProcessLinux::%s() pid %" PRIu64 " received thread creation event but GetEventMessage failed so we don't know the new tid", __FUNCTION__, pid);
-        }
+        } else 
+            WaitForNewThread(event_message);
 
         Resume (pid, LLDB_INVALID_SIGNAL_NUMBER);
         break;
     }
 
     case (SIGTRAP | (PTRACE_EVENT_EXEC << 8)):
     {
         NativeThreadProtocolSP main_thread_sp;
if (info->si_code == SI_TKILL || info->si_code == SI_USER)
                             info->si_pid,
                             is_from_llgs ? "from llgs" : "not from llgs",
                             pid);
     }
 
     // Check for new thread notification.
     if ((info->si_pid == 0) && (info->si_code == SI_USER))
     {
-        // A new thread creation is being signaled.  This is one of two parts that come in
-        // a non-deterministic order.  pid is the thread id.
+        // A new thread creation is being signaled. This is one of two parts that come in
+        // a non-deterministic order. This code handles the case where the new thread event comes
+        // before the event on the parent thread. For the opposite case see code in
+        // MonitorSIGTRAP.
         if (log)
             log->Printf ("NativeProcessLinux::%s() pid = %" PRIu64 " tid %" PRIu64 ": new thread notification",
                      __FUNCTION__, GetID (), pid);
 
         thread_sp = AddThread(pid);
         assert (thread_sp.get() && "failed to create the tracking data for newly created inferior thread");
         // We can now resume the newly created thread.
         std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetRunning ();