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

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

aadsm marked 4 inline comments as done.
aadsm 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));
+      });
----------------
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.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:83
+public:
+  using llvm::ErrorInfo<PacketUnimplementedError,
+                        llvm::StringError>::ErrorInfo; // inherit constructors
----------------
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?


================
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:
> 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.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:85
   lldb::StateType m_inferior_prev_state = lldb::StateType::eStateInvalid;
-  std::unique_ptr<llvm::MemoryBuffer> m_active_auxv_buffer_up;
+  std::unordered_map<std::string, std::shared_ptr<llvm::MemoryBuffer>>
+      m_xfer_buffer_map;
----------------
labath wrote:
> labath wrote:
> > This could be an `llvm::StringMap`. Also, it should be enough to use `unique_ptr` here. You just need to make sure that you don't copy the value out of the map. I recommend a pattern like
> > ```
> > iter = map.find("foo");
> > if (iter == end()) {
> >   auto buffer_or_error = getFoo();
> >   if (buffer_or_error)
> >     iter = map.insert("foo", std::move(*buffer_or_error))
> >   else
> >     report(buffer_or_error.getError());
> > }
> > StringRef buffer = iter->second->getBuffer();
> > 
> > do_stuff(buffer);
> > 
> > if (done)
> >   map.erase(iter);
> > ```
> What about the StringMap part ? :)
completely forgot about that :D


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62499/new/

https://reviews.llvm.org/D62499