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

Thu Jun 6 09:02:51 PDT 2019

aadsm added a comment.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62501