[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

[Pavel Labath via Phabricator via lldb-commits]

labath added subscribers: mgorny, krytarowski.
labath added a comment.

+ at mgorny, @krytarowski in case they know anything interesting about dynamic linkers.

NativeProcessProtocol is certainly too generic for this sort of thing, but perhaps it would make sense to put this in a slightly more generic place? After all, this approach should work on all elf platforms, right? So maybe we could create a (for lack of a better name) NativeProcessELF class to hold these kinds of things. You could just make NativeProcessLinux inherit from that, and the NetBSD folks can migrate NativeProcessNetBSD once they test things out. IIRC there are other things which could be shared between NativeProcessLinux and NetBSD that could be moved here in the future.

Another advantage of having this in an abstract class is that you could test this in isolation, as NativeProcessProtocol is already setup to mock memory accesses: https://github.com/llvm-mirror/lldb/blob/master/unittests/Host/NativeProcessProtocolTest.cpp.



================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1394
 lldb::addr_t NativeProcessLinux::GetSharedLibraryInfoAddress() {
-  // punt on this for now
-  return LLDB_INVALID_ADDRESS;
+  if (GetAddressByteSize() == 8)
+    return GetELFImageInfoAddress<llvm::ELF::Elf64_Ehdr, llvm::ELF::Elf64_Phdr,
----------------
I'd put handling of the caching into this function, simply because the other one is already busy with plenty of other things. Also it would be nicer if the member variable was an Optional<addr_t>.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2122
+  size_t phdr_num_entries = *maybe_phdr_num_entries;
+  lldb::addr_t load_base = phdr_addr - sizeof(ELF_EHDR);
+
----------------
This innocent-looking line assumes that the program headers will be the very next thing coming after the elf header, and that the elf header will be contained in the lowest-located segment. Although they are usually true, neither of the two assumptions are really guaranteed.  I spent a bunch of time figuring out if there's a way to avoid that, but I haven't come up with anything... :/


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2136
+                            sizeof(phdr_entry), bytes_read);
+    if (!error.Success())
+      return LLDB_INVALID_ADDRESS;
----------------
Are you sure that ReadMemory doesn't return "success" on partial reads too ?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2168
+      return LLDB_INVALID_ADDRESS;
+    // Return the &DT_DEBUG->d_ptr which points to r_debug which contains the
+    // link_map.
----------------
The address of r_debug shouldn't ever change, right?
Wouldn't it be better return &r_debug directly, instead of returning a pointer to a pointer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62501