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