[Lldb-commits] [PATCH] D62505: Fix multiple module loaded with the same UUID

Thu May 30 15:23:07 PDT 2019

> On May 28, 2019, at 7:59 AM, Pavel Labath via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> labath added a comment.
> 
> In D62505#1519166 <https://reviews.llvm.org/D62505#1519166>, @aadsm wrote:
> 
>> Interesting, I did miss that comment when I checked that class. This is something @clayborg was concerned with when I discussed this solution with him. The main reason I was not keen in having multiple load addresses for a section is because that would change the GetLoadAddress to return an array instead of an addr_t or some kind of mechanism to be able to correctly resolve the load address of an Address (felt like too big of a change for what it looks like an edge case?).
> 
> 
> I wouldn't say this is specific to some class of samsung phones. It is easy to use dlmopen(3) to open the same library multiple times. Just try calling `dlmopen(LM_ID_NEWLM, "/lib64/libc.so.6", RTLD_NOW)` in a loop a couple of times and see what happens in /proc/<pid>/maps. (I believe this is the same function that android uses to implement vndk separation.)
> 
> In D62505#1519178 <https://reviews.llvm.org/D62505#1519178>, @clayborg wrote:
> 
>> So if we end up going with one module, how do we propose to get around the fact that there are multiple platform paths ("/system/lib/libcutils.so" and "/system/lib/vndk-sp/libcutils.so") for this module? Just make platform path be an array instead of a single path?
> 
> 
> TBH, I'm not really convinced that the "platform path" should be a property of the Module.
> 
> Imagine a situation where I have the same module loaded in multiple targets (maybe on different remote platforms, or maybe not). It makes sense that each target can have the module located at a different path. So maybe the "platform path" should be a property of the target,  saying "where did this target load the module from"? Of course, this won't help when you have the same module loaded multiple times in the same target, so maybe this does need to be a list of paths? I'm not sure, I'm just thinking aloud here...
> 
> Right now, I'm also lean towards making a Section be loadable multiple times, however this definitely needs more discussion. I'm interested to hear what @jingham thinks of this.

I agree that the platform path has to be held by the target, not by the module.  In your example lldb would love to have a single local copy of the executable so it can do reads locally, which makes it clear that there are real cases where it would be obvious two targets should share the same Module, but would need to have different remote names for the it.  

I don't see how extending the Platform Path in the module to be a vector can handle this situation.  We have tried to keep Modules from knowing about Targets, but the only way to make sense of all the platform paths associated with the Module is to know which one is used by a given Target.  So you'd really have to have a pair of Name & Target.  That argues that the Module was the wrong place for this information to begin with.

You could imagine doing this by having a map of platform path -> module in the target, but then there are all sorts of places you'd have to be careful to consult this, and that seems fallible.

It really sounds like the Target should be dealing with TargetModules (a pair of Module & PlatformPath) and not straight Modules.  Probably for the sake of reducing changes we could have "CacheModules" which are what Modules are today, and Modules that are the pair of PlatformPath and Module.  I think this is what Greg was talking about with his BaseModule class.

Then if you want to support the same CacheModule being reused in a given target, the SectionLoadList  would have to hold a pair of Section/TargetModule - a TargetSection? - so it would know which instance of the module was meant.  I think you also have to do the same thing with Address.  It currently holds a Section, but that isn't fully specified in the case where the Module can appear twice.  It would have to have a TargetSection instead.  Except that you can pull Address's out of Modules w/o going through a Target.  So again you might have to make a distinction between Address and TargetAddress, which might get messy.

And again, if we do any of this we probably want to leave the bare names (Address, Section, Module) be the things you get from a Target - which is how most of the upper layers of lldb get these entities, and have some other name for the equivalents as they live in the Cache separate from Targets.

This looks like it could be fairly intrusive, but OTOH seems to accurately represent the more complex situation you are trying to describe, so will probably cause less pain in the long run.

Jim

> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D62505/new/
> 
> https://reviews.llvm.org/D62505
> 
> 
> 