[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D62502#1528382 <https://reviews.llvm.org/D62502#1528382>, @aadsm wrote:

> @labath
>
> > Regarding the test, I am wondering whether requiring the /proc/%d/maps and the linker rendezvous structure to match isn't too strict of a requirement. Dynamic linkers (and particularly android dynamic linkers) do a lot of crazy stuff, and trying to assert this invariant exposes us to all kinds of "optimizations" that the linkers do or may do in the future. I'm wondering if it wouldn't be better to just build an executable with one or two dependent libraries, and then just assert that their entries are present in the library list. However, I'm not completely sure on this, so I'd like to hear what you think about that..
>
> Yeah, that was actually my first idea for the test but then realized we already had that main.c all set up and ready to use so I decide to use proc maps instead. I was not aware that things like that could happen (although I had a suspicion this might be brittle) so let me revisit this.


I'm not sure whether it will be brittle, but I do know that the android linker does (or used to do?) some tricks where it does not really load libdl.so, but instead inserts a fake entry into the link map which pretends to implement that library (but in reality the entry points to the linker itself). That sounds like the kind of thing that could break if you try to correlate the linker data with /proc/%d/maps.

So, I think it would be better to create a separate test binary for this, particularly as it seems some tests with funny library names will be interesting too... Note that when you're creating a special-purpose executable, you don't have to do the whole wait-for-inferior-to-print-something-and-then-send-CTRL-C dance. You can just have the inferior dereference a null pointer (or something of the sort) to force a stop once it has loaded the necessary shared libraries.



================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:96-99
+  virtual Status
+  GetLoadedSVR4Libraries(std::vector<SVR4LibraryInfo> &library_list) {
+    return Status("Not implemented");
+  }
----------------
This should return an `Expected<std::vector<...>>`


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteLibrariesSvr4Support.py:51-52
+
+        OFFSET = 0
+        LENGTH = 0xFFFF
+
----------------
Why are these all caps? TBH, you probably don't need to make these variables at all. You can just place them directly into the packet as strings.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2181
+template <typename T>
+Status NativeProcessLinux::ReadSVR4LibraryInfo(lldb::addr_t link_map_addr,
+                                               SVR4LibraryInfo &info,
----------------
This too could return an `llvm::Error`. `Expected<std::pair<SVR4LibraryInfo, addr_t>>` is a bit of a mouthful, but I'd consider that instead of by-ref returns too..


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2197
+
+  info.name = std::string(name_buffer);
+  info.link_map = link_map_addr;
----------------
What if `name_buffer` is not null-terminated?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h:141-147
+  template <typename T> struct ELFLinkMap {
+    T l_addr;
+    T l_name;
+    T l_ld;
+    T l_next;
+    T l_prev;
+  };
----------------
It looks like this doesn't need to be in a header. You could just define the struct inside `ReadSVR4LibraryInfo`


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2768
 
+#if defined(__linux__)
+  if (object == "libraries-svr4") {
----------------
Now that we have removed ifdefs around the auxv, we should do that here too..


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2778
+    for (auto const &library : library_list) {
+      response.Printf("<library name=\"%s\" ", library.name.c_str());
+      response.Printf("lm=\"0x%" PRIx64 "\" ", library.link_map);
----------------
You're completely ignoring escaping here, which is a pretty complex topic, and one of the reasons why constructing this via the DOM api is so complicated. There are a couple of considerations here:
- `"`, `&`, and other special xml characters: these will definitely need to be xml-escaped properly
- non-7-bit characters: how these will come out depends on whether the client&server agree on the encoding used. I'm not sure what's the best thing to do here
- low ascii (nonprintable) characters: it looks like these cannot be represented in xml (1.0) at all (?)

It might be good to check what gdb does in these situations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502