[PATCH] D35166: [DWARF] Introduce verification for the unit header chain in .debug_info section to llvm-dwarfdump.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Just a few drive by thoughts, but happy to leave this mostly to Adrian



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:250
+  static uint32_t computeDWARF5HeaderSize(uint8_t UnitType) {
+    uint32_t HeaderSize = 12;
+    switch (UnitType) {
----------------
Might be nice for consistency to have the default in the switch as well.

Also might be easier to return directly rather than having an extra variable:

  switch (UnitType) {
  case skeleton:
  case split_compile:
    return 20;
  case type:
  case split-type:
    return 24;
  case compile:
  case partial:
  default:
    return 12;
  }

That said, should this switch have a default? Or is it reasonable to say "this must be called with a valid unit type, of which there are only these 6"?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFVerifier.h:33
   std::map<uint64_t, std::set<uint32_t>> ReferenceToDIEOffsets;
+  uint32_t NumDebugInfoUnitHeaderChainErrors = 0;
   uint32_t NumDebugInfoErrors = 0;
----------------
Why is this a member variable? It looks like it's only used within a local scope of handleDebugInfoUnitHeaderChain


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:423-425
+    OS << "Verification of .debug_info unit header chain failed; no need to "
+          "verify .debug_info contents\n";
+    Success = false;
----------------
Arguably, each unit with a valid header (that doesn't go off the end, etc) could be validated - just because one header is mangled doesn't mean the rest are (granted if it's mangled in a way that means the length is wrong and the rest of the headers from then on are probably off - then there's not much you can do with everything after the broken one, of course). Not necessary, but not impossible either. If it's going to stay this way maybe the error could be rephrased "unit contents will not be verified" (not that tehy don't need to be, or that they wouldn't benefit from being verified, etc)


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:50
+    OffsetStart = Offset;
+    ValidOffset = DebugInfoData.isValidOffset(Offset);
+    Length = DebugInfoData.getU32(&Offset);
----------------
Looks like this could never return false - since if the Offset+HeaderSize is a valid offset, then 'Offset' must be too (since it's a zero based array index - so X + N (where X and N are positive) can't be valid while X is invalid)


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:74
+        !ValidAbbrevOffset || !ValidType) {
+      ++NumDebugInfoUnitHeaderChainErrors;
+      OS << format("Unit Start Offset: 0x%08x \n", OffsetStart);
----------------
Looks like this probably only needs to be a boolean flag (& a local, as mentioned earlier) rather than a counter.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:75
+      ++NumDebugInfoUnitHeaderChainErrors;
+      OS << format("Unit Start Offset: 0x%08x \n", OffsetStart);
+      if (ValidOffset) {
----------------
When does this happen? Seems like it could only happen if the debug_info section was empty, maybe? (Otherwise the previous unit would've already failed because it was too long) Perhaps it'd be better to have a special case for empty? (or maybe it's not an error, really?)


https://reviews.llvm.org/D35166