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

[Paul Robinson via Phabricator via llvm-commits]

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.


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


================
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?  I couldn't find any.  There is a test/tools/llvm-dwarfdump directory (with AArch64 and ARM subdirectories) but not actual files.

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