Page MenuHomePhabricator

Implement GetSharedLibraryInfoAddress
ClosedPublic

Authored by aadsm on May 27 2019, 3:20 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

aadsm created this revision.May 27 2019, 3:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2019, 3:20 PM

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.

clayborg requested changes to this revision.Jun 3 2019, 10:21 AM
clayborg added inline comments.
lldb/include/lldb/Host/common/NativeProcessProtocol.h
430 ↗(On Diff #202651)

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

This revision now requires changes to proceed.Jun 3 2019, 10:21 AM
aadsm marked an inline comment as done.Jun 3 2019, 10:42 AM
aadsm added inline comments.
lldb/include/lldb/Host/common/NativeProcessProtocol.h
430 ↗(On Diff #202651)

oops, it certainly does not, my mistake.

aadsm updated this revision to Diff 202854.Jun 3 2019, 10:22 PM

Address comments

+@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 ↗(On Diff #202854)

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

2122 ↗(On Diff #202854)

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

2136 ↗(On Diff #202854)

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

2168 ↗(On Diff #202854)

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 marked 2 inline comments as done.Jun 4 2019, 7:33 AM
aadsm added inline comments.
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
2136 ↗(On Diff #202854)

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?

2168 ↗(On Diff #202854)

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 marked an inline comment as done.Jun 4 2019, 10:00 PM
aadsm added inline comments.
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
2122 ↗(On Diff #202854)

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 added inline comments.Jun 5 2019, 12:14 AM
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
2122 ↗(On Diff #202854)

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

2136 ↗(On Diff #202854)

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.

2168 ↗(On Diff #202854)

Ok, sounds good then.

aadsm added a comment.Jun 5 2019, 9:58 AM

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?

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 added a comment.Jun 5 2019, 12:23 PM

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 added a comment.Jun 5 2019, 11:10 PM

@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 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 added a comment.Jun 6 2019, 9:01 AM

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 added a comment.Jun 6 2019, 9:09 AM

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 updated this revision to Diff 203476.Jun 6 2019, 7:49 PM

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 added a comment.Jun 7 2019, 2:14 AM

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.

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 ↗(On Diff #203476)

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 updated this revision to Diff 203653.Jun 7 2019, 5:59 PM
aadsm marked 2 inline comments as done.

Address comments

aadsm updated this revision to Diff 203654.Jun 7 2019, 6:04 PM

Removed unwanted change

labath accepted this revision.Jun 9 2019, 11:11 PM

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 added a comment.Jun 10 2019, 9:14 AM

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 updated this revision to Diff 204422.Jun 12 2019, 10:05 PM

Improve tests

clayborg accepted this revision.Jun 14 2019, 10:57 AM
This revision is now accepted and ready to land.Jun 14 2019, 10:57 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2019, 2:12 PM