[NativeProcessLinux] Fix race condition during inferior thread creation

The following situation occured if we were stopping a process (due to breakpoint, watchpoint, ...
hit) while a new thread was being created.
- process has two threads: A and B.
- thread A hits a breakpoint: we send a STOP signal to thread B and register a callback with
  ThreadStateCoordinator to send a stop notification after the thread stops.
- thread B stops, but not due to the SIGSTOP, but on a thread creation event (of a new thread C).
  We are unaware of our desire to stop, so we queue ThreadStopped and RequestResume operations
  with TSC, so the thread can continue running.
- TSC receives the ThreadStopped event, sees that all threads are stopped and fires the delayed
  stop notification.
- immediately after that TSC gets the RequestResume operation, so it resumes the thread.

At this point the state is inconsistent because LLDB thinks the process is stopped and will start
issuing commands to it, but one of the threads is in fact running. Things eventually break.

I address this problem by omitting the two TSC events altogether and Resuming the thread B
directly. This way the short stop is invisible to the TSC and the delayed notification will not
fire. We will fire the notification when we actually process the SIGSTOP on thread B.

When we get the initial SIGSTOP for thread C, we also resume the thread and send a
ThreadWasCreated message (is_stopped = false) to the TSC. This way, the TSC can stop the thread
on its own and handle the stop event later. This way the state of the new thread is correctly
handled as well (thanks Chaoren for the idea).

This patch also removes the synchronisation between the thread creation notifications on threads
B and C. The need for this synchronisation is unclear (the comments seem to hint that the new
thread is "fully created" only after we process both events, but I have noticed no regressions in
treating it as "created" even after just processing the initial C event), but it is a source for
many kinds of obscure races, since it introduces a new thread state "Launching" and the rest of
the code does not handle this state at all (what happens if we get a resume request from LLDB
while this thread is launching? what happens if we get a stop request? etc.).

This fixes the "spurious $O packet" problem in TestPrintStackTraces.py. However, the test remains
disabled on i386 due to the VDSO issue.

Test Plan:
TestPrintStackTraces works on x86_64. No regressions in the rest of the test suite.

Reviewers: vharron, chaoren

Subscribers: lldb-commits

Differential Revision: http://reviews.llvm.org/D9145

llvm-svn: 235579

4 files changed, 16 insertions, 134 deletions

diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
index 205faf0f8f5..0de12da2296 100644
--- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -2260,62 +2260,25 @@ NativeProcessLinux::MonitorSIGTRAP(const siginfo_t *info, lldb::pid_t pid)
 
     case (SIGTRAP | (PTRACE_EVENT_CLONE << 8)):
     {
-        lldb::tid_t tid = LLDB_INVALID_THREAD_ID;
+        // 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.
 
-        // The main thread is stopped here.
-        if (thread_sp)
-            std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedBySignal (SIGTRAP);
-        NotifyThreadStop (pid);
-
-        unsigned long event_message = 0;
-        if (GetEventMessage (pid, &event_message).Success())
+        if (log)
         {
-            tid = static_cast<lldb::tid_t> (event_message);
-            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);
 
-            // If we don't track the thread yet: create it, mark as stopped.
-            // If we do track it, this is the wait we needed.  Now resume the new thread.
-            // In all cases, resume the current (i.e. main process) thread.
-            bool created_now = false;
-            NativeThreadProtocolSP new_thread_sp = GetOrCreateThread (tid, created_now);
-            assert (new_thread_sp.get() && "failed to get or create the tracking data for newly created inferior thread");
-
-            // If the thread was already tracked, it means the created thread already received its SI_USER notification of creation.
-            if (!created_now)
-            {
-                // We can now resume the newly created thread since it is fully created.
-                NotifyThreadCreateStopped (tid);
-                m_coordinator_up->RequestThreadResume (tid,
-                                                       [=](lldb::tid_t tid_to_resume, bool supress_signal)
-                                                       {
-                                                           std::static_pointer_cast<NativeThreadLinux> (new_thread_sp)->SetRunning ();
-                                                           return Resume (tid_to_resume, LLDB_INVALID_SIGNAL_NUMBER);
-                                                       },
-                                                       CoordinatorErrorHandler);
             }
             else
-            {
-                // Mark the thread as currently launching.  Need to wait for SIGTRAP clone on the main thread before
-                // this thread is ready to go.
-                std::static_pointer_cast<NativeThreadLinux> (new_thread_sp)->SetLaunching ();
-            }
-        }
-        else
-        {
-            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);
         }
 
-        // In all cases, we can resume the main thread here.
-        m_coordinator_up->RequestThreadResume (pid,
-                                               [=](lldb::tid_t tid_to_resume, bool supress_signal)
-                                               {
-                                                   std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetRunning ();
-                                                   return Resume (tid_to_resume, LLDB_INVALID_SIGNAL_NUMBER);
-                                               },
-                                               CoordinatorErrorHandler);
-
+        Resume (pid, LLDB_INVALID_SIGNAL_NUMBER);
         break;
     }
 
@@ -2640,31 +2603,12 @@ NativeProcessLinux::MonitorSignal(const siginfo_t *info, lldb::pid_t pid, bool e
             log->Printf ("NativeProcessLinux::%s() pid = %" PRIu64 " tid %" PRIu64 ": new thread notification",
                      __FUNCTION__, GetID (), pid);
 
-        // Did we already create the thread?
-        bool created_now = false;
-        thread_sp = GetOrCreateThread (pid, created_now);
-        assert (thread_sp.get() && "failed to get or create the tracking data for newly created inferior thread");
-
-        // If the thread was already tracked, it means the main thread already received its SIGTRAP for the create.
-        if (!created_now)
-        {
-            // We can now resume the newly created thread since it is fully created.
-            NotifyThreadCreateStopped (pid);
-            m_coordinator_up->RequestThreadResume (pid,
-                                                   [=](lldb::tid_t tid_to_resume, bool supress_signal)
-                                                   {
-                                                       std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetRunning ();
-                                                       return Resume (tid_to_resume, LLDB_INVALID_SIGNAL_NUMBER);
-                                                   },
-                                                   CoordinatorErrorHandler);
-        }
-        else
-        {
-            // Mark the thread as currently launching.  Need to wait for SIGTRAP clone on the main thread before
-            // this thread is ready to go.
-            std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetLaunching ();
-        }
-
+        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 ();
+        Resume (pid, LLDB_INVALID_SIGNAL_NUMBER);
+        m_coordinator_up->NotifyThreadCreate (pid, false, CoordinatorErrorHandler);
         // Done handling.
         return;
     }
@@ -4109,48 +4053,6 @@ NativeProcessLinux::AddThread (lldb::tid_t thread_id)
     return thread_sp;
 }
 
-NativeThreadProtocolSP
-NativeProcessLinux::GetOrCreateThread (lldb::tid_t thread_id, bool &created)
-{
-    Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
-
-    Mutex::Locker locker (m_threads_mutex);
-    if (log)
-    {
-        log->Printf ("NativeProcessLinux::%s pid %" PRIu64 " get/create thread with tid %" PRIu64,
-                     __FUNCTION__,
-                     GetID (),
-                     thread_id);
-    }
-
-    // Retrieve the thread if it is already getting tracked.
-    NativeThreadProtocolSP thread_sp = MaybeGetThreadNoLock (thread_id);
-    if (thread_sp)
-    {
-        if (log)
-            log->Printf ("NativeProcessLinux::%s pid %" PRIu64 " tid %" PRIu64 ": thread already tracked, returning",
-                         __FUNCTION__,
-                         GetID (),
-                         thread_id);
-        created = false;
-        return thread_sp;
-
-    }
-
-    // Create the thread metadata since it isn't being tracked.
-    if (log)
-        log->Printf ("NativeProcessLinux::%s pid %" PRIu64 " tid %" PRIu64 ": thread didn't exist, tracking now",
-                     __FUNCTION__,
-                     GetID (),
-                     thread_id);
-
-    thread_sp.reset (new NativeThreadLinux (this, thread_id));
-    m_threads.push_back (thread_sp);
-    created = true;
-    
-    return thread_sp;
-}
-
 Error
 NativeProcessLinux::FixupBreakpointPCAsNeeded (NativeThreadProtocolSP &thread_sp)
 {
diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.h b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
index c0f68bede0f..259b921d18f 100644
--- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -322,9 +322,6 @@ namespace process_linux {
         NativeThreadProtocolSP
         AddThread (lldb::tid_t thread_id);
 
-        NativeThreadProtocolSP
-        GetOrCreateThread (lldb::tid_t thread_id, bool &created);
-
         Error
         GetSoftwareBreakpointPCOffset (NativeRegisterContextSP context_sp, uint32_t &actual_opcode_size);
 
diff --git a/lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
index ee916e8e948..134e6a0eec6 100644
--- a/lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
@@ -300,20 +300,6 @@ NativeThreadLinux::RemoveWatchpoint (lldb::addr_t addr)
 }
 
 void
-NativeThreadLinux::SetLaunching ()
-{
-    const StateType new_state = StateType::eStateLaunching;
-    MaybeLogStateChange (new_state);
-    m_state = new_state;
-
-    // Also mark it as stopped since launching temporarily stops the newly created thread
-    // in the ptrace machinery.
-    m_stop_info.reason = StopReason::eStopReasonSignal;
-    m_stop_info.details.signal.signo = SIGSTOP;
-}
-
-
-void
 NativeThreadLinux::SetRunning ()
 {
     const StateType new_state = StateType::eStateRunning;
diff --git a/lldb/source/Plugins/Process/Linux/NativeThreadLinux.h b/lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
index 4170fd9556b..c24faf8b811 100644
--- a/lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
+++ b/lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
@@ -54,9 +54,6 @@ namespace process_linux {
         // Interface for friend classes
         // ---------------------------------------------------------------------
         void
-        SetLaunching ();
-
-        void
         SetRunning ();
 
         void