aadsm marked an inline comment as done.
aadsm added inline comments.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2770
LLDB_LOG(log, "no auxv data retrieved: {0}", ec.message());
- return SendErrorResponse(ec.value());
+ return llvm::make_error<PacketError>(ec.value());
}
----------------
labath wrote:
> aadsm wrote:
> > labath wrote:
> > > I am wondering whether we actually need the `PacketError` class. Such a
> > > class would be useful if we actually wanted to start providing meaningful
> > > error codes to the other side (as we would give us tighter control over
> > > the allocation of error codes). However, here you're just taking random
> > > numbers and sticking them into the `PacketError` class, so I am not sure
> > > that adds any value.
> > >
> > > So, what I'd do here is just delete the PacketError class, and just do a
> > > `return llvm::errorCodeToError(ec)` here. I'd also delete the log message
> > > as the error message will implicitly end up in the packet log via the
> > > error message extension (if you implement my suggestion
> > > `SendErrorResponse`).
> > I thought it would be nice to have a little abstraction layer around the
> > packet errors overall. My purpose with the PacketError is to make it more
> > obvious in the code that the number given will be sent back as an error
> > number.
> > I didn't realize the numbers we were using were meaningless today though
> > (now that I think of it this ec.value is really whatever GetAuxvData
> > returns). I searched at the time and found a few different numbers being
> > used: 9, 4, 10, etc. I guess these numbers are just lies then :D.
> >
> Yeah, the only way you can assign meaning to these numbers today is if you
> open the source code and search for the occurrences of the given number. :)
> That will be even easier if we switch to using strings. :)
But we can't return strings on the protocol though, it will have to be a `E NN`.
I'm going with your suggestions but how about this in a future patch:
Have a base `PacketError(num, message)` and then subclass that one with the
different errors we have like `NoProcessAvailablePacketError()` that would code
their own error number and message (or maybe we just need to have static
functions like PacketError::createNoProcessAvailableError()?).
Then, on the client side we could add an `Optional<PacketError>
GetResponseError()` to `StringExtractorGDBRemote` that would create the right
packet error given the number, so we can print a descriptive error message on
the lldb terminal. (maybe GDBResponseError instead of PacketError...)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62499/new/
https://reviews.llvm.org/D62499
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
