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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for the update. The actual code looks mostly good, but I have some comments on the error handling infrastructure. I am sorry that this is taking a bit long, but I am trying to make sure it's not unnecessarily overcomplicated (in the past we've generally not paid much attention to the error codes, and it doesn't look like we're about to start now), but instead it makes good use of our existing features (like error strings), and is generally unobtrusive so the actual code stands out (not needing to both log and create an error object when something happens helps that). Since you're building this as a general tool that can be used for future packets (or refactors of old ones), I believe it's worth the trouble.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:118
+  GDBRemoteCommunication::PacketResult result;
+  llvm::handleAllErrors(
+      std::move(error),
----------------
I am sorry, but I just remembered there's one more case to think about here. The `error` might actually be a `ErrorList` and contain multiple errors. It's not a commonly used feature (in fact, it might go away at some point), but it exists and if such an error happens to make it's way here it will cause us to send multiple error messages and completely mess up the packet synchronization.

So we should at least guard against that with `assert(!error.isA<ErrorList>())`; or implement this in a way that guarantees only one response would be sent. One way to do that would be to just send one of the errors based on some semi-arbitrary priority list. Maybe something like:
```
std::unique_ptr<PacketError> PE;
std::unique_ptr<PacketUnimplementedError> UE;
...
llvm::handleAllErrors(std::move(error),
  [&] (std::unique_ptr<PacketError> E) { PE = std::move(E); },
  ...);
if (PE)
  return SendErrorResponse(...);
if (UE)
  return SendUnimplementedError(...);
...
```



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:125
+      [&](std::unique_ptr<llvm::ErrorInfoBase> e) {
+        result = SendErrorResponse(std::move(e));
+      });
----------------
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.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:83
+public:
+  using llvm::ErrorInfo<PacketUnimplementedError,
+                        llvm::StringError>::ErrorInfo; // inherit constructors
----------------
You need to define `static char ID` here too. Otherwise the dynamic type detection magic will not work..


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2758-2762
       if (log)
-        log->Printf(
-            "GDBRemoteCommunicationServerLLGS::%s failed, no process available",
-            __FUNCTION__);
-      return SendErrorResponse(0x10);
+        log->Printf("GDBRemoteCommunicationServerLLGS::%s failed, no process "
+                    "available",
+                    __FUNCTION__);
+      return llvm::make_error<PacketError>(0x10);
----------------
In the spirit of the comment below, here I'd just do something like `return createStringError(inconvertibleErrorCode(), "No process");`


================
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());
     }
----------------
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`).


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2793-2794
+    return SendUnimplementedResponse("qXfer action not supported");
+  std::string buffer_key = std::string(xfer_object) + std::string(xfer_action) +
+                           std::string(xfer_annex);
+  // Parse offset.
----------------
A slightly more efficient (and shorter) way to concatenate this would be `(xfer_object + xfer_action + xfer_annex).str()`. "Adding" two StringRefs produces an `llvm::Twine` (https://llvm.org/doxygen/classllvm_1_1Twine.html), which can then be converted to a string by calling `.str()`


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2811
+  // Get a previously constructed buffer if it exists or create it now.
+  std::shared_ptr<llvm::MemoryBuffer> memory_buffer_up;
+  auto buffer_it = m_xfer_buffer_map.find(buffer_key);
----------------
unused variable.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2849
   if (done_with_buffer)
-    m_active_auxv_buffer_up.reset();
+    m_xfer_buffer_map.erase(buffer_key);
 
----------------
since you already have the iterator around, it's more efficient to use that for the `erase` operation.


================
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:
> 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 ? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62499