[lldb][NFC] Move few static helpers to the class Socket

[slydiman]

It is just an optimization.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

This is the prerequisite for #104238.

---

Full diff: https://github.com/llvm/llvm-project/pull/106640.diff

3 Files Affected:

• (modified) lldb/include/lldb/Host/Socket.h (+10-6)
• (modified) lldb/source/Host/common/Socket.cpp (+19-5)
• (modified) lldb/source/Host/common/TCPSocket.cpp (+5-24)

diff --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index 304a91bdf6741b..4c2f07b796d615 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -34,9 +34,11 @@ namespace lldb_private {
 #if defined(_WIN32)
 typedef SOCKET NativeSocket;
 typedef lldb::pipe_t shared_fd_t;
+typedef const char *set_socket_option_arg_type;
 #else
 typedef int NativeSocket;
 typedef NativeSocket shared_fd_t;
+typedef const void *set_socket_option_arg_type;
 #endif
 class Socket;
 class TCPSocket;
@@ -132,12 +134,8 @@ class Socket : public IOObject {
   // If this Socket is connected then return the URI used to connect.
   virtual std::string GetRemoteConnectionURI() const { return ""; };
 
-protected:
-  Socket(SocketProtocol protocol, bool should_close,
-         bool m_child_process_inherit);
-
-  virtual size_t Send(const void *buf, const size_t num_bytes);
-
+  static int CloseSocket(NativeSocket sockfd);
+  static Status GetLastError();
   static void SetLastError(Status &error);
   static NativeSocket CreateSocket(const int domain, const int type,
                                    const int protocol,
@@ -146,6 +144,12 @@ class Socket : public IOObject {
                                    socklen_t *addrlen,
                                    bool child_processes_inherit, Status &error);
 
+protected:
+  Socket(SocketProtocol protocol, bool should_close,
+         bool m_child_process_inherit);
+
+  virtual size_t Send(const void *buf, const size_t num_bytes);
+
   SocketProtocol m_protocol;
   NativeSocket m_socket;
   bool m_child_processes_inherit;
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index 1a506aa95b2465..62473a79e290e8 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -386,11 +386,7 @@ Status Socket::Close() {
   LLDB_LOGF(log, "%p Socket::Close (fd = %" PRIu64 ")",
             static_cast<void *>(this), static_cast<uint64_t>(m_socket));
 
-#if defined(_WIN32)
-  bool success = closesocket(m_socket) == 0;
-#else
-  bool success = ::close(m_socket) == 0;
-#endif
+  bool success = CloseSocket(m_socket) == 0;
   // A reference to a FD was passed in, set it to an invalid value
   m_socket = kInvalidSocketValue;
   if (!success) {
@@ -427,6 +423,24 @@ void Socket::SetLastError(Status &error) {
 #endif
 }
 
+Status Socket::GetLastError() {
+  std::error_code EC;
+#ifdef _WIN32
+  EC = llvm::mapWindowsError(WSAGetLastError());
+#else
+  EC = std::error_code(errno, std::generic_category());
+#endif
+  return EC;
+}
+
+int Socket::CloseSocket(NativeSocket sockfd) {
+#ifdef _WIN32
+  return ::closesocket(sockfd);
+#else
+  return ::close(sockfd);
+#endif
+}
+
 NativeSocket Socket::CreateSocket(const int domain, const int type,
                                   const int protocol,
                                   bool child_processes_inherit, Status &error) {
diff --git a/lldb/source/Host/common/TCPSocket.cpp b/lldb/source/Host/common/TCPSocket.cpp
index ea26d8433c370a..86c6ff510e2db8 100644
--- a/lldb/source/Host/common/TCPSocket.cpp
+++ b/lldb/source/Host/common/TCPSocket.cpp
@@ -32,28 +32,9 @@
 #include <winsock2.h>
 #endif
 
-#ifdef _WIN32
-#define CLOSE_SOCKET closesocket
-typedef const char *set_socket_option_arg_type;
-#else
-#include <unistd.h>
-#define CLOSE_SOCKET ::close
-typedef const void *set_socket_option_arg_type;
-#endif
-
 using namespace lldb;
 using namespace lldb_private;
 
-static Status GetLastSocketError() {
-  std::error_code EC;
-#ifdef _WIN32
-  EC = llvm::mapWindowsError(WSAGetLastError());
-#else
-  EC = std::error_code(errno, std::generic_category());
-#endif
-  return EC;
-}
-
 static const int kType = SOCK_STREAM;
 
 TCPSocket::TCPSocket(bool should_close, bool child_processes_inherit)
@@ -212,7 +193,7 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
         reinterpret_cast<set_socket_option_arg_type>(&option_value);
     if (::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
                      sizeof(option_value)) == -1) {
-      CLOSE_SOCKET(fd);
+      CloseSocket(fd);
       continue;
     }
 
@@ -228,8 +209,8 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
       err = ::listen(fd, backlog);
 
     if (err == -1) {
-      error = GetLastSocketError();
-      CLOSE_SOCKET(fd);
+      error = GetLastError();
+      CloseSocket(fd);
       continue;
     }
 
@@ -250,7 +231,7 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
 
 void TCPSocket::CloseListenSockets() {
   for (auto socket : m_listen_sockets)
-    CLOSE_SOCKET(socket.first);
+    CloseSocket(socket.first);
   m_listen_sockets.clear();
 }
 
@@ -295,7 +276,7 @@ Status TCPSocket::Accept(Socket *&conn_socket) {
     lldb_private::SocketAddress &AddrIn = m_listen_sockets[listen_sock];
     if (!AddrIn.IsAnyAddr() && AcceptAddr != AddrIn) {
       if (sock != kInvalidSocketValue) {
-        CLOSE_SOCKET(sock);
+        CloseSocket(sock);
         sock = kInvalidSocketValue;
       }
       llvm::errs() << llvm::formatv(

[labath]

I don't think this is a good change.

[slydiman]

I don't think this is a good change.

What exactly is wrong here?

Is that code good duplicated in Socket.cpp and TCPSocket.cpp?

#if defined(_WIN32)
  bool success = closesocket(m_socket) == 0;
#else
  bool success = ::close(m_socket) == 0;
#endif

SetLastError(Status &error) is placed in Socket, but GetLastError() in TCPSocket. Is it ok?

[labath]

Moving the common API (deduplicating) into the Socket class is fine. Making it public is not. There shouldn't be a reason to make e.g. GetLastError public, because noone should be making raw socket calls.

[labath]

Instead of exposing this quirk, it would be better to have a static version of Socket::SetOption to hide it.

[slydiman]

Updated. Fixed also a typo
set_socket_option_arg_type option_value_p =
reinterpret_cast<get_socket_option_arg_type>(&option_value);

[labath]

cool. thanks.