Remove handling of eStateStopped from NativeProcessLinux::Resume

[labath]

Summary

NPL::Resume attempted to handle eStateStopped as a resume action. However:

• GDBRemoteCommunicationServerLLGS (the only user of NPL) never sets this action
• it could set this action in response to a vCont:t packet, but LLDB never produces this packet
• gdb-remote protocol documentation says vCont:t packet is used only in non-stop mode, but LLDB does not support non-stop mode
• even if LLDB supported non-stop mode, this implementation of eStateStopped does something different from what the spec says it should (according to spec, it should stop the specified thread, but this seems to want to stop all threads).

Given the facts above, I believe we should remove this unused and untested code, as it probably
doesn't even work and removing it makes the rest of the code noticably simpler.

[ovyalov]

Looks good.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

3096 ↗

(On Diff #25460)

We not always thoroughly examine logs (especially considering their amounts..) - what do you think about leaving these states for now with assert?

[labath]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

3096 ↗

(On Diff #25460)

Sure. Sounds like a job for one of those lldbasserts.

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

#endif
         typedef std::unordered_set<lldb::tid_t> ThreadIDSet;
 
         // This method is requests a stop on all threads which are still running. It sets up a
         // deferred delegate notification, which will fire once threads report as stopped. The
         // triggerring_tid will be set as the current thread (main stop reason).
         void
         StopRunningThreads(lldb::tid_t triggering_tid);
 
-        // Notify the delegate after all non-stopped threads stop. The triggering_tid will be set
-        // as the current thread. The error_function will be fired if either the triggering tid
-        // or any of the wait_for_stop_tids are unknown.  This variant will send stop requests to
-        // all non-stopped threads except skip_stop_request_tid.
-        void
-        StopRunningThreadsWithSkipTID(lldb::tid_t triggering_tid, lldb::tid_t skip_stop_request_tid);
-
-    private:
         struct PendingNotification
         {
-            PendingNotification (lldb::tid_t triggering_tid,
-                                 const ThreadIDSet &wait_for_stop_tids,
-                                 const ThreadIDSet &skip_stop_request_tids):
-                triggering_tid (triggering_tid),
-                wait_for_stop_tids (wait_for_stop_tids),
-                original_wait_for_stop_tids (wait_for_stop_tids),
-                request_stop_on_all_unstopped_threads (false),
-                skip_stop_request_tids (skip_stop_request_tids)
-            {
-            }
-
             PendingNotification (lldb::tid_t triggering_tid):
                 triggering_tid (triggering_tid),
-                wait_for_stop_tids (),
-                original_wait_for_stop_tids (),
-                request_stop_on_all_unstopped_threads (true),
-                skip_stop_request_tids ()
+                wait_for_stop_tids ()
             {
             }
 
             const lldb::tid_t  triggering_tid;
             ThreadIDSet        wait_for_stop_tids;
-            const ThreadIDSet  original_wait_for_stop_tids;
-            const bool         request_stop_on_all_unstopped_threads;
-            ThreadIDSet        skip_stop_request_tids;
         };
         typedef std::unique_ptr<PendingNotification> PendingNotificationUP;
 
         // Fire pending notification if no pending thread stops remain.
         void SignalIfRequirementsSatisfied();
 
-        bool
-        RequestStopOnAllSpecifiedThreads();
-
         void
         RequestStopOnAllRunningThreads();
 
         Error
         ThreadDidStop(lldb::tid_t tid, bool initiated_by_llgs);
 
         // Resume the thread with the given thread id using the request_thread_resume_function
         // called. If error_when_already_running is then then an error is raised if we think this

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

 
 Error
 NativeProcessLinux::Resume (const ResumeActionList &resume_actions)
 {
     Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS | LIBLLDB_LOG_THREAD));
     if (log)
         log->Printf ("NativeProcessLinux::%s called: pid %" PRIu64, __FUNCTION__, GetID ());
 
-    lldb::tid_t deferred_signal_tid = LLDB_INVALID_THREAD_ID;
-    lldb::tid_t deferred_signal_skip_tid = LLDB_INVALID_THREAD_ID;
-    int deferred_signo = 0;
-    NativeThreadProtocolSP deferred_signal_thread_sp;
     bool stepping = false;
     bool software_single_step = !SupportHardwareSingleStepping();
 
     Monitor::ScopedOperationLock monitor_lock(*m_monitor_up);
     Mutex::Locker locker (m_threads_mutex);
 
     if (software_single_step)
     {
for (auto thread_sp : m_threads)
                     },
                     false);
             stepping = true;
             break;
         }
 
         case eStateSuspended:
         case eStateStopped:
-            // if we haven't chosen a deferred signal tid yet, use this one.
-            if (deferred_signal_tid == LLDB_INVALID_THREAD_ID)
-            {
-                deferred_signal_tid = thread_sp->GetID ();
-                deferred_signal_thread_sp = thread_sp;
-                deferred_signo = SIGSTOP;
-            }
-            break;
+            lldbassert(0 && "Unexpected state");
 
         default:
             return Error ("NativeProcessLinux::%s (): unexpected state %s specified for pid %" PRIu64 ", tid %" PRIu64,
                     __FUNCTION__, StateAsCString (action->state), GetID (), thread_sp->GetID ());
         }
     }
 
-    // If we had any thread stopping, then do a deferred notification of the chosen stop thread id and signal
-    // after all other running threads have stopped.
-    // If there is a stepping thread involved we'll be eventually stopped by SIGTRAP trace signal.
-    if (deferred_signal_tid != LLDB_INVALID_THREAD_ID && !stepping)
-        StopRunningThreadsWithSkipTID(deferred_signal_tid, deferred_signal_skip_tid);
-
     return Error();
 }
 
 Error
 NativeProcessLinux::Halt ()
 {
     Error error;
 
if (!is_stopped)
         if (log)
             log->Printf("NativeProcessLinux::%s tid %" PRIu64 " optional resume skipped since it is already running",
                     __FUNCTION__,
                     tid);
         return Error();
     }
 
     // Before we do the resume below, first check if we have a pending
-    // stop notification this is currently or was previously waiting for
+    // stop notification that is currently waiting for
     // this thread to stop.  This is potentially a buggy situation since
     // we're ostensibly waiting for threads to stop before we send out the
     // pending notification, and here we are resuming one before we send
     // out the pending stop notification.
-    if (m_pending_notification_up && log)
-    {
-        if (m_pending_notification_up->wait_for_stop_tids.count (tid) > 0)
-        {
-            log->Printf("NativeProcessLinux::%s about to resume tid %" PRIu64 " per explicit request but we have a pending stop notification (tid %" PRIu64 ") that is actively waiting for this thread to stop. Valid sequence of events?", __FUNCTION__, tid, m_pending_notification_up->triggering_tid);
-        }
-        else if (m_pending_notification_up->original_wait_for_stop_tids.count (tid) > 0)
-        {
-            log->Printf("NativeProcessLinux::%s about to resume tid %" PRIu64 " per explicit request but we have a pending stop notification (tid %" PRIu64 ") that hasn't fired yet and this is one of the threads we had been waiting on (and already marked satisfied for this tid). Valid sequence of events?", __FUNCTION__, tid, m_pending_notification_up->triggering_tid);
-            for (auto tid : m_pending_notification_up->wait_for_stop_tids)
-            {
-                log->Printf("NativeProcessLinux::%s tid %" PRIu64 " deferred stop notification still waiting on tid  %" PRIu64,
-                                 __FUNCTION__,
-                                 m_pending_notification_up->triggering_tid,
-                                 tid);
-            }
-        }
-    }
+    if (m_pending_notification_up && log && m_pending_notification_up->wait_for_stop_tids.count (tid) > 0)
 
     // Request a resume.  We expect this to be synchronous and the system
     // to reflect it is running after this completes.
     const auto error = request_thread_resume_function (tid, false);
     if (error.Success())
         context.request_resume_function = request_thread_resume_function;
     else if (log)
     {
NativeProcessLinux::StopRunningThreads(const lldb::tid_t triggering_tid)
 
     if (log)
     {
         log->Printf("NativeProcessLinux::%s event processing done", __FUNCTION__);
     }
 }
 
 void
-NativeProcessLinux::StopRunningThreadsWithSkipTID(lldb::tid_t triggering_tid,
-                                                  lldb::tid_t skip_stop_request_tid)
-{
-    Log *const log = GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD);
-
-    if (log)
-    {
-        log->Printf("NativeProcessLinux::%s about to process event: (triggering_tid: %" PRIu64 ", skip_stop_request_tid: %" PRIu64 ")",
-                __FUNCTION__, triggering_tid, skip_stop_request_tid);
-    }
-
-    DoStopThreads(PendingNotificationUP(new PendingNotification(
-                triggering_tid,
-                ThreadIDSet(),
-                skip_stop_request_tid != LLDB_INVALID_THREAD_ID ? NativeProcessLinux::ThreadIDSet {skip_stop_request_tid} : ThreadIDSet ())));
-
-    if (log)
-    {
-        log->Printf("NativeProcessLinux::%s event processing done", __FUNCTION__);
-    }
-}
-
-void
 NativeProcessLinux::SignalIfRequirementsSatisfied()
 {
     if (m_pending_notification_up && m_pending_notification_up->wait_for_stop_tids.empty ())
     {
         SetCurrentThreadID(m_pending_notification_up->triggering_tid);
         SetState(StateType::eStateStopped, true);
         m_pending_notification_up.reset();
     }
 }
 
-bool
-NativeProcessLinux::RequestStopOnAllSpecifiedThreads()
-{
-    // Request a stop for all the thread stops that need to be stopped
-    // and are not already known to be stopped.  Keep a list of all the
-    // threads from which we still need to hear a stop reply.
-
-    ThreadIDSet sent_tids;
-    for (auto tid : m_pending_notification_up->wait_for_stop_tids)
-    {
-        // Validate we know about all tids for which we must first receive a stop before
-        // triggering the deferred stop notification.
-        auto thread_sp = std::static_pointer_cast<NativeThreadLinux>(GetThreadByID(tid));
-        lldbassert(thread_sp != nullptr);
-
-        // If the pending stop thread is currently running, we need to send it a stop request.
-        if (StateIsRunningState(thread_sp->GetState()))
-        {
-            thread_sp->RequestStop();
-            sent_tids.insert (tid);
-        }
-    }
-    // We only need to wait for the sent_tids - so swap our wait set
-    // to the sent tids.  The rest are already stopped and we won't
-    // be receiving stop notifications for them.
-    m_pending_notification_up->wait_for_stop_tids.swap (sent_tids);
-
-    // Succeeded, keep running.
-    return true;
-}
-
 void
 NativeProcessLinux::RequestStopOnAllRunningThreads()
 {
     // Request a stop for all the thread stops that need to be stopped
     // and are not already known to be stopped.  Keep a list of all the
     // threads from which we still need to hear a stop reply.
 
     ThreadIDSet sent_tids;
     for (const auto &thread_sp: m_threads)
     {
         // We only care about running threads
         if (StateIsStoppedState(thread_sp->GetState(), true))
             continue;
 
-        const lldb::tid_t tid = thread_sp->GetID();
-
-        // Request this thread stop if the tid stop request is not explicitly ignored.
-        const bool skip_stop_request = m_pending_notification_up->skip_stop_request_tids.count (tid) > 0;
-        if (!skip_stop_request)
-            static_pointer_cast<NativeThreadLinux>(thread_sp)->RequestStop();
-
-        // Even if we skipped sending the stop request for other reasons (like stepping),
-        // we still need to wait for that stepping thread to notify completion/stop.
-        sent_tids.insert (tid);
+        sent_tids.insert (thread_sp->GetID());
     }
 
     // Set the wait list to the set of tids for which we requested stops.
     m_pending_notification_up->wait_for_stop_tids.swap (sent_tids);
 }
 
 
 Error
if (m_pending_notification_up && log)
         // Log this info.  We lose the pending notification here.
         log->Printf("NativeProcessLinux::%s dropping existing pending signal notification for tid %" PRIu64 ", to be replaced with signal for tid %" PRIu64,
                    __FUNCTION__,
                    m_pending_notification_up->triggering_tid,
                    notification_up->triggering_tid);
     }
     m_pending_notification_up = std::move(notification_up);
 
-    if (m_pending_notification_up->request_stop_on_all_unstopped_threads)
-        RequestStopOnAllRunningThreads();
-    else
-    {
-        if (!RequestStopOnAllSpecifiedThreads())
-            return;
-    }
 
     SignalIfRequirementsSatisfied();
 }
 
 void
 NativeProcessLinux::ThreadWasCreated (lldb::tid_t tid)
 {
     Log *const log = GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD);