[Lldb-commits] [PATCH] D62499: Create a generic handler for Xfer packets

[Pavel Labath via Phabricator via lldb-commits]

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