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

[Frederic Riss via Phabricator via llvm-commits]

friss added a comment.

Some random comments:



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:241
 
+  static bool validateUnitType(uint8_t UnitType) {
+    return UnitType == dwarf::DW_UT_compile || UnitType == dwarf::DW_UT_type ||
----------------
Not important, but I would call this isValidUnitType().


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:254
+  /// DWARF5 is defined as one of the following six types.
+  static uint32_t computeDWARF5HeaderSize(uint8_t UnitType) {
+    switch (UnitType) {
----------------
Really nit-picky: This doesn't seem to 'compute' anything. Why not just getDWARF5HeaderSize()?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:264
+    case dwarf::DW_UT_partial:
+    default:
+      return 12;
----------------
I don't think we should return 12 when hitting the default case. If every use of computeDWARF5HeaderSize() is guarded by validateUnitType(), then having the default case be llvm_unreachable makes more sense.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFVerifier.h:39
   DWARFContext &DCtx;
+  VerifyFlags HeaderFlags;
   /// A map that tracks all references (converted absolute references) so we
----------------
I don't like having that state which is only used for one check at the class level. Although, the name of the struct is way too generic. 'struct UnitHeaderFlags' maybe?


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:47-48
+    AbbrOffset = DebugInfoData.getU32(Offset);
+    HeaderFlags.ValidType = DWARFUnit::validateUnitType(UnitType);
+    HeaderSize = DWARFUnit::computeDWARF5HeaderSize(UnitType);
+  } else {
----------------
I wouldn't try to get a size if the unit type is not recognized. It seems wrong. I'm not sure we should try to go any further than this when we can't figure out the unit type anyway.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:85
+        OS << "Unit is in 64-bit DWARF format; cannot verify.\n";
+        return true;
+      }
----------------
I understand that we do not really support DWARF64, but why make it completely fatal. We could still extract the length of the unit and skip it, right? Of course, I doubt many programs link DWARF64 with 32...


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:87-98
+      if (!HeaderFlags.ValidLength)
+        OS << "\tError: The length for this unit is too "
+              "large for the .debug_info provided.\n";
+      if (!HeaderFlags.ValidVersion)
+        OS << "\tError: The 16 bit unit header version is not valid.\n";
+      if (!HeaderFlags.ValidType)
+        OS << "\tError: The unit type encoding is not valid.\n";
----------------
Why not emit the verifier errors from inside the verifyUnit() function? This would remove the need for the VerifyFlags struct. I believe it would also make the logic in verifyUnit simpler.


https://reviews.llvm.org/D35166