Don't resume when creating new threads if there is a pending thread stop.

[chaoren]

Summary

In the rare event that a thread is created immediately before a breakpoint hit,
the newly created thread and the main thread would be incorrectly resumed
by the creation process.

[chaoren]

Need to prevent the main thread from resuming as well.

[chaoren]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2487

I don't think this gets called at all. Is the duplicate code here necessary?

[chaoren]

Remove xfail caused by this.

[ovyalov]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

2135–2151

It looks to me there might be a race - if you'll be calling this method when TSC inside of ProcessEvent call between RequestStopOnAllRunningThreads() and coordinator.SetPendingNotification() a thread might be mistakenly resumed. So, you may to synchronize IsPendingThreadStop with ProcessEvent().

Can we make RequestThreadResume take additional boolean argument to check for pending stopped state - so, effectively move this conditional logic into EventRequestResume?

source/Plugins/Process/Linux/ThreadStateCoordinator.h

152

This method should be synchronized - GetPendingThreadStopNotification() is used now only within TSC thread and that's why doesn't need synchronization. But if you're going to call it from another thread GetPendingThreadStopNotification should be protected by mutex.

[chaoren]

I think I'm mistaken about the actual cause of the race condition.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

case (SIGTRAP | (PTRACE_EVENT_CLONE << 8)):
             // 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);
+                if (m_coordinator_up->IsPendingThreadStop())
+                {
+                    // If there is a pending stop for some other reason,
+                    // don't resume the thread and mark it as stopped.
+                    std::static_pointer_cast<NativeThreadLinux>(new_thread_sp)->SetStoppedBySignal(0);
+                }
+                else
+                {
+                    // We can now resume the newly created thread since it is fully created.
+                    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);
                 }
+            }
             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);
+        if (m_coordinator_up->IsPendingThreadStop())
+        {
+            // If there is a pending stop for some other reason,
+            // don't resume the thread and mark it as stopped.
+            std::static_pointer_cast<NativeThreadLinux>(thread_sp)->SetStoppedBySignal(0);
+        }
+        else
+        {
+            // If no stop is pending, 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);
+        }
 
         break;
     }
 
     case (SIGTRAP | (PTRACE_EVENT_EXEC << 8)):
     {
         NativeThreadProtocolSP main_thread_sp;
         if (log)
if (info->si_code == SI_TKILL || info->si_code == SI_USER)
                             signo,
                             (info->si_code == SI_TKILL ? "SI_TKILL" : "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.
         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);
+            if (m_coordinator_up->IsPendingThreadStop())
+            {
+                // If there is a pending stop for some other reason,
+                // don't resume the thread and mark it as stopped.
+                std::static_pointer_cast<NativeThreadLinux>(thread_sp)->SetStoppedBySignal(0);
+            }
+            else
+            {
+                // We can now resume the newly created thread since it is fully created.
+                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);
             }
+        }
         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 ();
         }
 
         // Done handling.

source/Plugins/Process/Linux/ThreadStateCoordinator.h

public:
         // logic assumes the execution of this is implicitly serialized.
         EventLoopResult
         ProcessNextEvent ();
 
         // Enable/disable verbose logging of event processing.
         void
         LogEnableEventProcessing (bool enabled);
 
+        bool
+        IsPendingThreadStop() {
+            return GetPendingThreadStopNotification();
+        }
     private:
 
         // Typedefs.
         class EventBase;
 
         class EventCallAfterThreadsStop;
         class EventThreadStopped;
         class EventThreadCreate;

test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py

class SetWatchlocationAPITestCase(TestBase):
     @dsym_test
     def test_watch_location_with_dsym(self):
         """Exercise SBValue.WatchPointee() API to set a watchpoint."""
         self.buildDsym()
         self.do_set_watchlocation()
 
     @python_api_test
     @dwarf_test
-    @expectedFailureLinux # fails in 10/100 dosep runs
     def test_watch_location_with_dwarf(self):
         """Exercise SBValue.WatchPointee() API to set a watchpoint."""
         self.buildDwarf()
         self.do_set_watchlocation()
 
     def do_set_watchlocation(self):
         """Use SBValue.WatchPointee() to set a watchpoint and verify that the program stops later due to the watchpoint."""
         exe = os.path.join(os.getcwd(), "a.out")

test/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py

class TargetWatchAddressAPITestCase(TestBase):
     @dsym_test
     def test_watch_address_with_dsym(self):
         """Exercise SBTarget.WatchAddress() API to set a watchpoint."""
         self.buildDsym()
         self.do_set_watchaddress()
 
     @python_api_test
     @dwarf_test
-    @expectedFailureLinux # failed 11/100 dosep runs
     def test_watch_address_with_dwarf(self):
         """Exercise SBTarget.WatchAddress() API to set a watchpoint."""
         self.buildDwarf()
         self.do_set_watchaddress()
 
     @skipUnlessDarwin
     @python_api_test
     @dsym_test