[LLGS] Don't forward I/O when process is stopped

[labath]

Summary

This makes sure we do not attempt to send output over the gdb-remote protocol when the client is
not expecting it (i.e., after sending the stop-reply packet). Normally, this should not happen
(the process cannot generate output when it is stopped), but due to the fact that pty
communication is asynchronous in the linux kernel (llvm.org/pr25652), we may sometimes get this
output too late. Instead, we just hold the output, and send it next time we resume. This is not
ideal, but at least it makes sure we do not violate the remote protocol. Given that this happens
extremely rarely it's not worth trying to work around it with sleeps or something like that.

I also remove the m_stdio_communication_mutex, as all of LLGS is now single-threaded anyway.

[tberghammer]

LGTM

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

1014 ↗

(On Diff #41241)

Can you add an assert that m_stdio_handle_up is nullptr when StartSTDIOForwarding is called?

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

protected:
     lldb::PlatformSP m_platform_sp;
     MainLoop &m_mainloop;
     MainLoop::ReadHandleUP m_network_handle_up;
     lldb::tid_t m_current_tid;
     lldb::tid_t m_continue_tid;
     Mutex m_debugged_process_mutex;
     NativeProcessProtocolSP m_debugged_process_sp;
 
-    Mutex m_stdio_communication_mutex; // Protects m_stdio_communication and m_stdio_handle_up
     Communication m_stdio_communication;
     MainLoop::ReadHandleUP m_stdio_handle_up;
 
     lldb::StateType m_inferior_prev_state;
     lldb::DataBufferSP m_active_auxv_buffer_sp;
     Mutex m_saved_registers_mutex;
     std::unordered_map<uint32_t, lldb::DataBufferSP> m_saved_registers_map;
     uint32_t m_next_saved_registers_id;
private:
 
     void
     DataAvailableCallback ();
 
     void
     SendProcessOutput ();
 
     void
+    StartSTDIOForwarding();
+
+    void
     StopSTDIOForwarding();
 
     //------------------------------------------------------------------
     // For GDBRemoteCommunicationServerLLGS only
     //------------------------------------------------------------------
     DISALLOW_COPY_AND_ASSIGN (GDBRemoteCommunicationServerLLGS);
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif  // liblldb_GDBRemoteCommunicationServerLLGS_h_

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

 #include "lldb/Host/TimeValue.h"
 #include "lldb/Target/FileAction.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Platform.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/common/NativeProcessProtocol.h"
 #include "lldb/Host/common/NativeThreadProtocol.h"
 #include "lldb/Utility/JSON.h"
+#include "lldb/Utility/LLDBAssert.h"
 
 // Project includes
 #include "Utility/StringExtractorGDBRemote.h"
 #include "Utility/UriParser.h"
 #include "ProcessGDBRemote.h"
 #include "ProcessGDBRemoteLog.h"
 
 using namespace lldb;
GDBRemoteCommunicationServerLLGS::ProcessStateChanged (NativeProcessProtocol *process, lldb::StateType state)
     if (log)
     {
         log->Printf ("GDBRemoteCommunicationServerLLGS::%s called with NativeProcessProtocol pid %" PRIu64 ", state: %s",
                 __FUNCTION__,
                 process->GetID (),
                 StateAsCString (state));
     }
 
+    switch (state)
+    {
+    case StateType::eStateRunning:
+        StartSTDIOForwarding();
+        break;
+
+    case StateType::eStateStopped:
         // Make sure we get all of the pending stdout/stderr from the inferior
         // and send it to the lldb host before we send the state change
         // notification
         SendProcessOutput();
+        // Then stop the forwarding, so that any late output (see llvm.org/pr25652) does not
+        // interfere with our protocol.
+        StopSTDIOForwarding();
+        HandleInferiorState_Stopped (process);
+        break;
 
-    switch (state)
-    {
     case StateType::eStateExited:
+        // Same as above
+        SendProcessOutput();
+        StopSTDIOForwarding();
         HandleInferiorState_Exited (process);
         break;
 
-    case StateType::eStateStopped:
-        HandleInferiorState_Stopped (process);
-        break;
-
     default:
         if (log)
         {
             log->Printf ("GDBRemoteCommunicationServerLLGS::%s didn't handle state change for pid %" PRIu64 ", new state: %s",
                     __FUNCTION__,
                     process->GetID (),
                     StateAsCString (state));
         }
GDBRemoteCommunicationServerLLGS::SetSTDIOFileDescriptor (int fd)
     // Set up the reading/handling of process I/O
     std::unique_ptr<ConnectionFileDescriptor> conn_up (new ConnectionFileDescriptor (fd, true));
     if (!conn_up)
     {
         error.SetErrorString ("failed to create ConnectionFileDescriptor");
         return error;
     }
 
-    Mutex::Locker locker(m_stdio_communication_mutex);
     m_stdio_communication.SetCloseOnEOF (false);
     m_stdio_communication.SetConnection (conn_up.release());
     if (!m_stdio_communication.IsConnected ())
     {
         error.SetErrorString ("failed to set connection for inferior I/O communication");
         return error;
     }
 
+    return Error();
+}
+
+void
+GDBRemoteCommunicationServerLLGS::StartSTDIOForwarding()
+{
+    // Don't forward if not connected (e.g. when attaching).
+    if (! m_stdio_communication.IsConnected())
+        return;
+
     // llgs local-process debugging may specify PTY paths, which will make these
     // file actions non-null
     // process launch -e/o will also make these file actions non-null
     // nullptr means that the traffic is expected to flow over gdb-remote protocol
-    if (
-        m_process_launch_info.GetFileActionForFD(STDOUT_FILENO) == nullptr ||
-        m_process_launch_info.GetFileActionForFD(STDERR_FILENO) == nullptr
-        )
-    {
-        // output from the process must be forwarded over gdb-remote
-        m_stdio_handle_up = m_mainloop.RegisterReadObject(
-                m_stdio_communication.GetConnection()->GetReadObject(),
-                [this] (MainLoopBase &) { SendProcessOutput(); }, error);
-        if (! m_stdio_handle_up)
-        {
-            m_stdio_communication.Disconnect();
-            return error;
-        }
+    if ( m_process_launch_info.GetFileActionForFD(STDOUT_FILENO) &&
+         m_process_launch_info.GetFileActionForFD(STDERR_FILENO))
+        return;
+
+    Error error;
+    lldbassert(! m_stdio_handle_up);
+
+        // Not much we can do about the failure. Log it and continue without forwarding.
+        if (Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS))
+            log->Printf("GDBRemoteCommunicationServerLLGS::%s Failed to set up stdio forwarding: %s",
+                        __FUNCTION__, error.AsCString());
     }
-
-    return Error();
 }
 
 void
 GDBRemoteCommunicationServerLLGS::StopSTDIOForwarding()
 {
-    Mutex::Locker locker(m_stdio_communication_mutex);
     m_stdio_handle_up.reset();
 }
 
 void
 GDBRemoteCommunicationServerLLGS::SendProcessOutput()
 {
     char buffer[1024];
     ConnectionStatus status;
     Error error;
-    Mutex::Locker locker(m_stdio_communication_mutex);
     while (true)
     {
         size_t bytes_read = m_stdio_communication.Read(buffer, sizeof buffer, 0, status, &error);
         switch (status)
         {
         case eConnectionStatusSuccess:
             SendONotification(buffer, bytes_read);
             break;
for (;;)
         if (read == 0)
         {
             break;
         }
         // write directly to stdin *this might block if stdin buffer is full*
         // TODO: enqueue this block in circular buffer and send window size to remote host
         ConnectionStatus status;
         Error error;
-        Mutex::Locker locker(m_stdio_communication_mutex);
         m_stdio_communication.Write(tmp, read, status, &error);
         if (error.Fail())
         {
             return SendErrorResponse (0x15);
         }
     }
 
     return SendOKResponse();
GDBRemoteCommunicationServerLLGS::Handle_qFileLoadAddress (StringExtractorGDBRemote &packet)
     return SendPacketNoLock(response.GetData(), response.GetSize());
 }
 
 void
 GDBRemoteCommunicationServerLLGS::MaybeCloseInferiorTerminalConnection ()
 {
     Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
 
-    Mutex::Locker locker(m_stdio_communication_mutex);
     // Tell the stdio connection to shut down.
     if (m_stdio_communication.IsConnected())
     {
         auto connection = m_stdio_communication.GetConnection();
         if (connection)
         {
             Error error;
             connection->Disconnect (&error);