[lldb-server] Introduce Socket::Initialize and Terminate to simply WSASocket setup

[labath]

Thank you for doing this. We probably should have done something like this a long time ago. I have a couple of inline comments, but they're all very trivial. Feel free to commit, if you agree with them.

source/Host/common/Socket.cpp

95–96

I'd make this llvm::errorCodeToError(llvm::mapWindowsError(::WSAGetLastError())); as it's somewhat shorter and hopefully produces a better error message.

103

we don't use (void) in functions taking no arguments.

source/Initialization/SystemInitializerCommon.cpp

123–126

These calls should be roughly in dependency order. Since Sockets are in Host, I'd probably put this somewhere around the HostInfo::Initialize call on line 93. (same goes for the terminate call below)

unittests/Host/MainLoopTest.cpp

23

ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded()). That way, if this ever fails, you'll get the actual error message instead of "false is not true". (you might need to include something to have these available).

[asmith]

Addressed the review comments

include/lldb/Host/Socket.h

typedef enum {
     ProtocolUnixDomain,
     ProtocolUnixAbstract
   } SocketProtocol;
 
   static const NativeSocket kInvalidSocketValue;
 
   ~Socket() override;
 
+  static llvm::Error Initialize();
+  static void Terminate();
+
   static std::unique_ptr<Socket> Create(const SocketProtocol protocol,
                                         bool child_processes_inherit,
                                         Status &error);
 
   virtual Status Connect(llvm::StringRef name) = 0;
   virtual Status Listen(llvm::StringRef name, int backlog) = 0;
   virtual Status Accept(Socket *&socket) = 0;
 

source/Host/common/Socket.cpp

 #include "lldb/Host/StringConvert.h"
 #include "lldb/Host/common/TCPSocket.h"
 #include "lldb/Host/common/UDPSocket.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegularExpression.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/WindowsError.h"
 
 #ifndef LLDB_DISABLE_POSIX
 #include "lldb/Host/posix/DomainSocket.h"
 
 #include <arpa/inet.h>
 #include <netdb.h>
 #include <netinet/in.h>
 #include <netinet/tcp.h>

 Socket::Socket(SocketProtocol protocol, bool should_close,
                bool child_processes_inherit)
     : IOObject(eFDTypeSocket, should_close), m_protocol(protocol),
       m_socket(kInvalidSocketValue),
       m_child_processes_inherit(child_processes_inherit) {}
 
 Socket::~Socket() { Close(); }
 
+llvm::Error Socket::Initialize() {
+#if defined(_WIN32)
+  auto wVersion = WINSOCK_VERSION;
+  WSADATA wsaData;
+  int err = ::WSAStartup(wVersion, &wsaData);
+  if (err == 0) {
+    if (wsaData.wVersion < wVersion) {
+      WSACleanup();
+      return llvm::make_error<llvm::StringError>(
+          "WSASock version is not expected.", llvm::inconvertibleErrorCode());
+    }
+  } else {
+    return llvm::errorCodeToError(llvm::mapWindowsError(::WSAGetLastError()));
+  }
+#endif
+
+  return llvm::Error::success();
+}
+
+void Socket::Terminate() {
+#if defined(_WIN32)
+  ::WSACleanup();
+#endif
+}
+
 std::unique_ptr<Socket> Socket::Create(const SocketProtocol protocol,
                                        bool child_processes_inherit,
                                        Status &error) {
   error.Clear();
 
   std::unique_ptr<Socket> socket_up;
   switch (protocol) {
   case ProtocolTcp:

source/Initialization/SystemInitializerCommon.cpp

 #include "Plugins/Instruction/MIPS/EmulateInstructionMIPS.h"
 #include "Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.h"
 #include "Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.h"
 #include "Plugins/ObjectContainer/Universal-Mach-O/ObjectContainerUniversalMachO.h"
 #include "Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Host/Socket.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/Timer.h"
 
 #if defined(__linux__) || defined(__FreeBSD__) || defined(__NetBSD__)
 #include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
 #endif
 
if (repro::Loader *loader = r.GetLoader()) {
     repro::FileProvider &fp = g->GetOrCreate<repro::FileProvider>();
     FileSystem::Initialize(fp.GetFileCollector());
   } else {
     FileSystem::Initialize();
   }
 
   Log::Initialize();
   HostInfo::Initialize();
+
+  llvm::Error error = Socket::Initialize();
+  if (error)
+    return error;
+
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(func_cat, LLVM_PRETTY_FUNCTION);
 
   process_gdb_remote::ProcessGDBRemoteLog::Initialize();
 
   // Initialize plug-ins
   ObjectContainerBSDArchive::Initialize();
 
   EmulateInstructionARM::Initialize();
   EmulateInstructionMIPS::Initialize();
   EmulateInstructionMIPS64::Initialize();
 
   //----------------------------------------------------------------------
   // Apple/Darwin hosted plugins
   //----------------------------------------------------------------------
   ObjectContainerUniversalMachO::Initialize();
 
 #if defined(__linux__) || defined(__FreeBSD__) || defined(__NetBSD__)
   ProcessPOSIXLog::Initialize();
 #endif
 #if defined(_MSC_VER)
   ProcessWindowsLog::Initialize();
 #endif
 
   return llvm::Error::success();
 }
 
 void SystemInitializerCommon::Terminate() {
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(func_cat, LLVM_PRETTY_FUNCTION);
   ObjectContainerBSDArchive::Terminate();
 
   EmulateInstructionARM::Terminate();
   EmulateInstructionMIPS::Terminate();
   EmulateInstructionMIPS64::Terminate();
 
   ObjectContainerUniversalMachO::Terminate();
 
 #if defined(_MSC_VER)
   ProcessWindowsLog::Terminate();
 #endif
 
+  Socket::Terminate();
   HostInfo::Terminate();
   Log::DisableAllLogChannels();
   FileSystem::Terminate();
   Reproducer::Terminate();
 }

source/Plugins/Platform/Windows/PlatformWindows.cpp

lldb_private::ConstString PlatformWindows::GetPluginName() {
   return GetPluginNameStatic(IsHost());
 }
 
 void PlatformWindows::Initialize() {
   Platform::Initialize();
 
   if (g_initialize_count++ == 0) {
 #if defined(_WIN32)
-    WSADATA dummy;
-    WSAStartup(MAKEWORD(2, 2), &dummy);
     // Force a host flag to true for the default platform object.
     PlatformSP default_platform_sp(new PlatformWindows(true));
     default_platform_sp->SetSystemArchitecture(HostInfo::GetArchitecture());
     Platform::SetHostPlatform(default_platform_sp);
 #endif
     PluginManager::RegisterPlugin(
         PlatformWindows::GetPluginNameStatic(false),
         PlatformWindows::GetPluginDescriptionStatic(false),
         PlatformWindows::CreateInstance);
   }
 }
 
-void PlatformWindows::Terminate(void) {
+void PlatformWindows::Terminate() {
   if (g_initialize_count > 0) {
     if (--g_initialize_count == 0) {
-#ifdef _WIN32
-      WSACleanup();
-#endif
       PluginManager::UnregisterPlugin(PlatformWindows::CreateInstance);
     }
   }
 
   Platform::Terminate();
 }
 
 //------------------------------------------------------------------

unittests/Host/MainLoopTest.cpp

 //===-- MainLoopTest.cpp ----------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/MainLoop.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Host/common/TCPSocket.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 #include <future>
 
 using namespace lldb_private;
 
 namespace {
 class MainLoopTest : public testing::Test {
 public:
   static void SetUpTestCase() {
-#ifdef _MSC_VER
-    WSADATA data;
-    ASSERT_EQ(0, WSAStartup(MAKEWORD(2, 2), &data));
-#endif
+    ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
   }
 
-  static void TearDownTestCase() {
-#ifdef _MSC_VER
-    ASSERT_EQ(0, WSACleanup());
-#endif
-  }
+  static void TearDownTestCase() { Socket::Terminate(); }
 
   void SetUp() override {
     bool child_processes_inherit = false;
     Status error;
     std::unique_ptr<TCPSocket> listen_socket_up(
         new TCPSocket(true, child_processes_inherit));
     ASSERT_TRUE(error.Success());
     error = listen_socket_up->Listen("localhost:0", 5);

unittests/Host/SocketAddressTest.cpp

 //===-- SocketAddressTest.cpp -----------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
+#include "lldb/Host/SocketAddress.h"
+#include "lldb/Host/Socket.h"
+#include "llvm/Testing/Support/Error.h"
+
 #include "gtest/gtest.h"
 
-#include "lldb/Host/SocketAddress.h"
+using namespace lldb_private;
 
 namespace {
 class SocketAddressTest : public testing::Test {
 public:
   static void SetUpTestCase() {
-#ifdef _MSC_VER
-    WSADATA data;
-    ASSERT_EQ(0, WSAStartup(MAKEWORD(2, 2), &data));
-#endif
-  }
-  static void TearDownTestCase() {
-#ifdef _MSC_VER
-    ASSERT_EQ(0, WSACleanup());
-#endif
+    ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
   }
+  static void TearDownTestCase() { Socket::Terminate(); }
 };
 } // namespace
 
-using namespace lldb_private;
-
 TEST_F(SocketAddressTest, Set) {
   SocketAddress sa;
   ASSERT_TRUE(sa.SetToLocalhost(AF_INET, 1138));
   ASSERT_STREQ("127.0.0.1", sa.GetIPAddress().c_str());
   ASSERT_EQ(1138, sa.GetPort());
 
   ASSERT_TRUE(sa.SetToAnyAddress(AF_INET, 0));
   ASSERT_STREQ("0.0.0.0", sa.GetIPAddress().c_str());

unittests/Host/SocketTest.cpp

 #include "gtest/gtest.h"
 
 #include "lldb/Host/Config.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Host/common/TCPSocket.h"
 #include "lldb/Host/common/UDPSocket.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Error.h"
 
 #ifndef LLDB_DISABLE_POSIX
 #include "lldb/Host/posix/DomainSocket.h"
 #endif
 
 using namespace lldb_private;
 
 class SocketTest : public testing::Test {
 public:
   void SetUp() override {
-#if defined(_MSC_VER)
-    WSADATA data;
-    ::WSAStartup(MAKEWORD(2, 2), &data);
-#endif
+    ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
   }
 
-  void TearDown() override {
-#if defined(_MSC_VER)
-    ::WSACleanup();
-#endif
-  }
+  void TearDown() override { Socket::Terminate(); }
 
 protected:
   static void AcceptThread(Socket *listen_socket,
                            bool child_processes_inherit, Socket **accept_socket,
                            Status *error) {
     *error = listen_socket->Accept(*accept_socket);
   }
 

unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp

 //===-- GDBRemoteTestUtils.cpp ----------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "GDBRemoteTestUtils.h"
-
-#if defined(_MSC_VER)
-#include "lldb/Host/windows/windows.h"
-#include <WinSock2.h>
-#endif
+#include "lldb/Host/Socket.h"
+#include "llvm/Testing/Support/Error.h"
 
 namespace lldb_private {
 namespace process_gdb_remote {
 
 void GDBRemoteTest::SetUpTestCase() {
-#if defined(_MSC_VER)
-  WSADATA data;
-  ::WSAStartup(MAKEWORD(2, 2), &data);
-#endif
+  ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
 }
 
-void GDBRemoteTest::TearDownTestCase() {
-#if defined(_MSC_VER)
-  ::WSACleanup();
-#endif
-}
+void GDBRemoteTest::TearDownTestCase() { Socket::Terminate(); }
 
 } // namespace process_gdb_remote
 } // namespace lldb_private