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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
