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

[António Afonso via Phabricator via lldb-commits]

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