[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);
----------------
probinson wrote:
> beanz wrote:
> > 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
> What do you mean by "could be bad"?  We can consider it a bug that the code you cite does not guard against a unit too big for DWARF32 format, but failing to support DWARF64 seems like a missing feature rather than something inherently bad.
I was perhaps being a bit glib there, and I apologize.

When I said it could be bad I was referring to what you more clearly stated; that we could be overflowing the size calculation on Units. In practice I'm sure that is highly unlikely.


https://reviews.llvm.org/D30357