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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
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);
+
----------------
aadsm wrote:
> labath wrote:
> > 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... :/
> Ah, I thought that was always true. I checked gdb and they're actually using the PT_PHDR entry (which gives the location of the program table itself) to calculate the relocation. We should probably do the same.
Ah, of course. So, the load bias could be calculated as the difference between the PHDR address we got from the aux vector and the address that is contained in the phdr itself. That's really neat. :)


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2136
+                            sizeof(phdr_entry), bytes_read);
+    if (!error.Success())
+      return LLDB_INVALID_ADDRESS;
----------------
aadsm wrote:
> labath wrote:
> > Are you sure that ReadMemory doesn't return "success" on partial reads too ?
> We correctly return a non-success response if ptrace failed. However, from what I could see ptrace was successfully reading 0's when the memory segment was not readable for instance. I'm not concern if we're reading 0's if the segment is not readable, or should I be?
> We correctly return a non-success response if ptrace failed.
Hmm... that's odd. I had a feeling most of our read-like functions return "success" if they got at least some data. But, anyway, that's not your problem.

> However, from what I could see ptrace was successfully reading 0's when the memory segment was not readable for instance. I'm not concern if we're reading 0's if the segment is not readable, or should I be?
By, "not readable", you mean mapped into process memory, but without a read permission (e.g., mmap(..., PROT_NONE)). In this case, I don't think it will return zero, but it will actually ignore the read permission and just return whatever is the actual contents of the memory.

However, I don't think we have to worry about that. I was mainly worried that ReadMemory will not fill in the `phdr_entry` struct and that we will later access uninitialized memory (sounding all kinds of asan warnings and stuff). The fact that ReadMemory may return incorrect data is something we have to be prepared to handle anyway, as we cannot really trust the application to provide the correct data here.


================
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.
----------------
aadsm wrote:
> labath wrote:
> > 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?
> It should not change but it might not be initialized yet. But the big reason is because `GetSharedLibraryInfoAddress` should return (which I'm planning to hook up to `qShlibInfoAddr`) the address of where the debug structure pointer is.
> At least that's what I gather from reading the `ResolveRendezvousAddress` function, the documentation is not 100% clear (imho).
Ok, sounds good then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62501