Implement GetSharedLibraryInfoAddress

[aadsm]

Summary

This is the third patch to improve module loading in a series that started here (where I explain the motivation and solution): D62499

Add functions to read the r_debug location to know where the linked list of loaded libraries are so I can generate the xfer:libraries-svr4 packet.
I'm also using this function to implement GetSharedLibraryInfoAddress that was "not implemented" for linux.
Most of this code was inspired by the current ds2 implementation here: https://github.com/facebook/ds2/blob/master/Sources/Target/POSIX/ELFProcess.cpp.

[labath]

Looks relatively straight-forward, but I'll wait with the full review of this and subsequent patches until we sort out the first two patches in this series.

[aadsm]

Rebase

[aadsm]

Rebase

[clayborg]

lldb/include/lldb/Host/common/NativeProcessProtocol.h

430

This doesn't seem like it belongs in the base class. Maybe just add it to NativeProcessLinux.h?

[aadsm]

lldb/include/lldb/Host/common/NativeProcessProtocol.h

430

oops, it certainly does not, my mistake.

[aadsm]

Address comments

[labath]

+@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.

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

1394–1399

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

2121

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... :/

2135

Are you sure that ReadMemory doesn't return "success" on partial reads too ?

2167

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?

[aadsm]

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

2135

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?

2167

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

[aadsm]

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

2121

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.

[labath]

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

2121

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

2135

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.

2167

Ok, sounds good then.

[aadsm]

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.

I might be missing something here, I'm not sure how having this in a NativeProcessELF class instead of NativeProcessLinux would make things easier for testing. Like you said, NativeProcessProtocol is the one set up to mock memory access. I still need to create my own MockProcessELF, which makes me think if there's a way to somehow reuse MockProcess to create MockProcessELF?

[labath]

In D62501#1531202, @aadsm wrote:

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.

I might be missing something here, I'm not sure how having this in a NativeProcessELF class instead of NativeProcessLinux would make things easier for testing. Like you said, NativeProcessProtocol is the one set up to mock memory access. I still need to create my own MockProcessELF, which makes me think if there's a way to somehow reuse MockProcess to create MockProcessELF?

Yeah, sorry, I guess that came out more optimistic then what I meant. What I was trying to say, that it is possible to create NativeProcessProtocol in a unit test, which means it would be also possible for the NativeProcessELF.

I haven't given this much thought, but it may be possible to reuse the stuff in MockProcess by making it a template (so you'd have a MockProcess<NativeProcessProtocol>, and a MockProcess<NativeProcessELF>)

[aadsm]

I haven't given this much thought, but it may be possible to reuse the stuff in MockProcess by making it a template (so you'd have a MockProcess<NativeProcessProtocol>, and a MockProcess<NativeProcessELF>)

Ah interesting, will explore that instead. I was actually thinking if it would be possible to extract the common stuff into a MockProcessCommon and then use multiple inheritance like MockProcess : MockProcessCommon, NativeProcessProtocol and MockProcessELF : MockProcessCommon, NativeProcessELF but I don't know much about C++ to know if it's feasible so I'll just need to try, it will be a good learning experience.

[aadsm]

@labath while working on this I had to also move the GetAuxValue function into the NativeProcessELF, which I think it's fine. However, this means this class now depends on the AuxValue class defined in the PluginUtility. Initially I was going to put the NativeProcessELF in the lldbHost but it feels weird that this will now depend on a plugin, even if it's the utility one. Should I create a new ELF plugin just for this?

[labath]

In D62501#1531959, @aadsm wrote:

@labath while working on this I had to also move the GetAuxValue function into the NativeProcessELF, which I think it's fine. However, this means this class now depends on the AuxValue class defined in the PluginUtility. Initially I was going to put the NativeProcessELF in the lldbHost but it feels weird that this will now depend on a plugin, even if it's the utility one. Should I create a new ELF plugin just for this?

Yeah, having Host depend on Process/Utility would be suboptimal, as we've just spend a lot of time making sure Host does not depend on anything. I would be fine with creating a new plugin for this. A couple of other possible solutions I see are:

• put this in Plugins/Process/POSIX: These days, the folder is mostly empty, so we could start repurposing it for this. Having an "ELF" process inside "POSIX" folder looks a bit weird, but my reasoning behind that is that we will probably one day need a NativeProcessPOSIX to share the common code between non-elf posix platform (i.e. darwin). Then NativeProcessELF would be a refinement (subclass) of NativeProcessPOSIX
• move all Native***Protocol classes out of Host into some new place. This is pretty specialized code, and I don't think it needs to live in a relatively general place. At that point it becomes less important to have to outgoing dependencies here.
• make the rendezvous structure parser a "utility" too. All this needs to function is the aux vector and something to read the memory with. You could provide these as arguments to its constructor. This way we won't need a NativeProcessELF, side-stepping the issue of where to place it. Additionally, this would open the door for using this code on the client side. Right now, DynamicLoaderPOSIXDYLD plugin has it's own code for parsing this structure, but I don't see a reason why this code couldn't be used there too. It would also solve the testing problems, as you'd just need to mock these two things (auxv, memory), and not the full NativeProcessProtocol API.

When I spell things out this way, I think I'm starting to prefer the last option. Let me know what you think.

[aadsm]

Interesting, I actually don't think there's much to gain from turning this into an utility. Creating a NativeProcessELF makes sense to me (I already did it and have all the tests up and running as well) and putting it into POSIX also makes sense to me based on what you said.

The big motivation I can see with an utility would be to share the logic between client and server but in this particular situation I don't think that's an advantage.
If you look at DYLDRendezvous we don't read the image info address there, this is done by the process, which in turn will actually use gdb server to get this data otherwise it will use the structures it already has parsed (the image section etc.) to access this.

The other part is to read the libraries' linked list. If we used a common function for this it would force us to create an interface to read memory making both client and server go for a common denominator that might not be optimal for neither of them.
I'm usually a bit wary of trying to generalize so much because of this. For example, today on the server we do a single read for the entire link_map structure and on the client we perform 5 ReadPointers.
Fwiw, I did think about this at the beginning (just like I did for the AuxVector, but that one is different because it's just parsing a block of data) and that was the reason why I didn't pursue it.

Having said this, I am a bit biased just because I've already implemented the NativeProcessELF solution and have very little cycles left to work on this (was hoping to land this week :D), but I do believe that having this on NativeProcess* will be better.

PS: I like your 2) point and it makes sense to me as well but I think that would be outside the scope of this stack of diffs (I'd be happy to work on this in the future).

[labath]

Ok, makes sense, I'm happy that you've considered the dynamic loader sharing tradeoffs already. Let's go with the NativeProcessELF thingy then.

[aadsm]

This creates a NativeProcessELF to deal with ELF specific things. The NativeProcessLinux now extends this one.
Added a test case for the GetAuxValue and GetELFImageInfoAddress. I refactored NativeProcessTestUtils to reuse that code.
I also addressed other comments on the review.

Note about the tests: I only have one test for the GetELFImageInfoAddress that follows the 32bit version and also makes sure we correctly read the load bias. Tbh I'm not sure if this is enough but should be easy to add more cases now.

[labath]

I think this is pretty good now. The only concern I have is about the placement NativeProcessTestUtils. The way you're including it now looks weird. I think we should put that in a separate place, as per the inline coment.

In D62501#1533483, @aadsm wrote:

Note about the tests: I only have one test for the GetELFImageInfoAddress that follows the 32bit version and also makes sure we correctly read the load bias. Tbh I'm not sure if this is enough but should be easy to add more cases now.

The main advantage of this kind of tests I see is that they make it possible to test what happens if things don't go as expected. You don't have to go overboard on that (you've already done a lot more here than most other code reviews do), but it may be nice to include one test where this lookup fails (e.g. the DT_DEBUG entry is missing, or something).

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

41

I guess this is no longer needed.

lldb/unittests/Process/POSIX/NativeProcessELFTest.cpp

9 ↗

(On Diff #203476)

This is the second time we've needed to share code between various unit tests. Let's just try start a convention for putting these into a separate place and see how it develops. I'd say we should put this into unittests/TestingSupport/Host/NativeProcessTestUtils.h (so, #includable as "TestingSupport/Host/NativeProcessTestUtils.h"), for the time being.

35 ↗

(On Diff #203476)

s/vector/ArrayRef

45–46 ↗

(On Diff #203476)

you could say toStringRef(buffer_sp->GetData())

61–64 ↗

(On Diff #203476)

Optional has overloaded equality operators which "do the right thing", though they are a bit tricky to use with integral types, as they kick in only if the contained type matches exactly. If you changed the type of phdr_addr to uint64_t, then you could just write ASSERT_EQ(phdr_addr, process.GetAuxValue(AuxVector::AUXV_AT_PHDR)) here.

83–117 ↗

(On Diff #203476)

What if we just defined a struct which described the final memory layout, and then gave that as an argument to the FakeMemory object?

I'm thinking of something like:

struct MemoryContents {
  Elf32_Phdr phdr_load;
  Elf32_Phdr phdr_dynamic;
  Elf32_Dyn dyn_debug;
} MC;
MC.phdr_load.p_type = PT_DYNAMIC;
...
FakeMemory M(&MC, sizeof MC, phdr_addr); // This assumes adding a (const void *, size_t) constructor to the class

[aadsm]

Address comments

[aadsm]

Removed unwanted change

[labath]

LGTM.

BTW, I see you still haven't submitted the previous two changes in the stack. Are you planning to submit all of them together or what? It's usually better to avoid batch submitting a bunch of dependent changes, as that way, if any problem shows up with the first one, the whole stack needs to be reverted

[aadsm]

Initially one at a time but then thought it might be better to do it as a batch because I was afraid I was missing some dependency and would brake something unexpectedly. But I guess that since I've already landed D62168 it's probably fine to land one at a time.

lldb/unittests/Process/POSIX/NativeProcessELFTest.cpp

83–117 ↗

(On Diff #203476)

Ah nice, why didn't I think about this before :D

[aadsm]

Improve tests

lldb/include/lldb/Host/common/NativeProcessProtocol.h

protected:
   /// Notify the delegate that an exec occurred.
   ///
   /// Provide a mechanism for a delegate to clear out any exec-
   /// sensitive data.
   void NotifyDidExec();
 
   NativeThreadProtocol *GetThreadByIDUnlocked(lldb::tid_t tid);
 
+  lldb::addr_t GetELFImageInfoAddress();
+
 private:
   void SynchronouslyNotifyProcessStateChanged(lldb::StateType state);
   llvm::Expected<SoftwareBreakpoint>
   EnableSoftwareBreakpoint(lldb::addr_t addr, uint32_t size_hint);
 };
 } // namespace lldb_private
 
 #endif // #ifndef liblldb_NativeProcessProtocol_h_

lldb/source/Plugins/Process/Linux/NativeProcessLinux.h

static Status PtraceWrapper(int req, lldb::pid_t pid, void *addr = nullptr,
                               void *data = nullptr, size_t data_size = 0,
                               long *result = nullptr);
 
   bool SupportHardwareSingleStepping() const;
 
 protected:
   llvm::Expected<llvm::ArrayRef<uint8_t>>
   GetSoftwareBreakpointTrapOpcode(size_t size_hint) override;
+  template <typename ELF_EHDR, typename ELF_PHDR, typename ELF_DYN>
+  lldb::addr_t GetELFImageInfoAddress();
 
 private:
   MainLoop::SignalHandleUP m_sigchld_handle;
   ArchSpec m_arch;
   std::unique_ptr<ELFAuxVector> m_aux_vector;
+  lldb::addr_t m_elf_image_info_addr = LLDB_INVALID_ADDRESS;
 
   LazyBool m_supports_mem_region = eLazyBoolCalculate;
   std::vector<std::pair<MemoryRegionInfo, FileSpec>> m_mem_region_cache;
 
   lldb::tid_t m_pending_notification_tid = LLDB_INVALID_THREAD_ID;
 
   // List of thread ids stepping with a breakpoint with the address of
   // the relevan breakpoint

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringExtractor.h"
+#include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Support/Errno.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 
 #include "NativeThreadLinux.h"
 #include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
 #include "Plugins/Process/Utility/LinuxProcMaps.h"
 #include "Procfs.h"

 
 Status NativeProcessLinux::DeallocateMemory(lldb::addr_t addr) {
   // FIXME see comments in AllocateMemory - required lower-level
   // bits not in place yet (ThreadPlans)
   return Status("not implemented");
 }
 
 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,
+                                  llvm::ELF::Elf64_Dyn>();
+  else
+    return GetELFImageInfoAddress<llvm::ELF::Elf32_Ehdr, llvm::ELF::Elf32_Phdr,
+                                  llvm::ELF::Elf32_Dyn>();
 }
 
 size_t NativeProcessLinux::UpdateThreads() {
   // The NativeProcessLinux monitoring threads are always up to date with
   // respect to thread state and they keep the thread list populated properly.
   // All this method needs to do is return the thread count.
   return m_threads.size();
 }
if (m_aux_vector == nullptr) {
     DataExtractor auxv_data(buffer_or_error.get()->getBufferStart(),
                             buffer_or_error.get()->getBufferSize(),
                             GetByteOrder(), GetAddressByteSize());
     m_aux_vector = llvm::make_unique<ELFAuxVector>(auxv_data);
   }
 
   return m_aux_vector->GetAuxValue(type);
 }
+
+template <typename ELF_EHDR, typename ELF_PHDR, typename ELF_DYN>
+lldb::addr_t NativeProcessLinux::GetELFImageInfoAddress() {
+  if (m_elf_image_info_addr != LLDB_INVALID_ADDRESS)
+    return m_elf_image_info_addr;
+
+  lldb::addr_t phdr_addr = GetAuxValue(ELFAuxVector::AUXV_AT_PHDR);
+  size_t phdr_entry_size = GetAuxValue(ELFAuxVector::AUXV_AT_PHENT);
+  size_t phdr_num_entries = GetAuxValue(ELFAuxVector::AUXV_AT_PHNUM);
+  lldb::addr_t load_base = phdr_addr - sizeof(ELF_EHDR);
+  if (phdr_addr == 0 || phdr_entry_size == 0 || phdr_num_entries == 0)
+    return LLDB_INVALID_ADDRESS;
+
+  // Find the PT_DYNAMIC segment (.dynamic section) in the program header and
+  // what the ELF believes to be the load base.
+  lldb::addr_t elf_load_base = 0;
+  bool found_elf_load_base = false;
+  lldb::addr_t dynamic_section_addr = 0;
+  uint64_t dynamic_section_size = 0;
+  bool found_dynamic_section = false;
+  ELF_PHDR phdr_entry;
+  for (size_t i = 0; i < phdr_num_entries; i++) {
+    size_t bytes_read;
+    auto error = ReadMemory(phdr_addr + i * phdr_entry_size, &phdr_entry,
+                            sizeof(phdr_entry), bytes_read);
+    if (!error.Success())
+      return LLDB_INVALID_ADDRESS;
+
+    if ((!found_elf_load_base || phdr_entry.p_vaddr < elf_load_base) &&
+        phdr_entry.p_type == llvm::ELF::PT_LOAD) {
+      elf_load_base = phdr_entry.p_vaddr;
+      found_elf_load_base = true;
+    }
+
+    if (phdr_entry.p_type == llvm::ELF::PT_DYNAMIC) {
+      dynamic_section_addr = phdr_entry.p_vaddr;
+      dynamic_section_size = phdr_entry.p_memsz;
+      found_dynamic_section = true;
+    }
+  }
+
+  if (!found_elf_load_base || !found_dynamic_section)
+    return LLDB_INVALID_ADDRESS;
+
+  // Find the DT_DEBUG entry in the .dynamic section
+  // The load base we got from the auxiliary vector is the source of truth. If
+  // we got a different load base from the ELF then we need to correct the
+  // .dynamic section address with the difference we found.
+  dynamic_section_addr += (load_base - elf_load_base);
+  ELF_DYN dynamic_entry;
+  size_t dynamic_num_entries = dynamic_section_size / sizeof(dynamic_entry);
+  for (size_t i = 0; i < dynamic_num_entries; i++) {
+    size_t bytes_read;
+    auto error = ReadMemory(dynamic_section_addr + i * sizeof(dynamic_entry),
+                            &dynamic_entry, sizeof(dynamic_entry), bytes_read);
+    if (!error.Success())
+      return LLDB_INVALID_ADDRESS;
+    // Return the &DT_DEBUG->d_ptr which points to r_debug which contains the
+    // link_map.
+    if (dynamic_entry.d_tag == llvm::ELF::DT_DEBUG) {
+      m_elf_image_info_addr = dynamic_section_addr + i * sizeof(dynamic_entry) +
+                              sizeof(dynamic_entry.d_tag);
+      return m_elf_image_info_addr;
+    }
+  }
+
+  return LLDB_INVALID_ADDRESS;
+}