[lldb] [Host] Move port predicate-related logic to gdb-remote

[mgorny]

Summary

Remove the port predicate from Socket and ConnectionFileDescriptor,
and move it to gdb-remote. It is specifically relevant to the threading
used inside gdb-remote and with the new port callback API, we can
reliably move it there. While at it, switch from the custom Predicate
to std::promise/std::future.

[labath]

This is pure goodness.

[mib]

Hi @mgorny, this broke green-dragon (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/37098/consoleText). You should be able to reproduce by running $ ninja check-lldb-unit

[mgorny]

In D112357#3088305, @mib wrote:

Hi @mgorny, this broke green-dragon (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/37098/consoleText). You should be able to reproduce by running $ ninja check-lldb-unit

I'm sorry about that. I don't think I can test it since AFAICS debugserver isn't built on non-Apple systems. However, I've pushed a fix and hopefully I haven't made a mistake.

[mib]

In D112357#3088505, @mgorny wrote:

In D112357#3088305, @mib wrote:

Hi @mgorny, this broke green-dragon (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/37098/consoleText). You should be able to reproduce by running $ ninja check-lldb-unit

I'm sorry about that. I don't think I can test it since AFAICS debugserver isn't built on non-Apple systems. However, I've pushed a fix and hopefully I haven't made a mistake.

Thanks, that fixed the bot https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/37105

lldb/include/lldb/Host/Socket.h

 
 #include <memory>
 #include <string>
 
 #include "lldb/lldb-private.h"
 
 #include "lldb/Host/SocketAddress.h"
 #include "lldb/Utility/IOObject.h"
-#include "lldb/Utility/Predicate.h"
 #include "lldb/Utility/Status.h"
 
 #ifdef _WIN32
 #include "lldb/Host/windows/windows.h"
 #include <winsock2.h>
 #include <ws2tcpip.h>
 #endif
 
public:
   virtual Status Listen(llvm::StringRef name, int backlog) = 0;
   virtual Status Accept(Socket *&socket) = 0;
 
   // Initialize a Tcp Socket object in listening mode.  listen and accept are
   // implemented separately because the caller may wish to manipulate or query
   // the socket after it is initialized, but before entering a blocking accept.
   static llvm::Expected<std::unique_ptr<TCPSocket>>
   TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
-            Predicate<uint16_t> *predicate, int backlog = 5);
+            int backlog = 5);
 
   static llvm::Expected<std::unique_ptr<Socket>>
   TcpConnect(llvm::StringRef host_and_port, bool child_processes_inherit);
 
   static llvm::Expected<std::unique_ptr<UDPSocket>>
   UdpConnect(llvm::StringRef host_and_port, bool child_processes_inherit);
 
   static Status UnixDomainConnect(llvm::StringRef host_and_port,

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

 #include <memory>
 #include <mutex>
 
 #include "lldb/lldb-forward.h"
 
 #include "lldb/Host/Pipe.h"
 #include "lldb/Utility/Connection.h"
 #include "lldb/Utility/IOObject.h"
-#include "lldb/Utility/Predicate.h"
 
 namespace lldb_private {
 
 class Status;
 class Socket;
 class SocketAddress;
 
 class ConnectionFileDescriptor : public Connection {
public:
 
   lldb::ConnectionStatus BytesAvailable(const Timeout<std::micro> &timeout,
                                         Status *error_ptr);
 
   bool InterruptRead() override;
 
   lldb::IOObjectSP GetReadObject() override { return m_io_sp; }
 
-  uint16_t GetListeningPort(const Timeout<std::micro> &timeout);
-
   bool GetChildProcessesInherit() const;
   void SetChildProcessesInherit(bool child_processes_inherit);
 
 protected:
   void OpenCommandPipe();
 
   void CloseCommandPipe();
 
protected:
 
   lldb::ConnectionStatus ConnectSerialPort(
       llvm::StringRef args,
       std::function<void(llvm::StringRef local_socket_id)> socket_id_callback,
       Status *error_ptr);
 
   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_child_processes_inherit;
 
   std::string m_uri;

lldb/source/Host/common/Socket.cpp

Socket::TcpConnect(llvm::StringRef host_and_port,
   if (error.Success())
     return std::move(connect_socket);
 
   return error.ToError();
 }
 
 llvm::Expected<std::unique_ptr<TCPSocket>>
 Socket::TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
-                  Predicate<uint16_t> *predicate, int backlog) {
+                  int backlog) {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   LLDB_LOG(log, "host_and_port = {0}", host_and_port);
 
   std::string host_str;
   std::string port_str;
   uint16_t port;
   if (llvm::Error decode_error =
           DecodeHostAndPort(host_and_port, host_str, port_str, port))
     return std::move(decode_error);
 
   std::unique_ptr<TCPSocket> listen_socket(
       new TCPSocket(true, child_processes_inherit));
 
   Status error = listen_socket->Listen(host_and_port, backlog);
   if (error.Fail())
     return error.ToError();
 
   // We were asked to listen on port zero which means we must now read the
   // actual port that was given to us as port zero is a special code for
   // "find an open port for me".
   if (port == 0)
     port = listen_socket->GetLocalPortNumber();
 
-  // Set the port predicate since when doing a listen://<host>:<port> it
-  // often needs to accept the incoming connection which is a blocking system
-  // call. Allowing access to the bound port using a predicate allows us to
-  // wait for the port predicate to be set to a non-zero value from another
-  // thread in an efficient manor.
-  if (predicate)
-    predicate->SetValue(port, eBroadcastAlways);
   return std::move(listen_socket);
 }
 
 llvm::Expected<std::unique_ptr<UDPSocket>>
 Socket::UdpConnect(llvm::StringRef host_and_port,
                    bool child_processes_inherit) {
   return UDPSocket::Connect(host_and_port, child_processes_inherit);
 }

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

 }
 
 ConnectionStatus ConnectionFileDescriptor::SocketListenAndAccept(
     llvm::StringRef s,
     std::function<void(llvm::StringRef local_socket_id)> socket_id_callback,
     Status *error_ptr) {
   if (error_ptr)
     *error_ptr = Status();
-  m_port_predicate.SetValue(0, eBroadcastNever);
 
   llvm::Expected<std::unique_ptr<TCPSocket>> listening_socket =
-      Socket::TcpListen(s, m_child_processes_inherit, &m_port_predicate);
+      Socket::TcpListen(s, m_child_processes_inherit);
   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;
   }
#if LLDB_ENABLE_POSIX
   }
   m_io_sp = std::move(serial_sp.get());
 
   return eConnectionStatusSuccess;
 #endif // LLDB_ENABLE_POSIX
   llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
 }
 
-uint16_t
-ConnectionFileDescriptor::GetListeningPort(const Timeout<std::micro> &timeout) {
-  auto Result = m_port_predicate.WaitForValueNotEqualTo(0, timeout);
-  return Result ? *Result : 0;
-}
-
 bool ConnectionFileDescriptor::GetChildProcessesInherit() const {
   return m_child_processes_inherit;
 }
 
 void ConnectionFileDescriptor::SetChildProcessesInherit(
     bool child_processes_inherit) {
   m_child_processes_inherit = child_processes_inherit;
 }

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

 //===-- GDBRemoteCommunication.h --------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATION_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATION_H
 
 #include "GDBRemoteCommunicationHistory.h"
 
 #include <condition_variable>
+#include <future>
 #include <mutex>
 #include <queue>
 #include <string>
 #include <vector>
 
 #include "lldb/Core/Communication.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Host/HostThread.h"
protected:
   void AppendBytesToCache(const uint8_t *bytes, size_t len, bool broadcast,
                           lldb::ConnectionStatus status) override;
 
 private:
   std::queue<StringExtractorGDBRemote> m_packet_queue; // The packet queue
   std::mutex m_packet_queue_mutex; // Mutex for accessing queue
   std::condition_variable
       m_condition_queue_not_empty; // Condition variable to wait for packets
+  // Promise used to grab the port number from listening thread
+  std::promise<uint16_t> m_port_promise;
 
   HostThread m_listen_thread;
   std::string m_listen_url;
 
 #if defined(HAVE_LIBCOMPRESSION)
   CompressionType m_decompression_scratch_type = CompressionType::None;
   void *m_decompression_scratch = nullptr;
 #endif

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

 GDBRemoteCommunication::ListenThread(lldb::thread_arg_t arg) {
   GDBRemoteCommunication *comm = (GDBRemoteCommunication *)arg;
   Status error;
   ConnectionFileDescriptor *connection =
       (ConnectionFileDescriptor *)comm->GetConnection();
 
   if (connection) {
     // Do the listen on another thread so we can continue on...
-    if (connection->Connect(comm->m_listen_url.c_str(), &error) !=
-        eConnectionStatusSuccess)
+    if (connection->Connect(
+            comm->m_listen_url.c_str(), [comm](llvm::StringRef port_str) {
+              uint16_t port = 0;
+              llvm::to_integer(port_str, port, 10);
+              comm->m_port_promise.set_value(port);
+            },
+            &error) != eConnectionStatusSuccess)
       comm->SetConnection(nullptr);
   }
   return {};
 }
 
 Status GDBRemoteCommunication::StartDebugserverProcess(
     const char *url, Platform *platform, ProcessLaunchInfo &launch_info,
     uint16_t *port, const Args *inferior_args, int pass_comm_fd) {
#endif
         if (error.Fail()) {
           LLDB_LOGF(log,
                     "GDBRemoteCommunication::%s() unable to start listen "
                     "thread: %s",
                     __FUNCTION__, error.AsCString());
           return error;
         }
 
-        ConnectionFileDescriptor *connection =
-            (ConnectionFileDescriptor *)GetConnection();
         // Wait for 10 seconds to resolve the bound port
-        uint16_t port_ = connection->GetListeningPort(std::chrono::seconds(10));
+        std::future<uint16_t> port_future = m_port_promise.get_future();
+        uint16_t port_ = port_future.wait_for(std::chrono::seconds(10)) ==
+                                 std::future_status::ready
+                             ? port_future.get()
+                             : 0;
         if (port_ > 0) {
           char port_cstr[32];
           snprintf(port_cstr, sizeof(port_cstr), "127.0.0.1:%i", port_);
           // Send the host and port down that debugserver and specify an option
           // so that it connects back to the port we are listening to in this
           // process
           debugserver_args.AppendArgument(llvm::StringRef("--reverse-connect"));
           debugserver_args.AppendArgument(llvm::StringRef(port_cstr));

lldb/unittests/Host/SocketTest.cpp

TEST_P(SocketTest, UDPConnect) {
 
   ASSERT_THAT_EXPECTED(socket, llvm::Succeeded());
   EXPECT_TRUE(socket.get()->IsValid());
 }
 
 TEST_P(SocketTest, TCPListen0GetPort) {
   if (!HostSupportsIPv4())
     return;
-  Predicate<uint16_t> port_predicate;
-  port_predicate.SetValue(0, eBroadcastNever);
   llvm::Expected<std::unique_ptr<TCPSocket>> sock =
-      Socket::TcpListen("10.10.12.3:0", false, &port_predicate);
+      Socket::TcpListen("10.10.12.3:0", false);
   ASSERT_THAT_EXPECTED(sock, llvm::Succeeded());
   ASSERT_TRUE(sock.get()->IsValid());
   EXPECT_NE(sock.get()->GetLocalPortNumber(), 0);
 }
 
 TEST_P(SocketTest, TCPGetConnectURI) {
   std::unique_ptr<TCPSocket> socket_a_up;
   std::unique_ptr<TCPSocket> socket_b_up;

lldb/unittests/Host/SocketTestUtilities.cpp

void lldb_private::CreateDomainConnectedSockets(
   return CreateConnectedSockets<DomainSocket>(
       path, [=](const DomainSocket &) { return path.str(); }, socket_a_up,
       socket_b_up);
 }
 #endif
 
 static bool CheckIPSupport(llvm::StringRef Proto, llvm::StringRef Addr) {
   llvm::Expected<std::unique_ptr<TCPSocket>> Sock = Socket::TcpListen(
-      Addr, /*child_processes_inherit=*/false, /*predicate=*/nullptr);
+      Addr, /*child_processes_inherit=*/false);
   if (Sock)
     return true;
   llvm::Error Err = Sock.takeError();
   GTEST_LOG_(WARNING) << llvm::formatv(
                              "Creating a canary {0} TCP socket failed: {1}.",
                              Proto, Err)
                              .str();
   bool HasProtocolError = false;