[lldb] [lldb-gdbserver] Unify listen/connect code to use ConnectionFileDescriptor

[mgorny]

Summary

Unify the listen and connect code inside lldb-server to use
ConnectionFileDescriptor uniformly rather than a mix of it and Acceptor.
This involves:

• adding a function to map legacy values of host:port parameter (including legacy server URLs) into CFD-style URLs

• adding a callback to return "local socket id" (i.e. UNIX socket path or TCP port number) between listen() and accept() calls in CFD

• adding a "unix-abstract-accept" scheme to CFD

As an additional advantage, this permits lldb-server to accept any URL
known to CFD including the new serial:// scheme. Effectively,
lldb-server can now listen on the serial port. Tests for connecting
over a pty are added to test that.

[labath]

What's the test strategy for this?

This is not fully functional yet, as lldb-server
crashes when attempting to send long packets (e.g. target.xml contents).

I am guessing you'll also want to disable QStartNoAckMode for these connections (?)

lldb/tools/lldb-server/lldb-gdbserver.cpp

259–260

Maybe we should have this accept any lldb "URL", and have the host:port support be a special case of "legacy" connection strings?

[mgorny]

In D111964#3069414, @labath wrote:

What's the test strategy for this?

Still thinking about it. I'm mostly PoC-ing this while other changes are waiting.

This is not fully functional yet, as lldb-server
crashes when attempting to send long packets (e.g. target.xml contents).

I am guessing you'll also want to disable QStartNoAckMode for these connections (?)

I suppose this makes sense. Are you suggesting special-casing serial:// or do you have something else in mind?

[labath]

In D111964#3071292, @mgorny wrote:

This is not fully functional yet, as lldb-server
crashes when attempting to send long packets (e.g. target.xml contents).

I am guessing you'll also want to disable QStartNoAckMode for these connections (?)

I suppose this makes sense. Are you suggesting special-casing serial:// or do you have something else in mind?

I think we could introduce something like Connection::IsReliable and key that behavior off of that.

Although I can't say I actually understand how the "ack" mode is supposed to work. It seems like it would be very hard to recover from transmission errors that affect the $ or # parts of the packet.

[mgorny]

In D111964#3072088, @labath wrote:

I think we could introduce something like Connection::IsReliable and key that behavior off of that.

Although I can't say I actually understand how the "ack" mode is supposed to work. It seems like it would be very hard to recover from transmission errors that affect the $ or # parts of the packet.

FWICS, gdb uses "noack" mode unconditionally. I somewhat suspect that the "ack" mode might just be a historical thing that doesn't actually serve any purpose.

[labath]

In D111964#3071292, @mgorny wrote:

In D111964#3069414, @labath wrote:

What's the test strategy for this?

Still thinking about it. I'm mostly PoC-ing this while other changes are waiting.

For the current patch, I guess it would be sufficient to establish a connection using _any_ url.

lldb/tools/lldb-server/lldb-gdbserver.cpp

259–260

I am wondering what's the impact of removing this. If we get some semi-reasonable message, then it'd be nice to get rid of it. Otherwise, we'll have a weird difference between the user typing host:0 and connect://host:0.

259–260

If we don't get a reasonable error message, we could probably create one by pushing this check into the lower layers.

[mgorny]

Add a PoC for testing lldb-server over a pty.

[mgorny]

Make the test verify that (potentially large) target.xml can be successfully transferred.

[mgorny]

Turned it into an überpatch upgrading CFD and refactoring lldb-server to reduce duplication of code.

[labath]

I like this. For lack of a better place, I'd place the LLGSArgToURL function into GDBRemoteCommunicationServerLLGS.h. That is the ~highest level file that can still be unit tested.

lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h

49

I'd use llvm::function_ref here

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

546–550

you can merge these two

577–608

This is identical to NamedSocketAccept modulo the socket type constant, right? Can you merge these?

638

Can this ever be zero ?

854

I'd "reverse" this condition :P

872

Can this string be empty? Maybe use url_arg.startswith(), just in case.

[mgorny]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

577–608

Yes; in fact, most of the functions are pretty similar. However, I'd prefer to defer merging them until after the port predicate removal patch, as that should also open the possibility of combining them with the TCP socket methods too.

638

Yes, when getsockname() fails. I suppose we could modify it to use llvm::Optional<...> or even llvm::Expected<...> but this is one place where I don't think we need to care much about proper error handling.

872

I don't think it can, or at least I wasn't able to get an empty string in (the argument parser seems to reject them) but sure, no harm in being extra secure.

[mgorny]

Move to gdb-remote. Update per requests.

I've also added a typedef for the callback and optimized the URI parsing a bit to hopefully involve less temporary strings.

[labath]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

577–608

Ok, sounds good.

638

But how can it fail? This is a socket that we've just created and its totally under our control...

[mgorny]

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

638

Yeah, exactly, that's why I said proper error handling is not very important there — but technically the function provides some potential error conditions. Are you suggesting we just pass zero as-is then?

[mgorny]

Remove error handling from socket name getters.

[mgorny]

Remove error handling from URI mapping to make the code simpler. In the end, the only possible error here was '--reverse-connect without valid host:port' and we can just treat that as UNIX connect for symmetry with non-reverse-connect mode.

lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h

 namespace lldb_private {
 
 class Status;
 class Socket;
 class SocketAddress;
 
 class ConnectionFileDescriptor : public Connection {
 public:
+  typedef llvm::function_ref<void(llvm::StringRef local_socket_id)>
+      socket_id_callback_type;
+
   ConnectionFileDescriptor(bool child_processes_inherit = false);
 
   ConnectionFileDescriptor(int fd, bool owns_fd);
 
   ConnectionFileDescriptor(Socket *socket);
 
   ~ConnectionFileDescriptor() override;
 
   bool IsConnected() const override;
 
-  lldb::ConnectionStatus Connect(llvm::StringRef s, Status *error_ptr) override;
+  lldb::ConnectionStatus Connect(llvm::StringRef url,
+                                 Status *error_ptr) override;
+
+  lldb::ConnectionStatus Connect(llvm::StringRef url,
+                                 socket_id_callback_type socket_id_callback,
+                                 Status *error_ptr);
 
   lldb::ConnectionStatus Disconnect(Status *error_ptr) override;
 
   size_t Read(void *dst, size_t dst_len, const Timeout<std::micro> &timeout,
               lldb::ConnectionStatus &status, Status *error_ptr) override;
 
   size_t Write(const void *src, size_t src_len, lldb::ConnectionStatus &status,
                Status *error_ptr) override;
public:
   bool GetChildProcessesInherit() const;
   void SetChildProcessesInherit(bool child_processes_inherit);
 
 protected:
   void OpenCommandPipe();
 
   void CloseCommandPipe();
 
-  lldb::ConnectionStatus SocketListenAndAccept(llvm::StringRef host_and_port,
-                                               Status *error_ptr);
+  lldb::ConnectionStatus
+  SocketListenAndAccept(llvm::StringRef host_and_port,
+                        socket_id_callback_type socket_id_callback,
 
   lldb::ConnectionStatus ConnectTCP(llvm::StringRef host_and_port,
+                                    socket_id_callback_type socket_id_callback,
                                     Status *error_ptr);
 
-  lldb::ConnectionStatus ConnectUDP(llvm::StringRef args, Status *error_ptr);
+  lldb::ConnectionStatus ConnectUDP(llvm::StringRef args,
+                                    socket_id_callback_type socket_id_callback,
+                                    Status *error_ptr);
 
-  lldb::ConnectionStatus NamedSocketConnect(llvm::StringRef socket_name,
-                                            Status *error_ptr);
+  lldb::ConnectionStatus
+  NamedSocketConnect(llvm::StringRef socket_name,
+                     socket_id_callback_type socket_id_callback,
 
-  lldb::ConnectionStatus NamedSocketAccept(llvm::StringRef socket_name,
-                                           Status *error_ptr);
+  lldb::ConnectionStatus
+  NamedSocketAccept(llvm::StringRef socket_name,
+                    socket_id_callback_type socket_id_callback,
 
-  lldb::ConnectionStatus UnixAbstractSocketConnect(llvm::StringRef socket_name,
-                                                   Status *error_ptr);
+  lldb::ConnectionStatus
+  UnixAbstractSocketAccept(llvm::StringRef socket_name,
+                           socket_id_callback_type socket_id_callback,
 
-  lldb::ConnectionStatus ConnectFD(llvm::StringRef args, Status *error_ptr);
+  lldb::ConnectionStatus
+  UnixAbstractSocketConnect(llvm::StringRef socket_name,
+                            socket_id_callback_type socket_id_callback,
+                            Status *error_ptr);
+
+  lldb::ConnectionStatus ConnectFD(llvm::StringRef args,
+                                   socket_id_callback_type socket_id_callback,
+                                   Status *error_ptr);
 
-  lldb::ConnectionStatus ConnectFile(llvm::StringRef args, Status *error_ptr);
+  lldb::ConnectionStatus ConnectFile(llvm::StringRef args,
+                                     socket_id_callback_type socket_id_callback,
+                                     Status *error_ptr);
 
-  lldb::ConnectionStatus ConnectSerialPort(llvm::StringRef args,
-                                           Status *error_ptr);
+  lldb::ConnectionStatus
+  ConnectSerialPort(llvm::StringRef args,
+                    socket_id_callback_type socket_id_callback,
 
   lldb::IOObjectSP m_io_sp;
 
   Predicate<uint16_t>
       m_port_predicate; // Used when binding to port zero to wait for the thread
                         // that creates the socket, binds and listens to
                         // resolve the port number.
 
   Pipe m_pipe;
   std::recursive_mutex m_mutex;
   std::atomic<bool> m_shutting_down; // This marks that we are shutting down so
                                      // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.
-  bool m_waiting_for_accept = false;
   bool m_child_processes_inherit;
 
   std::string m_uri;
 
 private:
   void InitializeSocket(Socket *socket);
 
   ConnectionFileDescriptor(const ConnectionFileDescriptor &) = delete;
   const ConnectionFileDescriptor &
   operator=(const ConnectionFileDescriptor &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_HOST_POSIX_CONNECTIONFILEDESCRIPTORPOSIX_H

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

 //===-- ConnectionFileDescriptorPosix.cpp ---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #if defined(__APPLE__)
 // Enable this special support for Apple builds where we can have unlimited
 // select bounds. We tried switching to poll() and kqueue and we were panicing
 // the kernel, so we have to stick with select for now.
 #define _DARWIN_UNLIMITED_SELECT
 #endif
 
-#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Host/SocketAddress.h"
+#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
 #include "lldb/Utility/SelectHelper.h"
 #include "lldb/Utility/Timeout.h"
 
 #include <cerrno>
 #include <cstdlib>
 #include <cstring>
 #include <fcntl.h>
 #include <sys/types.h>
ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit)
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION |
                                                   LIBLLDB_LOG_OBJECT));
   LLDB_LOGF(log, "%p ConnectionFileDescriptor::ConnectionFileDescriptor ()",
             static_cast<void *>(this));
 }
 
 ConnectionFileDescriptor::ConnectionFileDescriptor(int fd, bool owns_fd)
     : Connection(), m_pipe(), m_mutex(), m_shutting_down(false),
-      m_waiting_for_accept(false), m_child_processes_inherit(false) {
+      m_child_processes_inherit(false) {
   m_io_sp =
       std::make_shared<NativeFile>(fd, File::eOpenOptionReadWrite, owns_fd);
 
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION |
                                                   LIBLLDB_LOG_OBJECT));
   LLDB_LOGF(log,
             "%p ConnectionFileDescriptor::ConnectionFileDescriptor (fd = "
             "%i, owns_fd = %i)",
             static_cast<void *>(this), fd, owns_fd);
   OpenCommandPipe();
 }
 
 ConnectionFileDescriptor::ConnectionFileDescriptor(Socket *socket)
     : Connection(), m_pipe(), m_mutex(), m_shutting_down(false),
-      m_waiting_for_accept(false), m_child_processes_inherit(false) {
+      m_child_processes_inherit(false) {
   InitializeSocket(socket);
 }
 
 ConnectionFileDescriptor::~ConnectionFileDescriptor() {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION |
                                                   LIBLLDB_LOG_OBJECT));
   LLDB_LOGF(log, "%p ConnectionFileDescriptor::~ConnectionFileDescriptor ()",
             static_cast<void *>(this));

 }
 
 bool ConnectionFileDescriptor::IsConnected() const {
   return m_io_sp && m_io_sp->IsValid();
 }
 
 ConnectionStatus ConnectionFileDescriptor::Connect(llvm::StringRef path,
                                                    Status *error_ptr) {
+  return Connect(path, nullptr, error_ptr);
+}
+
+ConnectionStatus
+ConnectionFileDescriptor::Connect(llvm::StringRef path,
+                                  socket_id_callback_type socket_id_callback,
+                                  Status *error_ptr) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   LLDB_LOGF(log, "%p ConnectionFileDescriptor::Connect (url = '%s')",
             static_cast<void *>(this), path.str().c_str());
 
   OpenCommandPipe();
 
   if (path.empty()) {
     if (error_ptr)
       error_ptr->SetErrorString("invalid connect arguments");
     return eConnectionStatusError;
   }
 
   llvm::StringRef scheme;
   std::tie(scheme, path) = path.split("://");
 
   if (!path.empty()) {
     auto method =
         llvm::StringSwitch<ConnectionStatus (ConnectionFileDescriptor::*)(
-            llvm::StringRef, Status *)>(scheme)
+            llvm::StringRef, socket_id_callback_type, Status *)>(scheme)
             .Case("listen", &ConnectionFileDescriptor::SocketListenAndAccept)
             .Cases("accept", "unix-accept",
                    &ConnectionFileDescriptor::NamedSocketAccept)
+            .Case("unix-abstract-accept",
+                  &ConnectionFileDescriptor::UnixAbstractSocketAccept)
             .Cases("connect", "tcp-connect",
                    &ConnectionFileDescriptor::ConnectTCP)
             .Case("udp", &ConnectionFileDescriptor::ConnectUDP)
             .Case("unix-connect", &ConnectionFileDescriptor::NamedSocketConnect)
             .Case("unix-abstract-connect",
                   &ConnectionFileDescriptor::UnixAbstractSocketConnect)
 #if LLDB_ENABLE_POSIX
             .Case("fd", &ConnectionFileDescriptor::ConnectFD)
             .Case("file", &ConnectionFileDescriptor::ConnectFile)
             .Case("serial", &ConnectionFileDescriptor::ConnectSerialPort)
 #endif
             .Default(nullptr);
 
     if (method)
-      return (this->*method)(path, error_ptr);
+      return (this->*method)(path, socket_id_callback, error_ptr);
   }
 
   if (error_ptr)
     error_ptr->SetErrorStringWithFormat("unsupported connection URL: '%s'",
                                         path.str().c_str());
   return eConnectionStatusError;
 }
 
while (handle == m_io_sp->GetWaitableHandle()) {
         if (select_helper.FDIsSetRead(handle))
           return eConnectionStatusSuccess;
 
         if (select_helper.FDIsSetRead(pipe_fd)) {
           // There is an interrupt or exit command in the command pipe Read the
           // data from that pipe:
           char c;
 
-          ssize_t bytes_read = llvm::sys::RetryAfterSignal(-1, ::read, pipe_fd, &c, 1);
+          ssize_t bytes_read =
+              llvm::sys::RetryAfterSignal(-1, ::read, pipe_fd, &c, 1);
           assert(bytes_read == 1);
           (void)bytes_read;
           switch (c) {
           case 'q':
             LLDB_LOGF(log,
                       "%p ConnectionFileDescriptor::BytesAvailable() "
                       "got data: %c from the command channel.",
                       static_cast<void *>(this), c);
             return eConnectionStatusEndOfFile;
           case 'i':
             // Interrupt the current read
             return eConnectionStatusInterrupted;
           }
         }
       }
     }
   }
 
   if (error_ptr)
     error_ptr->SetErrorString("not connected");
   return eConnectionStatusLostConnection;
 }
 
-ConnectionStatus
-ConnectionFileDescriptor::NamedSocketAccept(llvm::StringRef socket_name,
-                                            Status *error_ptr) {
+ConnectionStatus ConnectionFileDescriptor::NamedSocketAccept(
+    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
+  Status error;
+  std::unique_ptr<Socket> listen_socket = Socket::Create(
+      Socket::ProtocolUnixDomain, m_child_processes_inherit, error);
   Socket *socket = nullptr;
-  Status error =
-      Socket::UnixDomainAccept(socket_name, m_child_processes_inherit, socket);
+
+  if (!error.Fail())
+    error = listen_socket->Listen(socket_name, 5);
+
+  if (!error.Fail()) {
+    socket_id_callback(socket_name);
+    error = listen_socket->Accept(socket);
+  }
+
+  if (!error.Fail()) {
+    m_io_sp.reset(socket);
+    m_uri.assign(socket_name.str());
+    return eConnectionStatusSuccess;
+  }
+
   if (error_ptr)
     *error_ptr = error;
-  m_io_sp.reset(socket);
-  if (error.Fail())
-    return eConnectionStatusError;
-  m_uri.assign(std::string(socket_name));
-  return eConnectionStatusSuccess;
 }
 
-ConnectionStatus
-ConnectionFileDescriptor::NamedSocketConnect(llvm::StringRef socket_name,
-                                             Status *error_ptr) {
+ConnectionStatus ConnectionFileDescriptor::NamedSocketConnect(
+    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
   Socket *socket = nullptr;
   Status error =
       Socket::UnixDomainConnect(socket_name, m_child_processes_inherit, socket);
   if (error_ptr)
     *error_ptr = error;
   m_io_sp.reset(socket);
   if (error.Fail())
     return eConnectionStatusError;
   m_uri.assign(std::string(socket_name));
   return eConnectionStatusSuccess;
 }
 
-lldb::ConnectionStatus
-ConnectionFileDescriptor::UnixAbstractSocketConnect(llvm::StringRef socket_name,
-                                                    Status *error_ptr) {
+ConnectionStatus ConnectionFileDescriptor::UnixAbstractSocketAccept(
+    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
+    Status *error_ptr) {
+  Status error;
+  std::unique_ptr<Socket> listen_socket = Socket::Create(
+      Socket::ProtocolUnixAbstract, m_child_processes_inherit, error);
+  Socket *socket = nullptr;
+
+  if (!error.Fail())
+    error = listen_socket->Listen(socket_name, 5);
+
+  if (!error.Fail())
+    socket_id_callback(socket_name);
+
+  if (!error.Fail())
+    error = listen_socket->Accept(socket);
+
+  if (!error.Fail()) {
+    m_io_sp.reset(socket);
+    m_uri.assign(socket_name.str());
+    return eConnectionStatusSuccess;
+  }
+
+  if (error_ptr)
+    *error_ptr = error;
+  return eConnectionStatusError;
+}
+
+lldb::ConnectionStatus ConnectionFileDescriptor::UnixAbstractSocketConnect(
+    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
   Socket *socket = nullptr;
   Status error = Socket::UnixAbstractConnect(socket_name,
                                              m_child_processes_inherit, socket);
   if (error_ptr)
     *error_ptr = error;
   m_io_sp.reset(socket);
   if (error.Fail())
     return eConnectionStatusError;
   m_uri.assign(std::string(socket_name));
   return eConnectionStatusSuccess;
 }
 
-ConnectionStatus
-ConnectionFileDescriptor::SocketListenAndAccept(llvm::StringRef s,
-                                                Status *error_ptr) {
+ConnectionStatus ConnectionFileDescriptor::SocketListenAndAccept(
+    llvm::StringRef s, socket_id_callback_type socket_id_callback,
   if (error_ptr)
     *error_ptr = Status();
   m_port_predicate.SetValue(0, eBroadcastNever);
 
-  m_waiting_for_accept = true;
   llvm::Expected<std::unique_ptr<TCPSocket>> listening_socket =
       Socket::TcpListen(s, m_child_processes_inherit, &m_port_predicate);
   if (!listening_socket) {
     if (error_ptr)
       *error_ptr = listening_socket.takeError();
     else
       LLDB_LOG_ERROR(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION),
                      listening_socket.takeError(), "tcp listen failed: {0}");
     return eConnectionStatusError;
   }
 
+  uint16_t port = listening_socket.get()->GetLocalPortNumber();
+  socket_id_callback(std::to_string(port));
 
   Socket *accepted_socket;
   Status error = listening_socket.get()->Accept(accepted_socket);
   if (error_ptr)
     *error_ptr = error;
   if (error.Fail())
     return eConnectionStatusError;
 
   InitializeSocket(accepted_socket);
   return eConnectionStatusSuccess;
 }
 
-ConnectionStatus ConnectionFileDescriptor::ConnectTCP(llvm::StringRef s,
-                                                      Status *error_ptr) {
+ConnectionStatus
+ConnectionFileDescriptor::ConnectTCP(llvm::StringRef s,
+                                     socket_id_callback_type socket_id_callback,
   if (error_ptr)
     *error_ptr = Status();
 
   llvm::Expected<std::unique_ptr<Socket>> socket =
       Socket::TcpConnect(s, m_child_processes_inherit);
   if (!socket) {
     if (error_ptr)
       *error_ptr = socket.takeError();
     else
       LLDB_LOG_ERROR(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION),
                      socket.takeError(), "tcp connect failed: {0}");
     return eConnectionStatusError;
   }
   m_io_sp = std::move(*socket);
   m_uri.assign(std::string(s));
   return eConnectionStatusSuccess;
 }
 
-ConnectionStatus ConnectionFileDescriptor::ConnectUDP(llvm::StringRef s,
-                                                      Status *error_ptr) {
+ConnectionStatus
+ConnectionFileDescriptor::ConnectUDP(llvm::StringRef s,
+                                     socket_id_callback_type socket_id_callback,
   if (error_ptr)
     *error_ptr = Status();
   llvm::Expected<std::unique_ptr<UDPSocket>> socket =
       Socket::UdpConnect(s, m_child_processes_inherit);
   if (!socket) {
     if (error_ptr)
       *error_ptr = socket.takeError();
     else
       LLDB_LOG_ERROR(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION),
                      socket.takeError(), "tcp connect failed: {0}");
     return eConnectionStatusError;
   }
   m_io_sp = std::move(*socket);
   m_uri.assign(std::string(s));
   return eConnectionStatusSuccess;
 }
 
-ConnectionStatus ConnectionFileDescriptor::ConnectFD(llvm::StringRef s,
-                                                     Status *error_ptr) {
+ConnectionStatus
+ConnectionFileDescriptor::ConnectFD(llvm::StringRef s,
+                                    socket_id_callback_type socket_id_callback,
 #if LLDB_ENABLE_POSIX
   // Just passing a native file descriptor within this current process that
   // is already opened (possibly from a service or other source).
   int fd = -1;
 
   if (!s.getAsInteger(0, fd)) {
     // We have what looks to be a valid file descriptor, but we should make
     // sure it is. We currently are doing this by trying to get the flags
if (error_ptr)
     error_ptr->SetErrorStringWithFormat("invalid file descriptor: \"%s\"",
                                         s.str().c_str());
   m_io_sp.reset();
   return eConnectionStatusError;
 #endif // LLDB_ENABLE_POSIX
   llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
 }
 
-ConnectionStatus ConnectionFileDescriptor::ConnectFile(llvm::StringRef s,
-                                                       Status *error_ptr) {
+ConnectionStatus ConnectionFileDescriptor::ConnectFile(
+    llvm::StringRef s, socket_id_callback_type socket_id_callback,
 #if LLDB_ENABLE_POSIX
   std::string addr_str = s.str();
   // file:///PATH
   int fd = llvm::sys::RetryAfterSignal(-1, ::open, addr_str.c_str(), O_RDWR);
   if (fd == -1) {
     if (error_ptr)
       error_ptr->SetErrorToErrno();
     return eConnectionStatusError;
if (::isatty(fd)) {
 
     // Make sure only one character is needed to return from a read
     options.c_cc[VMIN] = 1;
     options.c_cc[VTIME] = 0;
 
     llvm::sys::RetryAfterSignal(-1, ::tcsetattr, fd, TCSANOW, &options);
   }
 
-  m_io_sp =
-      std::make_shared<NativeFile>(fd, File::eOpenOptionReadWrite, true);
+  m_io_sp = std::make_shared<NativeFile>(fd, File::eOpenOptionReadWrite, true);
   return eConnectionStatusSuccess;
 #endif // LLDB_ENABLE_POSIX
   llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
 }
 
-ConnectionStatus
-ConnectionFileDescriptor::ConnectSerialPort(llvm::StringRef s,
-                                            Status *error_ptr) {
+ConnectionStatus ConnectionFileDescriptor::ConnectSerialPort(
+    llvm::StringRef s, socket_id_callback_type socket_id_callback,
 #if LLDB_ENABLE_POSIX
   llvm::StringRef path, qs;
   // serial:///PATH?k1=v1&k2=v2...
   std::tie(path, qs) = s.split('?');
 
   llvm::Expected<SerialPort::Options> serial_options =
       SerialPort::OptionsFromURL(qs);
   if (!serial_options) {

 void ConnectionFileDescriptor::SetChildProcessesInherit(
     bool child_processes_inherit) {
   m_child_processes_inherit = child_processes_inherit;
 }
 
 void ConnectionFileDescriptor::InitializeSocket(Socket *socket) {
   m_io_sp.reset(socket);
   m_uri = socket->GetRemoteConnectionURI();
 }

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

private:
 
   // For GDBRemoteCommunicationServerLLGS only
   GDBRemoteCommunicationServerLLGS(const GDBRemoteCommunicationServerLLGS &) =
       delete;
   const GDBRemoteCommunicationServerLLGS &
   operator=(const GDBRemoteCommunicationServerLLGS &) = delete;
 };
 
+std::string LLGSArgToURL(llvm::StringRef url_arg, bool reverse_connect);
+
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H

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

 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/Debug.h"
 #include "lldb/Host/File.h"
 #include "lldb/Host/FileAction.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/PosixApi.h"
+#include "lldb/Host/Socket.h"
 #include "lldb/Host/common/NativeProcessProtocol.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/common/NativeThreadProtocol.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/GDBRemote.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UnimplementedError.h"
+#include "lldb/Utility/UriParser.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #include "ProcessGDBRemote.h"
 #include "ProcessGDBRemoteLog.h"
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 

 }
 
 void GDBRemoteCommunicationServerLLGS::SetEnabledExtensions(
     NativeProcessProtocol &process) {
   NativeProcessProtocol::Extension flags = m_extensions_supported;
   assert(!bool(flags & ~m_process_factory.GetSupportedExtensions()));
   process.SetEnabledExtensions(flags);
 }
+
+std::string
+lldb_private::process_gdb_remote::LLGSArgToURL(llvm::StringRef url_arg,
+                                               bool reverse_connect) {
+  // Try parsing the argument as URL.
+  if (llvm::Optional<URI> url = URI::Parse(url_arg)) {
+    if (reverse_connect)
+      return url_arg.str();
+
+    // Translate the scheme from LLGS notation to ConnectionFileDescriptor.
+    // If the scheme doesn't match any, pass it through to support using CFD
+    // schemes directly.
+    std::string new_url = llvm::StringSwitch<std::string>(url->scheme)
+                              .Case("tcp", "listen")
+                              .Case("unix", "unix-accept")
+                              .Case("unix-abstract", "unix-abstract-accept")
+                              .Default(url->scheme.str());
+    llvm::append_range(new_url, url_arg.substr(url->scheme.size()));
+    return new_url;
+  }
+
+  std::string host_port = url_arg.str();
+  // If host_and_port starts with ':', default the host to be "localhost" and
+  // expect the remainder to be the port.
+  if (url_arg.startswith(":"))
+    host_port.insert(0, "localhost");
+
+  std::string host_str;
+  std::string port_str;
+  uint16_t port;
+  // Try parsing the (preprocessed) argument as host:port pair.
+  if (!llvm::errorToBool(
+          Socket::DecodeHostAndPort(host_port, host_str, port_str, port)))
+    return (reverse_connect ? "connect://" : "listen://") + host_port;
+
+  // If none of the above applied, interpret the argument as UNIX socket path.
+  return (reverse_connect ? "unix-connect://" : "unix-accept://") +
+         url_arg.str();
+}

lldb/test/API/tools/lldb-server/TestPtyServer.py

+import gdbremote_testcase
+import lldbgdbserverutils
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbgdbserverutils import *
+
+import xml.etree.ElementTree as ET
+
+
+@skipIfWindows
+class PtyServerTestCase(gdbremote_testcase.GdbRemoteTestCaseBase):
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        super().setUp()
+        import pty
+        import tty
+        master, slave = pty.openpty()
+        tty.setraw(master)
+        self._master = io.FileIO(master, 'r+b')
+        self._slave = io.FileIO(slave, 'r+b')
+
+    def get_debug_monitor_command_line_args(self, attach_pid=None):
+        commandline_args = self.debug_monitor_extra_args
+        if attach_pid:
+            commandline_args += ["--attach=%d" % attach_pid]
+
+        libc = ctypes.CDLL(None)
+        libc.ptsname.argtypes = (ctypes.c_int,)
+        libc.ptsname.restype = ctypes.c_char_p
+        pty_path = libc.ptsname(self._master.fileno()).decode()
+        commandline_args += ["serial://%s" % (pty_path,)]
+        return commandline_args
+
+    def connect_to_debug_monitor(self, attach_pid=None):
+        self.reverse_connect = False
+        server = self.launch_debug_monitor(attach_pid=attach_pid)
+        self.assertIsNotNone(server)
+
+        # TODO: make it into proper abstraction
+        class FakeSocket:
+            def __init__(self, fd):
+                self.fd = fd
+
+            def sendall(self, frame):
+                self.fd.write(frame)
+
+            def recv(self, count):
+                return self.fd.read(count)
+
+        self.sock = FakeSocket(self._master)
+        self._server = Server(self.sock, server)
+        return server
+
+    @add_test_categories(["llgs"])
+    def test_pty_server(self):
+        self.build()
+        self.set_inferior_startup_launch()
+        self.prep_debug_monitor_and_inferior()
+
+        # target.xml transfer should trigger a large enough packet to check
+        # for partial write regression
+        self.test_sequence.add_log_lines([
+            "read packet: $qXfer:features:read:target.xml:0,200000#00",
+            {
+                "direction": "send",
+                "regex": re.compile("^\$l(.+)#[0-9a-fA-F]{2}$"),
+                "capture": {1: "target_xml"},
+            }],
+            True)
+        context = self.expect_gdbremote_sequence()
+        # verify that we have received a complete, non-malformed XML
+        self.assertIsNotNone(ET.fromstring(context.get("target_xml")))

lldb/tools/lldb-server/lldb-gdbserver.cpp

 #include <cstdlib>
 #include <cstring>
 
 #ifndef _WIN32
 #include <csignal>
 #include <unistd.h>
 #endif
 
-#include "Acceptor.h"
 #include "LLDBServerUtilities.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h"
 #include "Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Pipe.h"
 #include "lldb/Host/Socket.h"
void handle_launch(GDBRemoteCommunicationServerLLGS &gdb_server,
   Status error = gdb_server.LaunchProcess();
   if (error.Fail()) {
     llvm::errs() << llvm::formatv("error: failed to launch '{0}': {1}\n",
                                   Arguments[0], error);
     exit(1);
   }
 }
 
-Status writeSocketIdToPipe(Pipe &port_pipe, const std::string &socket_id) {
+Status writeSocketIdToPipe(Pipe &port_pipe, llvm::StringRef socket_id) {
   size_t bytes_written = 0;
   // Write the port number as a C string with the NULL terminator.
-  return port_pipe.Write(socket_id.c_str(), socket_id.size() + 1,
-                         bytes_written);
+  return port_pipe.Write(socket_id.data(), socket_id.size() + 1, bytes_written);
 }
 
 Status writeSocketIdToPipe(const char *const named_pipe_path,
-                           const std::string &socket_id) {
+                           llvm::StringRef socket_id) {
   Pipe port_name_pipe;
   // Wait for 10 seconds for pipe to be opened.
   auto error = port_name_pipe.OpenAsWriterWithTimeout(named_pipe_path, false,
                                                       std::chrono::seconds{10});
   if (error.Fail())
     return error;
   return writeSocketIdToPipe(port_name_pipe, socket_id);
 }
 
 Status writeSocketIdToPipe(lldb::pipe_t unnamed_pipe,
-                           const std::string &socket_id) {
+                           llvm::StringRef socket_id) {
   Pipe port_pipe{LLDB_INVALID_PIPE, unnamed_pipe};
   return writeSocketIdToPipe(port_pipe, socket_id);
 }
 
 void ConnectToRemote(MainLoop &mainloop,
                      GDBRemoteCommunicationServerLLGS &gdb_server,
                      bool reverse_connect, llvm::StringRef host_and_port,
                      const char *const progname, const char *const subcommand,
                      const char *const named_pipe_path, pipe_t unnamed_pipe,
                      int connection_fd) {
   Status error;
 
   std::unique_ptr<Connection> connection_up;
+  std::string url;
+
   if (connection_fd != -1) {
-    // Build the connection string.
-    std::string connection_url = llvm::formatv("fd://{0}", connection_fd).str();
+    url = llvm::formatv("fd://{0}", connection_fd).str();
 
     // Create the connection.
 #if LLDB_ENABLE_POSIX && !defined _WIN32
     ::fcntl(connection_fd, F_SETFD, FD_CLOEXEC);
 #endif
-    connection_up.reset(new ConnectionFileDescriptor);
-    auto connection_result = connection_up->Connect(connection_url, &error);
-    if (error.Fail()) {
-      llvm::errs() << llvm::formatv(
-          "error: failed to connect to client at '{0}': {1}\n", connection_url,
-          error);
-      exit(-1);
-    }
-    if (connection_result != eConnectionStatusSuccess) {
-      llvm::errs() << llvm::formatv(
-          "error: failed to connect to client at '{0}' "
-          "(connection status: {1})\n",
-          connection_url, static_cast<int>(connection_result));
+  } else if (!host_and_port.empty()) {
+    llvm::Expected<std::string> url_exp =
+        LLGSArgToURL(host_and_port, reverse_connect);
+    if (!url_exp) {
+      llvm::errs() << llvm::formatv("error: invalid host:port or URL '{0}': "
+                                    "{1}\n",
+                                    host_and_port,
+                                    llvm::toString(url_exp.takeError()));
       exit(-1);
     }
-  } else if (!host_and_port.empty()) {
-    // Parse out host and port.
-    std::string final_host_and_port;
 
-    // If host_and_port starts with ':', default the host to be "localhost" and
-    // expect the remainder to be the port.
-    if (host_and_port[0] == ':')
-      final_host_and_port.append("localhost");
-    final_host_and_port.append(host_and_port.str());
-
-    if (reverse_connect) {
-      // llgs will connect to the gdb-remote client.
-
-      // Ensure we have a port number for the connection.
-      // Note: use rfind, because the host/port may look like "[::1]:12345".
-      uint32_t connection_portno = 0;
-      const std::string::size_type colon_pos = final_host_and_port.rfind(':');
-      if (colon_pos != std::string::npos)
-        llvm::to_integer(final_host_and_port.substr(colon_pos + 1),
-                         connection_portno);
-      if (connection_portno == 0) {
-        llvm::errs() << "error: port number must be specified on when using "
-                        "reverse connect\n";
-        exit(1);
-      }
+    url = std::move(url_exp.get());
 
-      // Build the connection string.
-      final_host_and_port.insert(0, "connect://");
-
-      // Create the connection.
-      connection_up.reset(new ConnectionFileDescriptor);
-      auto connection_result =
-          connection_up->Connect(final_host_and_port, &error);
-      if (error.Fail()) {
-        llvm::errs() << llvm::formatv(
-            "error: failed to connect to client at '{0}': {1}\n",
-            final_host_and_port, error);
-        exit(-1);
-      }
-      if (connection_result != eConnectionStatusSuccess) {
-        llvm::errs() << llvm::formatv(
-            "error: failed to connect to client at '{0}' "
-            "(connection status: {1})\n",
-            final_host_and_port, static_cast<int>(connection_result));
-        exit(-1);
-      }
-    } else {
-      std::unique_ptr<Acceptor> acceptor_up(
-          Acceptor::Create(final_host_and_port, false, error));
-      if (error.Fail()) {
-        llvm::errs() << llvm::formatv("failed to create acceptor: {0}\n",
-                                      error);
-        exit(1);
-      }
-      error = acceptor_up->Listen(1);
-      if (error.Fail()) {
-        llvm::errs() << llvm::formatv("failed to listen: {0}\n", error);
-        exit(1);
-      }
-      const std::string socket_id = acceptor_up->GetLocalSocketId();
-      if (!socket_id.empty()) {
-        // If we have a named pipe to write the socket id back to, do that now.
+  if (!url.empty()) {
+    // Create the connection or server.
+    std::unique_ptr<ConnectionFileDescriptor> conn_fd_up{
+        new ConnectionFileDescriptor};
+    auto connection_result = conn_fd_up->Connect(
+        url,
+        [named_pipe_path, unnamed_pipe](llvm::StringRef socket_id) {
+          // If we have a named pipe to write the socket id back to, do that
+          // now.
           if (named_pipe_path && named_pipe_path[0]) {
-          error = writeSocketIdToPipe(named_pipe_path, socket_id);
+            Status error = writeSocketIdToPipe(named_pipe_path, socket_id);
             if (error.Fail())
               llvm::errs() << llvm::formatv(
                   "failed to write to the named peipe '{0}': {1}\n",
                   named_pipe_path, error.AsCString());
           }
-        // If we have an unnamed pipe to write the socket id back to, do that
-        // now.
+          // If we have an unnamed pipe to write the socket id back to, do
+          // that now.
           else if (unnamed_pipe != LLDB_INVALID_PIPE) {
-          error = writeSocketIdToPipe(unnamed_pipe, socket_id);
+            Status error = writeSocketIdToPipe(unnamed_pipe, socket_id);
             if (error.Fail())
               llvm::errs() << llvm::formatv(
                   "failed to write to the unnamed pipe: {0}\n", error);
           }
-      } else {
-        llvm::errs()
-            << "unable to get the socket id for the listening connection\n";
-      }
+        },
+        &error);
 
-      Connection *conn = nullptr;
-      error = acceptor_up->Accept(false, conn);
-      if (error.Fail()) {
-        llvm::errs() << llvm::formatv("failed to accept new connection: {0}\n",
-                                      error);
-        exit(1);
-      }
-      connection_up.reset(conn);
+      llvm::errs() << llvm::formatv(
+          "error: failed to connect to client at '{0}': {1}\n", url, error);
+      exit(-1);
+    if (connection_result != eConnectionStatusSuccess) {
+      llvm::errs() << llvm::formatv(
+          "error: failed to connect to client at '{0}' "
+          "(connection status: {1})\n",
+          url, static_cast<int>(connection_result));
+      exit(-1);
     }
+    connection_up = std::move(conn_fd_up);
   }
   error = gdb_server.InitializeConnection(std::move(connection_up));
   if (error.Fail()) {
     llvm::errs() << llvm::formatv("failed to initialize connection\n", error);
     exit(-1);
   }
   llvm::outs() << "Connection established.\n";
 }

lldb/unittests/Process/gdb-remote/CMakeLists.txt

 add_lldb_unittest(ProcessGdbRemoteTests
   GDBRemoteClientBaseTest.cpp
   GDBRemoteCommunicationClientTest.cpp
+  GDBRemoteCommunicationServerLLGSTest.cpp
   GDBRemoteCommunicationServerTest.cpp
   GDBRemoteCommunicationTest.cpp
   GDBRemoteTestUtils.cpp
   PortMapTest.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerLLGSTest.cpp

+//===-- GDBRemoteCommunicationServerLLGSTest.cpp --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h"
+
+using namespace lldb_private::process_gdb_remote;
+
+TEST(GDBRemoteCommunicationServerLLGSTest, LLGSArgToURL) {
+  // LLGS new-style URLs should be passed through (indepenently of
+  // --reverse-connect)
+  EXPECT_EQ(LLGSArgToURL("listen://127.0.0.1:1234", false),
+            "listen://127.0.0.1:1234");
+  EXPECT_EQ(LLGSArgToURL("listen://127.0.0.1:1234", true),
+            "listen://127.0.0.1:1234");
+  EXPECT_EQ(LLGSArgToURL("connect://127.0.0.1:1234", false),
+            "connect://127.0.0.1:1234");
+  EXPECT_EQ(LLGSArgToURL("connect://127.0.0.1:1234", true),
+            "connect://127.0.0.1:1234");
+
+  // LLGS legacy listen URLs should be converted if !reverse_connect
+  EXPECT_EQ(LLGSArgToURL("tcp://127.0.0.1:1234", false),
+            "listen://127.0.0.1:1234");
+  EXPECT_EQ(LLGSArgToURL("unix:///tmp/foo", false), "unix-accept:///tmp/foo");
+  EXPECT_EQ(LLGSArgToURL("unix-abstract://foo", false),
+            "unix-abstract-accept://foo");
+
+  // LLGS listen host:port pairs should be converted to listen://
+  EXPECT_EQ(LLGSArgToURL("127.0.0.1:1234", false), "listen://127.0.0.1:1234");
+  EXPECT_EQ(LLGSArgToURL("[::1]:1234", false), "listen://[::1]:1234");
+  EXPECT_EQ(LLGSArgToURL("[[::1]:1234]", false), "listen://[[::1]:1234]");
+  EXPECT_EQ(LLGSArgToURL("localhost:1234", false), "listen://localhost:1234");
+  EXPECT_EQ(LLGSArgToURL("*:1234", false), "listen://*:1234");
+
+  // LLGS listen :port special-case should be converted to listen://
+  EXPECT_EQ(LLGSArgToURL(":1234", false), "listen://localhost:1234");
+
+  // LLGS listen UNIX sockets should be converted to unix-accept://
+  EXPECT_EQ(LLGSArgToURL("/tmp/foo", false), "unix-accept:///tmp/foo");
+  EXPECT_EQ(LLGSArgToURL("127.0.0.1", false), "unix-accept://127.0.0.1");
+  EXPECT_EQ(LLGSArgToURL("[::1]", false), "unix-accept://[::1]");
+  EXPECT_EQ(LLGSArgToURL("localhost", false), "unix-accept://localhost");
+  EXPECT_EQ(LLGSArgToURL(":frobnicate", false), "unix-accept://:frobnicate");
+
+  // LLGS reverse connect host:port pairs should be converted to connect://
+  EXPECT_EQ(LLGSArgToURL("127.0.0.1:1234", true), "connect://127.0.0.1:1234");
+  EXPECT_EQ(LLGSArgToURL("[::1]:1234", true), "connect://[::1]:1234");
+  EXPECT_EQ(LLGSArgToURL("[[::1]:1234]", true), "connect://[[::1]:1234]");
+  EXPECT_EQ(LLGSArgToURL("localhost:1234", true), "connect://localhost:1234");
+
+  // with LLGS reverse connect, anything else goes as unix-connect://
+  EXPECT_EQ(LLGSArgToURL("/tmp/foo", true), "unix-connect:///tmp/foo");
+  EXPECT_EQ(LLGSArgToURL("127.0.0.1", true), "unix-connect://127.0.0.1");
+  EXPECT_EQ(LLGSArgToURL("[::1]", true), "unix-connect://[::1]");
+  EXPECT_EQ(LLGSArgToURL("localhost", true), "unix-connect://localhost");
+  EXPECT_EQ(LLGSArgToURL(":frobnicate", true), "unix-connect://:frobnicate");
+}