On Jan 22, 2013, at 5:23 PM, Chandler Carruth <[email protected]> wrote:
> On Tue, Jan 22, 2013 at 4:15 PM, Daniel Dunbar <[email protected]> wrote: > Hi Richard, > > I object pretty strongly to using unreachable here for C++ code bases. > > There was some discussion surrounding this, and I'm still pretty strongly in > favor of it... IMHO you still haven't given enough justification; your proposed optimization opportunity in http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-October/025326.html is specific to a full covered switch, which can be handled with an optimization that targets this case. -Argyrios > > The problem is that this can (and has, and will) subtly break code in ways > users might not notice. > > 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. > > 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. > > I disagree. Previously, the code had undefined behavior but got away with it. > After editing the code, it has well defined behavior. 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. > > 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. > > > 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). 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. > > > Can we revert to just emitting undef here? > > - Daniel > > > On Thu, Oct 4, 2012 at 4:52 PM, Richard Smith <[email protected]> > wrote: > Author: rsmith > Date: Thu Oct 4 18:52:29 2012 > New Revision: 165273 > > URL: http://llvm.org/viewvc/llvm-project?rev=165273&view=rev > Log: > If we flow off the end of a value-returning function: > - outside C++, return undef (behavior is not undefined unless the value is > used) > - in C++, with -fcatch-undefined-behavior, perform an appropriate trap > - in C++, produce an 'unreachable' (behavior is undefined immediately) > > Added: > cfe/trunk/test/CodeGenCXX/return.cpp > Modified: > cfe/trunk/lib/CodeGen/CodeGenFunction.cpp > cfe/trunk/test/CodeGen/catch-undef-behavior.c > cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp > > Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=165273&r1=165272&r2=165273&view=diff > ============================================================================== > --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original) > +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Thu Oct 4 18:52:29 2012 > @@ -535,6 +535,20 @@ > else > EmitFunctionBody(Args); > > + // C++11 [stmt.return]p2: > + // Flowing off the end of a function [...] results in undefined behavior > in > + // a value-returning function. > + // C11 6.9.1p12: > + // If the '}' that terminates a function is reached, and the value of the > + // function call is used by the caller, the behavior is undefined. > + if (getContext().getLangOpts().CPlusPlus && !FD->hasImplicitReturnZero() && > + !FD->getResultType()->isVoidType() && Builder.GetInsertBlock()) { > + if (CatchUndefined) > + EmitCheck(Builder.getFalse()); > + Builder.CreateUnreachable(); > + Builder.ClearInsertionPoint(); > + } > + > // Emit the standard function epilogue. > FinishFunction(BodyRange.getEnd()); > > > Modified: cfe/trunk/test/CodeGen/catch-undef-behavior.c > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/catch-undef-behavior.c?rev=165273&r1=165272&r2=165273&view=diff > ============================================================================== > --- cfe/trunk/test/CodeGen/catch-undef-behavior.c (original) > +++ cfe/trunk/test/CodeGen/catch-undef-behavior.c Thu Oct 4 18:52:29 2012 > @@ -44,3 +44,11 @@ > // CHECK-NEXT: ret i32 %[[RET]] > return a >> b; > } > + > +// CHECK: @no_return > +int no_return() { > + // Reaching the end of a noreturn function is fine in C. > + // CHECK-NOT: call > + // CHECK-NOT: unreachable > + // CHECK: ret i32 > +} > > Modified: cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp?rev=165273&r1=165272&r2=165273&view=diff > ============================================================================== > --- cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp (original) > +++ cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp Thu Oct 4 18:52:29 > 2012 > @@ -86,3 +86,9 @@ > // CHECK-NEXT: ret i32 %[[RET]] > return a << b; > } > + > +// CHECK: @_Z9no_return > +int no_return() { > + // CHECK: call void @llvm.trap > + // CHECK: unreachable > +} > > Added: cfe/trunk/test/CodeGenCXX/return.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/return.cpp?rev=165273&view=auto > ============================================================================== > --- cfe/trunk/test/CodeGenCXX/return.cpp (added) > +++ cfe/trunk/test/CodeGenCXX/return.cpp Thu Oct 4 18:52:29 2012 > @@ -0,0 +1,6 @@ > +// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s > + > +// CHECK: @_Z9no_return > +int no_return() { > + // CHECK: unreachable > +} > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
