<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Apr 20, 2017 at 9:49 AM Adrian Prantl 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">aprantl added a comment.<br>
<br>
Besides my above comment, I'm very much looking forward to this commit. It will also help a lot with implementing dwarfdump -verify functionality.<br>
<br>
<br>
<br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:82-89<br>
+    if (!dwarf::isValidFormForVersion(AttrData.getForm(),<br>
+                                      AP->getDwarfVersion())) {<br>
+      std::string msg;<br>
+      raw_string_ostream Msg(msg);<br>
+      Msg << "Invalid form " << format("0x%x", AttrData.getForm())<br>
+          << " for DWARF version " << AP->getDwarfVersion();<br>
+      report_fatal_error(Msg.str());<br>
----------------<br>
probinson wrote:<br>
> dblaikie wrote:<br>
> > Should this be an assertion instead?<br>
> 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>
> 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>
This is something I'm still getting confused about. Doesn't `llvm_unreachable()` give the compiler the license to optimize away everything in the path leading up to the unreachable?</blockquote><div><br>Yep.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Would `report_fatal_error()` be more appropriate here?<br></blockquote><div><br></div><div>Not if it's a programmer error, imho. That's what asserts/unreachable are for - but a correct program should never hit these and so it doesn't make sense to have an error path here (any error path should have a test case - if it's untestable because only incorrect code would reach it, then it should be an assertion).<br><br>It's a bit of an awkward thing to explain well, I'm sorry - happy to try more, maybe over IRC, etc.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D30785" rel="noreferrer" target="_blank">https://reviews.llvm.org/D30785</a><br>
<br>
<br>
<br>
</blockquote></div></div>