[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:4
+# RUN: llvm-mc --filetype=obj --split-dwarf-file=%t.dwo -dwarf-version=5 %s -o %t.o
+# RUN: llvm-dwarfdump -v %t.dwo | FileCheck %s
+
----------------
Skip this - the DWARFv5 dwo files/dumping I think is already pretty well tested, I don't think there's much need to retest it here. (handy to test personally to validate some things, but probably not worth carrying as a checked in test)


================
Comment at: llvm/test/tools/llvm-dwp/X86/info-v5.s:9
+
+#CHECK-DAG: .debug_abbrev.dwo contents:
+#CHECK-DAG: .debug_info.dwo contents:
----------------
Not sure what this check for debug_abbrev is? Check that it's present? 

It might be more valuable to check the cu_index to check the contribution sizes of the various sections & demonstrate that the CU DWO ID was correctly parsed and inserted into the cu_index table. Please do that instead.


================
Comment at: llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s:9
+	.short	5                       # DWARF version number
+	.byte	8                       # Address Size (in bytes)
+	.long	0                       # Offset Into Abbrev. Section
----------------
Rather than removing the unit type (& causing the address size to be read as the unit type) - please include an explicit unit type that is incorrect. I think that'll make the test easier to read - so it's obvious what value the error is referring to, rather than the comment misleadingly suggesting this is the address size field when it's really the unit type field.


================
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);
----------------
SouraVX wrote:
> dblaikie wrote:
> > 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 ^ ?
> Yes, I noticed this thing in Spec couple of times. But since llvm-dwarfdump is also not detecting while parsing, I took  it as an expected/planned behavior.
I don't think that's correct - it looks like this code is buggy but just not failing in a way that's been tested/examined yet.

Ah, I see - looks like this is ignoring the abbrev offset, assuming it will be zero which is sort of OK, but sort of not. Then it's reading the address size which it's not using (surprising there's no unused variable warning)

The code should not be left as-is for very long because it is erroneously parsing the content & while the values may not currently be used (partly sub-optimal/possibly buggy, partly because they aren't needed) it's still a trap set to trip over anyone working with this code when that's not actually the order of fields in DWARFv5.

Please add a FIXME around the AddrSize and AbbrevOffset parsing logic to fix it to parse these correctly & possibly respect the abbrev offset, or at least error-check that it's always zero.






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

https://reviews.llvm.org/D74425