[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

324 ↗

(On Diff #367401)

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

324 ↗

(On Diff #367401)

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

58

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 ↗

(On Diff #367508)

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 ↗

(On Diff #367508)

Same as above?

323 ↗

(On Diff #367508)

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 ↗

(On Diff #367508)

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 ↗

(On Diff #367508)

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

58

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 ↗

(On Diff #367508)

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 ↗

(On Diff #367508)

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,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,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,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,ALL %s
 CONN: error: no connection arguments
 
 ALL: Use '{{.*}} g[dbserver] --help' for a complete list of options.
 

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
 //
 //===----------------------------------------------------------------------===//
 

 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);
   }
 
+  int ret = 0;
   switch (argv[1][0]) {
   case 'g':
     llgs::initialize();
-    main_gdbserver(argc, argv);
+    ret = main_gdbserver(argc, argv);
     llgs::terminate_debugger();
     break;
   case 'p':
     llgs::initialize();
-    main_platform(argc, argv);
+    ret = main_platform(argc, argv);
     llgs::terminate_debugger();
     break;
   case 'v':
     fprintf(stderr, "%s\n", lldb_private::GetVersion());
+    ret = 0;
     break;
   default:
     display_usage(progname);
-    exit(option_error);
+    ret = 1;
+    break;
   }
+  return ret;
 }