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

[Adrian Prantl via llvm-commits]

> On Feb 21, 2017, at 9:55 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Feb 21, 2017 at 9:46 AM Paul Robinson via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> probinson added a comment.
> 
> In https://reviews.llvm.org/D30206#682359 <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 don't think we need to go that far. A round-trip test would be good enough, since the testcase included in this review already checks for the correct binary output format of the DWARF generator.

>  
>   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.

And if there aren't any, we should create that directory to encourage future use :-)

>  
> 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 <https://reviews.llvm.org/D30206>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170221/7ed8dca0/attachment.html>