[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:82-89
+    if (!dwarf::isValidFormForVersion(AttrData.getForm(),
+                                      AP->getDwarfVersion())) {
+      std::string msg;
+      raw_string_ostream Msg(msg);
+      Msg << "Invalid form " << format("0x%x", AttrData.getForm())
+          << " for DWARF version " << AP->getDwarfVersion();
+      report_fatal_error(Msg.str());
----------------
dblaikie wrote:
> Should this be an assertion instead?
I dithered on that.  An assertion would tell you "something bad happened somewhere" with no details, because there's no way I know of to report a runtime value in an assertion. So you don't know what FORM is causing the problem.  Doing it this way was more valuable when figuring out the test failures that I had to fix.
But, it's true if we detect a problem here, that indicates an internal inconsistency rather than a problem with our external input, so on that basis an assertion would be more the thing.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:918-919
     addFlag(Buffer, dwarf::DW_AT_declaration);
-    return addDIETypeSignature(Buffer, dwarf::DW_AT_signature, Identifier);
+    if (getDwarfVersion() >= 4)
+      addDIETypeSignature(Buffer, dwarf::DW_AT_signature, Identifier);
+    return;
----------------
dblaikie wrote:
> This is probably incorrect (short of the broader discussion I started about how references to type units should be implemented - I should ping that thread).
> 
> If the user requests type units, either it should fail if asking for a DWARF version that doesn't support type units - or ignore the conflict and produce the signature anyway?
The signature attribute was coming out even without type units.  This was added by Adrian for "external" types, something to do with modules.
I would be okay with emitting the v4 attribute in earlier versions, but not the v4 FORM.  We'd have to pick something other than FORM_ref_sig8 for that case.


https://reviews.llvm.org/D30785