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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you for working on this. This can speed up the handling of shared library loading by a lot. Also, thank you for splitting the work up into logical pieces. Please find my comments inline.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2697-2717
+  std::string xfer_object;
+  if (packet.GetStringBeforeChar(xfer_object, ':') == 0)
+    return SendIllFormedResponse(packet, "qXfer packet missing object name");
+  // Consume ':'
+  packet.GetChar();
+  // Parse action (read/write).
+  std::string xfer_action;
----------------
I think it would be simpler to just forget about the StringExtractor here, and do something like
```
SmallVector<StringRef, 4> fields;
StringRef(packet.GetStringRef()).split(fields, 3);
if (fields.size() != 4) return "Malformed packet";
// After this, "object" is in fields[0], "action" is in fields[1], annex is in fields[2], and the rest is in fields[3]
std::string buffer_key = fields[0] + fields[2];
...
```


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2738
+  if (!memory_buffer_sp) {
+    if (xfer_object == "auxv") {
+// *BSD impls should be able to do this too.
----------------
Given that this function is going to grow, it would be good to split it in smaller chunks. Maybe move this code into something like `ErrorOr<xfer_map::iterator> ReadObject(StringRef object)`?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2740
+// *BSD impls should be able to do this too.
+#if defined(__linux__) || defined(__NetBSD__)
+      Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
----------------
I think we should just remove this ifdef. NetBSD and linux are the only platforms supported by lldb-server right now. And anyway, any implementation which does not support auxv reading can just return an error from `process->GetAuxvData`...

If we want to differentiate between SendErrorResponse and SendUnimplementedResponse we can just develop a convention that a specific error code (`llvm::errc::not_supported`) means "unimplemented".


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62499