[cfe-commits] r165273 - in /cfe/trunk: lib/CodeGen/CodeGenFunction.cpp test/CodeGen/catch-undef-behavior.c test/CodeGenCXX/catch-undef-behavior.cpp test/CodeGenCXX/return.cpp
Chandler Carruth
chandlerc at google.com
Tue Jan 22 17:23:18 PST 2013
On Tue, Jan 22, 2013 at 4:15 PM, Daniel Dunbar <daniel at zuster.org> 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...
> 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 <richard-llvm at metafoo.co.uk>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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130122/5d6f2008/attachment.html>
More information about the cfe-commits
mailing list