<div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hi Dave, Aaron,</div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">and I spent a lot more time debugging than I should have because I was using<br>a release + asserts build and the semantics of llvm_unreachable made<br>unfortunate codegen (switching to an assert makes the issue<br>immediately obvious).</blockquote></div><div><br></div><div>Huh. I think we should be using llvm_unreachable here, but I would have expected it to behave similarly to assert(false && ...) in +Asserts builds. If it isn't that might be a bug?</div><div><br></div><div>-- Lang. <br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Mar 22, 2020 at 9:19 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Sun, Mar 22, 2020 at 6:34 AM Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">On Sat, Mar 21, 2020 at 11:31 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
> Why the change? this seems counter to LLVM's style which pretty consistently uses unreachable rather than assert(false), so far as I know?<br>
<br>
We're not super consistent (at least within Clang), but the rules as<br>
I've generally understood them are to use llvm_unreachable only for<br>
truly unreachable code and to use assert(false) when the code is<br>
technically reachable but is a programmer mistake to have gotten<br>
there.</blockquote><div><br>I don't see those as two different things personally - llvm_unreachable is used when the programmer believes it to be unreachable (not that it must be proven to be unreachable - we have message text there so it's informative if the assumption turns out not to hold)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> In this particular case, the code is very much reachable </blockquote><div><br>In what sense? If it is actually reachable - shouldn't it be tested? (& in which case the assert(false) will get in the way of that testing)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">and I<br>
spent a lot more time debugging than I should have because I was using<br>
a release + asserts build and the semantics of llvm_unreachable made<br>
unfortunate codegen (switching to an assert makes the issue<br>
immediately obvious).<br></blockquote><div><br>I think it might be reasonable to say that release+asserts to have unreachable behave the same way unreachable behaves at -O0 (which is to say, much like assert(false)). Clearly release+asserts effects code generation, so there's nothing like the "debug info invariance" goal that this would be tainting, etc.<br><br>Certainly opinions vary here, but here are some commits that show the sort of general preference:<br><a href="http://llvm.org/viewvc/llvm-project?view=revision&revision=259302" target="_blank">http://llvm.org/viewvc/llvm-project?view=revision&revision=259302</a><br><a href="http://llvm.org/viewvc/llvm-project?view=revision&revision=253005" target="_blank">http://llvm.org/viewvc/llvm-project?view=revision&revision=253005</a><br><a href="http://llvm.org/viewvc/llvm-project?view=revision&revision=251266" target="_blank">http://llvm.org/viewvc/llvm-project?view=revision&revision=251266</a><br><br>And some counter examples:<br><a href="http://llvm.org/viewvc/llvm-project?view=revision&revision=225043" target="_blank">http://llvm.org/viewvc/llvm-project?view=revision&revision=225043</a><br>Including this thread where Chandler originally (not sure what his take on it is these days) expressed some ideas more along your lines:<br><a href="http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110919/thread.html#46583" target="_blank">http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110919/thread.html#46583</a><br><br>But I'm always pretty concerned about the idea that assertions should be used in places where the behavior of the program has any constraints when the assertion is false... <br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
><br>
> On Tue, Mar 10, 2020 at 11:22 AM Aaron Ballman via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>><br>
>> Author: Aaron Ballman<br>
>> Date: 2020-03-10T14:22:21-04:00<br>
>> New Revision: 4a0267e3ad8c4d47f267d7d960f127e099fb4818<br>
>><br>
>> URL: <a href="https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818</a><br>
>> DIFF: <a href="https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818.diff</a><br>
>><br>
>> LOG: Convert a reachable llvm_unreachable into an assert.<br>
>><br>
>> Added:<br>
>><br>
>><br>
>> Modified:<br>
>> clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp<br>
>><br>
>> Removed:<br>
>><br>
>><br>
>><br>
>> ################################################################################<br>
>> diff --git a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp<br>
>> index 01ac2bc83bb6..99e16752b51a 100644<br>
>> --- a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp<br>
>> +++ b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp<br>
>> @@ -134,9 +134,9 @@ StringRef AnalyzerOptions::getCheckerStringOption(StringRef CheckerName,<br>
>> CheckerName = CheckerName.substr(0, Pos);<br>
>> } while (!CheckerName.empty() && SearchInParents);<br>
>><br>
>> - llvm_unreachable("Unknown checker option! Did you call getChecker*Option "<br>
>> - "with incorrect parameters? User input must've been "<br>
>> - "verified by CheckerRegistry.");<br>
>> + assert(false && "Unknown checker option! Did you call getChecker*Option "<br>
>> + "with incorrect parameters? User input must've been "<br>
>> + "verified by CheckerRegistry.");<br>
>><br>
>> return "";<br>
>> }<br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>
</blockquote></div></div></div></div>