[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/wrong-unit-type-info-v5.s:4
+
+# CHECK: error: {{.*}}: unit type DW_UT_split_compile type not found in debug_info header. Unexpected unit type '0x12'found!
+	.section	.debug_info.dwo,"e", at progbits
----------------
missing space between the unexpected unit type and the word 'found'.

(do other error messages use quotation marks around integer constants like this? I'd guess not & would recommend following that convention here - but could you check/verify that?)


================
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:
> > 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.
> > 
> > 
> > 
> > 
> > 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)
> 
> It;s getting used while skipping the DIE. Line:201. + I think, that has to be read anyways, to advance the offset/cursor by that amount for reading further data.
> 
> > 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.
> 
> Can you please, hint/describe a scenario, when AbbrevOffset different than zero in a DWO object.
> 
>>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)
>
>It;s getting used while skipping the DIE. Line:201. + I think, that has to be read anyways, to advance the offset/cursor by that amount for reading further data.

Then it seems important that this gets parsed correctly in v4 and v5 when the fields change location? Otherwise the address size that will be used would actually be the DWARF version number instead?

>>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.
>
>Can you please, hint/describe a scenario, when AbbrevOffset different than zero in a DWO object.

The DWARF spec is generally fairly permissive - so "if it doesn't disallow it, it's probably/kind-of allowed". In this case there's no reason a DWARF producer couldn't hide an extra payload in debug_abbrev before the actual debug abbrev contribution, for instance.

At some point we might eventually want to figure out what/if it looks like to support LTO and Split DWARF which might involve multiple CUs in a single .dwo file, in which case then it could involve multiple debug_abbrev contributions and non-zero abbrev offsets.


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

https://reviews.llvm.org/D74425