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

[Spyridoula Gravani via Phabricator via llvm-commits]

sgravani added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:250
+  static uint32_t computeDWARF5HeaderSize(uint8_t UnitType) {
+    uint32_t HeaderSize = 12;
+    switch (UnitType) {
----------------
dblaikie wrote:
> 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"?
I think it makes more sense not to have the default. I modified this to reflect that the only valid arguments are these 6 types for dwarf5.
Thanks!


================
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;
----------------
dblaikie wrote:
> 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)
I agree that there's still value in verifying the contents of .debug_info, even when the  unit header  chain check fails. I'm changing this to have verification of the debug info section in any case. Thanks.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:50
+    OffsetStart = Offset;
+    ValidOffset = DebugInfoData.isValidOffset(Offset);
+    Length = DebugInfoData.getU32(&Offset);
----------------
dblaikie wrote:
> 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)
You're right. I fixed this part. 


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:75
+      ++NumDebugInfoUnitHeaderChainErrors;
+      OS << format("Unit Start Offset: 0x%08x \n", OffsetStart);
+      if (ValidOffset) {
----------------
dblaikie wrote:
> 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?)
You're right. The only case would be if .debug_info section was empty.
I'm not sure if we should handle this as an error.
I think I would like to go with printing a warning message for this case (since we're verifying the .debug_info section and .debug_info is empty).


https://reviews.llvm.org/D35166