[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

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

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

2610

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

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

2663

Let's hope Todd is wrong.

2665

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

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

done.

2610

That is an excellent idea. Done as you suggested.

2645

Good idea. Removed.

2663

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

2665

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

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

[vharron]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2264

points->point

2281

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

23

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

[labath]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2264

done.

2281

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

23

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

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

[labath]

test/python_api/lldbutil/process/TestPrintStackTraces.py

23

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.

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 points (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);
+        thread_sp = AddThread(pid);
         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 ();
-        }
+        // We can now resume the newly created thread.
+        // TODO: There is a bug here that if we are in the process of stopping the process while
+        // this thread is being created, this new thread will continue to execute.
 
         // Done handling.
         return;
     }
 
     // Check for thread stop notification.
     if (is_from_llgs && (info->si_code == SI_TKILL) && (signo == SIGSTOP))
     {

test/python_api/lldbutil/process/TestPrintStackTraces.py

class ThreadsStackTracesTestCase(TestBase):
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to break inside main().
         self.line = line_number('main.cpp', '// Set break point at this line.')
 
     # fails on Linux i386 llvm.org/pr23043
-    # fails 78/100 on Linux x86_64 when running against remote lldb-platform
     # failed 11/200 on the linux build bot
     @expectedFailureLinux
     @python_api_test
     def test_stack_traces(self):
         """Test SBprocess and SBThread APIs with printing of the stack traces."""
         self.buildDefault()
         self.break_and_print_stacktraces()
 
     def break_and_print_stacktraces(self):
         """Break at main.cpp:68 and do a threads dump"""