[lldb] [lldb-server] Refactor ConnectToRemote()

[mgorny]

Summary

Refactor ConnectToRemote() to improve readability and make future
changes easier:

1. Replace static buffers with std::string.
2. When handling errors, prefer reporting the actual error over dumb 'connection status is not success'.
3. Move host/port parsing directly into reverse_connection condition that is its only user, and simplify it to make its purpose (verifying that a valid port is provided) clear.

[labath]

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

216–220

maybe replace with llvm::errs(), while you're in there?

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

void ConnectToRemote(MainLoop &mainloop,
                      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;
   if (connection_fd != -1) {
     // Build the connection string.
-    char connection_url[512];
-    snprintf(connection_url, sizeof(connection_url), "fd://%d", connection_fd);
+    std::string connection_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 (connection_result != eConnectionStatusSuccess) {
-      fprintf(stderr, "error: failed to connect to client at '%s' "
-                      "(connection status: %d)\n",
-              connection_url, static_cast<int>(connection_result));
+    if (error.Fail()) {
+      llvm::errs() << llvm::formatv(
+          "error: failed to connect to client at '{0}': {1}\n", connection_url,
+          error);
       exit(-1);
     }
-    if (error.Fail()) {
-      fprintf(stderr, "error: failed to connect to client at '%s': %s\n",
-              connection_url, error.AsCString());
+    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));
       exit(-1);
     }
   } else if (!host_and_port.empty()) {
     // Parse out host and port.
     std::string final_host_and_port;
-    std::string connection_host;
-    std::string connection_port;
-    uint32_t connection_portno = 0;
 
     // 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());
 
-    // Note: use rfind, because the host/port may look like "[::1]:12345".
-    const std::string::size_type colon_pos = final_host_and_port.rfind(':');
-    if (colon_pos != std::string::npos) {
-      connection_host = final_host_and_port.substr(0, colon_pos);
-      connection_port = final_host_and_port.substr(colon_pos + 1);
-      // FIXME: improve error handling
-      llvm::to_integer(connection_port, connection_portno);
-    }
-
-
     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) {
-        fprintf(stderr, "error: port number must be specified on when using "
-                        "reverse connect\n");
+        llvm::errs() << "error: port number must be specified on when using "
+                        "reverse connect\n";
         exit(1);
       }
 
       // Build the connection string.
-      char connection_url[512];
-      snprintf(connection_url, sizeof(connection_url), "connect://%s",
-               final_host_and_port.c_str());
+      final_host_and_port.insert(0, "connect://");
 
       // Create the connection.
       connection_up.reset(new ConnectionFileDescriptor);
-      auto connection_result = connection_up->Connect(connection_url, &error);
-      if (connection_result != eConnectionStatusSuccess) {
-        fprintf(stderr, "error: failed to connect to client at '%s' "
-                        "(connection status: %d)\n",
-                connection_url, static_cast<int>(connection_result));
+      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 (error.Fail()) {
-        fprintf(stderr, "error: failed to connect to client at '%s': %s\n",
-                connection_url, error.AsCString());
+      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()) {
-        fprintf(stderr, "failed to create acceptor: %s\n", error.AsCString());
+        llvm::errs() << llvm::formatv("failed to create acceptor: {0}\n",
+                                      error);
         exit(1);
       }
       error = acceptor_up->Listen(1);
       if (error.Fail()) {
-        fprintf(stderr, "failed to listen: %s\n", error.AsCString());
+        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 (named_pipe_path && named_pipe_path[0]) {
           error = writeSocketIdToPipe(named_pipe_path, socket_id);
           if (error.Fail())
-            fprintf(stderr, "failed to write to the named pipe \'%s\': %s\n",
-                    named_pipe_path, error.AsCString());
+            llvm::errs() << llvm::formatv(
+                "failed to write to the named peipe '{0}': {1}\n",
         }
         // 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);
           if (error.Fail())
-            fprintf(stderr, "failed to write to the unnamed pipe: %s\n",
-                    error.AsCString());
+            llvm::errs() << llvm::formatv(
+                "failed to write to the unnamed pipe: {0}\n", error);
         }
       } else {
-        fprintf(stderr,
-                "unable to get the socket id for the listening connection\n");
+        llvm::errs()
+            << "unable to get the socket id for the listening connection\n";
       }
 
       Connection *conn = nullptr;
       error = acceptor_up->Accept(false, conn);
       if (error.Fail()) {
-        printf("failed to accept new connection: %s\n", error.AsCString());
+        llvm::errs() << llvm::formatv("failed to accept new connection: {0}\n",
+                                      error);
         exit(1);
       }
       connection_up.reset(conn);
     }
   }
   error = gdb_server.InitializeConnection(std::move(connection_up));
   if (error.Fail()) {
-    fprintf(stderr, "Failed to initialize connection: %s\n",
-            error.AsCString());
+    llvm::errs() << llvm::formatv("failed to initialize connection\n", error);
     exit(-1);
   }
-  printf("Connection established.\n");
+  llvm::outs() << "Connection established.\n";
 }
 
 namespace {
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
                HELPTEXT, METAVAR, VALUES)                                      \
   OPT_##ID,