[PATCH] D30357: [ObjectYAML] NFC. Refactor DWARFYAML CompileUnit dump code

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: lib/ObjectYAML/DWARFVisitor.cpp:59
+              writeSize =
+                  DebugInfo.DebugLines[0].TotalLength == UINT32_MAX ? 8 : 4;
+            OnVariableSizeValue(FormVal->Value, writeSize);
----------------
beanz wrote:
> beanz wrote:
> > probinson wrote:
> > > beanz wrote:
> > > > aprantl wrote:
> > > > > This would be easier to read if written like this:
> > > > > ```
> > > > > uint64_t?? writeSize;
> > > > > if (!DebugInfo.DebugLines.empty())
> > > > >   writeSize =  DebugInfo.DebugLines[0].TotalLength == UINT32_MAX ? 8 : 4;
> > > > > else if (Unit.Version == 2)
> > > > >   writeSize = Unit.AddrSize
> > > > > else
> > > > >   writeSize = 4;
> > > > > ```
> > > > There is actually some slightly incorrect logic here. The correct logic should be more like:
> > > > 
> > > > 
> > > > ```
> > > > size_t writeSize = 4;
> > > > if (Unit.Version == 2)
> > > >   writeSize = Unit.AddrSize;
> > > > else if (!DebugInfo.DebugLines.empty())
> > > >   writeSize = DebugInfo.DebugLines[0].TotalLength == UINT32_MAX ? 8 : 4;
> > > > ```
> > > > 
> > > > The idea being that size 4 as a default is sane. If version == 2, then you use AddrSize, otherwise it is based on whether it is DWARF32 or DWARF64 which requires the Line table.
> > > Why do you need the line table?  Every unit has a length that lets you determine its 32/64 format.
> > Reading the DWARF 4 spec (7.2.2 Initial Length Values) I believe you are correct that the unit's should start with a length that lets you determine the 32/64 format, however I don't believe that is how our parser is written:
> > 
> > https://github.com/llvm-mirror/llvm/blob/master/lib/DebugInfo/DWARF/DWARFUnit.cpp#L91
> > 
> > It looks to me like we only ever read a 32-bit value for the length from the unit.
> > 
> > Am I misunderstanding something here?
> Answered this one for myself... not misunderstanding. LLVM's DWARF parser is handling DWARF64 wrong. That will need to get fixed.
Also looks like we don't emit DWARF64 Units in LLVM. That seems like it could be bad:

https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/DwarfUnit.cpp#L1549


https://reviews.llvm.org/D30357