<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Apr 10, 2017 at 4:41 PM 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:85-90<br class="gmail_msg">
+    if (!dwarf::isValidFormForVersion(AttrData.getForm(),<br class="gmail_msg">
+                                      AP->getDwarfVersion())) {<br class="gmail_msg">
+      DEBUG(dbgs() << "Invalid form " << format("0x%x", AttrData.getForm())<br class="gmail_msg">
+                   << " for DWARF version " << AP->getDwarfVersion() << "\n");<br class="gmail_msg">
+      assert(0 && "Invalid form for specified DWARF version");<br class="gmail_msg">
+    }<br class="gmail_msg">
----------------<br class="gmail_msg">
probinson wrote:<br class="gmail_msg">
> dblaikie wrote:<br class="gmail_msg">
> > 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.<br class="gmail_msg">
> I will look into that.  I put it here because that's where the check for implicit_const had been.<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br>Walking up to the unit for each form insertion, only in a +Asserts build seems fine/not too expensive to me.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
(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.)<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:297-298<br class="gmail_msg">
   Die.addValue(DIEValueAllocator, dwarf::DW_AT_signature,<br class="gmail_msg">
-               dwarf::DW_FORM_ref_sig8, DIEInteger(Signature));<br class="gmail_msg">
+               getDwarfVersion() >= 4 ? dwarf::DW_FORM_ref_sig8<br class="gmail_msg">
+                                      : dwarf::DW_FORM_data8,<br class="gmail_msg">
+               DIEInteger(Signature));<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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.<br class="gmail_msg">
><br class="gmail_msg">
> Is this still needed?<br class="gmail_msg">
Now that Adrian has removed the other usage, yes this is no longer necessary.<br class="gmail_msg">
<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>