[NativeProcessLinux] (Partially) fix race condition during inferior thread creation

[labath]

Summary

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.

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, I cannot enable
the test yet, as a different, stack-unwinding, issue still sporadically crops up.

Note that there is still one issue remaining in the situation described above: Once the get the
"new thread notification" for thread C, we will immediately resume it and we will end up with a
thread running anyway. This issue needs to be handled differently (since the thread C was never
sent a SIGSTOP in the first place, and I will address this in a follow-up patch).

[chaoren]

Please see inline comments:

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2289 ↗

(On Diff #24114)

It seems that this and the SI_USER signal could happen in any order, are you sure it's safe to resume the new thread if this signal hasn't been received yet? Is it possible that you've just been lucky and have always been getting this before the SI_USER signal.

2606 ↗

(On Diff #24114)

Please remove this comment. It doesn't make sense anymore.

2610 ↗

(On Diff #24114)

I think if we resumed the thread directly and used m_coordinator_up->NotifyThreadCreate (not stopped), we can bypass this issue, since TSC::ThreadWasCreated has a check for pending thread stop notifications.

2645 ↗

(On Diff #24114)

This function is only used here and the other place you deleted. Should we get rid of it?

2663 ↗

(On Diff #24114)

Let's hope Todd is wrong.

2665 ↗

(On Diff #24114)

We might as well remove the launching state entirely then?

[labath]

Update based on review comments.

[labath]

I've updated the code based on your suggestions. Please take another look.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2289 ↗

(On Diff #24114)

Yes, the two events indeed do come in random order. I have verified this. This comes from the fact that they happen on two threads and the order depends on which order waitpid visits the threads when looking for events. However, I postulate that we do not need to synchronize here to be correct. I have gone through the manual page backwards and forwards and I have found nothing to indicate that this would be necessary. Furthermore, I can't think of a valid reason why the kernel would choose to stop the two threads at a point where they already exist as separate entities, but they are still linked in a way that the valid operations on one of them still depend on the state of the other thread. It would be much simpler to stop them a few (kernel) instructions further where the new thread's state is initialised and decoupled from the parent thread.

2606 ↗

(On Diff #24114)

done.

2610 ↗

(On Diff #24114)

That is an excellent idea. Done as you suggested.

2645 ↗

(On Diff #24114)

Good idea. Removed.

2663 ↗

(On Diff #24114)

I've been running the test suite for a while and I see no ill effects from deleting this.

2665 ↗

(On Diff #24114)

Good point. Done.

[chaoren]

LGTM

source/Plugins/Process/Linux/NativeThreadLinux.cpp

305 ↗

(On Diff #24206)

I wonder if we could get rid of StateType::eStateLaunching entirely. We'd have fewer cases to handle.

test/python_api/lldbutil/process/TestPrintStackTraces.py

24 ↗

(On Diff #24206)

This probably solves a bunch of other flakey failures too (e.g., TestSetWatchlocation, TestTargetWatchAddress).

[vharron]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2264 ↗

(On Diff #24206)

points->point

2281 ↗

(On Diff #24206)

I was thinking about adding a variable somewhere that tracks our current goal. Do we want all of the threads to be running or stopped?

If the goal is running, we resume. If not, we shouldn't resume, right?

I think we can flip this bit at the same time we send the stop request to all threads.

We can flip it back when we get a continue from the host.
(You don't need to address this in this patch, just sharing an idea I had)

[vharron]

test/python_api/lldbutil/process/TestPrintStackTraces.py

21 ↗

(On Diff #24206)

I would leave this as @expectedFailureLinux unless you've run it 100x successfully.

[labath]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2264 ↗

(On Diff #24206)

done.

2281 ↗

(On Diff #24206)

I was considering this as well, but I believe this flag will need to be per thread (and perhaps a default flag for threads that created in the meantime), since we sometimes get partial resume requests from the client (e.g. resume threads A and B but keep everything stopped). Knowledge about this is scattered around TSC and NPL, but i would rather not access it now, since I don't know what the other threads are doing. Once we get rid of the TSC thread, I will reconsider this.

source/Plugins/Process/Linux/NativeThreadLinux.cpp

305 ↗

(On Diff #24206)

What do you mean by that?
eStateLaunching comes from a global lldb enum, which used in a bunch of places, so we cannot just nix this field out. We could possibly introduce our own enum (or just a "bool running" filed), but I'm not sure if that's a good idea.

test/python_api/lldbutil/process/TestPrintStackTraces.py

21 ↗

(On Diff #24206)

I've run it a bunch (around 20-30) of times and it was ok. I'll run it some more and see what happens. I believe the best way to verify it's fixed is to enable it and follow the build bot. It's easy to XFAIL it again if it still turns out to be broken.

24 ↗

(On Diff #24206)

Right. I'll run the tests and enable them if they pass.

[labath]

test/python_api/lldbutil/process/TestPrintStackTraces.py

21 ↗

(On Diff #24206)

On second though, there is still the issue Chaoren pointed out yesterday, which can theoretically break things. I will submit a CL for this and enable the tests then.

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

#endif
         MaybeGetThreadNoLock (lldb::tid_t thread_id);
 
         bool
         StopTrackingThread (lldb::tid_t thread_id);
 
         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);
 
         Error
         FixupBreakpointPCAsNeeded (NativeThreadProtocolSP &thread_sp);
 
         /// Writes a siginfo_t structure corresponding to the given thread ID to the
         /// memory region pointed to by @p siginfo.

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

NativeProcessLinux::MonitorSIGTRAP(const siginfo_t *info, lldb::pid_t pid)
     switch (info->si_code)
     {
     // 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)):
     {
-        lldb::tid_t tid = LLDB_INVALID_THREAD_ID;
-
-        // The main thread is stopped here.
-        if (thread_sp)
-            std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedBySignal (SIGTRAP);
-        NotifyThreadStop (pid);
+        // 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.
 
+        if (log)
+        {
             unsigned long event_message = 0;
             if (GetEventMessage (pid, &event_message).Success())
             {
-            tid = static_cast<lldb::tid_t> (event_message);
-            if (log)
+                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;
     }
 
     case (SIGTRAP | (PTRACE_EVENT_EXEC << 8)):
     {
         NativeThreadProtocolSP main_thread_sp;
         if (log)
             log->Printf ("NativeProcessLinux::%s() received exec event, code = %d", __FUNCTION__, info->si_code ^ SIGTRAP);
NativeProcessLinux::MonitorSignal(const siginfo_t *info, lldb::pid_t pid, bool exited)
     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.
         if (log)
             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.
+        Resume (pid, LLDB_INVALID_SIGNAL_NUMBER);
+        m_coordinator_up->NotifyThreadCreate (pid, false, CoordinatorErrorHandler);
         // Done handling.
         return;
     }
 
     // Check for thread stop notification.
     if (is_from_llgs && (info->si_code == SI_TKILL) && (signo == SIGSTOP))
     {
         // This is a tgkill()-based stop.
if (m_threads.empty ())
         SetCurrentThreadID (thread_id);
 
     NativeThreadProtocolSP thread_sp (new NativeThreadLinux (this, thread_id));
     m_threads.push_back (thread_sp);
 
     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)
 {
     Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_BREAKPOINTS));
 
     Error error;
 
     // Get a linux thread pointer.

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

public:
         Error
         RemoveWatchpoint (lldb::addr_t addr) override;
 
     private:
         // ---------------------------------------------------------------------
         // Interface for friend classes
         // ---------------------------------------------------------------------
         void
-        SetLaunching ();
-
-        void
         SetRunning ();
 
         void
         SetStepping ();
 
         void
         SetStoppedBySignal (uint32_t signo);
 

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

NativeThreadLinux::RemoveWatchpoint (lldb::addr_t addr)
     uint32_t wp_index = wp->second;
     m_watchpoint_index_map.erase(wp);
     if (GetRegisterContext()->ClearHardwareWatchpoint(wp_index))
         return Error ();
     return Error ("Clearing hardware watchpoint failed.");
 }
 
 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;
     MaybeLogStateChange (new_state);
     m_state = new_state;
 
     m_stop_info.reason = StopReason::eStopReasonNone;
     m_stop_description.clear();