jasonmolenda added a comment.

In D144528#4145610 <https://reviews.llvm.org/D144528#4145610>, @bulbazord wrote:

> In D144528#4145593 <https://reviews.llvm.org/D144528#4145593>, @jasonmolenda 
> wrote:
>
>> 



>> tl;dr it's fine if LINKEDIT's "overlap" because lldb will never need to take 
>> an addr_t and figure out which Section it is located in.  (because an addr_t 
>> in the LINKEDIT segment of the shared cache would point to EVERY ObjectFile 
>> in the shared cache, if it was all reported correctly.)
>>
>> We may find that enabling this warning fires for some unintended situation 
>> that we're not looking at right now, but we can re-evaluate if that turns 
>> out to be the case.
>
> Out of curiosity, is the scenario you're describing not already handled by 
> `DynamicLoaderDarwin::UpdateImageLoadAddress`? That method specifically 
> checks to see if a section is `__LINKEDIT` to figure out if we should be 
> emitting a warning. Should I be doing the same thing here?

We don't get any new warnings with your patch when it is run against a macOS 
etc process using a shared cache; it behaves correctly.  The comment in 
`SectionLoadList::SetSectionLoadAddress` specifically talks about the LINKEDIT 
case, saying that the DynamicLoader will do the right thing, as you noted.  But 
it made me try to think through if there might be similar metadata segments 
that aren't actually loaded in memory, or are inconsequential if they're loaded 
or not.  We may find when some living on time that there are other cases, but I 
can't think of them right now - I say give the patch a try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144528

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to