[llvm-jitlink] Add diagnostic output and port executor to getaddrinfo(3) as well

[sgraenitz]

Summary

Add diagnostic output for TCP connections on both sides, llvm-jitlink and llvm-jitlink-executor.
Port the executor to use getaddrinfo(3) as well. This makes the code more symmetric and seems to be the recommended way for implementing the server side.

[sgraenitz]

Follow-up patch that submits learnings from fixing D98579. The error it fixed would now print as:
Failed to connect TCP socket 'localhost:13921': Address resolution failed (Bad hints)

[sgraenitz]

Some sample output for llvm-jitlink with various broken addresses:
Failed to connect TCP socket 'localhost:1': Connection refused (inactive)
Failed to connect TCP socket '8.8.8.8:1': Operation timed out (non-responding)
Failed to connect TCP socket '127.0.0.1:0': Can't assign requested address
Failed to connect TCP socket '127.0.0.1:-1': Address resolution failed (nodename nor servname provided, or not known)
Failed to connect TCP socket '127.0.0.1': Port for -oop-executor-connect can not be empty

Running llvm-jitlink-executor on a port that is busy:
Error on binding: Address already in use

In success case llvm-jitlink-executor dumps some convenient server'ish output:

> bin/llvm-jitlink-executor listen=localhost:13921
Listening at localhost:13921
Connection established. Running OrcRPCTPCServer...

[sgraenitz]

Print 'Connection established' only when connecting via a TCP socket

[rzurob]

llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp

94

This line fails compilation on AIX in 64-bit mode:

llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp:93:10: error: no matching function for call to 'accept'
  return accept(SockFD, AI->ai_addr, &AI->ai_addrlen);
         ^~~~~~
/usr/include/sys/socket.h:635:9: note: candidate function not viable: no known conversion from 'size_t *' (aka 'unsigned long *') to 'socklen_t *' (aka 'unsigned int *') for 3rd argument
int     accept(int, struct sockaddr *__restrict__, socklen_t *__restrict__);
        ^
1 error generated.
Error while processing llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp.

The size is always going to fit into an unsigned int. So perhaps we need to store AI->ai_addrlen into a socklen_t temp first?

[sgraenitz]

Add special-case for calling accept(2) on AIX. I cannot test it here, @rzurob does this work for you?

llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp

94

Interesting, the ai_addrlen field should be of type socklen_t naturally:

struct addrinfo {
  int              ai_flags;
  int              ai_family;
  int              ai_socktype;
  int              ai_protocol;
  socklen_t        ai_addrlen;
  struct sockaddr *ai_addr;
  char            *ai_canonname;
  struct addrinfo *ai_next;
};

Do you think it's fine to keep this as a special-case for AIX? Does the connect call on the client side in D98579 suffer from the same issue?

[rzurob]

LGTM

llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp

94

You're right that the AIX header is not declaring ai_addrlen as what POSIX says. I've reported it to them.
In the meantime, I confirmed that the AIX workaround you added works. Thanks!

[sgraenitz]

Thanks for the confirmation. I will land this as soon as D98579 is done.

[thakis]

This breaks the build on Windows: http://45.33.8.238/win/35472/step_4.txt

Please take a look, and please revert for now and fix async if it takes a while to fix.

[sgraenitz]

@thakis Thanks for the note. I guess it's D98579. Fix will be quick.

[thakis]

llvm/tools/llvm-jitlink/llvm-jitlink.cpp

680

No, I think it's this one (at least partially). This here used to be behind LLVM_ON_UNIX but now it isn't. Probably just needs to go back behind that check.

[sgraenitz]

Alright fixed with 9cdbdbea29ce

[thakis]

Thanks, bot is happy again :)

llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp

 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ExecutionEngine/Orc/Shared/FDRawByteChannel.h"
 #include "llvm/ExecutionEngine/Orc/TargetProcess/JITLoaderGDB.h"
 #include "llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h"
 #include "llvm/ExecutionEngine/Orc/TargetProcess/RegisterEHFrames.h"
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
+#include <cstring>
 #include <sstream>
 
 #ifdef LLVM_ON_UNIX
 
+#include <netdb.h>
 #include <netinet/in.h>
 #include <sys/socket.h>
 
 #endif
 
 using namespace llvm;
 using namespace llvm::orc;
 
 ExitOnError ExitOnErr;
 
 LLVM_ATTRIBUTE_USED void linkComponents() {
   errs() << (void *)&llvm_orc_registerEHFrameSectionWrapper
          << (void *)&llvm_orc_deregisterEHFrameSectionWrapper
          << (void *)&llvm_orc_registerJITLoaderGDBWrapper;
 }
 
 void printErrorAndExit(Twine ErrMsg) {
   errs() << "error: " << ErrMsg.str() << "\n\n"
          << "Usage:\n"
          << "  llvm-jitlink-executor filedescs=<infd>,<outfd> [args...]\n"
          << "  llvm-jitlink-executor listen=<host>:<port> [args...]\n";
   exit(1);
 }
 
-int openListener(std::string Host, int Port) {
+int openListener(std::string Host, std::string PortStr) {
 #ifndef LLVM_ON_UNIX
   // FIXME: Add TCP support for Windows.
   printErrorAndExit("listen option not supported");
   return 0;
 #else
-  int SockFD = socket(PF_INET, SOCK_STREAM, 0);
-  struct sockaddr_in ServerAddr, ClientAddr;
-  socklen_t ClientAddrLen = sizeof(ClientAddr);
-  memset(&ServerAddr, 0, sizeof(ServerAddr));
-  ServerAddr.sin_family = PF_INET;
-  ServerAddr.sin_family = INADDR_ANY;
-  ServerAddr.sin_port = htons(Port);
-
-  {
-    // lose the "Address already in use" error message
-    int Yes = 1;
-    if (setsockopt(SockFD, SOL_SOCKET, SO_REUSEADDR, &Yes, sizeof(int)) == -1) {
-      errs() << "Error calling setsockopt.\n";
-      exit(1);
-    }
+  addrinfo Hints{};
+  Hints.ai_family = AF_INET;
+  Hints.ai_socktype = SOCK_STREAM;
+  Hints.ai_flags = AI_PASSIVE;
+
+  addrinfo *AI;
+  if (int EC = getaddrinfo(nullptr, PortStr.c_str(), &Hints, &AI)) {
+    errs() << "Error setting up bind address: " << gai_strerror(EC) << "\n";
+
+  // Create a socket from first addrinfo structure returned by getaddrinfo.
+  int SockFD;
+  if ((SockFD = socket(AI->ai_family, AI->ai_socktype, AI->ai_protocol)) < 0) {
+    errs() << "Error creating socket: " << std::strerror(errno) << "\n";
+    exit(1);
   }
 
-  if (bind(SockFD, (struct sockaddr *)&ServerAddr, sizeof(ServerAddr)) < 0) {
-    errs() << "Error on binding.\n";
+  // Avoid "Address already in use" errors.
+  const int Yes = 1;
+  if (setsockopt(SockFD, SOL_SOCKET, SO_REUSEADDR, &Yes, sizeof(int)) == -1) {
+    errs() << "Error calling setsockopt: " << std::strerror(errno) << "\n";
     exit(1);
   }
 
-  listen(SockFD, 1);
-  return accept(SockFD, (struct sockaddr *)&ClientAddr, &ClientAddrLen);
+  // Bind the socket to the desired port.
+  if (bind(SockFD, AI->ai_addr, AI->ai_addrlen) < 0) {
+    errs() << "Error on binding: " << std::strerror(errno) << "\n";
+    exit(1);
+  }
+
+  // Listen for incomming connections.
+  static constexpr int ConnectionQueueLen = 1;
+  listen(SockFD, ConnectionQueueLen);
+
+  outs() << "Listening at " << Host << ":" << PortStr << "\n";
+
+#if defined(_AIX)
+  assert(Hi_32(AI->ai_addrlen) == 0 && "Field is a size_t on 64-bit AIX");
+  socklen_t AddrLen = Lo_32(AI->ai_addrlen);
+  return accept(SockFD, AI->ai_addr, &AddrLen);
+#else
+  return accept(SockFD, AI->ai_addr, &AI->ai_addrlen);
 #endif
+
+#endif // LLVM_ON_UNIX
 }
 
 int main(int argc, char *argv[]) {
 
   ExitOnErr.setBanner(std::string(argv[0]) + ": ");
 
   int InFD = 0;
   int OutFD = 0;
if (SpecifierType == "filedescs") {
       if (FD2Str.getAsInteger(10, OutFD))
         printErrorAndExit(FD2Str + " is not a valid file descriptor");
     } else if (SpecifierType == "listen") {
       StringRef Host, PortStr;
       std::tie(Host, PortStr) = Specifier.split(':');
 
       int Port = 0;
       if (PortStr.getAsInteger(10, Port))
-        printErrorAndExit("port" + PortStr + " is not a valid integer");
+        printErrorAndExit("port number '" + PortStr +
+                          "' is not a valid integer");
 
-      InFD = OutFD = openListener(Host.str(), Port);
+      InFD = OutFD = openListener(Host.str(), PortStr.str());
+      outs() << "Connection established. Running OrcRPCTPCServer...\n";
     } else
       printErrorAndExit("invalid specifier type \"" + SpecifierType + "\"");
   }
 
   ExitOnErr.setBanner(std::string(argv[0]) + ":");
 
   using JITLinkExecutorEndpoint =
       shared::MultiThreadedRPCEndpoint<shared::FDRawByteChannel>;

llvm/tools/llvm-jitlink/llvm-jitlink.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/Timer.h"
 
+#include <cstring>
 #include <list>
 #include <string>
 
 #ifdef LLVM_ON_UNIX
 #include <netdb.h>
 #include <netinet/in.h>
 #include <sys/socket.h>
 #include <unistd.h>
std::unique_ptr<LLVMJITLinkRemoteTargetProcessControl> RTPC(
           std::move(SSP), std::move(Channel), std::move(Endpoint),
           std::move(ReportError), Err));
   if (Err)
     return std::move(Err);
   return std::move(RTPC);
 #endif
 }
 
-Expected<std::unique_ptr<TargetProcessControl>>
-LLVMJITLinkRemoteTargetProcessControl::ConnectToExecutor() {
-#ifndef LLVM_ON_UNIX
-  // FIXME: Add TCP support for Windows.
-  return make_error<StringError>("-" + OutOfProcessExecutorConnect.ArgStr +
-                                     " not supported on non-unix platforms",
-                                 inconvertibleErrorCode());
-#else
-
-  shared::registerStringError<LLVMJITLinkChannel>();
-
-  StringRef HostNameStr, PortStr;
-  std::tie(HostNameStr, PortStr) =
-      StringRef(OutOfProcessExecutorConnect).split(':');
-
-  if (HostNameStr.empty())
-    return make_error<StringError>("host name for -" +
-                                       OutOfProcessExecutorConnect.ArgStr +
-                                       " can not be empty",
-                                   inconvertibleErrorCode());
-  if (PortStr.empty())
-    return make_error<StringError>(
-        "port for -" + OutOfProcessExecutorConnect.ArgStr + " can not be empty",
-        inconvertibleErrorCode());
-
-  std::string HostName = HostNameStr.str();
-  int Port = 0;
-  if (PortStr.getAsInteger(10, Port))
-    return make_error<StringError>("port number " + PortStr +
-                                       " is not a valid integer",
-                                   inconvertibleErrorCode());
+static Error createTCPSocketError(Twine Details) {
+      formatv("Failed to connect TCP socket '{0}': {1}",
+              OutOfProcessExecutorConnect, Details),
+}
 
+static Expected<int> connectTCPSocket(std::string Host, std::string PortStr) {
   addrinfo *AI;
   addrinfo Hints{};
   Hints.ai_family = AF_INET;
   Hints.ai_socktype = SOCK_STREAM;
   Hints.ai_flags = AI_NUMERICSERV;
-  if (int EC =
-          getaddrinfo(HostName.c_str(), PortStr.str().c_str(), &Hints, &AI))
-    return make_error<StringError>(formatv("Failed to resolve {0}:{1} ({2})",
-                                           HostName, Port, gai_strerror(EC)),
-                                   inconvertibleErrorCode());
 
-  // getaddrinfo returns a list of address structures.  Go through the list
-  // to find one we can connect to.
+  if (int EC = getaddrinfo(Host.c_str(), PortStr.c_str(), &Hints, &AI))
+    return createTCPSocketError("Address resolution failed (" +
+                                StringRef(gai_strerror(EC)) + ")");
+
+  // Cycle through the returned addrinfo structures and connect to the first
+  // reachable endpoint.
   int SockFD;
-  int ConnectRC = -1;
-  for (addrinfo *Server = AI; Server; Server = Server->ai_next) {
-    // If socket fails, maybe it's because the address family is not supported.
-    // Skip to the next addrinfo structure.
+  addrinfo *Server;
+  for (Server = AI; Server != nullptr; Server = Server->ai_next) {
+    // socket might fail, e.g. if the address family is not supported. Skip to
+    // the next addrinfo structure in such a case.
     if ((SockFD = socket(AI->ai_family, AI->ai_socktype, AI->ai_protocol)) < 0)
       continue;
 
-    ConnectRC = connect(SockFD, Server->ai_addr, Server->ai_addrlen);
-    if (ConnectRC == 0)
+    // If connect returns null, we exit the loop with a working socket.
+    if (connect(SockFD, Server->ai_addr, Server->ai_addrlen) == 0)
       break;
 
     close(SockFD);
   }
   freeaddrinfo(AI);
-  if (ConnectRC == -1)
-    return make_error<StringError>("Failed to connect to " + HostName + ":" +
-                                       Twine(Port),
-                                   inconvertibleErrorCode());
+
+  // If we reached the end of the loop without connecting to a valid endpoint,
+  // dump the last error that was logged in socket() or connect().
+  if (Server == nullptr)
+    return createTCPSocketError(std::strerror(errno));
+
+  return SockFD;
+}
+
+Expected<std::unique_ptr<TargetProcessControl>>
+LLVMJITLinkRemoteTargetProcessControl::ConnectToExecutor() {
+#ifndef LLVM_ON_UNIX
+  // FIXME: Add TCP support for Windows.
+  return make_error<StringError>("-" + OutOfProcessExecutorConnect.ArgStr +
+                                     " not supported on non-unix platforms",
+#else
+
+  shared::registerStringError<LLVMJITLinkChannel>();
+
+  StringRef Host, PortStr;
+  std::tie(Host, PortStr) = StringRef(OutOfProcessExecutorConnect).split(':');
+  if (Host.empty())
+    return createTCPSocketError("Host name for -" +
+                                OutOfProcessExecutorConnect.ArgStr +
+                                " can not be empty");
+  if (PortStr.empty())
+    return createTCPSocketError("Port number in -" +
+                                OutOfProcessExecutorConnect.ArgStr +
+                                " can not be empty");
+  int Port = 0;
+  if (PortStr.getAsInteger(10, Port))
+    return createTCPSocketError("Port number '" + PortStr +
+                                "' is not a valid integer");
+
+  Expected<int> SockFD = connectTCPSocket(Host.str(), PortStr.str());
+  if (!SockFD)
+    return SockFD.takeError();
 
   auto SSP = std::make_shared<SymbolStringPool>();
-  auto Channel = std::make_unique<shared::FDRawByteChannel>(SockFD, SockFD);
+  auto Channel = std::make_unique<shared::FDRawByteChannel>(*SockFD, *SockFD);
   auto Endpoint = std::make_unique<LLVMJITLinkRPCEndpoint>(*Channel, true);
 
   auto ReportError = [](Error Err) {
     logAllUnhandledErrors(std::move(Err), errs(), "");
   };
 
   Error Err = Error::success();
   std::unique_ptr<LLVMJITLinkRemoteTargetProcessControl> RTPC(