[lldb server] Tidy up LLDB server return codes and associated tests

[saschwartz]

Summary

This diff modifies the LLDB server return codes to more accurately reflect usage error paths. Specifically we always propagate the return codes from the main entrypoints into GDB remote LLDB server, and platform LLDB server. This way, the top-level caller of LLDB server will be able to correctly check whether the executable exited with or without an error.

We additionally modify and extend the associated shell unit tests to expect nonzero return codes on error conditions.

Test Plan:
LLDB tests pass:

ninja check-lldb

[saschwartz]

Fix unannotated fallthrough in switch statement.

[saschwartz]

Also propagate platform mode return codes and add additional test cases

[teemperor]

Thanks for the patch (and tests!), this LGTM for the most part.

I'm maybe being nitpicky here, but could we replace all the ret = ... stuff and exit(...) with just return ...? That is more in line with LLVM's early-exit approach and makes the control flow easier to follow. The g/p cases could just use a scope_exit to guarantee we destroy the global state:

case 'g':
    llgs::initialize();
    auto deinit = llvm::make_scope_exit([]() { llgs::terminate_debugger(); });
    return main_gdbserver(argc, argv);
case 'p':
    llgs::initialize();
    auto deinit = llvm::make_scope_exit([]() { llgs::terminate_debugger(); });
    return main_platform(argc, argv);

lldb/tools/lldb-server/lldb-platform.cpp

323

FWIW, socket_error seems to be just a (constant) -1? I think this could just be return 1.

[saschwartz]

Use make_scope_exit to avoid superfluous return variable declaration

[saschwartz]

lldb/tools/lldb-server/lldb-platform.cpp

323

Sounds fine, will change.

[saschwartz]

Fix lint

[teemperor]

Some nits but this LGTM, thanks! I'll accept & land this for you once the last few things are addressed :)

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

59

return 1;

67

That break and the ones below are now dead code.

[teemperor]

Eh, I didn't want to accept this now. Just marking this as 'request changes' so it goes back to you for the last few things.

[clayborg]

lldb/tools/lldb-server/lldb-platform.cpp

289

Should we return error.GetError() if it is non zero? IIRC it will be the actual errno. And best to not return -1, just return 1.

uint32_t SBError::GetError() const;

295

Same as above?

323

Same as my above comment, "error" will contain the "errno" value I believe. So maybe return "error.GetError()" as suggested above?

[teemperor]

lldb/tools/lldb-server/lldb-platform.cpp

289

If we force the caller to convert errno to an exit code, then we could also just return the Status error itself (and then the caller can just return 0/1 depending on success or error)? That seems more clear than returning errno from a function with main signature (which makes it look like it would return an exit code).

[saschwartz]

Responded to comments - will tidy up the nits.

lldb/tools/lldb-server/lldb-platform.cpp

289

Sounds fine to me - I went with -1 because that was the original value for socket_error, but don't think anything should be conditioning on that.

I'm pretty ambivalent to the Status error vs an error code directly myself, mainly because I don't know LLVM well enough to know what the convention might be. Will error.GetError always be nonzero if error.Fail() is true?

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

59

Thanks, good catch.

67

Hmm, I thought the compiler would complain without a break statement but let me try. Thanks for review.

[saschwartz]

Address review feedback

[saschwartz]

Fix one missed return nit.

[teemperor]

lldb/tools/lldb-server/lldb-platform.cpp

289

As said above, for this to work we need to have the caller still transform the error code into a valid exit code. Status will give us any integer back (errno or something else we made up), but if we return that from main then the exit code will be set on UNIX to the lowest 8 bits of that value. So essentially right now we implicitly do exit_code = error % 256. That only works if the system agrees to never use a multiple of 256 as an error code and the same goes for LLDB internally. And then there is also all the other weird stuff other operating systems will do here.

I don't see any other error code in LLVM beside 0/1/-1 so let's just keep this patch simple and return one of those from main. I don't think it matters a lot what we return from this artificial main method, but if it's an int then it looks like an exit code from the function signature and it should be a reasonable exit code. So if we want to return an actual error code then it should be wrapped in a Status to make it clear to the caller that they need to convert it to an exit code.

Will error.GetError always be nonzero if error.Fail() is true?

Yes, GetError returns non-zero on failure, but the clearer check is !error.Success() (which does the same check under the hood).

[saschwartz]

lldb/tools/lldb-server/lldb-platform.cpp

289

Sounds fine. That's a good argument about the implicit % 256 operation which I tend to agree would be an unexpected operation for the caller to have to deal with. Sounds fine to stick with the typical semantics of a main function and just return 1 for all errors.

[saschwartz]

Stick with integer return codes from lldb-platform

[teemperor]

LGTM, thanks! I'll merge this on Monday when I have time to watch the bots (unless ofc someone else merges this before then).

[saschwartz]

Hey there - just wondering if someone might be able to merge this in a while?

[jankratochvil]

The new test is failing on all Linux buildbots such as:
lldb-x86_64-fedora = https://lab.llvm.org/staging/#/builders/16
lldb-x86_64-debian = https://lab.llvm.org/buildbot/#/builders/68
https://lab.llvm.org/buildbot/#/builders?tags=lldb

[teemperor]

Thanks, reverted the commit.

@saschwartz Can you take a look at the failure?

[saschwartz]

Yes, will take a look. I had check-lldb pass for the new tests locally previously - I'll rebase and rerun and see what happens.

lldb/test/Shell/lldb-server/TestErrorMessages.test

-RUN: %lldb-server gdbserver --fd 2>&1 | FileCheck --check-prefixes=FD1,ALL %s
+RUN: not %lldb-server gdbserver --fd 2>&1 | FileCheck --check-prefixes=FD1,GDB_REMOTE_ALL %s
 FD1: error: --fd: missing argument
 
-RUN: %lldb-server gdbserver --fd three 2>&1 | FileCheck --check-prefixes=FD2,ALL %s
+RUN: not %lldb-server gdbserver --fd three 2>&1 | FileCheck --check-prefixes=FD2,GDB_REMOTE_ALL %s
 FD2: error: invalid '--fd' argument
 
-RUN: %lldb-server gdbserver --bogus 2>&1 | FileCheck --check-prefixes=BOGUS,ALL %s
+RUN: not %lldb-server gdbserver --bogus 2>&1 | FileCheck --check-prefixes=BOGUS,GDB_REMOTE_ALL %s
 BOGUS: error: unknown argument '--bogus'
 
-RUN: %lldb-server gdbserver 2>&1 | FileCheck --check-prefixes=CONN,ALL %s
+RUN: not %lldb-server gdbserver 2>&1 | FileCheck --check-prefixes=CONN,GDB_REMOTE_ALL %s
 CONN: error: no connection arguments
 
-ALL: Use '{{.*}} g[dbserver] --help' for a complete list of options.
+RUN: %lldb-server platform 2>&1 | FileCheck --check-prefixes=LLDB_PLATFORM_ALL %s
+
+RUN: %lldb-server platform --fd 2>&1 | FileCheck --check-prefixes=FD3,LLDB_PLATFORM_ALL %s
+FD3: lldb-server: unrecognized option `--fd'
+
+RUN: not %lldb-server platform --min-gdbserver-port 42 --max-gdbserver-port 43 2>&1 | FileCheck --check-prefixes=PORT1,LLDB_PLATFORM_ALL %s
+PORT1: error: port number 42 is not in the valid user port range of 1024 - 49152 
+
+RUN: %lldb-server version 2>&1 | FileCheck --check-prefixes=VERSION %s
+VERSION: lldb version
+
+GDB_REMOTE_ALL: Use '{{.*}} g[dbserver] --help' for a complete list of options.
+LLDB_PLATFORM_ALL: lldb-server platform [--log-file log-file-name] [--log-channels log-channel-list] [--port-file port-file-path] --server --listen port 
 

lldb/test/Shell/lldb-server/TestGdbserverPort.test

 # Windows does not build lldb-server.
 # UNSUPPORTED: system-windows
-# RUN: %platformserver --server --listen :1234 --min-gdbserver-port 1234 --max-gdbserver-port 1234 2>&1 | FileCheck %s
+# RUN: not %platformserver --server --listen :1234 --min-gdbserver-port 1234 --max-gdbserver-port 1234 2>&1 | FileCheck %s
 # CHECK: error: --min-gdbserver-port (1234) is not lower than --max-gdbserver-port (1234)

lldb/tools/lldb-server/lldb-platform.cpp

 }
 #endif
 
 static void display_usage(const char *progname, const char *subcommand) {
   fprintf(stderr, "Usage:\n  %s %s [--log-file log-file-name] [--log-channels "
                   "log-channel-list] [--port-file port-file-path] --server "
                   "--listen port\n",
           progname, subcommand);
-  exit(0);
 }
 
 static Status save_socket_id_to_file(const std::string &socket_id,
                                      const FileSpec &file_spec) {
   FileSpec temp_file_spec(file_spec.GetDirectory().GetStringRef());
   Status error(llvm::sys::fs::create_directory(temp_file_spec.GetPath()));
   if (error.Fail())
     return Status("Failed to create directory %s: %s",
#endif
   GDBRemoteCommunicationServerPlatform::PortMap gdbserver_portmap;
   int min_gdbserver_port = 0;
   int max_gdbserver_port = 0;
   uint16_t port_offset = 0;
 
   FileSpec socket_file;
   bool show_usage = false;
   int option_error = 0;
-  int socket_error = -1;
 
   std::string short_options(OptionParser::GetShortOptionString(g_long_options));
 
 #if __GLIBC__
   optind = 0;
 #else
   optreset = 1;
   optind = 1;
#endif
   }
 
   // Print usage and exit if no listening port is specified.
   if (listen_host_port.empty())
     show_usage = true;
 
   if (show_usage || option_error) {
     display_usage(progname, subcommand);
-    exit(option_error);
+    return option_error;
   }
 
   // Skip any options we consumed with getopt_long_only.
   argc -= optind;
   argv += optind;
   lldb_private::Args inferior_arguments;
   inferior_arguments.SetArguments(argc, const_cast<const char **>(argv));
 
   const bool children_inherit_listen_socket = false;
   // the test suite makes many connections in parallel, let's not miss any.
   // The highest this should get reasonably is a function of the number
   // of target CPUs. For now, let's just use 100.
   const int backlog = 100;
 
   std::unique_ptr<Acceptor> acceptor_up(Acceptor::Create(
       listen_host_port, children_inherit_listen_socket, error));
   if (error.Fail()) {
     fprintf(stderr, "failed to create acceptor: %s", error.AsCString());
-    exit(socket_error);
+    return -1;
   }
 
   error = acceptor_up->Listen(backlog);
   if (error.Fail()) {
     printf("failed to listen: %s\n", error.AsCString());
-    exit(socket_error);
+    return -1;
   }
   if (socket_file) {
     error =
         save_socket_id_to_file(acceptor_up->GetLocalSocketId(), socket_file);
     if (error.Fail()) {
       fprintf(stderr, "failed to write socket id to %s: %s\n",
               socket_file.GetPath().c_str(), error.AsCString());
       return 1;
if (!gdbserver_portmap.empty()) {
       platform.SetPortMap(std::move(gdbserver_portmap));
     }
 
     const bool children_inherit_accept_socket = true;
     Connection *conn = nullptr;
     error = acceptor_up->Accept(children_inherit_accept_socket, conn);
     if (error.Fail()) {
       WithColor::error() << error.AsCString() << '\n';
-      exit(socket_error);
+      return -1;
     }
     printf("Connection established.\n");
     if (g_server) {
       // Collect child zombie processes.
 #if !defined(_WIN32)
       while (waitpid(-1, nullptr, WNOHANG) > 0)
         ;
 #endif

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

 //===-- lldb-server.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 "SystemInitializerLLGS.h"
 #include "lldb/Initialization/SystemLifetimeManager.h"
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 
 #include <cstdio>
 #include <cstdlib>

 static void terminate_debugger() { g_debugger_lifetime->Terminate(); }
 } // namespace llgs
 
 // main
 int main(int argc, char *argv[]) {
   llvm::InitLLVM IL(argc, argv, /*InstallPipeSignalExitHandler=*/false);
   llvm::PrettyStackTraceProgram X(argc, argv);
 
-  int option_error = 0;
   const char *progname = argv[0];
   if (argc < 2) {
     display_usage(progname);
-    exit(option_error);
+    exit(1);
   }
 
   switch (argv[1][0]) {
-  case 'g':
+  case 'g': {
     llgs::initialize();
-    main_gdbserver(argc, argv);
-    llgs::terminate_debugger();
+    auto deinit = llvm::make_scope_exit([]() { llgs::terminate_debugger(); });
+    return main_gdbserver(argc, argv);
     break;
-  case 'p':
+  }
+  case 'p': {
     llgs::initialize();
-    main_platform(argc, argv);
-    llgs::terminate_debugger();
+    auto deinit = llvm::make_scope_exit([]() { llgs::terminate_debugger(); });
+    return main_platform(argc, argv);
     break;
+  }
   case 'v':
     fprintf(stderr, "%s\n", lldb_private::GetVersion());
+    return 0;
     break;
   default:
     display_usage(progname);
-    exit(option_error);
+    return 1;
+    break;
   }
 }