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

[Paul Robinson via Phabricator via llvm-commits]

probinson 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.
+///
----------------
dblaikie wrote:
> 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?
The policy for different constants will vary widely.  For example, DW_AT_MIPS_linkage_name really should be used only *before* the equivalent standard attribute existed.   The GNU forms for split DWARF should be used only in v4 with experimental split-DWARF.  The GNU TLS opcode should be used only with GDB tuning.
This means I think there is no concise contract to specify; it would have to be something along these lines:

For constants defined in the DWARF standard, returns the version when the constant was first defined.  For vendor extensions, returns a number that might be useful in policy decisions about when the constant can be used.



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


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:89
+                   << " for DWARF version " << AP->getDwarfVersion() << "\n");
+      assert(0 && "Invalid form for specified DWARF version");
+    }
----------------
dblaikie wrote:
> llvm_unreachable is preferred over assert(0)
I knew that didn't look right...


https://reviews.llvm.org/D30785