[Lldb-commits] [PATCH] D108351: [lldb server] Tidy up LLDB server return codes and associated tests

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/tools/lldb-server/lldb-platform.cpp:289
     fprintf(stderr, "failed to create acceptor: %s", error.AsCString());
-    exit(socket_error);
+    return -1;
   }
----------------
saschwartz wrote:
> teemperor wrote:
> > clayborg wrote:
> > > 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;
> > > ```
> > 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).
> 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?
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108351/new/

https://reviews.llvm.org/D108351