[PATCH] D30785: [DWARF] Versioning for DWARF constants; verify FORMs

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:85-90
+    if (!dwarf::isValidFormForVersion(AttrData.getForm(),
+                                      AP->getDwarfVersion())) {
+      DEBUG(dbgs() << "Invalid form " << format("0x%x", AttrData.getForm())
+                   << " for DWARF version " << AP->getDwarfVersion() << "\n");
+      assert(0 && "Invalid form for specified DWARF version");
+    }
----------------
probinson wrote:
> dblaikie wrote:
> > Might it be more helpful to fail at the point where the DIEAbbrev is created, rather than emitted? (more debuggable - the smoking gun, as it were) - would also save/avoid the need to plumb the dwarf version down through some of the layers added in this change.
> I will look into that.  I put it here because that's where the check for implicit_const had been.
The form/value information is created by DIE::addValue() (inherited from DIEValueList::addValue()).  These places do not have direct access to the DWARF version, we'd have to follow the DIE parent chain up to the unit.  Is that cheap enough to do every time we add any attribute to any DIE?  Alternatively, caching the version in the DIE makes every DIE a word bigger.  Or leave the check here.

(There's currently no "plumbing" of the version down through layers in this patch, except for dsymutil which for correctness has to remember the max version number it has seen anyway. Moving the check to DIEValueList would introduce some plumbing.)


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:297-298
   Die.addValue(DIEValueAllocator, dwarf::DW_AT_signature,
-               dwarf::DW_FORM_ref_sig8, DIEInteger(Signature));
+               getDwarfVersion() >= 4 ? dwarf::DW_FORM_ref_sig8
+                                      : dwarf::DW_FORM_data8,
+               DIEInteger(Signature));
----------------
dblaikie wrote:
> Still have uncertainty about this change - type units are only supported in DWARF4 and above. I think you were saying this was used by a particular prototype of modular debug info that ended up being a dead-end and Adrian removed.
> 
> Is this still needed?
Now that Adrian has removed the other usage, yes this is no longer necessary.


https://reviews.llvm.org/D30785