[PATCH] D30206: [DWARF5] Emit new unit header

[David Blaikie via llvm-commits]

On Tue, Feb 21, 2017 at 9:46 AM Paul Robinson via Phabricator <
reviews at reviews.llvm.org> wrote:

> probinson added a comment.
>
> In https://reviews.llvm.org/D30206#682359, @dblaikie wrote:
>
> >   (is the DwarfGenerator change tested at all? I mean it's a test
> utility, so maybe that's overkill, not sure)
>
>
> It's like this: I was originally going to avoid the llvm-dwarfdump changes
> in the first patch, but it turns out there are a couple of tests that
> specify version 5 in the IR and use llvm-dwarfdump, so I had to make the
> DebugInfo lib changes.  Having done that, the unit tests failed because the
> generator was not producing the new format.  So I had to fix the generator.
>
>
>
> ================
> Comment at: include/llvm/Support/Dwarf.def:806
> +HANDLE_DW_UT(0x05, split_compile)
> +HANDLE_DW_UT(0x06, split_type)
>
> ----------------
> dblaikie wrote:
> > Guess I should've read the DWARF5 spec. Why is there a split type unit?
> I remember Cary getting rid of this shortly after the first prototype of
> Fission. I'm asuming it's at least not required (ie: it's optional)?
> These are the unit types defined in the spec.  Split and non-split type
> units are nearly identical, the difference being that a split type unit
> can't use DW_AT_str_offsets_base, and of course where the units actually go.
>

Oh, right, sorry - got confused. I was worried about /skeleton/ type units,
but I see they're not there (there's just "skeleton" which is a skeleton
CU, I assume).


>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:693-694
>
> -  DwarfUnit::emitHeader(UseOffsets);
> +  DwarfUnit::emitCommonHeader(UseOffsets, Skeleton ? dwarf::DW_UT_skeleton
> +                                                   :
> dwarf::DW_UT_compile);
>  }
> ----------------
> dblaikie wrote:
> > This is probably backwards - "Skeleton" is a pointer to the skeleton (so
> it implies this unit is /not/ a skeleton). Test coverage should
> demonstrate/confirm this?
> So, you think I'm missing a split-dwarf test that uses v5?  I'll look into
> this.
>

Yep


>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:99
> +    AddrSize = debug_info.getU8(offset_ptr);
> +  }
>    if (IndexEntry) {
> ----------------
> aprantl wrote:
> > This should probably be tested by a llvm-dwarfdump test?
> Are there llvm-dwarfdump tests?


There are some - using checked in binary/object files.


>   I couldn't find any.  There is a test/tools/llvm-dwarfdump directory
> (with AArch64 and ARM subdirectories) but not actual files.
>

There are some in test/DebugInfo/dwarfdump* I think.


> This code was needed because there are a couple of debug-info tests that
> specify version 5 in the IR, and then use llvm-dwarfdump in their RUN
> lines.  Having done this change, I had to fix the unit-test DWARF generator
> because the unit tests were failing.


What was it that caused breakage in the DwarfGenerator once llvm-dwarfdump
had v5 support?


>   That was plenty to convince me that things were working.
>
> If you want a separate llvm-dwarfdump test as well, I can do that.
>
>
> https://reviews.llvm.org/D30206
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170221/9dfc7381/attachment.html>