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());
}
----------------
aadsm wrote:
> 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...)
Nevermind what I said about sending strings, I just noticed
`m_send_error_strings`.
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
