[DWARF5][DWARFVerifier] Check that Skeleton compilation unit does not have children.

[avl]

Summary

That patch adds checking into DWARFVerifier that the Skeleton
compilation unit does not have children.

[probinson]

I like it, but the others should have a chance to say something.

llvm/test/DebugInfo/skeleton-unit-verify.ll

3 ↗

(On Diff #233024)

There is no llc option to control this? (Sorry, I'm lazy, haven't looked...)

[aprantl]

The LLVM part LGTM, thanks!

clang/lib/Driver/ToolChains/Clang.cpp

3542 ↗

(On Diff #233024)

What does this have to do with the dwarfdump patch?

[aprantl]

llvm/test/DebugInfo/skeleton-unit-verify.ll

3 ↗

(On Diff #233024)

You might want to use an assembler test instead. For broken debug info, that seems reasonable.

[probinson]

clang/lib/Driver/ToolChains/Clang.cpp

3542 ↗

(On Diff #233024)

So that by default we emit DWARF that passes the verifier?

llvm/test/DebugInfo/skeleton-unit-verify.ll

3 ↗

(On Diff #233024)

We want to be able to emit compliant DWARF (with splitDebugInlining = false) and DWARF that is useful for backtraces in the absence of the .dwo file, that we properly detect as non-compliant (with splitDebugInlining = true).

[aprantl]

clang/lib/Driver/ToolChains/Clang.cpp

3542 ↗

(On Diff #233024)

Sorry for being terse earlier. The review says this is a DWARFVerifier patch so it shouldn't touch clang. The clang part should be split into a separate commit with a different commit message.

[dblaikie]

clang/lib/Driver/ToolChains/Clang.cpp

3542 ↗

(On Diff #233024)

+1 to this needing to be separate patches (& if there's any automation that's testing that some clang compiled binary passes the verifier that this verifier change would break, then the change in clang's defaults should go in first & then the verifier change after that to ensure things aren't broken between them)

llvm/test/DebugInfo/skeleton-unit-verify.ll

1 ↗

(On Diff #233024)

What does this functionality/test have to do with split-dwarf-cross-cu-references? That seems orthogonal/unrelated to this change? (& that feature is only related to LTO - this code change seems unrelated to LTO & the problematic cases would appear with or without LTO)

9–21 ↗

(On Diff #233024)

The simplest example would be:

void f1();
__attribute__((always_inline)) void f2() {
  f1();
}
void f3() {
  f2();
}

That produces inlining without extra parameter types, etc, etc. Doesn't require optimizations & I'm not sure that "-gno-column-info" would be related/relevant (nor DWARFv5 - though I'm happy for split-dwarf related tests to be written in DWARFv5 by default so we can maybe remove the v4 support one day) - presumably the command line should have -gsplit-dwarf on it?

[probinson]

clang/lib/Driver/ToolChains/Clang.cpp

3542 ↗

(On Diff #233024)

Oh, good point. And a more explicit driver test, now that I look at it that way.

clang/test/Driver/split-debug.c

82 ↗

(On Diff #233024)

There should be a test verifying that -fno-split-dwarf-inlining is the default. Up around line 67 there's a test that passes it to the driver and then checks it gets passed to cc1; remove the option from that driver command line.

[avl]

clang/lib/Driver/ToolChains/Clang.cpp

3542 ↗

(On Diff #233024)

Ok, I would split the patch into two pieces.

clang/test/Driver/split-debug.c

82 ↗

(On Diff #233024)

Ok.

llvm/test/DebugInfo/skeleton-unit-verify.ll

1 ↗

(On Diff #233024)

initially I created the test for two compilation units, default state of split-dwarf-cross-cu-references avoids -fsplit-dwarf-inlining thus I added it. Now it is redundant. would delete it.

3 ↗

(On Diff #233024)

There is no llc option to control this? (Sorry, I'm lazy, haven't looked...)

right. there is no such an option.

9–21 ↗

(On Diff #233024)

Ok, I will update the test accordingly.

as to the -gsplit-dwarf - it is not required for generating .ll files by clang. for the llc there was set --split-dwarf-file=test.dwo.

[avl]

addressed comments(removed clang part, reworked testcase).

[aprantl]

llvm/test/DebugInfo/X86/skeleton-unit-verify.ll

3 ↗

(On Diff #233318)

If you really want to use sed here (I think an assembler or yaml2obj test would be more appropriate for malformed dwarf) you need to add REQUIRES: shell

[probinson]

llvm/test/DebugInfo/X86/skeleton-unit-verify.ll

3 ↗

(On Diff #233318)

I now agree with Adrian, a hand-coded .debug_info section in assembler would be better. There are other tests that verify splitDebugInlining=true does the right thing. You will want one split CU with a tag that has no children, and one with a tag that does; the verifier should detect only the one error.
The test does not need a lot of attributes in the compile_unit tags, it can be very minimal.

[avl]

addressed comments(changed test, used an asm test)

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

unsigned DWARFVerifier::verifyUnitContents(DWARFUnit &Unit) {
   uint8_t UnitType = Unit.getUnitType();
   if (!DWARFUnit::isMatchingUnitTypeAndTag(UnitType, Die.getTag())) {
     error() << "Compilation unit type (" << dwarf::UnitTypeString(UnitType)
             << ") and root DIE (" << dwarf::TagString(Die.getTag())
             << ") do not match.\n";
     NumUnitErrors++;
   }
 
+  //  According to DWARF Debugging Information Format Version 5,
+  //  3.1.2 Skeleton Compilation Unit Entries:
+  //  "A skeleton compilation unit has no children."
+  if (Die.getTag() == dwarf::DW_TAG_skeleton_unit && Die.hasChildren()) {
+    error() << "Skeleton compilation unit has children.\n";
+    NumUnitErrors++;
+  }
+
   DieRangeInfo RI;
   NumUnitErrors += verifyDieRanges(Die, RI);
 
   return NumUnitErrors;
 }
 
 unsigned DWARFVerifier::verifyDebugInfoCallSite(const DWARFDie &Die) {
   if (Die.getTag() != DW_TAG_call_site && Die.getTag() != DW_TAG_GNU_call_site)

llvm/test/DebugInfo/X86/skeleton-unit-verify.s

+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t.o
+# RUN: not llvm-dwarfdump --verify %t.o | FileCheck %s
+
+# CHECK: Verifying .debug_abbrev...
+# CHECK-NEXT: Verifying .debug_info Unit Header Chain...
+# CHECK-NEXT: error: Skeleton compilation unit has children.
+# CHECK-NEXT: Verifying .debug_info references...
+# CHECK-NEXT: Verifying .debug_types Unit Header Chain...
+# CHECK-NEXT: Errors detected.
+
+        .section .debug_abbrev,"",@progbits
+        .byte   1                       # Abbreviation Code
+        .byte   74                      # DW_TAG_skeleton_unit
+        .byte   0                       # DW_CHILDREN_no
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
+        .byte   2                       # Abbreviation Code
+        .byte   74                      # DW_TAG_skeleton_unit
+        .byte   1                       # DW_CHILDREN_yes
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
+        .byte   0                       # EOM(3)
+
+        .section        .debug_info,"",@progbits
+.Lcu_begin0:
+        .long   .Lcu_end0-.Lcu_start0 # Length of Unit
+.Lcu_start0:
+        .short  5                       # DWARF version number
+        .byte   4                       # DWARF Unit Type
+        .byte   8                       # Address Size (in bytes)
+        .long   .debug_abbrev           # Offset Into Abbrev. Section
+        .quad   -6573227469967412476
+        .byte   1                       # Abbrev [1] 
+        .byte   0
+.Lcu_end0:
+        .long   .Lcu_end1-.Lcu_start1 # Length of Unit
+.Lcu_start1:
+        .short  5                       # DWARF version number
+        .byte   4                       # DWARF Unit Type
+        .byte   8                       # Address Size (in bytes)
+        .long   .debug_abbrev           # Offset Into Abbrev. Section
+        .quad   -6573227469967412476
+        .byte   2                       # Abbrev [2]
+        .byte   0
+.Lcu_end1:
+
+