[PATCH] D36097: Prototype fix for lld DWARF parsing of base address selection entries in range lists

[David Blaikie via llvm-commits]

On Tue, Aug 1, 2017 at 3:16 AM George Rimar via Phabricator <
reviews at reviews.llvm.org> wrote:

> grimar added a comment.
>
> > A recent improvement to LLVM produces DWARF compliant, but (apparently)
> > infrequently used range lists - using base address selection entries to
> > reduce the number of relocations required (& reduce file size &
> > hopefully link time)
> >
> > This breaks lld's use of LLVM's libDebugInfo which isn't correctly
> > tracking the section index in the presence of this kind of debug info.
>
> Do you have any sample code/testcase/anything that shows/can help to
> reproduce the issue ?
> (looking at implementation I think I know how to generate DWARF section
> content manually, but I
> am not sure what should I do to reproduce issue using cpp+clang for
> example).
>

Ah, sure. The functionality was committed to clang over the weekend, but
placed behind a flag on Monday, but you can produce a baes address
specifier with for example the following code:

  void f2() {
  }
  __attribute__((nodebug)) void f1() {
  }
  void f3() {
  }

  $ clang-tot test.cpp -g -c -mllvm -use-dwarf-ranges-base-address-specifier

The assembly for this has a debug_ranges that looks like this:

        .section        .debug_ranges,"", at progbits
.Ldebug_ranges0:
        .quad   -1
        .quad   .Lfunc_begin0
        .quad   .Lfunc_begin0-.Lfunc_begin0
        .quad   .Lfunc_end0-.Lfunc_begin0
        .quad   .Lfunc_begin2-.Lfunc_begin0
        .quad   .Lfunc_end2-.Lfunc_begin0
        .quad   0
        .quad   0


> > Here's my first blush at a fix for libDebugInfo - though it does rais
> > some questions and needs testing (is there any testing in LLVM (not lld)
> > for the section index API in libDebugInfo? it'd be good if there was,
> > maybe API level testing).
>
> I do not think we have. That looks to be my fault, I don't think I added
> LLVM testcase when implemented section index API.
> I'll check what can I do for it.
>

I'm guessing it'll need to be a unit test - I don't imagine the section
index is used by any of the LLVM tools (llvm-dwarfdump or llvm-symbolizer).
/maybe/ there's a useful thing that could be exposed using SectionIndex in
dwarfdump (eg: llvm-dwarfdump on an object could print the name of a
section next to any addresses - which could be handy when reading ranges
and the like)


>
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDebugRangeList.cpp:75
> +                     BaseAddress + RLE.EndAddress,
> +                     BaseAddress ? SectionIndex : RLE.SectionIndex});
>      }
> ----------------
> >How should/does this differentiate between the case where the unit's
> >base address is actually zero? compared to when it's not present? Does
> >that matter? I'm not sure.
>
> So DWARF spec says:
> //The applicable base address of a range list entry is determined by the
> closest preceding base
> address selection entry (see below) in the same range list. If there is no
> such selection entry, then
> the applicable base address defaults to the base address of the
> compilation unit (see Section
> 3.1.1).//
>
> Should `BaseAddress` and `SectionIndex` be optional then ? Then code can
> be like following probably:
>
> ```
> DWARFAddressRangesVector
> DWARFDebugRangeList::getAbsoluteRanges(Optional<uint64_t> BaseAddress,
>                                         Optional<uint64_t>
> BaseSectionIndex) const {
>   DWARFAddressRangesVector Res;
>   for (const RangeListEntry &RLE : Entries) {
>     if (RLE.isBaseAddressSelectionEntry(AddressSize)) {
>       BaseAddress = RLE.EndAddress;
>       BaseSectionIndex = RLE.SectionIndex;
>       continue;
>     }
>
>     uint64_t Start = RLE.StartAddress;
>     uint64_t End = RLE.EndAddress;
>     uint64_t SectionIndex = RLE.SectionIndex;
>     if (BaseAddress) {
>       Start  += *BaseAddress;
>       End += *BaseAddress;
>       SectionIndex = *BaseSectionIndex;
>     }
>
>     Res.push_back({Start, End, SectionIndex });
>   }
> }
> ```
>
>
Yeah, may be the right thing - at that point I'd suggest moving the
BaseAddress and BaseSectionIndex into a pair/struct and wrapping that in
Optional (rather than each being separately optional).


>
>
> https://reviews.llvm.org/D36097
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170801/6975498a/attachment.html>