Create a generic handler for Xfer packets

[aadsm]

Summary

This is the first of a few patches I have to improve the performance of dynamic module loading on Android.

In this first diff I'll describe the context of my main motivation and will then link to it in the other diffs to avoid repeating myself.

Motivation

I have a few scenarios where opening a specific feature on an Android app takes around 40s when lldb is attached to it. The reason for that is because 40 modules are dynamicly loaded at that point in time and each one of them is taking ~1s.

The problem

To learn about new modules we have a breakpoint on a linker function that is called twice whenever a module is loaded. One time just before it's loaded (so lldb can check which modules are loaded) and another right after it's loaded (so lldb can check again which ones are loaded and calculate the diference).
It's figuring out which modules are loaded that is taking quite some time. This is currently done by traversing the linked list of loaded shared libraries that the linker maintains in memory. Each item in the linked list requires its own x packet sent to the gdb server (this is android so the network also plays a part). In my scenario there are 400+ loaded libraries and even though we read 0x800 worth of bytes at a time we still make ~180 requests that end up taking 150-200ms.
We also do this twice, once before the module is loaded (state = eAdd) and another right after (state = eConsistent) which easly adds up to ~400ms per module.

A solution

Implement xfer:libraries-svr4 in lldb-server:
I noticed in the code that loads the new modules that it had support for the xfer:libraries-svr4 packet (added ~4 years ago to support the ds2 debug server) but we didn't support it in lldb-server. This single packet returns an xml list of all the loaded modules by the process. The advantage is that there's no more need to make 180 requests to read the linked list. Additionally this new requests takes around 10ms.

More efficient usage of the xfer:libraries-svr4 packet in lldb:
When xfer:libraries-svr4 is available the Process class has a LoadModules function that requests this packet and then loads or unloads modules based on the current list of loaded modules by the process.
This is the function that is used by the DYLDRendezvous class to get the list of loaded modules before and after the module is loaded. However, this is really not needed since the LoadModules function already loaded or unloaded the modules accordingly. I changed this strategy to call LoadModules only once (after the process has loaded the module).

Bugs
I found a few issues in lldb while implementing this and have submitted independent patches for them.

I tried to devide this into multiple logical patches to make it easier to review and discuss.

Tests

I wanted to put these set of diffs up before having all the tests up and running to start having them reviewed from a techical point of view. I'm also having some trouble making the tests running on linux so I need more time to make that happen.

This diff

The xfer packages follow the same protocol, they are requested with xfer:<object>:<read|write>:<annex>:<offset,length> and a return that starts with l or m depending if the offset and length covers the entire data or not. Before implementing the xfer:libraries-svr4 I refactored the xfer:auxv to generically handle xfer packets so we can easly add new ones.

The overall structure of the function ends up being:

• Parse the packet into its components: object, offset etc.
• Depending on the object do its own logic to generate the data.
• Return the data based on its size, the requested offset and length.

[labath]

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.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2751–2771

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];
...

2767

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

2769

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".

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

85

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

[xiaobai]

Thanks for doing this work! I'd love to see a faster LLDB. :)

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2751–2771

I agree, that would make it simpler.

2767

+1

You could have smaller methods like "ParseAuxvPacket" and "ReadObject"

[aadsm]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2767

I'm actually trying to return an llvm::Expected so I can return a createStringError that will have a message and an error code (as far as I can see ErrorOronly allows to return an error code).
However, I can't figure out how to get both the error code and error message from the takeError() function. I found llvm::errorToErrorCode and llvm::toString but they both require to pass ownership of the error to them so I can only use one of them.
Is the only way to use the handleErrors function that will give me access to the underlying ErrorInfo where I can then call convertToErrorCode and getMessage on it? Sounds overly complicated for something that's probably simpler than this.

[labath]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2767

Yeah, using Expected<T> is a even better idea.

handleErrors is the right way to do this kind of thing. If you're using Expected then the best way to handle this situation would be to define a custom error type to mean "unimplemented". Then you could do something like:

Expected<T> t = getT();
if (!t) {
 ??? result;
  handleAllErrors(t.takeError(),
    [&] (UnimplementedError &e) { result = SendUnimplementedResponse(e.message()); },
   // TODO: We should have a SendErrorResponse version that takes a llvm::Error
    [&] (std::unique_ptr<ErrorInfo> e) { result = SendErrorResponse(Status(Error(std::move(e)));
  );
  return result;
}
do_stuff(*e);

Ideally this code wouldn't even live inside the packet handler function, but we would have a separate function for that, and packet handlers would just return Expected<Packet>. However, that's for another patch...

[krytarowski]

One time just before it's loaded (so lldb can check which modules are loaded) and another right after it's loaded (so lldb can check again which ones are loaded and calculate the difference).

There is on NetBSD and on a selection of other OSs: _rtld_debug_state integrated as a part of ELF dynamic loader.

Is there something like that on Android that could be reused?

[labath]

In D62499#1520610, @krytarowski wrote:

One time just before it's loaded (so lldb can check which modules are loaded) and another right after it's loaded (so lldb can check again which ones are loaded and calculate the difference).

There is on NetBSD and on a selection of other OSs: _rtld_debug_state integrated as a part of ELF dynamic loader.

Is there something like that on Android that could be reused?

Yes, there is, and it's being used now. The question here is what do we do *after* we hit the dynamic loader breakpoint...

[krytarowski]

In D62499#1520654, @labath wrote:

In D62499#1520610, @krytarowski wrote:

One time just before it's loaded (so lldb can check which modules are loaded) and another right after it's loaded (so lldb can check again which ones are loaded and calculate the difference).

There is on NetBSD and on a selection of other OSs: _rtld_debug_state integrated as a part of ELF dynamic loader.

Is there something like that on Android that could be reused?

Yes, there is, and it's being used now. The question here is what do we do *after* we hit the dynamic loader breakpoint...

First I will need to make research of it locally as it was probably not used in some time.

[aadsm]

• Introduce better error handling by creating 2 new error classes, one for generic packet errors that contain a number and another for unimplemented features.
• The logic for reading the xfer object was moved into its own function.
• Removes the define for linux || netbsd for the auxv xfer object.
• Switched from shared pointer to a unique pointer to store the memory buffers in the map

[labath]

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.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp

118

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

125

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.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

83

You need to define static char ID here too. Otherwise the dynamic type detection magic will not work..

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2756–2759

In the spirit of the comment below, here I'd just do something like return createStringError(inconvertibleErrorCode(), "No process");

2762–2763

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).

2786–2787

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()

2804

unused variable.

2840

since you already have the iterator around, it's more efficient to use that for the erase operation.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

85

What about the StringMap part ? :)

[aadsm]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp

125

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.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

83

Ah, I guess it would only match against the StringError because it's inheriting that one?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2762–2763

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.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

85

completely forgot about that :D

[labath]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp

125

It doesn't look like it should be too hard to instantiate GDBRemoteCommunicationServer from a unit test.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

83

I think it would match against *all* StringErrors, even those that are not PacketUnimplementedErrors.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2762–2763

Yeah, the only way you can assign meaning to these numbers today is if you open the source code and search for the occurrences of the given number. :)
That will be even easier if we switch to using strings. :)

[aadsm]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2762–2763

But we can't return strings on the protocol though, it will have to be a E NN.
I'm going with your suggestions but how about this in a future patch:

Have a base PacketError(num, message) and then subclass that one with the different errors we have like NoProcessAvailablePacketError() that would code their own error number and message (or maybe we just need to have static functions like PacketError::createNoProcessAvailableError()?).

Then, on the client side we could add an Optional<PacketError> GetResponseError() to StringExtractorGDBRemote that would create the right packet error given the number, so we can print a descriptive error message on the lldb terminal. (maybe GDBResponseError instead of PacketError...)

[aadsm]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2762–2763

Nevermind what I said about sending strings, I just noticed m_send_error_strings.

[aadsm]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2762

@labath, I was thinking about the auto rule and I'm not sure how it would play here. I remember having to check the GetAuvData to know what this was returning. However, it's quite verbose but I have found instances like this in the codebase:

ErrorOr<std::unique_ptr<WritableMemoryBuffer>> buf =
      llvm::WritableMemoryBuffer::getNewMemBuffer(auxv_size);

[aadsm]

Address comments and add tests

[aadsm]

lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp

40–42 ↗

(On Diff #202640)

Is there a better way to do this? I found the copy function but it accepts a char *, not a const char *.

[labath]

Thanks for writing the test, I have a bunch of more comments on the test itself, but hopefully this will be the last iteration. :)

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2762

Yeah, for more complex types like this, the readability question gets a bit fuzzier. Right now, I think I'd spell this type out fully, but even I'm not fully consistent about that (I'm pretty sure I originally wrote the auto line on the LHS). Overall, I wouldn't worry too much about this particular case.

lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp

1 ↗

(On Diff #202640)

Given the current layout of the code, a better place for this would be unittests/Process/gdb-remote, because that's where the class it is testing lives, and it is also the place where the "client" unit tests are located. The "client" unit tests are more similar to this than the "lldb-server" "unit" tests, because the latter test the lldb-server as a whole, whereas the client tests do mocking and other unittest-y stuff.

(Also, please name it GDBRemoteCommunicationServerTest, based on the class-under-test name)

40–42 ↗

(On Diff #202640)

I'm not sure which copy function you meant, but I'd just do something like packets.emplace_back(static_cast<const char *>(dst), dst_len)

47 ↗

(On Diff #202640)

You could just make this class take a std::vector<std::string> &, and make the MockServer class responsible for owning the packet array and handing it out via GetPackets as llvm::ArrayRef<std::string>.

Shared pointers are used much more frequently than they need to be in lldb. If you look at the llvm codebase, you'll see that it uses shared ownership extremely rarely. For simple types like this it doesn't matter, but once you start having objects with non-trivial destructors, and cross-referencing them via shared pointers, it becomes hard to reason about what are the possible orders destruction of various things. That's why it's better to just not use them at all, if possible.

50 ↗

(On Diff #202640)

I'd make this a class, according to http://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords.

59–69 ↗

(On Diff #202640)

It looks like you could replace these by using GDBRemoteCommunicationServer::SendErrorResponse

81–82 ↗

(On Diff #202640)

EXPECT_THAT(server.GetPackets(), testing::ElementsAre("$E42#ab"));

Besides being a one-liner, this will give you better error messages if we ever have more than one packet here (it will print out the packets instead of just saying "1 != 2").

[aadsm]

Move test to a better place, address other comments related to tests.

[labath]

Looks good to me now. Thanks for your patience.

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp

1–2

This line wrapped accidentally.

lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h

28

A reference would be slightly better, as it is implicitly non-null.

[aadsm]

Address final 2 comments

lldb/include/lldb/Utility/StringExtractorGDBRemote.h

enum ServerPacketType {
     eServerPacketType_qStepPacketSupported,
     eServerPacketType_qSupported,
     eServerPacketType_qSyncThreadStateSupported,
     eServerPacketType_qThreadExtraInfo,
     eServerPacketType_qThreadStopInfo,
     eServerPacketType_qVAttachOrWaitSupported,
     eServerPacketType_qWatchpointSupportInfo,
     eServerPacketType_qWatchpointSupportInfoSupported,
-    eServerPacketType_qXfer_auxv_read,
+    eServerPacketType_qXfer,
 
     eServerPacketType_jSignalsInfo,
     eServerPacketType_jModulesInfo,
 
     eServerPacketType_vAttach,
     eServerPacketType_vAttachWait,
     eServerPacketType_vAttachOrWait,
     eServerPacketType_vAttachName,

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

 #define liblldb_GDBRemoteCommunicationServer_h_
 
 #include <functional>
 #include <map>
 
 #include "GDBRemoteCommunication.h"
 #include "lldb/lldb-private-forward.h"
 
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+
 class StringExtractorGDBRemote;
 
 namespace lldb_private {
 namespace process_gdb_remote {
 
 class ProcessGDBRemote;
 
 class GDBRemoteCommunicationServer : public GDBRemoteCommunication {
protected:
 
   bool m_send_error_strings = false; // If the client enables this then
                                      // we will send error strings as well.
 
   PacketResult Handle_QErrorStringEnable(StringExtractorGDBRemote &packet);
 
   PacketResult SendErrorResponse(const Status &error);
 
+  PacketResult SendErrorResponse(llvm::Error error);
+
   PacketResult SendUnimplementedResponse(const char *packet);
 
   PacketResult SendErrorResponse(uint8_t error);
 
   PacketResult SendIllFormedResponse(const StringExtractorGDBRemote &packet,
                                      const char *error_message);
 
   PacketResult SendOKResponse();
 
 private:
   DISALLOW_COPY_AND_ASSIGN(GDBRemoteCommunicationServer);
 };
 
+class PacketUnimplementedError
+    : public llvm::ErrorInfo<PacketUnimplementedError, llvm::StringError> {
+public:
+  static char ID;
+  using llvm::ErrorInfo<PacketUnimplementedError,
+                        llvm::StringError>::ErrorInfo; // inherit constructors
+  PacketUnimplementedError(const llvm::Twine &S)
+      : ErrorInfo(S, llvm::errc::not_supported) {}
+
+  PacketUnimplementedError() : ErrorInfo(llvm::errc::not_supported) {}
+};
+
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // liblldb_GDBRemoteCommunicationServer_h_

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp

if (m_send_error_strings) {
     packet.Printf("E%2.2x;", static_cast<uint8_t>(error.GetError()));
     packet.PutStringAsRawHex8(error.AsCString());
     return SendPacketNoLock(packet.GetString());
   } else
     return SendErrorResponse(error.GetError());
 }
 
 GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServer::SendErrorResponse(llvm::Error error) {
+  std::unique_ptr<llvm::ErrorInfoBase> EIB;
+  std::unique_ptr<PacketUnimplementedError> PUE;
+  llvm::handleAllErrors(
+      std::move(error),
+      [&](std::unique_ptr<PacketUnimplementedError> E) { PUE = std::move(E); },
+      [&](std::unique_ptr<llvm::ErrorInfoBase> E) { EIB = std::move(E); });
+
+  if (EIB)
+    return SendErrorResponse(Status(llvm::Error(std::move(EIB))));
+  if (PUE)
+    return SendUnimplementedResponse(PUE->message().c_str());
+  return SendErrorResponse(Status("Unknown Error"));
+}
+
+GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServer::Handle_QErrorStringEnable(
     StringExtractorGDBRemote &packet) {
   m_send_error_strings = true;
   return SendOKResponse();
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServer::SendIllFormedResponse(

 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServer::SendOKResponse() {
   return SendPacketNoLock("OK");
 }
 
 bool GDBRemoteCommunicationServer::HandshakeWithClient() {
   return GetAck() == PacketResult::Success;
 }
+
+char PacketUnimplementedError::ID;

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

protected:
   lldb::tid_t m_continue_tid = LLDB_INVALID_THREAD_ID;
   std::recursive_mutex m_debugged_process_mutex;
   std::unique_ptr<NativeProcessProtocol> m_debugged_process_up;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
 
   lldb::StateType m_inferior_prev_state = lldb::StateType::eStateInvalid;
-  std::unique_ptr<llvm::MemoryBuffer> m_active_auxv_buffer_up;
+  llvm::StringMap<std::unique_ptr<llvm::MemoryBuffer>> m_xfer_buffer_map;
   std::mutex m_saved_registers_mutex;
   std::unordered_map<uint32_t, lldb::DataBufferSP> m_saved_registers_map;
   uint32_t m_next_saved_registers_id = 1;
   bool m_handshake_completed = false;
 
   PacketResult SendONotification(const char *buffer, uint32_t len);
 
   PacketResult SendWResponse(NativeProcessProtocol *process);
protected:
   PacketResult Handle_qMemoryRegionInfo(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_Z(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_z(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_s(StringExtractorGDBRemote &packet);
 
-  PacketResult Handle_qXfer_auxv_read(StringExtractorGDBRemote &packet);
+  PacketResult Handle_qXfer(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_QSaveRegisterState(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_jTraceStart(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_jTraceRead(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_jTraceStop(StringExtractorGDBRemote &packet);
protected:
 
   lldb::tid_t GetContinueThreadID() const { return m_continue_tid; }
 
   Status SetSTDIOFileDescriptor(int fd);
 
   FileSpec FindModuleFile(const std::string &module_path,
                           const ArchSpec &arch) override;
 
+  llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
+  ReadXferObject(llvm::StringRef object, llvm::StringRef annex);
+
 private:
   void HandleInferiorState_Exited(NativeProcessProtocol *process);
 
   void HandleInferiorState_Stopped(NativeProcessProtocol *process);
 
   NativeThreadProtocol *GetThreadFromSuffix(StringExtractorGDBRemote &packet);
 
   uint32_t GetNextSavedRegistersID();

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

RegisterMemberFunctionHandler(
       &GDBRemoteCommunicationServerLLGS::Handle_qThreadStopInfo);
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_jThreadsInfo,
       &GDBRemoteCommunicationServerLLGS::Handle_jThreadsInfo);
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_qWatchpointSupportInfo,
       &GDBRemoteCommunicationServerLLGS::Handle_qWatchpointSupportInfo);
   RegisterMemberFunctionHandler(
-      StringExtractorGDBRemote::eServerPacketType_qXfer_auxv_read,
-      &GDBRemoteCommunicationServerLLGS::Handle_qXfer_auxv_read);
+      StringExtractorGDBRemote::eServerPacketType_qXfer,
+      &GDBRemoteCommunicationServerLLGS::Handle_qXfer);
   RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_s,
                                 &GDBRemoteCommunicationServerLLGS::Handle_s);
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_stop_reason,
       &GDBRemoteCommunicationServerLLGS::Handle_stop_reason); // ?
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_vAttach,
       &GDBRemoteCommunicationServerLLGS::Handle_vAttach);
if (log)
                   error.AsCString());
     return SendErrorResponse(0x49);
   }
 
   // No response here - the stop or exit will come from the resulting action.
   return PacketResult::Success;
 }
 
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunicationServerLLGS::Handle_qXfer_auxv_read(
-    StringExtractorGDBRemote &packet) {
-// *BSD impls should be able to do this too.
-#if defined(__linux__) || defined(__NetBSD__)
-  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
-
-  // Parse out the offset.
-  packet.SetFilePos(strlen("qXfer:auxv:read::"));
-  if (packet.GetBytesLeft() < 1)
-    return SendIllFormedResponse(packet,
-                                 "qXfer:auxv:read:: packet missing offset");
-
-  const uint64_t auxv_offset =
-      packet.GetHexMaxU64(false, std::numeric_limits<uint64_t>::max());
-  if (auxv_offset == std::numeric_limits<uint64_t>::max())
-    return SendIllFormedResponse(packet,
-                                 "qXfer:auxv:read:: packet missing offset");
-
-  // Parse out comma.
-  if (packet.GetBytesLeft() < 1 || packet.GetChar() != ',')
-    return SendIllFormedResponse(
-        packet, "qXfer:auxv:read:: packet missing comma after offset");
-
-  // Parse out the length.
-  const uint64_t auxv_length =
-      packet.GetHexMaxU64(false, std::numeric_limits<uint64_t>::max());
-  if (auxv_length == std::numeric_limits<uint64_t>::max())
-    return SendIllFormedResponse(packet,
-                                 "qXfer:auxv:read:: packet missing length");
-
-  // Grab the auxv data if we need it.
-  if (!m_active_auxv_buffer_up) {
+llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
+GDBRemoteCommunicationServerLLGS::ReadXferObject(llvm::StringRef object,
+                                                 llvm::StringRef annex) {
+  if (object == "auxv") {
     // Make sure we have a valid process.
     if (!m_debugged_process_up ||
         (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID)) {
-      if (log)
-        log->Printf(
-            "GDBRemoteCommunicationServerLLGS::%s failed, no process available",
-            __FUNCTION__);
-      return SendErrorResponse(0x10);
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "No process available");
     }
 
     // Grab the auxv data.
     auto buffer_or_error = m_debugged_process_up->GetAuxvData();
-    if (!buffer_or_error) {
-      std::error_code ec = buffer_or_error.getError();
-      LLDB_LOG(log, "no auxv data retrieved: {0}", ec.message());
-      return SendErrorResponse(ec.value());
-    }
-    m_active_auxv_buffer_up = std::move(*buffer_or_error);
+    if (!buffer_or_error)
+      return llvm::errorCodeToError(buffer_or_error.getError());
+    return std::move(*buffer_or_error);
+  }
+
+  return llvm::make_error<PacketUnimplementedError>(
+      "Xfer object not supported");
+
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::Handle_qXfer(
+    StringExtractorGDBRemote &packet) {
+  SmallVector<StringRef, 5> fields;
+  // The packet format is "qXfer:<object>:<action>:<annex>:offset,length"
+  StringRef(packet.GetStringRef()).split(fields, ':', 4);
+  if (fields.size() != 5)
+    return SendIllFormedResponse(packet, "malformed qXfer packet");
+  StringRef &xfer_object = fields[1];
+  StringRef &xfer_action = fields[2];
+  StringRef &xfer_annex = fields[3];
+  StringExtractor offset_data(fields[4]);
+  if (xfer_action != "read")
+    return SendUnimplementedResponse("qXfer action not supported");
+  // Parse offset.
+  const uint64_t xfer_offset =
+      offset_data.GetHexMaxU64(false, std::numeric_limits<uint64_t>::max());
+  if (xfer_offset == std::numeric_limits<uint64_t>::max())
+    return SendIllFormedResponse(packet, "qXfer packet missing offset");
+  // Parse out comma.
+  if (offset_data.GetChar() != ',')
+    return SendIllFormedResponse(packet,
+                                 "qXfer packet missing comma after offset");
+  // Parse out the length.
+  const uint64_t xfer_length =
+      offset_data.GetHexMaxU64(false, std::numeric_limits<uint64_t>::max());
+  if (xfer_length == std::numeric_limits<uint64_t>::max())
+    return SendIllFormedResponse(packet, "qXfer packet missing length");
+
+  // Get a previously constructed buffer if it exists or create it now.
+  std::string buffer_key = (xfer_object + xfer_action + xfer_annex).str();
+  auto buffer_it = m_xfer_buffer_map.find(buffer_key);
+  if (buffer_it == m_xfer_buffer_map.end()) {
+    auto buffer_up = ReadXferObject(xfer_object, xfer_annex);
+    if (!buffer_up)
+      return SendErrorResponse(buffer_up.takeError());
+    buffer_it = m_xfer_buffer_map
+                    .insert(std::make_pair(buffer_key, std::move(*buffer_up)))
+                    .first;
   }
 
+  // Send back the response
   StreamGDBRemote response;
   bool done_with_buffer = false;
-
-  llvm::StringRef buffer = m_active_auxv_buffer_up->getBuffer();
-  if (auxv_offset >= buffer.size()) {
+  llvm::StringRef buffer = buffer_it->second->getBuffer();
+  if (xfer_offset >= buffer.size()) {
     // We have nothing left to send.  Mark the buffer as complete.
     response.PutChar('l');
     done_with_buffer = true;
   } else {
     // Figure out how many bytes are available starting at the given offset.
-    buffer = buffer.drop_front(auxv_offset);
-
+    buffer = buffer.drop_front(xfer_offset);
     // Mark the response type according to whether we're reading the remainder
-    // of the auxv data.
-    if (auxv_length >= buffer.size()) {
+    // of the data.
+    if (xfer_length >= buffer.size()) {
       // There will be nothing left to read after this
       response.PutChar('l');
       done_with_buffer = true;
     } else {
       // There will still be bytes to read after this request.
       response.PutChar('m');
-      buffer = buffer.take_front(auxv_length);
+      buffer = buffer.take_front(xfer_length);
     }
-
     // Now write the data in encoded binary form.
     response.PutEscapedBytes(buffer.data(), buffer.size());
   }
 
   if (done_with_buffer)
-    m_active_auxv_buffer_up.reset();
+    m_xfer_buffer_map.erase(buffer_it);
 
   return SendPacketNoLock(response.GetString());
-#else
-  return SendUnimplementedResponse("not implemented on this platform");
-#endif
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_QSaveRegisterState(
     StringExtractorGDBRemote &packet) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_THREAD));
 
   // Move past packet name.

 uint32_t GDBRemoteCommunicationServerLLGS::GetNextSavedRegistersID() {
   std::lock_guard<std::mutex> guard(m_saved_registers_mutex);
   return m_next_saved_registers_id++;
 }
 
 void GDBRemoteCommunicationServerLLGS::ClearProcessSpecificData() {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
 
-  LLDB_LOG(log, "clearing auxv buffer: {0}", m_active_auxv_buffer_up.get());
-  m_active_auxv_buffer_up.reset();
+  LLDB_LOG(log, "clearing {0} xfer buffers", m_xfer_buffer_map.size());
+  m_xfer_buffer_map.clear();
 }
 
 FileSpec
 GDBRemoteCommunicationServerLLGS::FindModuleFile(const std::string &module_path,
                                                  const ArchSpec &arch) {
   if (m_debugged_process_up) {
     FileSpec file_spec;
     if (m_debugged_process_up

lldb/source/Utility/StringExtractorGDBRemote.cpp

case 'q':
     case 'W':
       if (PACKET_STARTS_WITH("qWatchpointSupportInfo:"))
         return eServerPacketType_qWatchpointSupportInfo;
       if (PACKET_MATCHES("qWatchpointSupportInfo"))
         return eServerPacketType_qWatchpointSupportInfoSupported;
       break;
 
     case 'X':
-      if (PACKET_STARTS_WITH("qXfer:auxv:read::"))
-        return eServerPacketType_qXfer_auxv_read;
+      if (PACKET_STARTS_WITH("qXfer:"))
+        return eServerPacketType_qXfer;
       break;
     }
     break;
 
   case 'j':
     if (PACKET_STARTS_WITH("jModulesInfo:"))
       return eServerPacketType_jModulesInfo;
     if (PACKET_MATCHES("jSignalsInfo"))

lldb/unittests/Process/gdb-remote/CMakeLists.txt

 add_lldb_unittest(ProcessGdbRemoteTests
   GDBRemoteClientBaseTest.cpp
   GDBRemoteCommunicationClientTest.cpp
+  GDBRemoteCommunicationServerTest.cpp
   GDBRemoteCommunicationTest.cpp
   GDBRemoteTestUtils.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost
     lldbPluginPlatformMacOSX
     lldbPluginProcessUtility
     lldbPluginProcessGDBRemote
 
     LLVMTestingSupport
 
   LINK_COMPONENTS
     Support
   )

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp

+//===-- GDBRemoteGDBRemoteCommunicationServerTest.cpp --------------------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "GDBRemoteTestUtils.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Utility/Connection.h"
+
+namespace lldb_private {
+namespace process_gdb_remote {
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_ErrorNumber) {
+  MockServerWithMockConnection server;
+  server.SendErrorResponse(0x42);
+
+  EXPECT_THAT(server.GetPackets(), testing::ElementsAre("$E42#ab"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_Status) {
+  MockServerWithMockConnection server;
+  Status status;
+
+  status.SetError(0x42, lldb::eErrorTypeGeneric);
+  status.SetErrorString("Test error message");
+  server.SendErrorResponse(status);
+
+  EXPECT_THAT(
+      server.GetPackets(),
+      testing::ElementsAre("$E42;54657374206572726f72206d657373616765#ad"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_UnimplementedError) {
+  MockServerWithMockConnection server;
+
+  auto error =
+      llvm::make_error<PacketUnimplementedError>("Test unimplemented error");
+  server.SendErrorResponse(std::move(error));
+
+  EXPECT_THAT(server.GetPackets(), testing::ElementsAre("$#00"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_StringError) {
+  MockServerWithMockConnection server;
+
+  auto error = llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                       "String error test");
+  server.SendErrorResponse(std::move(error));
+
+  EXPECT_THAT(
+      server.GetPackets(),
+      testing::ElementsAre("$Eff;537472696e67206572726f722074657374#b0"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_ErrorList) {
+  MockServerWithMockConnection server;
+
+  auto error = llvm::joinErrors(llvm::make_error<PacketUnimplementedError>(),
+                                llvm::make_error<PacketUnimplementedError>());
+
+  server.SendErrorResponse(std::move(error));
+  // Make sure only one packet is sent even when there are multiple errors.
+  EXPECT_EQ(server.GetPackets().size(), 1UL);
+}
+
+} // namespace process_gdb_remote
+} // namespace lldb_private

lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h

 //===-- GDBRemoteTestUtils.h ------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
 #define lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
 
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Utility/Connection.h"
 
 namespace lldb_private {
 namespace process_gdb_remote {
 
 class GDBRemoteTest : public testing::Test {
 public:
   static void SetUpTestCase();
   static void TearDownTestCase();
 };
 
-struct MockServer : public GDBRemoteCommunicationServer {
+class MockConnection : public lldb_private::Connection {
+public:
+  MockConnection(std::vector<std::string> *packets) { m_packets = packets; };
+
+  MOCK_METHOD2(Connect,
+               lldb::ConnectionStatus(llvm::StringRef url, Status *error_ptr));
+  MOCK_METHOD5(Read, size_t(void *dst, size_t dst_len,
+                            const Timeout<std::micro> &timeout,
+                            lldb::ConnectionStatus &status, Status *error_ptr));
+  MOCK_METHOD0(GetURI, std::string());
+  MOCK_METHOD0(InterruptRead, bool());
+
+  lldb::ConnectionStatus Disconnect(Status *error_ptr) {
+    return lldb::eConnectionStatusSuccess;
+  };
+
+  bool IsConnected() const { return true; };
+  size_t Write(const void *dst, size_t dst_len, lldb::ConnectionStatus &status,
+               Status *error_ptr) {
+    m_packets->emplace_back(static_cast<const char *>(dst), dst_len);
+    return dst_len;
+  };
+
+  std::vector<std::string> *m_packets;
+};
+
+class MockServer : public GDBRemoteCommunicationServer {
+public:
   MockServer()
       : GDBRemoteCommunicationServer("mock-server", "mock-server.listener") {
     m_send_acks = false;
+    m_send_error_strings = true;
   }
 
   PacketResult SendPacket(llvm::StringRef payload) {
     return GDBRemoteCommunicationServer::SendPacketNoLock(payload);
   }
 
   PacketResult GetPacket(StringExtractorGDBRemote &response) {
     const bool sync_on_timeout = false;
     return WaitForPacketNoLock(response, std::chrono::seconds(1),
                                sync_on_timeout);
   }
 
+  using GDBRemoteCommunicationServer::SendErrorResponse;
   using GDBRemoteCommunicationServer::SendOKResponse;
   using GDBRemoteCommunicationServer::SendUnimplementedResponse;
 };
 
+class MockServerWithMockConnection : public MockServer {
+public:
+  MockServerWithMockConnection() : MockServer() {
+    SetConnection(new MockConnection(&m_packets));
+  }
+
+  llvm::ArrayRef<std::string> GetPackets() { return m_packets; };
+
+  std::vector<std::string> m_packets;
+};
+
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h