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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Support/Dwarf.h:420
+/// These functions return the DWARF version when a constant was first defined.
+/// For most or all vendor extensions it returns 0.
+///
----------------
Not sure this comment quite makes sense (which is it, most or all? & it doesn't describe the contract). I'd be OK with "most" or "legacy" or some similar language?


================
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");
+    }
----------------
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.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:89
+                   << " for DWARF version " << AP->getDwarfVersion() << "\n");
+      assert(0 && "Invalid form for specified DWARF version");
+    }
----------------
llvm_unreachable is preferred over assert(0)


================
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));
----------------
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?


https://reviews.llvm.org/D30785