[PATCH] D31424: [ELF] - Use relocated content when generating .gdb_index

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:1739-1740
+  // consists of a 0 for the beginning address offset and a 0 for the ending
+  // address offset. We assume here that whole section data starting from offset
+  // is a range, terminated with null entry.
+  if (Optional<uint64_t> Offset = toSectionOffset(Die.find(DW_AT_ranges))) {
----------------
dblaikie wrote:
> I don't understand what assumption you're describing.
> 
> Oh, because relocations haven't been performed, nor handled at all - it's not possible to tell where the address ranges end, so you're providing an overestimate here? That's a bit subtle and the function name should at least mention that I would think (& the function that calls this one too)
> 
> Is there any penalty to this overestimate in how the gdb_index is created? Does a larger sized record need to be used to handle a maximum that may never be needed?
> 
> [looking at the implementation, it seems there is a penalty for overestimate - a table is reserved for that many address ranges, right? so much of that table may go unused if this is a significant overestimate (though that probably only happens if someone uses ld -r or, perhaps more likely: LTO)]
> 
> I wouldn't imagine it'd be too hard to look at enough relocation data (essentially parse the relocations applicable to debug_ranges and look for holes (bytes that don't have relocations for them) - those are the ends of range lists - you wouldn't need to apply any relocations or examine their addends, etc, only their offsets)
> 
> 
>I don't understand what assumption you're describing.
>
>Oh, because relocations haven't been performed, nor handled at all - it's not possible to tell where the address ranges end, so you're providing >an overestimate here? That's a bit subtle and the function name should at least mention that I would think (& the function that calls this one >too)

Right. Its straightforward and fastest way to do things I believe. It is possible to tell where is address ranges end only after handling or some parsing of relocations. I think we can find the relocation with largest offset in .debug_range section and that be enough to know that next offset is the end here. (but that would not solve everything, please see below)
I can probably rename getDieRangesCount to estimateDieRangesCount and getAddressRangesCount to estimateAddressRangesCount ?

>Is there any penalty to this overestimate in how the gdb_index is created? Does a larger sized record need to be used to handle a maximum >that may never be needed?
>
>[looking at the implementation, it seems there is a penalty for overestimate - a table is reserved for that many address ranges, right? so much >of that table may go unused if this is a significant overestimate (though that probably only happens if someone uses ld -r or, perhaps more >likely: LTO)]

Yes, penalty is size. I am not aware about LTO, I knew about ld -r here only. I think perfomance is preferable over possible size penalty in general. And this patch just a first step in direction to use relocated data, so we can think about how to solve that after this change probably.

>I wouldn't imagine it'd be too hard to look at enough relocation data (essentially parse the relocations applicable to debug_ranges and look for >holes (bytes that don't have relocations for them) - those are the ends of range lists - you wouldn't need to apply any relocations or examine >their addends, etc, only their offsets)

There is one more issue I mentioned in description - is when address range belongs to discarded section. This case can not be catched until relocations are scanned at later stages, because we cant just scan relocations early when we do not know which sections were discarded.
So since these issues are related, I would fix them in a separate patch(es). Scanning of relocations also will probably affect perfomance, and should be done in a some smart way.


https://reviews.llvm.org/D31424