[lldb] [Host] Refactor Socket::DecodeHostAndPort() to use LLVM API

[mgorny]

Summary

Refactor Socket::DecodeHostAndPort() to use LLVM API over redundant
LLDB API. In particular, this means llvm::Regex, llvm::Error return
type and llvm::StringRef.getAsInteger().

While at it, change the port type from int32_t to uint16_t. The method
never returns any value outside this range, and using the correct type
allows us to rely on getAsInteger()'s implicit overflow check.

[mgorny]

Change the return type to llvm::Error, replacing duplicate bool+Status.

[labath]

lldb/source/Host/common/Socket.cpp

305

Actually, to_integer is already an llvm API (accessed via ADL here :P), and overall I'd prefer to standardize on that, as the most common error with getAsInteger is to get the return value backwards.

(to_integer will also do an implicit StringRef conversion, so you can call it on a std::string without manual casts).

lldb/tools/lldb-server/Acceptor.cpp

101–103

if (!errorToBool(Socket::DecodeHostAndPort(...)))

[mgorny]

lldb/source/Host/common/Socket.cpp

305

Well, I personally prefer object-based API but sure, either way is fine.

lldb/tools/lldb-server/Acceptor.cpp

101–103

Note to self: if something takes more than two lines of code, LLVM probably has a helper ;-).

lldb/include/lldb/Host/Socket.h

public:
   Status Write(const void *buf, size_t &num_bytes) override;
 
   virtual Status PreDisconnect();
   Status Close() override;
 
   bool IsValid() const override { return m_socket != kInvalidSocketValue; }
   WaitableHandle GetWaitableHandle() override;
 
-  static bool DecodeHostAndPort(llvm::StringRef host_and_port,
-                                std::string &host_str, std::string &port_str,
-                                int32_t &port, Status *error_ptr);
+  static llvm::Error DecodeHostAndPort(llvm::StringRef host_and_port,
+                                       std::string &host_str,
+                                       std::string &port_str, uint16_t &port);
 
   // 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);
 

lldb/source/Host/common/Socket.cpp

 //===-- Socket.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 "lldb/Host/Socket.h"
 
 #include "lldb/Host/Config.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/SocketAddress.h"
-#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"
 
 #if LLDB_ENABLE_POSIX
 #include "lldb/Host/posix/DomainSocket.h"

 }
 
 llvm::Expected<std::unique_ptr<TCPSocket>>
 Socket::TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
                   Predicate<uint16_t> *predicate, int backlog) {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   LLDB_LOG(log, "host_and_port = {0}", host_and_port);
 
-  Status error;
   std::string host_str;
   std::string port_str;
-  int32_t port = INT32_MIN;
-  if (!DecodeHostAndPort(host_and_port, host_str, port_str, port, &error))
-    return error.ToError();
+  uint16_t port;
+  if (llvm::Error decode_error =
+          DecodeHostAndPort(host_and_port, host_str, port_str, port))
+    return decode_error;
 
   std::unique_ptr<TCPSocket> listen_socket(
       new TCPSocket(true, child_processes_inherit));
 
-  error = listen_socket->Listen(host_and_port, backlog);
+  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();
Status Socket::UnixAbstractAccept(llvm::StringRef name,
   error = listen_socket->Listen(name, 5);
   if (error.Fail())
     return error;
 
   error = listen_socket->Accept(socket);
   return error;
 }
 
-bool Socket::DecodeHostAndPort(llvm::StringRef host_and_port,
-                               std::string &host_str, std::string &port_str,
-                               int32_t &port, Status *error_ptr) {
-  static RegularExpression g_regex(
-      llvm::StringRef("([^:]+|\\[[0-9a-fA-F:]+.*\\]):([0-9]+)"));
+llvm::Error Socket::DecodeHostAndPort(llvm::StringRef host_and_port,
+                                      std::string &host_str,
+                                      std::string &port_str, uint16_t &port) {
+  static llvm::Regex g_regex("([^:]+|\\[[0-9a-fA-F:]+.*\\]):([0-9]+)");
   llvm::SmallVector<llvm::StringRef, 3> matches;
-  if (g_regex.Execute(host_and_port, &matches)) {
+  if (g_regex.match(host_and_port, &matches)) {
     host_str = matches[1].str();
     port_str = matches[2].str();
     // IPv6 addresses are wrapped in [] when specified with ports
     if (host_str.front() == '[' && host_str.back() == ']')
       host_str = host_str.substr(1, host_str.size() - 2);
-    bool ok = false;
-    port = StringConvert::ToUInt32(port_str.c_str(), UINT32_MAX, 10, &ok);
-    if (ok && port <= UINT16_MAX) {
-      if (error_ptr)
-        error_ptr->Clear();
-      return true;
-    }
-    // port is too large
-    if (error_ptr)
-      error_ptr->SetErrorStringWithFormat(
-          "invalid host:port specification: '%s'", host_and_port.str().c_str());
-    return false;
-  }
-
+    if (to_integer(matches[2], port, 10))
+      return llvm::Error::success();
+  } else {
     // If this was unsuccessful, then check if it's simply a signed 32-bit
     // integer, representing a port with an empty host.
     host_str.clear();
     port_str.clear();
-  if (to_integer(host_and_port, port, 10) && port < UINT16_MAX) {
-    port_str = std::string(host_and_port);
-    if (error_ptr)
-      error_ptr->Clear();
-    return true;
+    if (to_integer(host_and_port, port, 10)) {
+      port_str = host_and_port.str();
+      return llvm::Error::success();
+    }
   }
 
-  if (error_ptr)
-    error_ptr->SetErrorStringWithFormat("invalid host:port specification: '%s'",
-                                        host_and_port.str().c_str());
-  return false;
+  return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                 "invalid host:port specification: '%s'",
 }
 
 IOObject::WaitableHandle Socket::GetWaitableHandle() {
   // TODO: On Windows, use WSAEventSelect
   return m_socket;
 }
 
 Status Socket::Read(void *buf, size_t &num_bytes) {

lldb/source/Host/common/TCPSocket.cpp

 Status TCPSocket::Connect(llvm::StringRef name) {
 
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_COMMUNICATION));
   LLDB_LOGF(log, "TCPSocket::%s (host/port = %s)", __FUNCTION__, name.data());
 
   Status error;
   std::string host_str;
   std::string port_str;
-  int32_t port = INT32_MIN;
-  if (!DecodeHostAndPort(name, host_str, port_str, port, &error))
-    return error;
+  uint16_t port;
+  if (llvm::Error decode_error =
+          DecodeHostAndPort(name, host_str, port_str, port))
+    return Status(std::move(decode_error));
 
   std::vector<SocketAddress> addresses = SocketAddress::GetAddressInfo(
       host_str.c_str(), nullptr, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
   for (SocketAddress &address : addresses) {
     error = CreateSocket(address.GetFamily());
     if (error.Fail())
       continue;
 

 
 Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   LLDB_LOGF(log, "TCPSocket::%s (%s)", __FUNCTION__, name.data());
 
   Status error;
   std::string host_str;
   std::string port_str;
-  int32_t port = INT32_MIN;
-  if (!DecodeHostAndPort(name, host_str, port_str, port, &error))
-    return error;
+  uint16_t port;
+  if (llvm::Error decode_error =
+          DecodeHostAndPort(name, host_str, port_str, port))
+    return Status(std::move(decode_error));
 
   if (host_str == "*")
     host_str = "0.0.0.0";
   std::vector<SocketAddress> addresses = SocketAddress::GetAddressInfo(
       host_str.c_str(), nullptr, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
   for (SocketAddress &address : addresses) {
     int fd = Socket::CreateSocket(address.GetFamily(), kType, IPPROTO_TCP,
                                   m_child_processes_inherit, error);

lldb/source/Host/common/UDPSocket.cpp

UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit) {
   std::unique_ptr<UDPSocket> socket;
 
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   LLDB_LOG(log, "host/port = {0}", name);
 
   Status error;
   std::string host_str;
   std::string port_str;
-  int32_t port = INT32_MIN;
-  if (!DecodeHostAndPort(name, host_str, port_str, port, &error))
-    return error.ToError();
+  uint16_t port;
+  if (llvm::Error decode_error =
+          DecodeHostAndPort(name, host_str, port_str, port))
+    return decode_error;
 
   // At this point we have setup the receive port, now we need to setup the UDP
   // send socket
 
   struct addrinfo hints;
   struct addrinfo *service_info_list = nullptr;
 
   ::memset(&hints, 0, sizeof(hints));

lldb/tools/lldb-server/Acceptor.cpp

if (UriParser::Parse(name, scheme, host, port, path)) {
     if (!FindProtocolByScheme(scheme.str().c_str(), socket_protocol))
       error.SetErrorStringWithFormat("Unknown protocol scheme \"%s\"",
                                      scheme.str().c_str());
     else
       name = name.drop_front(scheme.size() + strlen("://"));
   } else {
     std::string host_str;
     std::string port_str;
-    int32_t port = INT32_MIN;
+    uint16_t port;
     // Try to match socket name as $host:port - e.g., localhost:5555
-    if (Socket::DecodeHostAndPort(name, host_str, port_str, port, nullptr))
+    if (!llvm::errorToBool(
+            Socket::DecodeHostAndPort(name, host_str, port_str, port)))
       socket_protocol = Socket::ProtocolTcp;
   }
 
   if (error.Fail())
     return std::unique_ptr<Acceptor>();
 
   std::unique_ptr<Socket> listener_socket_up =
       Socket::Create(socket_protocol, child_processes_inherit, error);
 

lldb/unittests/Host/SocketTest.cpp

 //===-- SocketTest.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 "SocketTestUtilities.h"
 #include "TestingSupport/SubsystemRAII.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Utility/UriParser.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
 
 struct SocketTestParams {
   bool is_ipv6;
   std::string localhost_ip;
 };
 
 class SocketTest : public testing::TestWithParam<SocketTestParams> {
 public:
   SubsystemRAII<Socket> subsystems;
 
 protected:
   bool HostSupportsProtocol() const {
     if (GetParam().is_ipv6)
       return HostSupportsIPv6();
     return HostSupportsIPv4();
   }
 };
 
 TEST_P(SocketTest, DecodeHostAndPort) {
   std::string host_str;
   std::string port_str;
-  int32_t port;
-  Status error;
-  EXPECT_TRUE(Socket::DecodeHostAndPort("localhost:1138", host_str, port_str,
-                                        port, &error));
+  uint16_t port;
+
+  EXPECT_THAT_ERROR(
+      Socket::DecodeHostAndPort("localhost:1138", host_str, port_str, port),
+      llvm::Succeeded());
   EXPECT_STREQ("localhost", host_str.c_str());
   EXPECT_STREQ("1138", port_str.c_str());
   EXPECT_EQ(1138, port);
-  EXPECT_TRUE(error.Success());
-
-  EXPECT_FALSE(Socket::DecodeHostAndPort("google.com:65536", host_str, port_str,
-                                         port, &error));
-  EXPECT_TRUE(error.Fail());
-  EXPECT_STREQ("invalid host:port specification: 'google.com:65536'",
-               error.AsCString());
-
-  EXPECT_FALSE(Socket::DecodeHostAndPort("google.com:-1138", host_str, port_str,
-                                         port, &error));
-  EXPECT_TRUE(error.Fail());
-  EXPECT_STREQ("invalid host:port specification: 'google.com:-1138'",
-               error.AsCString());
-
-  EXPECT_FALSE(Socket::DecodeHostAndPort("google.com:65536", host_str, port_str,
-                                         port, &error));
-  EXPECT_TRUE(error.Fail());
-  EXPECT_STREQ("invalid host:port specification: 'google.com:65536'",
-               error.AsCString());
 
-  EXPECT_TRUE(
-      Socket::DecodeHostAndPort("12345", host_str, port_str, port, &error));
+  EXPECT_THAT_ERROR(
+      Socket::DecodeHostAndPort("google.com:65536", host_str, port_str, port),
+      llvm::FailedWithMessage(
+          "invalid host:port specification: 'google.com:65536'"));
+
+  EXPECT_THAT_ERROR(
+      Socket::DecodeHostAndPort("google.com:-1138", host_str, port_str, port),
+      llvm::FailedWithMessage(
+          "invalid host:port specification: 'google.com:-1138'"));
+
+  EXPECT_THAT_ERROR(
+      Socket::DecodeHostAndPort("google.com:65536", host_str, port_str, port),
+      llvm::FailedWithMessage(
+          "invalid host:port specification: 'google.com:65536'"));
+
+  EXPECT_THAT_ERROR(
+      Socket::DecodeHostAndPort("12345", host_str, port_str, port),
+      llvm::Succeeded());
   EXPECT_STREQ("", host_str.c_str());
   EXPECT_STREQ("12345", port_str.c_str());
   EXPECT_EQ(12345, port);
-  EXPECT_TRUE(error.Success());
 
-  EXPECT_TRUE(
-      Socket::DecodeHostAndPort("*:0", host_str, port_str, port, &error));
+  EXPECT_THAT_ERROR(Socket::DecodeHostAndPort("*:0", host_str, port_str, port),
+                    llvm::Succeeded());
   EXPECT_STREQ("*", host_str.c_str());
   EXPECT_STREQ("0", port_str.c_str());
   EXPECT_EQ(0, port);
-  EXPECT_TRUE(error.Success());
 
-  EXPECT_TRUE(
-      Socket::DecodeHostAndPort("*:65535", host_str, port_str, port, &error));
+  EXPECT_THAT_ERROR(
+      Socket::DecodeHostAndPort("*:65535", host_str, port_str, port),
+      llvm::Succeeded());
   EXPECT_STREQ("*", host_str.c_str());
   EXPECT_STREQ("65535", port_str.c_str());
   EXPECT_EQ(65535, port);
-  EXPECT_TRUE(error.Success());
 
-  EXPECT_TRUE(
-      Socket::DecodeHostAndPort("[::1]:12345", host_str, port_str, port, &error));
+  EXPECT_THAT_ERROR(
+      Socket::DecodeHostAndPort("[::1]:12345", host_str, port_str, port),
+      llvm::Succeeded());
   EXPECT_STREQ("::1", host_str.c_str());
   EXPECT_STREQ("12345", port_str.c_str());
   EXPECT_EQ(12345, port);
-  EXPECT_TRUE(error.Success());
 
-  EXPECT_TRUE(
-      Socket::DecodeHostAndPort("[abcd:12fg:AF58::1]:12345", host_str, port_str, port, &error));
+  EXPECT_THAT_ERROR(Socket::DecodeHostAndPort("[abcd:12fg:AF58::1]:12345",
+                                              host_str, port_str, port),
+                    llvm::Succeeded());
   EXPECT_STREQ("abcd:12fg:AF58::1", host_str.c_str());
   EXPECT_STREQ("12345", port_str.c_str());
   EXPECT_EQ(12345, port);
-  EXPECT_TRUE(error.Success());
 }
 
 #if LLDB_ENABLE_POSIX
 TEST_P(SocketTest, DomainListenConnectAccept) {
   llvm::SmallString<64> Path;
   std::error_code EC = llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
   ASSERT_FALSE(EC);
   llvm::sys::path::append(Path, "test");