<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Mar 17, 2017 at 12:25 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: include/llvm/Support/Dwarf.h:420<br class="gmail_msg">
+/// These functions return the DWARF version when a constant was first defined.<br class="gmail_msg">
+/// For most or all vendor extensions it returns 0.<br class="gmail_msg">
+///<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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?<br class="gmail_msg">
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.<br class="gmail_msg">
This means I think there is no concise contract to specify; it would have to be something along these lines:<br class="gmail_msg">
<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br>Sounds like it isn't well defined/have a use yet - perhaps put an assertion in for now (return 0 for all cases that have a vendor version)? With a FIXME or other thing that'll describe some possible uses (first DWARF version it's compatible with, last DWARF version it was necessary in/superceded by a standard feature, etc?)<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">
================<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">
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">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:89<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">
dblaikie wrote:<br class="gmail_msg">
> llvm_unreachable is preferred over assert(0)<br class="gmail_msg">
I knew that didn't look right...<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>