<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Apr 20, 2017, at 10:19 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Thu, Apr 20, 2017 at 9:49 AM Adrian Prantl via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aprantl added a comment.<br class="">
<br class="">
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 class="">
<br class="">
<br class="">
<br class="">
================<br class="">
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:82-89<br class="">
+ if (!dwarf::isValidFormForVersion(AttrData.getForm(),<br class="">
+ AP->getDwarfVersion())) {<br class="">
+ std::string msg;<br class="">
+ raw_string_ostream Msg(msg);<br class="">
+ Msg << "Invalid form " << format("0x%x", AttrData.getForm())<br class="">
+ << " for DWARF version " << AP->getDwarfVersion();<br class="">
+ report_fatal_error(Msg.str());<br class="">
----------------<br class="">
probinson wrote:<br class="">
> dblaikie wrote:<br class="">
> > Should this be an assertion instead?<br class="">
> 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="">
> 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="">
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 class=""><br class="">Yep.<br class=""> </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 class=""></blockquote><div class=""><br class=""></div><div class="">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 class=""></div></div></div></div></blockquote><div><br class=""></div><div>Could you perhaps paste this paragraph into CodingStandards.rst? Might be useful to have this as a reference when this question unavoidably comes up again in the future.</div><div><br class=""></div><div>-- adrian</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><br class="">It's a bit of an awkward thing to explain well, I'm sorry - happy to try more, maybe over IRC, etc.<br class=""><br class="">- Dave</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="">
<br class="">
<a href="https://reviews.llvm.org/D30785" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D30785</a><br class="">
<br class="">
<br class="">
<br class="">
</blockquote></div></div>
</div></blockquote></div><br class=""></body></html>