<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jan 22, 2013 at 5:23 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="im">On Tue, Jan 22, 2013 at 4:15 PM, Daniel Dunbar <span dir="ltr"><<a href="mailto:daniel@zuster.org" target="_blank">daniel@zuster.org</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Richard,<div><br></div><div>I object pretty strongly to using unreachable here for C++ code bases.</div>
</div></blockquote><div><br></div></div><div>There was some discussion surrounding this, and I'm still pretty strongly in favor of it...</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div>The problem is that this can (and has, and will) subtly break code in ways users might not notice.</div></div></blockquote><div><br></div></div><div>I don't entirely understand this specific concern -- in non-optimized builds this becomes a trap pointing at the end of the function which lacked a return. While optimized builds can certainly fail in subtle ways, the non-optimized build seems to fail in a very understandable way here.</div>
</div></div></div></blockquote><div><br></div><div style>There are many users of the compiler who aren't necessarily building both non-optimized and optimized builds.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im"><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div> While I agree we are within our rights to do it, unlike other situations where we make changes that can break users' code (like strict aliasing, where it opens up many different optimization potentials) the optimizations that this is going to open up almost certainly *will* break the code that triggers this, but not otherwise improve things. So this change will just force users to edit their code in order to get the old behavior, without improving anything.<br>
</div></div></blockquote><div><br></div></div><div>I disagree. Previously, the code had undefined behavior but got away with it. After editing the code, it has well defined behavior.</div></div></div></div></blockquote><div>
<br></div><div style>This may not be the user perspective though. The user perspective could be that the new compiler breaks their code, in a hard to understand (for them) and track down way. After tracking it down, and modifying their code, they get their code back to working, but the compiler change that from their perspective *broke* their code doesn't have any other added value than having forced them to modify their code.</div>
<div style><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div> And it doesn't even seem possible to edit the code and "get the old behavior" back, because the old behavior returned 'undef', which AFAIK cannot be reasonably done in C++ without invoking undefined behavior... What are the semantics for non-primitive types? Destructors? I think there is a reason the C++ spec made this UB: the code and language work better if the user writes the return.</div>
</div></div></div></blockquote><div><br></div><div style>I was talking about the user perspective, this is a tangent.</div><div style> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div>There are plenty of cases where the user wants the C++ behavior. Look at all of the functions in LLVM and Clang which have llvm_unreachable just before their close curly brace (typically after returning from each case in a switch on an enumerator). While some of those cases are merely to silence GCC warnings, many do actually have a performance and code size impact. While I'm a fan of explicitly annotating such cases, there is a lot of code which does not follow this practice.</div>
</div></div></div></blockquote><div><br></div><div style>The right way to handle this is with noreturn attributes and so on, I don't think this is a convincing argument.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>The other thing that surprises me is that this behavior has been in place for a few months now without any significant user complaints that I have seen (in the open source community or internally within Google).</div>
</div></div></div></blockquote><div><br></div><div style>That doesn't surprise me, but I believe we get much heavier usage and a wider cross section of code bases trying to be the system compiler (I am positive FreeBSD would hit this too if it was turned on for C).</div>
<div style><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div> I'm assuming you're seeing complaints internally about this. How extensive is this problem? What is the nature of the patterns that trigger this warning? I ask in part because we had to fix up essentially none of the open source code we depend on here to deal with this change.</div>
</div></div></div></blockquote><div><br></div><div style>The pattern is just code that has a return type and misses the return statement (and the developers either had the warning off or ignored it, for whatever reason), but the return value was ignored or otherwise unimportant. The problem with cases like:</div>
<div style>--</div><div style><div>$ cat a.cpp<br></div><div>extern int g;</div><div>int f0() {</div><div> g = 10;</div><div>}</div><div><br></div><div>$ cat b.cpp</div><div>#include <stdio.h></div><div><br></div><div>
extern int f0();</div><div>int g = 0;</div><div><br></div><div>int main() {</div><div> f0();</div><div> fprintf(stderr, "g = %d\n", g);</div><div> return 0;</div><div>}</div><div><br></div><div><div>$ clang -w a.cpp b.cpp -O3 && ./a.out<br>
</div><div>Segmentation fault: 11<br></div><div>--</div></div><div style>is that the failure mode is just terrible from the users perspective. In this case it seg faults soon, but its possible to get a much more obscure failure because all the trailing stores in the f0() get DCEd because of the inferred noreturn attribute.</div>
<div><br></div><div style> - Daniel</div></div><div style> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div>
<div><br></div><div>Can we revert to just emitting undef here?</div><span><font color="#888888"><div><br></div><div> - Daniel</div></font></span></div><div><div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Thu, Oct 4, 2012 at 4:52 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard-llvm@metafoo.co.uk" target="_blank">richard-llvm@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: rsmith<br>
Date: Thu Oct 4 18:52:29 2012<br>
New Revision: 165273<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=165273&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=165273&view=rev</a><br>
Log:<br>
If we flow off the end of a value-returning function:<br>
- outside C++, return undef (behavior is not undefined unless the value is used)<br>
- in C++, with -fcatch-undefined-behavior, perform an appropriate trap<br>
- in C++, produce an 'unreachable' (behavior is undefined immediately)<br>
<br>
Added:<br>
cfe/trunk/test/CodeGenCXX/return.cpp<br>
Modified:<br>
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp<br>
cfe/trunk/test/CodeGen/catch-undef-behavior.c<br>
cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp<br>
<br>
Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=165273&r1=165272&r2=165273&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=165273&r1=165272&r2=165273&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Thu Oct 4 18:52:29 2012<br>
@@ -535,6 +535,20 @@<br>
else<br>
EmitFunctionBody(Args);<br>
<br>
+ // C++11 [stmt.return]p2:<br>
+ // Flowing off the end of a function [...] results in undefined behavior in<br>
+ // a value-returning function.<br>
+ // C11 6.9.1p12:<br>
+ // If the '}' that terminates a function is reached, and the value of the<br>
+ // function call is used by the caller, the behavior is undefined.<br>
+ if (getContext().getLangOpts().CPlusPlus && !FD->hasImplicitReturnZero() &&<br>
+ !FD->getResultType()->isVoidType() && Builder.GetInsertBlock()) {<br>
+ if (CatchUndefined)<br>
+ EmitCheck(Builder.getFalse());<br>
+ Builder.CreateUnreachable();<br>
+ Builder.ClearInsertionPoint();<br>
+ }<br>
+<br>
// Emit the standard function epilogue.<br>
FinishFunction(BodyRange.getEnd());<br>
<br>
<br>
Modified: cfe/trunk/test/CodeGen/catch-undef-behavior.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/catch-undef-behavior.c?rev=165273&r1=165272&r2=165273&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/catch-undef-behavior.c?rev=165273&r1=165272&r2=165273&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGen/catch-undef-behavior.c (original)<br>
+++ cfe/trunk/test/CodeGen/catch-undef-behavior.c Thu Oct 4 18:52:29 2012<br>
@@ -44,3 +44,11 @@<br>
// CHECK-NEXT: ret i32 %[[RET]]<br>
return a >> b;<br>
}<br>
+<br>
+// CHECK: @no_return<br>
+int no_return() {<br>
+ // Reaching the end of a noreturn function is fine in C.<br>
+ // CHECK-NOT: call<br>
+ // CHECK-NOT: unreachable<br>
+ // CHECK: ret i32<br>
+}<br>
<br>
Modified: cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp?rev=165273&r1=165272&r2=165273&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp?rev=165273&r1=165272&r2=165273&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp (original)<br>
+++ cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp Thu Oct 4 18:52:29 2012<br>
@@ -86,3 +86,9 @@<br>
// CHECK-NEXT: ret i32 %[[RET]]<br>
return a << b;<br>
}<br>
+<br>
+// CHECK: @_Z9no_return<br>
+int no_return() {<br>
+ // CHECK: call void @llvm.trap<br>
+ // CHECK: unreachable<br>
+}<br>
<br>
Added: cfe/trunk/test/CodeGenCXX/return.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/return.cpp?rev=165273&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/return.cpp?rev=165273&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGenCXX/return.cpp (added)<br>
+++ cfe/trunk/test/CodeGenCXX/return.cpp Thu Oct 4 18:52:29 2012<br>
@@ -0,0 +1,6 @@<br>
+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s<br>
+<br>
+// CHECK: @_Z9no_return<br>
+int no_return() {<br>
+ // CHECK: unreachable<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>