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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked an inline comment as done.
SouraVX added inline comments.


================
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:
> 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.



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

https://reviews.llvm.org/D74425