<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Mar 9, 2017 at 10:49 AM Paul Robinson via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">probinson added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:82-89<br class="gmail_msg">
+    if (!dwarf::isValidFormForVersion(AttrData.getForm(),<br class="gmail_msg">
+                                      AP->getDwarfVersion())) {<br class="gmail_msg">
+      std::string msg;<br class="gmail_msg">
+      raw_string_ostream Msg(msg);<br class="gmail_msg">
+      Msg << "Invalid form " << format("0x%x", AttrData.getForm())<br class="gmail_msg">
+          << " for DWARF version " << AP->getDwarfVersion();<br class="gmail_msg">
+      report_fatal_error(Msg.str());<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Should this be an assertion instead?<br class="gmail_msg">
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.<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br>I believe there are various forms of debug logging that are used for adding more complex messages near/with an assert. Perhaps you can use that? (I'd search for 'DEBUG' and 'dbg' in the codebase)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:918-919<br class="gmail_msg">
     addFlag(Buffer, dwarf::DW_AT_declaration);<br class="gmail_msg">
-    return addDIETypeSignature(Buffer, dwarf::DW_AT_signature, Identifier);<br class="gmail_msg">
+    if (getDwarfVersion() >= 4)<br class="gmail_msg">
+      addDIETypeSignature(Buffer, dwarf::DW_AT_signature, Identifier);<br class="gmail_msg">
+    return;<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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).<br class="gmail_msg">
><br class="gmail_msg">
> 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?<br class="gmail_msg">
The signature attribute was coming out even without type units.  This was added by Adrian for "external" types, something to do with modules.<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br>Adrian - want to weigh in here?<br><br>Much like type units, though, I think the same should hold - rather than silently omitting it, what's probably required is to either emit it anyway because the user asked for the feature and must have a compatible consumer to cope with it, or reject the combination of features that appear to be contradictory.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D30785" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D30785</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>