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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Jun 3 02:28:03 PDT 2019


labath added a comment.

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



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2766
     // Grab the auxv data.
     auto buffer_or_error = m_debugged_process_up->GetAuxvData();
     if (!buffer_or_error) {
----------------
aadsm wrote:
> @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);
> ```
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.


================
Comment at: lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:1
+//===-- CommunicationServerTest.cpp -----------------------------*- C++ -*-===//
+//
----------------
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)


================
Comment at: lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:40-42
+    for (size_t i = 0; i < dst_len; i++) {
+      packet.append(1, *(static_cast<const char *>(dst) + i));
+    }
----------------
aadsm wrote:
> Is there a better way to do this? I found the copy function but it accepts a `char *`, not a `const char *`.
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)`


================
Comment at: lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:47
+
+  PacketsSP m_packets_sp;
+};
----------------
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.


================
Comment at: lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:50
+
+struct MockServer : public GDBRemoteCommunicationServer {
+  MockServer()
----------------
I'd make this a class, according to <http://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords>.


================
Comment at: lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:59-69
+  PacketResult SendErrorResponse(uint8_t error) {
+    return GDBRemoteCommunicationServer::SendErrorResponse(error);
+  }
+
+  PacketResult SendErrorResponse(const Status &error) {
+    return GDBRemoteCommunicationServer::SendErrorResponse(error);
+  }
----------------
It looks like you could replace these by `using GDBRemoteCommunicationServer::SendErrorResponse`


================
Comment at: lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:81-82
+
+  EXPECT_EQ(server.GetPackets()->size(), 1UL);
+  EXPECT_EQ(server.GetPackets()->at(0), std::string("$E42#ab"));
+}
----------------
`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").


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62499





More information about the lldb-commits mailing list