This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by saschwartz on Aug 18 2021, 9:47 PM.

Details

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

Diff Detail

Event Timeline

saschwartz requested review of this revision.Aug 18 2021, 9:47 PM
saschwartz created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2021, 9:47 PM
saschwartz planned changes to this revision.Aug 18 2021, 9:49 PM

Fix unannotated fallthrough in switch statement.

saschwartz planned changes to this revision.Aug 18 2021, 10:11 PM
saschwartz edited the summary of this revision. (Show Details)Aug 18 2021, 10:49 PM

Also propagate platform mode return codes and add additional test cases

teemperor requested changes to this revision.Aug 19 2021, 1:13 AM
teemperor added a subscriber: 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.

This revision now requires changes to proceed.Aug 19 2021, 1:13 AM

Use make_scope_exit to avoid superfluous return variable declaration

saschwartz added inline comments.Aug 19 2021, 8:03 AM
lldb/tools/lldb-server/lldb-platform.cpp
323

Sounds fine, will change.

teemperor accepted this revision.Aug 19 2021, 10:11 AM

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.

This revision is now accepted and ready to land.Aug 19 2021, 10:11 AM
teemperor requested changes to this revision.Aug 19 2021, 10:11 AM

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.

This revision now requires changes to proceed.Aug 19 2021, 10:11 AM
clayborg added inline comments.Aug 19 2021, 11:39 AM
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 added inline comments.Aug 19 2021, 12:19 PM
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).

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.

Address review feedback

saschwartz marked an inline comment as done.

Fix one missed return nit.

teemperor requested changes to this revision.Aug 20 2021, 3:00 AM
teemperor added inline comments.
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).

This revision now requires changes to proceed.Aug 20 2021, 3:00 AM
saschwartz marked 5 inline comments as done.Aug 20 2021, 9:33 AM
saschwartz added inline comments.
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.

Stick with integer return codes from lldb-platform

teemperor accepted this revision.Aug 20 2021, 9:48 AM

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

This revision is now accepted and ready to land.Aug 20 2021, 9:48 AM

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

This revision was landed with ongoing or failed builds.Sep 2 2021, 2:17 AM
This revision was automatically updated to reflect the committed changes.
teemperor reopened this revision.Sep 2 2021, 6:29 AM

Thanks, reverted the commit.

@saschwartz Can you take a look at the failure?

This revision is now accepted and ready to land.Sep 2 2021, 6:29 AM

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.