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

[Sebastian Schwartz via Phabricator via lldb-commits]

saschwartz marked 5 inline comments as done.
saschwartz added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108351