[NativeProcessLinux] Remove the stop callback

[labath]

Summary

The stop callback is a remnant of the ThreadStateCoordinator. We don't need it now that TSC is
gone, as we know exactly which function to call when threads stop. This also removes some
stop-related functions, which were just forwarding calls to one another.

[ovyalov]

Please see my comments.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

4540

Please add a new ThreadContext constructor that takes ThreadState parameter.

4547

If RequestThreadStop fails is it necessary to call m_pending_notification_up->wait_for_stop_tids.insert(tid); ?

4548

Do we need to stop main thread of lldb-server here or RequestThreadStop(tid,.. ) ?

[labath]

source/Plugins/Process/Linux/NativeProcessLinux.cpp

4540

Done.

4547

I would leave this as-is, for now at least, as now I am trying to do cleanups that don't change functionality.

But let's think what are the possible reasons the tgkill may fail:
a) The thread has exited in the meantime.
b) The process has exited.
c) The process has done execve().
d) ???

If everything works as it should, a)-c) should not actually happen, since the thread should stop with PTRACE_EVENT_EXIT before this happens. At this point the thread still exists, and we can send signals to it. If we miss this event for whatever reason, and the thread is really dead, then it could happen that tgkill fails. However, we should still get WIFEXITED event, at which point we will delete the thread and fire the notification (and it wont matter whether we set the flag or not). If we don't get that event also then we will probably hang forever waiting for the thread the stop, but if we end up here, then we are probably in a deep mess anyway.

What do you think?

4548

Good catch. It's tid of course.

[labath]

Address review comments

[ovyalov]

Looks good.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

4547

I was thinking about possible EINTR error that tgkill may raise but it seems "man tgkill" doesn't mention such error as a possible outcome.
Let's leave this flow as-is - since there's logging within RequestThreadStop we'll be able to catch errors if something goes wrong.

source/Plugins/Process/Linux/NativeProcessLinux.h

#endif
         NotifyThreadCreateStopped (lldb::tid_t tid);
 
         void
         NotifyThreadCreateRunning (lldb::tid_t tid);
 
         void
         NotifyThreadDeath (lldb::tid_t tid);
 
-        void
-        StopRunningThreads (lldb::tid_t triggering_tid);
-
-        void
-        StopRunningThreadsWithSkipTID (lldb::tid_t deferred_signal_tid,
-                                                lldb::tid_t skip_stop_request_tid);
-
         Error
         Detach(lldb::tid_t tid);
 
-        Error
-        RequestThreadStop (const lldb::pid_t pid, const lldb::tid_t tid);
-
 
     public:
         // Typedefs.
         typedef std::unordered_set<lldb::tid_t> ThreadIDSet;
 
         // Callback/block definitions.
-        typedef std::function<Error (lldb::tid_t tid)> StopThreadFunction;
         typedef std::function<Error (lldb::tid_t tid, bool supress_signal)> ResumeThreadFunction;
 
     private:
         // Notify the coordinator when a thread is created and/or starting to be
         // tracked.  is_stopped should be true if the thread is currently stopped;
         // otherwise, it should be set false if it is already running.  Will
         // call the error function if the thread id is already tracked.
         void
         NotifyThreadCreate(lldb::tid_t tid, bool is_stopped);
 
 
         // Notify the delegate after a given set of threads stops. 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.
         void
-        StopThreads(lldb::tid_t triggering_tid,
-                              const ThreadIDSet &wait_for_stop_tids,
-                              const StopThreadFunction &request_thread_stop_function);
+        StopThreads(lldb::tid_t triggering_tid, const ThreadIDSet &wait_for_stop_tids);
 
         // 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 the triggering tid
         // is unknown.
         void
-        StopRunningThreads(lldb::tid_t triggering_tid,
-                                     const StopThreadFunction &request_thread_stop_function);
+        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 for any contained in skip_stop_request_tids.
+        // all non-stopped threads except skip_stop_request_tid.
         void
-        StopRunningThreadsWithSkipTID(lldb::tid_t triggering_tid,
-                                                 const ThreadIDSet &skip_stop_request_tids,
-                                                 const StopThreadFunction &request_thread_stop_function);
+        StopRunningThreadsWithSkipTID(lldb::tid_t triggering_tid, lldb::tid_t skip_stop_request_tid);
 
         // Notify the thread stopped.  Will trigger error at time of execution if we
         // already think it is stopped.
         Error
         NotifyThreadStop(lldb::tid_t tid, bool initiated_by_llgs);
 
         // Request that the given thread id should have the request_thread_resume_function
         // called. This call signals an error if the thread resume is for
private:
             bool m_stop_requested = false;
             ResumeThreadFunction m_request_resume_function;
         };
         typedef std::unordered_map<lldb::tid_t, ThreadContext> TIDContextMap;
 
         struct PendingNotification
         {
             PendingNotification (lldb::tid_t triggering_tid,
-                                       const ThreadIDSet &wait_for_stop_tids,
-                                       const StopThreadFunction &request_thread_stop_function):
+                                 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_thread_stop_function (request_thread_stop_function),
                 request_stop_on_all_unstopped_threads (false),
-            skip_stop_request_tids ()
+                skip_stop_request_tids (skip_stop_request_tids)
             {
             }
 
-            PendingNotification (lldb::tid_t triggering_tid,
-                                       const StopThreadFunction &request_thread_stop_function):
+            PendingNotification (lldb::tid_t triggering_tid):
                 triggering_tid (triggering_tid),
                 wait_for_stop_tids (),
                 original_wait_for_stop_tids (),
-            request_thread_stop_function (request_thread_stop_function),
                 request_stop_on_all_unstopped_threads (true),
                 skip_stop_request_tids ()
             {
             }
 
-            PendingNotification (lldb::tid_t triggering_tid,
-                                       const StopThreadFunction &request_thread_stop_function,
-                                       const ThreadIDSet &skip_stop_request_tids):
-            triggering_tid (triggering_tid),
-            wait_for_stop_tids (),
-            original_wait_for_stop_tids (),
-            request_thread_stop_function (request_thread_stop_function),
-            request_stop_on_all_unstopped_threads (true),
-            skip_stop_request_tids (skip_stop_request_tids)
-            {
-            }
-
             const lldb::tid_t  triggering_tid;
             ThreadIDSet        wait_for_stop_tids;
             const ThreadIDSet  original_wait_for_stop_tids;
-            StopThreadFunction request_thread_stop_function;
             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();
 
-        void
+        Error
         RequestThreadStop (lldb::tid_t tid, ThreadContext& context);
 
         std::mutex m_event_mutex; // Serializes execution of ProcessEvent. XXX
 
         Error
         ThreadDidStop(lldb::tid_t tid, bool initiated_by_llgs);
 
         Error

source/Plugins/Process/Linux/NativeProcessLinux.cpp

 
 void
 NativeProcessLinux::NotifyThreadCreateStopped (lldb::tid_t tid)
 {
     const bool is_stopped = true;
     NotifyThreadCreate (tid, is_stopped);
 }
 
-void
-NativeProcessLinux::StopRunningThreads(lldb::tid_t trigerring_tid)
-{
-    Log *const log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
-    if (log)
-        log->Printf("NativeProcessLinux::%s tid %" PRIu64, __FUNCTION__, trigerring_tid);
-
-    const lldb::pid_t pid = GetID ();
-    StopRunningThreads(trigerring_tid,
-            [=](lldb::tid_t request_stop_tid) { return RequestThreadStop(pid, request_stop_tid); });
-}
-
-void
-NativeProcessLinux::StopRunningThreadsWithSkipTID(lldb::tid_t deferred_signal_tid,
-                                                  lldb::tid_t skip_stop_request_tid)
-{
-    Log *const log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
-    if (log)
-        log->Printf("NativeProcessLinux::%s deferred_signal_tid %" PRIu64 ", skip_stop_request_tid %" PRIu64, __FUNCTION__, deferred_signal_tid, skip_stop_request_tid);
-
-    const lldb::pid_t pid = GetID ();
-    StopRunningThreadsWithSkipTID(deferred_signal_tid,
-            skip_stop_request_tid != LLDB_INVALID_THREAD_ID ? NativeProcessLinux::ThreadIDSet {skip_stop_request_tid} : NativeProcessLinux::ThreadIDSet (),
-            [=](lldb::tid_t request_stop_tid) { return RequestThreadStop(pid, request_stop_tid); });
-}
-
-Error
-NativeProcessLinux::RequestThreadStop (const lldb::pid_t pid, const lldb::tid_t tid)
-{
-    Log* log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
-    if (log)
-        log->Printf ("NativeProcessLinux::%s requesting thread stop(pid: %" PRIu64 ", tid: %" PRIu64 ")", __FUNCTION__, pid, tid);
-
-    Error err;
-    errno = 0;
-    if (::tgkill (pid, tid, SIGSTOP) != 0)
-    {
-        err.SetErrorToErrno ();
-        if (log)
-            log->Printf ("NativeProcessLinux::%s tgkill(%" PRIu64 ", %" PRIu64 ", SIGSTOP) failed: %s", __FUNCTION__, pid, tid, err.AsCString ());
-    }
-
-    return err;
-}
-
 Error
 NativeProcessLinux::GetLoadedModuleFileSpec(const char* module_path, FileSpec& file_spec)
 {
     char maps_file_name[32];
     snprintf(maps_file_name, sizeof(maps_file_name), "/proc/%" PRIu64 "/maps", GetID());
 
     FileSpec maps_file_spec(maps_file_name, false);
     if (!maps_file_spec.Exists()) {
NativeProcessLinux::DoResume(
 
     return error;
 }
 
 //===----------------------------------------------------------------------===//
 
 void
 NativeProcessLinux::StopThreads(const lldb::tid_t triggering_tid,
-                                              const ThreadIDSet &wait_for_stop_tids,
-                                              const StopThreadFunction &request_thread_stop_function)
+                                          const ThreadIDSet &wait_for_stop_tids)
 {
     Log *const log = GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD);
     std::lock_guard<std::mutex> lock(m_event_mutex);
 
     if (log)
     {
         log->Printf("NativeProcessLinux::%s about to process event: (triggering_tid: %" PRIu64 ", wait_for_stop_tids.size(): %zd)",
                 __FUNCTION__, triggering_tid, wait_for_stop_tids.size());
     }
 
     DoStopThreads(PendingNotificationUP(new PendingNotification(
-                triggering_tid, wait_for_stop_tids, request_thread_stop_function)));
+                triggering_tid, wait_for_stop_tids, ThreadIDSet())));
 
     if (log)
     {
         log->Printf("NativeProcessLinux::%s event processing done", __FUNCTION__);
     }
 }
 
 void
-NativeProcessLinux::StopRunningThreads(const lldb::tid_t triggering_tid,
-                                                     const StopThreadFunction &request_thread_stop_function)
+NativeProcessLinux::StopRunningThreads(const lldb::tid_t triggering_tid)
 {
     Log *const log = GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD);
     std::lock_guard<std::mutex> lock(m_event_mutex);
 
     if (log)
     {
         log->Printf("NativeProcessLinux::%s about to process event: (triggering_tid: %" PRIu64 ")",
                 __FUNCTION__, triggering_tid);
     }
 
-    DoStopThreads(PendingNotificationUP(new PendingNotification(
-                triggering_tid,
-                request_thread_stop_function)));
+    DoStopThreads(PendingNotificationUP(new PendingNotification(triggering_tid)));
 
     if (log)
     {
         log->Printf("NativeProcessLinux::%s event processing done", __FUNCTION__);
     }
 }
 
 void
 NativeProcessLinux::StopRunningThreadsWithSkipTID(lldb::tid_t triggering_tid,
-                                                                 const ThreadIDSet &skip_stop_request_tids,
-                                                                 const StopThreadFunction &request_thread_stop_function)
+                                                  lldb::tid_t skip_stop_request_tid)
 {
     Log *const log = GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD);
     std::lock_guard<std::mutex> lock(m_event_mutex);
 
     if (log)
     {
-        log->Printf("NativeProcessLinux::%s about to process event: (triggering_tid: %" PRIu64 ", skip_stop_request_tids.size(): %zd)",
-                __FUNCTION__, triggering_tid, skip_stop_request_tids.size());
+        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,
-                request_thread_stop_function,
-                skip_stop_request_tids)));
+                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
for (auto it = m_tid_map.begin(); it != m_tid_map.end(); ++it)
             sent_tids.insert (tid);
         }
     }
 
     // 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);
 }
 
-void
+Error
 NativeProcessLinux::RequestThreadStop (lldb::tid_t tid, ThreadContext& context)
 {
-    const auto error = m_pending_notification_up->request_thread_stop_function (tid);
-    if (error.Success ())
-        context.m_stop_requested = true;
-    else
+    Log* log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
+
+    lldb::pid_t pid = GetID();
+
+    if (log)
+        log->Printf ("NativeProcessLinux::%s requesting thread stop(pid: %" PRIu64 ", tid: %" PRIu64 ")", __FUNCTION__, pid, tid);
+
+    Error err;
+    errno = 0;
+    if (::tgkill (pid, tid, SIGSTOP) != 0)
     {
-        Log *const log = GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD);
+        err.SetErrorToErrno ();
         if (log)
-            log->Printf("NativeProcessLinux::%s failed to request thread stop tid  %" PRIu64 ": %s",
-                         __FUNCTION__, tid, error.AsCString ());
+            log->Printf ("NativeProcessLinux::%s tgkill(%" PRIu64 ", %" PRIu64 ", SIGSTOP) failed: %s", __FUNCTION__, pid, tid, err.AsCString ());
     }
+    else
+        context.m_stop_requested = true;
+
+    return err;
 }
 
 
 Error
 NativeProcessLinux::ThreadDidStop (lldb::tid_t tid, bool initiated_by_llgs)
 {
     // Ensure we know about the thread.
     auto find_it = m_tid_map.find (tid);

 
 void
 NativeProcessLinux::ThreadWasCreated (lldb::tid_t tid, bool is_stopped)
 {
     // Ensure we don't already know about the thread.
     lldbassert(m_tid_map.find(tid) == m_tid_map.end());
 
     // Add the new thread to the stop map.
-    ThreadContext ctx;
-    ctx.m_state = (is_stopped) ? ThreadState::Stopped : ThreadState::Running;
-    m_tid_map[tid] = std::move(ctx);
+    auto tid_it = m_tid_map.emplace(tid, ThreadContext()).first;
+    tid_it->second.m_state = (is_stopped) ? ThreadState::Stopped : ThreadState::Running;
 
     if (m_pending_notification_up && !is_stopped)
     {
         // We will need to wait for this new thread to stop as well before firing the
         // notification.
         m_pending_notification_up->wait_for_stop_tids.insert(tid);
-        m_pending_notification_up->request_thread_stop_function(tid);
+        RequestThreadStop(GetID(), tid_it->second);
     }
 }
 
 void
 NativeProcessLinux::ThreadDidDie (lldb::tid_t tid)
 {
     // Ensure we know about the thread.
     auto find_it = m_tid_map.find (tid);