labath added inline comments.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:125
+ [&](std::unique_ptr<llvm::ErrorInfoBase> e) {
+ result = SendErrorResponse(std::move(e));
+ });
----------------
aadsm wrote:
> labath wrote:
> > I'm a bit confused. What does this call exactly? It looks to me like this
> > will use the implicit (we should probably make it explicit)
> > `std::unique_ptr<ErrorInfoBase>` constructor of `Error` to call this method
> > again and cause infinite recursion.
> >
> > I'd suggest doing something like
> > `SendErrorResponse(Status(Error(std::move(e)))`. The advantage of this is
> > that the `Status` overload knows how to send an error as string (we have an
> > protocol extension for that), and so will provide a better error message on
> > the client side.
> ah, not sure what I was thinking here. I remember writing
> `SendErrorResponse(Status(Error(std::move(e)))` (or something to that effect)
> but somehow ended up with this recursion..
> I need to force these errors to happen to make sure everything makes sense.
It doesn't look like it should be too hard to instantiate
`GDBRemoteCommunicationServer` from a unit test.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:83
+public:
+ using llvm::ErrorInfo<PacketUnimplementedError,
+ llvm::StringError>::ErrorInfo; // inherit constructors
----------------
aadsm wrote:
> labath wrote:
> > You need to define `static char ID` here too. Otherwise the dynamic type
> > detection magic will not work..
> Ah, I guess it would only match against the StringError because it's
> inheriting that one?
I think it would match against *all* StringErrors, even those that are not
PacketUnimplementedErrors.
================
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:
> > 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. :)
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
