[PATCH] D74425: [DebugInfo]: Added support for DWARFv5 Info section header parsing in llvm-dwp utility.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/tools/llvm-dwp/X86/info-v5.s:24
+	.short	12                      # DW_AT_language
+        .asciz  "int.c"                 # DW_AT_name
+	.asciz  "int.dwo"               # DW_AT_dwo_name
----------------
Some indentation still needs fixing in this file - perhaps there's a tab/space issue, I'm not sure? But if you examine it here on the web you can see several are misaligned (DWARF version number comment, produce, language, name, dwo_name comments, external attribute, etc... )


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:156
+      return make_error<DWPError>(
+          "unit type DW_UT_split_compile type not found in debug_info header");
+  }
----------------
SouraVX wrote:
> dblaikie wrote:
> > Might be worth specifying the unit type that was found? At least even if it's just the integer value "unexpected unit type 0x3 found in debug_info".
> > 
> > This should also be tested. (the existing test case could have a second CU in it that just has an incorrect unit type in its header)
> Wrapped this as string, since DWPError expects a string.
> 
> > This should also be tested. (the existing test case could have a second CU in it that just has an incorrect unit type in its header)
> 
> I've tested it locally, error production works fine. Should I also add negative test case.?
> If I add this in existing test case, then DWP packaging will detect/produce error and we'll loose primary verification for positive/Everthing fine case/ working llvm-dwp.
Yes, please add a test case. If adding it to the same file invalidates the existing test coverage there, then please add it as a separate test case.


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:159-162
   uint8_t AddrSize = InfoData.getU8(&Offset);
-
+  if (Version >= 5)
+    Signature = InfoData.getU64(&Offset);
   uint32_t AbbrCode = InfoData.getULEB128(&Offset);
----------------
dblaikie wrote:
> DWARFv5 reorders the abbreviation offset and the address size - so this code is /probably/ wrong/needs support for this part of DWARFv5. But that could wait for another patch (might be worth checking how the failure to parse that correctly is manifesting - if there's a good spot to demonstrate it in the test case by adding an extra CHECK line and a FIXME saying "hey, this is broken here/fix it soon" would be good)
> DWARFv5 reorders the abbreviation offset and the address size - so this code is /probably/ wrong/needs support for this part of DWARFv5. But that could wait for another patch (might be worth checking how the failure to parse that correctly is manifesting - if there's a good spot to demonstrate it in the test case by adding an extra CHECK line and a FIXME saying "hey, this is broken here/fix it soon" would be good)
Any thoughts on this ^ ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74425/new/

https://reviews.llvm.org/D74425