[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
arphaman marked 5 inline comments as done.
Closed by commit rL290960: Add -f[no-]strict-return flag that can be used to 
avoid undefined behaviour (authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D27163?vs=81584=83041#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27163

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
  cfe/trunk/lib/Driver/Tools.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGenCXX/return.cpp
  cfe/trunk/test/CodeGenObjCXX/return.mm
  cfe/trunk/test/Driver/clang_f_opts.c

Index: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
===
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def
@@ -251,6 +251,10 @@
 /// Whether copy relocations support is available when building as PIE.
 CODEGENOPT(PIECopyRelocations, 1, 0)
 
+/// Whether we should use the undefined behaviour optimization for control flow
+/// paths that reach the end of a function without executing a required return.
+CODEGENOPT(StrictReturn, 1, 1)
+
 #undef CODEGENOPT
 #undef ENUM_CODEGENOPT
 #undef VALUE_CODEGENOPT
Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1331,6 +1331,12 @@
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group, Flags<[CC1Option]>;
 
+def fstrict_return : Flag<["-"], "fstrict-return">, Group,
+  Flags<[CC1Option]>,
+  HelpText<"Always treat control flow paths that fall off the end of a non-void"
+   "function as unreachable">;
+def fno_strict_return : Flag<["-"], "fno-strict-return">, Group,
+  Flags<[CC1Option]>;
 
 def fdebug_types_section: Flag <["-"], "fdebug-types-section">, Group,
   Flags<[CC1Option]>, HelpText<"Place debug types in their own section (ELF Only)">;
Index: cfe/trunk/test/CodeGenCXX/return.cpp
===
--- cfe/trunk/test/CodeGenCXX/return.cpp
+++ cfe/trunk/test/CodeGenCXX/return.cpp
@@ -1,12 +1,105 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -O -o - %s | FileCheck %s --check-prefix=CHECK-OPT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -o - %s | FileCheck --check-prefixes=CHECK,CHECK-COMMON %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -O -o - %s | FileCheck %s --check-prefixes=CHECK-OPT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -Wno-return-type -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -O -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT-OPT,CHECK-COMMON
 
-// CHECK: @_Z9no_return
-// CHECK-OPT: @_Z9no_return
+// CHECK-COMMON-LABEL: @_Z9no_return
 int no_return() {
   // CHECK:  call void @llvm.trap
   // CHECK-NEXT: unreachable
 
   // CHECK-OPT-NOT: call void @llvm.trap
   // CHECK-OPT: unreachable
+
+  // -fno-strict-return should not emit trap + unreachable but it should return
+  // an undefined value instead.
+
+  // CHECK-NOSTRICT: entry:
+  // CHECK-NOSTRICT-NEXT: alloca
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT: ret i32 undef
+}
+
+enum Enum {
+  A, B
+};
+
+// CHECK-COMMON-LABEL: @_Z27returnNotViableDontOptimize4Enum
+int returnNotViableDontOptimize(Enum e) {
+  switch (e) {
+  case A: return 1;
+  case B: return 2;
+  }
+  // Undefined behaviour optimization shouldn't be used when -fno-strict-return
+  // is turned on, even if all the enum cases are covered in this function.
+
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
+}
+
+struct Trivial {
+  int x;
+};
+
+// CHECK-NOSTRICT-LABEL: @_Z7trivialv
+Trivial trivial() {
+  // This function returns a trivial record so -fno-strict-return should avoid
+  // the undefined behaviour optimization.
+
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
+}
+
+struct NonTrivialCopy {
+  NonTrivialCopy(const NonTrivialCopy &);
+};
+
+// CHECK-NOSTRICT-LABEL: @_Z14nonTrivialCopyv
+NonTrivialCopy nonTrivialCopy() {
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
+}
+
+struct NonTrivialDefaultConstructor {
+  int x;
+

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: include/clang/Driver/Options.td:1324
+  HelpText<"Use C++ undefined behaviour optimization for control flow paths"
+ "that reach the end of the function without executing a required return">;
+def fno_strict_return : Flag<["-"], "fno-strict-return">, Group,

mehdi_amini wrote:
> Quuxplusone wrote:
> > Nit: looks like Clang spells it "behavior"?
> > 
> > Nit: I'm still pedantically uncomfortable with describing the strict-return 
> > behavior as an "optimization". I suggest rephrasing this as 
> > "-fstrict-return: Treat control flow paths that fall off the end of a 
> > non-void function as unreachable."
> > 
> > (I notice that neither -fstrict-aliasing nor -fstrict-overflow have any 
> > help text at all.)
> >  I suggest rephrasing this as "-fstrict-return: Treat control flow paths 
> > that fall off the end of a non-void function as unreachable."
> 
> Is it an accurate description? `-fno-strict-return` would only disable this 
> for trivial types.
> Nit: looks like Clang spells it "behavior"?

Thanks, I forgot to use the US spelling.

> Nit: I'm still pedantically uncomfortable with describing the strict-return 
> behavior as an "optimization". I suggest rephrasing this as "-fstrict-return: 
> Treat control flow paths that fall off the end of a non-void function as 
> unreachable."

I like your suggestion, but Mehdi brought up a good point. Maybe "Always treat 
control flow paths that fall off the end of a non-void function as unreachable" 
would be better, since then `-fno-strict-return` would imply that the 
optimisation can still be used sometimes, like for the non-trivially 
destructible types.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: lib/CodeGen/CodeGenFunction.cpp:1078
+  QualType T = FD->getReturnType();
+  if (T.isTriviallyCopyableType(Context)) {
+// Avoid the optimization for functions that return trivially copyable

rjmccall wrote:
> arphaman wrote:
> > Quuxplusone wrote:
> > > Peanut-gallery question: Does `isTriviallyCopyableType` also check that 
> > > the type is trivially destructible and/or default-constructible? Do those 
> > > things matter?
> > Yes for trivially destructible, since it calls 
> > `CXXRecordDecl::isTriviallyCopyable()` which checks. No for trivially 
> > default-constructible, I fixed that. 
> > 
> > I think that for us it doesn't really matter that much, since we're mostly 
> > worried about C code compiled in C++ mode.
> The right condition is just hasTrivialDestructor().  The goal of 
> -fno-strict-return is to not treat falling off the end of a function as 
> undefined behavior in itself: it might still be a *problem* if users don't 
> ignore the result, but we shouldn't escalate to UB in the callee because they 
> legitimately might ignore the result.  The argument for having an exception 
> around non-trivial types is that callers never *really* ignore the result 
> when there's a non-trivial destructor.  Calling a destructor on storage that 
> doesn't have a valid object of that type constructed there is already 
> definitely U.B. in the caller, so it's fine to also treat it as U.B. on the 
> callee side.  That reasoning is entirely based on the behavior of the 
> destructor, though, not any of the copy/move constructors, and definitely not 
> the presence or absence of a trivial default constructor.
> 
> In practice, all of these things tend to vary together, of course, except 
> that sometimes trivial types do have non-trivial default constructors.  We 
> should not treat that as a UB case.
I see what you mean, this kind of reasoning makes sense. I'll use just the 
`hasTrivialDestructor` check when committing.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D27163#623811, @majnemer wrote:

> My 2 cents: If making this a target-specific default is not OK, why not turn 
> on -fno-strict-return if -fapple-kext is specified? I believe that would 
> cover the critical use case in question.


We've had issues with more than just kexts, so this won't be that beneficial to 
us.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CodeGenFunction.cpp:1078
+  QualType T = FD->getReturnType();
+  if (T.isTriviallyCopyableType(Context)) {
+// Avoid the optimization for functions that return trivially copyable

arphaman wrote:
> Quuxplusone wrote:
> > Peanut-gallery question: Does `isTriviallyCopyableType` also check that the 
> > type is trivially destructible and/or default-constructible? Do those 
> > things matter?
> Yes for trivially destructible, since it calls 
> `CXXRecordDecl::isTriviallyCopyable()` which checks. No for trivially 
> default-constructible, I fixed that. 
> 
> I think that for us it doesn't really matter that much, since we're mostly 
> worried about C code compiled in C++ mode.
The right condition is just hasTrivialDestructor().  The goal of 
-fno-strict-return is to not treat falling off the end of a function as 
undefined behavior in itself: it might still be a *problem* if users don't 
ignore the result, but we shouldn't escalate to UB in the callee because they 
legitimately might ignore the result.  The argument for having an exception 
around non-trivial types is that callers never *really* ignore the result when 
there's a non-trivial destructor.  Calling a destructor on storage that doesn't 
have a valid object of that type constructed there is already definitely U.B. 
in the caller, so it's fine to also treat it as U.B. on the callee side.  That 
reasoning is entirely based on the behavior of the destructor, though, not any 
of the copy/move constructors, and definitely not the presence or absence of a 
trivial default constructor.

In practice, all of these things tend to vary together, of course, except that 
sometimes trivial types do have non-trivial default constructors.  We should 
not treat that as a UB case.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

(Wait a little in case @Quuxplusone still has comments.)


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: lib/CodeGen/CodeGenFunction.cpp:1086
+  }
+  return true;
+}

Just a nit: reversing the if condition allows early exit.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[David Majnemer via Phabricator via cfe-commits]

majnemer added a comment.

My 2 cents: If making this a target-specific default is not OK, why not turn on 
-fno-strict-return if -fapple-kext is specified? I believe that would cover the 
critical use case in question.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 81584.
arphaman marked 3 inline comments as done.
arphaman added a comment.

The updated patch removes the dependency on the "-Wreturn-type" diagnostic and 
verifies that functions that return a type with a non-trivial default 
constructor are always optimized.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CodeGenFunction.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGenCXX/return.cpp
  test/CodeGenObjCXX/return.mm
  test/Driver/clang_f_opts.c

Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -469,3 +469,8 @@
 // CHECK-WCHAR2: -fshort-wchar
 // CHECK-WCHAR2-NOT: -fno-short-wchar
 // DELIMITERS: {{^ *"}}
+
+// RUN: %clang -### -S -fstrict-return %s 2>&1 | FileCheck -check-prefix=CHECK-STRICT-RETURN %s
+// RUN: %clang -### -S -fno-strict-return %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-RETURN %s
+// CHECK-STRICT-RETURN-NOT: "-fno-strict-return"
+// CHECK-NO-STRICT-RETURN: "-fno-strict-return"
Index: test/CodeGenObjCXX/return.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/return.mm
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -fblocks -triple x86_64-apple-darwin -fstrict-return -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -fblocks -triple x86_64-apple-darwin -fstrict-return -O -o - %s | FileCheck %s
+
+@interface I
+@end
+
+@implementation I
+
+- (int)method {
+}
+
+@end
+
+enum Enum {
+  a
+};
+
+int (^block)(Enum) = ^int(Enum e) {
+  switch (e) {
+  case a:
+return 1;
+  }
+};
+
+// Ensure that both methods and blocks don't use the -fstrict-return undefined
+// behaviour optimization.
+
+// CHECK-NOT: call void @llvm.trap
+// CHECK-NOT: unreachable
Index: test/CodeGenCXX/return.cpp
===
--- test/CodeGenCXX/return.cpp
+++ test/CodeGenCXX/return.cpp
@@ -1,12 +1,105 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -O -o - %s | FileCheck %s --check-prefix=CHECK-OPT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -o - %s | FileCheck --check-prefixes=CHECK,CHECK-COMMON %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -O -o - %s | FileCheck %s --check-prefixes=CHECK-OPT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -Wno-return-type -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -O -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT-OPT,CHECK-COMMON
 
-// CHECK: @_Z9no_return
-// CHECK-OPT: @_Z9no_return
+// CHECK-COMMON-LABEL: @_Z9no_return
 int no_return() {
   // CHECK:  call void @llvm.trap
   // CHECK-NEXT: unreachable
 
   // CHECK-OPT-NOT: call void @llvm.trap
   // CHECK-OPT: unreachable
+
+  // -fno-strict-return should not emit trap + unreachable but it should return
+  // an undefined value instead.
+
+  // CHECK-NOSTRICT: entry:
+  // CHECK-NOSTRICT-NEXT: alloca
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT: ret i32 undef
+}
+
+enum Enum {
+  A, B
+};
+
+// CHECK-COMMON-LABEL: @_Z27returnNotViableDontOptimize4Enum
+int returnNotViableDontOptimize(Enum e) {
+  switch (e) {
+  case A: return 1;
+  case B: return 2;
+  }
+  // Undefined behaviour optimization shouldn't be used when -fno-strict-return
+  // is turned on, even if all the enum cases are covered in this function.
+
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
+}
+
+struct Trivial {
+  int x;
+};
+
+// CHECK-NOSTRICT-LABEL: @_Z7trivialv
+Trivial trivial() {
+  // This function returns a trivial record so -fno-strict-return should avoid
+  // the undefined behaviour optimization.
+
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
+}
+
+// Functions that return non-trivial records should always use the
+// -fstrict-return optimization.
+
+struct NonTrivialCopy {
+  NonTrivialCopy(const NonTrivialCopy &);
+};
+
+// CHECK-NOSTRICT-LABEL: @_Z14nonTrivialCopyv
+NonTrivialCopy nonTrivialCopy() {
+  // CHECK-NOSTRICT: call void @llvm.trap
+  // CHECK-NOSTRICT-NEXT: unreachable
+}
+
+struct NonTrivialDestructor {
+  ~NonTrivialDestructor();
+};
+
+// CHECK-NOSTRICT-LABEL: @_Z20nonTrivialDestructorv
+NonTrivialDestructor nonTrivialDestructor() {
+  // 

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: include/clang/Sema/AnalysisBasedWarnings.h:93
 
-  void IssueWarnings(Policy P, FunctionScopeInfo *fscope,
- const Decl *D, const BlockExpr *blkExpr);
+  void IssueWarnings(Policy P, FunctionScopeInfo *fscope, Decl *D,
+ const BlockExpr *blkExpr);

If you remove the coupling between -fno-strict-return and -Wreturn-type, then 
you won't need to remove `const` in all these places. I think that alone is a 
good enough reason not to couple them.



Comment at: lib/CodeGen/CodeGenFunction.cpp:1078
+  QualType T = FD->getReturnType();
+  if (T.isTriviallyCopyableType(Context)) {
+// Avoid the optimization for functions that return trivially copyable

Peanut-gallery question: Does `isTriviallyCopyableType` also check that the 
type is trivially destructible and/or default-constructible? Do those things 
matter?



Comment at: lib/CodeGen/CodeGenFunction.cpp:1173
+} else if (CGM.getCodeGenOpts().StrictReturn ||
+   shouldUseUndefinedBehaviorReturnOptimization(FD, getContext())) 
{
+  if (CGM.getCodeGenOpts().OptimizationLevel == 0)

I'd expect this to look more like

bool ShouldOmitUnreachable = !CGM.getCodeGenOpts().StrictReturn && 
FD->getReturnType().isTriviallyCopyableType(Context);
// same ifs as the old code
if (!ShouldOmitUnreachable) {
Builder.CreateUnreachable();
Builder.ClearInsertionPoint();
}

Or in fact, is there a good reason this codepath isn't piggybacking on 
`FD->hasImplicitReturnZero()`?  Why not just give every function 
`hasImplicitReturnZero` when `-fno-strict-return` is in effect?

At which point, `-fno-strict-return` might seem like the wrong name; you could 
just call it something like `-fimplicit-return-zero`, which would also have the 
minor benefit of making the `-fno-*` option the default.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Ping.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.



In https://reviews.llvm.org/D27163#607078, @rsmith wrote:

> A target-specific default for this, simply because there's a lot of code on 
> Darwin that happens to violate this language rule, doesn't make sense to me.
>
> Basing the behavior on whether a `-Wreturn-type` warning would have been 
> emitted seems like an extremely strange heuristic: only optimizing in the 
> cases where we provide the user no hint that we will do so seems incredibly 
> user-hostile.
>
> Regardless of anything else, it does not make any sense to "return" stack 
> garbage when the return type is a C++ class type, particularly one with a 
> non-trivial copy constructor or destructor. A trap at `-O0` is the kindest 
> thing we can do in that case.


Thanks, that makes sense. The updated patch avoids the trap and unreachable 
only for types that are trivially copyable and removes the Darwin specific code.

> In summary, I think it could be reasonable to have such a flag to disable the 
> trap/unreachable *only* for scalar types (or, at a push, trivially-copyable 
> types). But it seems unreasonable to have different defaults for Darwin, or 
> to look at whether a `-Wreturn-type` warning would have fired.

I understand that basing the optimisation avoidance on the analysis done for 
the `-Wreturn-type` warning might not be the best option, and a straightforward 
`-fno-strict-return` might be better as it doesn't use such hidden heuristics. 
However, AFAIK we only had problems with code that would've hit the 
`-Wreturn-type` warning, so it seemed like a good idea to keep the optimisation 
in functions where the analyser has determined that the flow with the no return 
is very unlikely (like in the `alwaysOptimizedReturn` function in the test 
case).  I kept it in this version of the patch, but if you think that the 
`-Wreturn-type` heuristic shouldn't be used I would be willing to remove it and 
go for the straightforward behaviour.

> Alternatively, since it seems you're only interested in the behavior of cases 
> where `-Wreturn-type` would have fired, how about using Clang's tooling 
> support to write a utility to add a return statement to the end of every 
> function where it would fire, and give that to your users to fix their code?

That's an interesting idea, but as John mentioned, some of the code that has 
these issues isn't written by Apple and it seems that it would be very 
difficult to convince code owners to run tooling and to fix such issues. But it 
still sounds like something that we should look into.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: test/CodeGenCXX/return.cpp:47
+  // CHECK-NOSTRICT-OPT-NEXT: zext
+  // CHECK-NOSTRICT-OPT-NEXT: ret i32
+}

mehdi_amini wrote:
> Document what's going on in the tests please.
> 
I added some comments that help explain what's being tested. I also removed the 
`-fno-strict-return` checks with `-O` (except for the checks in the first 
function) as the non-optimised `-fno-strict-return` tests should be sufficient.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:530
+  // Don't need to mark Objective-C methods or blocks since the undefined
+  // behaviour optimization isn't used for them.
+}

mehdi_amini wrote:
> Quuxplusone wrote:
> > This seems like a trap waiting to spring on someone, unless there's a 
> > technical reason that methods and blocks cannot possibly use the same 
> > optimization paths as regular functions. ("Nobody's gotten around to 
> > implementing it yet" is the most obvious nontechnical reason for the 
> > current difference.) Either way, I'd expect this patch to include test 
> > cases for both methods and blocks, to verify that the behavior you expect 
> > is actually the behavior that happens. Basically, it ought to have a 
> > regression test targeting the regression that I'm predicting is going to 
> > spring on someone as soon as they implement optimizations for methods and 
> > blocks.
> > 
> > Also, one dumb question: what about C++ lambdas? are they FunctionDecls 
> > too? test case?
> > This seems like a trap waiting to spring on someone, unless there's a 
> > technical reason that methods and blocks cannot possibly use the same 
> > optimization paths as regular functions.
> 
> The optimization path in LLVM is the same. I think the difference lies in 
> clang IRGen: there is no "unreachable" generated for these so the optimizer 
> can't be aggressive. So this patch is not changing anything for Objective-C 
> methods and blocks, and I expect that we *already* have a test that covers 
> this behavior (if not we should add one).
Mehdi is right, the difference is in IRGen - methods and blocks never get the 
trap and unreachable IR return optmisation. I don't think we have a test for 
this though, so I added a regression test case that verifies this.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 79556.
arphaman added a comment.

The updated diff has the following changes:

- The default value for '-fstrict-return' is now the same across all platforms, 
the Darwin specific -fno-strict-return was removed.
- The 'fno-strict-return' optimisation avoidance can only be done by functions 
that return trivially copyable types.
- A new test verifies that Objective-C++ methods and blocks never get the 
return optimisation regardless of the '-fstrict-return' flag.
- The updated test adds explanation comments and uses Mehdi's advice for LABEL 
checks and common prefix.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/LangOptions.def
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Sema/AnalysisBasedWarnings.h
  include/clang/Sema/Sema.h
  lib/CodeGen/CodeGenFunction.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/Sema.cpp
  test/CodeGenCXX/return.cpp
  test/CodeGenObjCXX/return.mm
  test/Driver/clang_f_opts.c

Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -469,3 +469,8 @@
 // CHECK-WCHAR2: -fshort-wchar
 // CHECK-WCHAR2-NOT: -fno-short-wchar
 // DELIMITERS: {{^ *"}}
+
+// RUN: %clang -### -S -fstrict-return %s 2>&1 | FileCheck -check-prefix=CHECK-STRICT-RETURN %s
+// RUN: %clang -### -S -fno-strict-return %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-RETURN %s
+// CHECK-STRICT-RETURN-NOT: "-fno-strict-return"
+// CHECK-NO-STRICT-RETURN: "-fno-strict-return"
Index: test/CodeGenObjCXX/return.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/return.mm
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -fblocks -triple x86_64-apple-darwin -fstrict-return -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -fblocks -triple x86_64-apple-darwin -fstrict-return -O -o - %s | FileCheck %s
+
+@interface I
+@end
+
+@implementation I
+
+- (int)method {
+}
+
+@end
+
+enum Enum {
+  a
+};
+
+int (^block)(Enum) = ^int(Enum e) {
+  switch (e) {
+  case a:
+return 1;
+  }
+};
+
+// Ensure that both methods and blocks don't use the -fstrict-return undefined
+// behaviour optimization.
+
+// CHECK-NOT: call void @llvm.trap
+// CHECK-NOT: unreachable
Index: test/CodeGenCXX/return.cpp
===
--- test/CodeGenCXX/return.cpp
+++ test/CodeGenCXX/return.cpp
@@ -1,12 +1,105 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -O -o - %s | FileCheck %s --check-prefix=CHECK-OPT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -o - %s | FileCheck --check-prefixes=CHECK,CHECK-COMMON %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -O -o - %s | FileCheck %s --check-prefixes=CHECK-OPT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -Wno-return-type -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT,CHECK-COMMON
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -std=c++11 -fno-strict-return -O -o - %s | FileCheck %s --check-prefixes=CHECK-NOSTRICT-OPT,CHECK-COMMON
 
-// CHECK: @_Z9no_return
-// CHECK-OPT: @_Z9no_return
+// CHECK-COMMON-LABEL: @_Z9no_return
 int no_return() {
   // CHECK:  call void @llvm.trap
   // CHECK-NEXT: unreachable
 
   // CHECK-OPT-NOT: call void @llvm.trap
   // CHECK-OPT: unreachable
+
+  // -fno-strict-return should not emit trap + unreachable but it should return
+  // an undefined value instead.
+
+  // CHECK-NOSTRICT: entry:
+  // CHECK-NOSTRICT-NEXT: alloca
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT: ret i32 undef
+}
+
+enum Enum {
+  A, B
+};
+
+// CHECK-COMMON-LABEL: @_Z21alwaysOptimizedReturn4Enum
+int alwaysOptimizedReturn(Enum e) {
+  switch (e) {
+  case A: return 1;
+  case B: return 2;
+  }
+  // This function covers all the cases of 'Enum', so the undefined behaviour
+  // optimization is allowed even if -fno-strict-return is used.
+
+  // CHECK-NOSTRICT:  call void @llvm.trap()
+  // CHECK-NOSTRICT-NEXT: unreachable
+}
+
+// CHECK-NOSTRICT-LABEL: @_Z23strictlyOptimizedReturn4Enum
+int strictlyOptimizedReturn(Enum e) {
+  switch (e) {
+  case A: return 22;
+  }
+  // Undefined behaviour optimization shouldn't be used when -fno-strict-return
+  // is turned on, as not all enum cases are covered in this function.
+
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // 

Re: [PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Richard Smith via cfe-commits]

On 28 November 2016 at 15:33, John McCall via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> On Mon, Nov 28, 2016 at 2:48 PM, Richard Smith 
> wrote:
>
>> (Dropping Phabricator, since this isn't really about D27163...)
>>
>> On 28 November 2016 at 14:27, John McCall via Phabricator via cfe-commits
>>  wrote:
>>
>>> In https://reviews.llvm.org/D27163#607100, @rsmith wrote:
>>> > C has rather different and much less useful TBAA rules
>>>
>>> [...] I don't think the TBAA rules are as different as you're suggesting,
>>
>>
>> I can offhand think of at least the following differences, some of which
>> seem fairly major:
>>  * C does not allow changing the effective type of a declared variable,
>> C++ does.
>>
>
> Well, except that the name of the variable itself becomes formally unbound
> to an object, no?  But yes, you can change the effective type via an alias
> as long as you're careful to only use it through that alias, which is not
> permitted in C.
>
>
>>  * C does not support struct-path TBAA in any form (its model is that
>> lumps of memory have effective types, and it doesn't care how you got
>> there), C++ does.
>>
>
> You're assuming that a lump of memory can't have multiple effective types
> at once due to subobject overlap, which doesn't seem justified by the
> text.  In particular, a variable of struct type definitely has the
> effective type of that struct.
>

Sure, but effective types are only checked when the stored value is
accessed, which doesn't happen when naming a member of a struct or union,
only at the subsequent assignment or lvalue conversion, at which point the
lvalue expression has the type of the accessed subobject, so is presumably
valid per C11 6.5/7. (The lvalue expression won't have the type of some
enclosing struct here regardless of whether you used that struct type to
form an lvalue designating one of its members.)

If we really were supposed to imagine all objects enclosing the accessed
object for the purpose of C11 6.5/7, it's missing a rule allowing an lvalue
expression whose type is a member of the effective type of the object to
access the object. (It has the opposite rule, in order to allow accesses of
lvalues denoting entire structs to access the values stored in scalar
subobjects.)

 * C allows any assignment to a union member to change the effective type,
>> no matter whether it's locally determinable that you're storing to a union
>> member, C++ requires the assignment's LHS to be a union member access.
>>  * C only permits common initial sequence shenanigans for unions where
>> the union type is visible, C++ permits it anywhere.
>>
>
> The union rules in C are all over the place.
>

Agreed :)


>   I gave up trying to pretend that C has consistent union semantics a long
> time ago.
>
>
>> except that C++ actually tries to provide specific rules for unions (that
>>> don't match what users actually want).
>>
>>
>> In what way don't C++'s union rules match user expectations where C's
>> rules do? Do you mean type punning via unions? C doesn't allow that either
>> (or maybe it does, if you think a footnote carries more normative weight
>> than rules in the main text).
>>
>
> I was careful to say "what users actually want".  Users *want* type
> punning via unions. C's rules are contradictory because, as I read it, the
> committee has never been able to come to a consensus about how to formalize
> allowing type punning via unions without breaking the entire model — and to
> be fair, I don't know how to do that either.  But I would say that the
> expressed user desires are pretty clear here.
>

Interesting. What I see from users is a strong desire to be able to do
bitwise reinterpretation of values /somehow/, not necessarily to type-pun
through unions, which I think is subtly but importantly different. The C
DRs list has some implication that the C committee at some point intended
to make type-punning through unions work, but failed to actually make the
necessary changes in the wording (and all that we have left is a footnote).

Anyway, this is not actually important here, and I have not seen any code
> that falls into the cracks between language standards here, at least not
> under LLVM's intentionally-more-permissive-than-standardized application
> of TBAA.
>

I've seen plenty of programs that would be valid under some interpretations
of C's rules, but not under C++'s rules. But since we effectively provide
the C++ rules in all cases regardless, I agree this is not relevant for the
issue at hand.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[John McCall via cfe-commits]

On Mon, Nov 28, 2016 at 2:48 PM, Richard Smith 
wrote:

> (Dropping Phabricator, since this isn't really about D27163...)
>
> On 28 November 2016 at 14:27, John McCall via Phabricator via cfe-commits
>  wrote:
>
>> In https://reviews.llvm.org/D27163#607100, @rsmith wrote:
>> > C has rather different and much less useful TBAA rules
>>
>> [...] I don't think the TBAA rules are as different as you're suggesting,
>
>
> I can offhand think of at least the following differences, some of which
> seem fairly major:
>  * C does not allow changing the effective type of a declared variable,
> C++ does.
>

Well, except that the name of the variable itself becomes formally unbound
to an object, no?  But yes, you can change the effective type via an alias
as long as you're careful to only use it through that alias, which is not
permitted in C.


>  * C does not support struct-path TBAA in any form (its model is that
> lumps of memory have effective types, and it doesn't care how you got
> there), C++ does.
>

You're assuming that a lump of memory can't have multiple effective types
at once due to subobject overlap, which doesn't seem justified by the
text.  In particular, a variable of struct type definitely has the
effective type of that struct.


>  * C allows any assignment to a union member to change the effective type,
> no matter whether it's locally determinable that you're storing to a union
> member, C++ requires the assignment's LHS to be a union member access.
>  * C only permits common initial sequence shenanigans for unions where the
> union type is visible, C++ permits it anywhere.
>

The union rules in C are all over the place.  I gave up trying to pretend
that C has consistent union semantics a long time ago.


> except that C++ actually tries to provide specific rules for unions (that
>> don't match what users actually want).
>
>
> In what way don't C++'s union rules match user expectations where C's
> rules do? Do you mean type punning via unions? C doesn't allow that either
> (or maybe it does, if you think a footnote carries more normative weight
> than rules in the main text).
>

I was careful to say "what users actually want".  Users *want* type punning
via unions. C's rules are contradictory because, as I read it, the
committee has never been able to come to a consensus about how to formalize
allowing type punning via unions without breaking the entire model — and to
be fair, I don't know how to do that either.  But I would say that the
expressed user desires are pretty clear here.

Anyway, this is not actually important here, and I have not seen any code
that falls into the cracks between language standards here, at least not
under LLVM's intentionally-more-permissive-than-standardized application of
TBAA.

John.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Adding the flag seems OK, but I'd rather not make the default setting be 
target-dependent, if that's workable.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Richard Smith via cfe-commits]

(Dropping Phabricator, since this isn't really about D27163...)

On 28 November 2016 at 14:27, John McCall via Phabricator via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> In https://reviews.llvm.org/D27163#607100, @rsmith wrote:
> > C has rather different and much less useful TBAA rules
>
> [...] I don't think the TBAA rules are as different as you're suggesting,


I can offhand think of at least the following differences, some of which
seem fairly major:
 * C does not allow changing the effective type of a declared variable, C++
does.
 * C does not support struct-path TBAA in any form (its model is that lumps
of memory have effective types, and it doesn't care how you got there), C++
does.
 * C allows any assignment to a union member to change the effective type,
no matter whether it's locally determinable that you're storing to a union
member, C++ requires the assignment's LHS to be a union member access.
 * C only permits common initial sequence shenanigans for unions where the
union type is visible, C++ permits it anywhere.


> except that C++ actually tries to provide specific rules for unions (that
> don't match what users actually want).


In what way don't C++'s union rules match user expectations where C's rules
do? Do you mean type punning via unions? C doesn't allow that either (or
maybe it does, if you think a footnote carries more normative weight than
rules in the main text).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D27163#607100, @rsmith wrote:

> In https://reviews.llvm.org/D27163#607078, @rsmith wrote:
>
> > A target-specific default for this, simply because there's a lot of code on 
> > Darwin that happens to violate this language rule, doesn't make sense to me.
>
>
> ... but rjmccall's explanation of the problem helps. The C compatibility 
> argument also suggests that this should only apply to trivially-copyable 
> types, and perhaps only scalar types.


I would agree with this.  The right rule is probably whether the type is 
trivially-destructed.

> The same issue presumably arises for `-fstrict-enums`, which is again only UB 
> in C++? (Also, C has rather different and much less useful TBAA rules.) 
> Perhaps some catch-all "I want defined behavior for things that C defines 
> even though I'm compiling in C++" flag would make sense here?

I don't think we've seen problems from any of these.  Also I don't think the 
TBAA rules are as different as you're suggesting, except that C++ actually 
tries to provide specific rules for unions (that don't match what users 
actually want).


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D27163#607078, @rsmith wrote:

> A target-specific default for this, simply because there's a lot of code on 
> Darwin that happens to violate this language rule, doesn't make sense to me.


... but rjmccall's explanation of the problem helps. The C compatibility 
argument also suggests that this should only apply to trivially-copyable types, 
and perhaps only scalar types. The same issue presumably arises for 
`-fstrict-enums`, which is again only UB in C++? (Also, C has rather different 
and much less useful TBAA rules.) Perhaps some catch-all "I want defined 
behavior for things that C defines even though I'm compiling in C++" flag would 
make sense here?


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

A target-specific default for this, simply because there's a lot of code on 
Darwin that happens to violate this language rule, doesn't make sense to me.

Basing the behavior on whether a `-Wreturn-type` warning would have been 
emitted seems like an extremely strange heuristic: only optimizing in the cases 
where we provide the user no hint that we will do so seems incredibly 
user-hostile.

Regardless of anything else, it does not make any sense to "return" stack 
garbage when the return type is a C++ class type, particularly one with a 
non-trivial copy constructor or destructor. A trap at `-O0` is the kindest 
thing we can do in that case.

In summary, I think it could be reasonable to have such a flag to disable the 
trap/unreachable *only* for scalar types (or, at a push, trivially-copyable 
types). But it seems unreasonable to have different defaults for Darwin, or to 
look at whether a `-Wreturn-type` warning would have fired.

Alternatively, since it seems you're only interested in the behavior of cases 
where `-Wreturn-type` would have fired, how about using Clang's tooling support 
to write a utility to add a return statement to the end of every function where 
it would fire, and give that to your users to fix their code?


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: test/CodeGenCXX/return.cpp:21
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }

rjmccall wrote:
> mehdi_amini wrote:
> > Quuxplusone wrote:
> > > Can you explain why a load from an uninitialized stack location would be 
> > > *better* than a trap and/or `unreachable`? IIUC this is basically what 
> > > Mehdi is asking: i.e., can you explain the rationale for this patch, 
> > > because I don't get it either. It *seems* like a strict regression in 
> > > code quality.
> > There is a difference. LLVM will optimize:
> > 
> > ```
> > define i32 @foo() {
> >   %1 = alloca i32, align 4
> >   %2 = load i32, i32* %1, align 4
> >   ret i32 %2
> > }
> > ```
> > 
> > to:
> > 
> > ```
> > define i32 @foo() {
> >   ret i32 undef
> > }
> > ```
> > 
> > Which means "return an undefined value" (basically any valide bit-pattern 
> > for an i32). This is not undefined behavior if the caller does not use the 
> > value with a side-effect.
> > 
> > However with:
> > 
> > ```
> > define i32 @foo() {
> >   unreachable
> > }
> > ```
> > 
> > Just calling `foo()` is undefined behavior, even if the returned value 
> > isn't used.
> > 
> > So what this patch does is actually making the compiled-code `safer` by 
> > inhibiting some optimizations based on this UB. 
> We've been disabling this optimization completely in Apple compilers for 
> years, so allowing it to ever kick in is actually an improvement.  I'll try 
> to elaborate on our reasoning for this change, which *is* actually somewhat 
> Apple-specific.
> 
> Falling off the end of a non-void function is only undefined behavior in C++, 
> not in C.  It is fairly common practice to compile C code as C++, and while 
> there's a large number of potential language incompatibilities there, they 
> tend to be rare or trivial to fix in practice.  At Apple, we have a specific 
> need to compile C code as C++ because of the C++-based IOKit API: while 
> drivers are overwhelmingly written as C code, at Apple they have to be 
> compiled as C++ in order to talk to the kernel.  Moreover, often Apple did 
> not write the drivers in question, and maintaining a large set of patches in 
> order to eliminate undefined behavior that wasn't actually UB in the 
> originally-intended build configuration is not really seen as acceptable.
> 
> It makes sense to me to expose this as a flag akin to -fno-strict-aliasing in 
> order to support C-in-C++ code bases like Apple's drivers.  It is possible 
> that we don't actually have to change the default for the flag on Apple 
> platforms and can instead pursue more targeted build changes.
I totally understand why such flag is desirable, it is just not a clear cut to 
make it the default on one platform only (and having this flag available 
upstream does not prevent the Xcode clang from enabling this by default though, 
if fixing the build settings isn't possible/desirable). 


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:530
+  // Don't need to mark Objective-C methods or blocks since the undefined
+  // behaviour optimization isn't used for them.
+}

Quuxplusone wrote:
> This seems like a trap waiting to spring on someone, unless there's a 
> technical reason that methods and blocks cannot possibly use the same 
> optimization paths as regular functions. ("Nobody's gotten around to 
> implementing it yet" is the most obvious nontechnical reason for the current 
> difference.) Either way, I'd expect this patch to include test cases for both 
> methods and blocks, to verify that the behavior you expect is actually the 
> behavior that happens. Basically, it ought to have a regression test 
> targeting the regression that I'm predicting is going to spring on someone as 
> soon as they implement optimizations for methods and blocks.
> 
> Also, one dumb question: what about C++ lambdas? are they FunctionDecls too? 
> test case?
> This seems like a trap waiting to spring on someone, unless there's a 
> technical reason that methods and blocks cannot possibly use the same 
> optimization paths as regular functions.

The optimization path in LLVM is the same. I think the difference lies in clang 
IRGen: there is no "unreachable" generated for these so the optimizer can't be 
aggressive. So this patch is not changing anything for Objective-C methods and 
blocks, and I expect that we *already* have a test that covers this behavior 
(if not we should add one).



Comment at: test/CodeGenCXX/return.cpp:21
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }

Quuxplusone wrote:
> Can you explain why a load from an uninitialized stack location would be 
> *better* than a trap and/or `unreachable`? IIUC this is basically what Mehdi 
> is asking: i.e., can you explain the rationale for this patch, because I 
> don't get it either. It *seems* like a strict regression in code quality.
There is a difference. LLVM will optimize:

```
define i32 @foo() {
  %1 = alloca i32, align 4
  %2 = load i32, i32* %1, align 4
  ret i32 %2
}
```

to:

```
define i32 @foo() {
  ret i32 undef
}
```

Which means "return an undefined value" (basically any valide bit-pattern for 
an i32). This is not undefined behavior if the caller does not use the value 
with a side-effect.

However with:

```
define i32 @foo() {
  unreachable
}
```

Just calling `foo()` is undefined behavior, even if the returned value isn't 
used.

So what this patch does is actually making the compiled-code `safer` by 
inhibiting some optimizations based on this UB. 



Comment at: test/CodeGenCXX/return.cpp:35
+// CHECK-NOSTRICT: @_Z21alwaysOptimizedReturn4Enum
+// CHECK-NOSTRICT-OPT: @_Z21alwaysOptimizedReturn4Enum
+int alwaysOptimizedReturn(Enum e) {

This should be `-LABEL` check lines. And instead of repeating 4 times the same 
check, you could add a common prefix.



Comment at: test/CodeGenCXX/return.cpp:47
+  // CHECK-NOSTRICT-OPT-NEXT: zext
+  // CHECK-NOSTRICT-OPT-NEXT: ret i32
+}

Document what's going on in the tests please.



Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:530
+  // Don't need to mark Objective-C methods or blocks since the undefined
+  // behaviour optimization isn't used for them.
+}

This seems like a trap waiting to spring on someone, unless there's a technical 
reason that methods and blocks cannot possibly use the same optimization paths 
as regular functions. ("Nobody's gotten around to implementing it yet" is the 
most obvious nontechnical reason for the current difference.) Either way, I'd 
expect this patch to include test cases for both methods and blocks, to verify 
that the behavior you expect is actually the behavior that happens. Basically, 
it ought to have a regression test targeting the regression that I'm predicting 
is going to spring on someone as soon as they implement optimizations for 
methods and blocks.

Also, one dumb question: what about C++ lambdas? are they FunctionDecls too? 
test case?



Comment at: test/CodeGenCXX/return.cpp:21
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }

Can you explain why a load from an uninitialized stack location would be 
*better* than a trap and/or `unreachable`? IIUC this is basically what Mehdi is 
asking: i.e., can you explain the rationale for this patch, because I don't get 
it either. It *seems* like a strict regression in code quality.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D27163#606744, @arphaman wrote:

> In https://reviews.llvm.org/D27163#606695, @mehdi_amini wrote:
>
> > What is the justification for a platform specific default change here?
>
>
> The flag itself is platform agnostic, however, the default value is different 
> on Darwin (-fno-strict-return). The reason for this difference is because of 
> how I interpreted the internal discussion for this issue: it seems that some 
> of our internal builds had problems with this flag, so to me it seemed that 
> people would've wanted this specific difference upstream.


I was not involved in the internal discussion, and the pre-existing internal 
arguments should be repeated here when needed to drive the patch.

I find dubious that the compiler wouldn't have specific handling for undefined 
behavior on a specific platform, without a strong argument that justify it 
(like a platform with a different hardware memory model making it more 
sensitive, etc.). In the current case, it seems like a "vendor specific choice" 
of being more conservative rather than something attached to the platform 
itself, so I'm not sure it makes sense to hard-code this upstream.

Do we have past records of doing this?


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D27163#606695, @mehdi_amini wrote:

> What is the justification for a platform specific default change here?


The flag itself is platform agnostic, however, the default value is different 
on Darwin (-fno-strict-return). The reason for this difference is because of 
how I interpreted the internal discussion for this issue: it seems that some of 
our internal builds had problems with this flag, so to me it seemed that people 
would've wanted this specific difference upstream. I might be wrong though and 
this might be not even the best approach, so let me know if you think there's a 
better solution.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

What is the justification for a platform specific default change here?


Repository:
  rL LLVM

https://reviews.llvm.org/D27163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 79396.
arphaman added a comment.

Fixed comment typo.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/LangOptions.def
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Sema/AnalysisBasedWarnings.h
  include/clang/Sema/Sema.h
  lib/CodeGen/CodeGenFunction.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/Sema.cpp
  test/CodeGenCXX/return.cpp
  test/Driver/clang_f_opts.c
  test/Driver/darwin-no-strict-return.c

Index: test/Driver/darwin-no-strict-return.c
===
--- /dev/null
+++ test/Driver/darwin-no-strict-return.c
@@ -0,0 +1,7 @@
+// Darwin should have -fno-strict-return turned on by default
+// rdar://13102603
+
+// RUN: %clang -### -S -target x86_64-apple-macosx10.7.0 %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-RETURN %s
+// RUN: %clang -### -S -target x86_64-apple-macosx10.7.0 -fstrict-return %s 2>&1 | FileCheck -check-prefix=CHECK-STRICT-RETURN %s
+// CHECK-NO-STRICT-RETURN: "-fno-strict-return"
+// CHECK-STRICT-RETURN-NOT: "-fno-strict-return"
Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -469,3 +469,8 @@
 // CHECK-WCHAR2: -fshort-wchar
 // CHECK-WCHAR2-NOT: -fno-short-wchar
 // DELIMITERS: {{^ *"}}
+
+// RUN: %clang -### -S -fstrict-return %s 2>&1 | FileCheck -check-prefix=CHECK-STRICT-RETURN %s
+// RUN: %clang -### -S -fno-strict-return %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-RETURN %s
+// CHECK-STRICT-RETURN-NOT: "-fno-strict-return"
+// CHECK-NO-STRICT-RETURN: "-fno-strict-return"
Index: test/CodeGenCXX/return.cpp
===
--- test/CodeGenCXX/return.cpp
+++ test/CodeGenCXX/return.cpp
@@ -1,12 +1,61 @@
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -O -o - %s | FileCheck %s --check-prefix=CHECK-OPT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -fno-strict-return -o - %s | FileCheck %s --check-prefix=CHECK-NOSTRICT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -fno-strict-return -Wno-return-type -o - %s | FileCheck %s --check-prefix=CHECK-NOSTRICT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -fno-strict-return -O -o - %s | FileCheck %s --check-prefix=CHECK-NOSTRICT-OPT
 
 // CHECK: @_Z9no_return
 // CHECK-OPT: @_Z9no_return
+// CHECK-NOSTRICT: @_Z9no_return
+// CHECK-NOSTRICT-OPT: @_Z9no_return
 int no_return() {
   // CHECK:  call void @llvm.trap
   // CHECK-NEXT: unreachable
 
   // CHECK-OPT-NOT: call void @llvm.trap
   // CHECK-OPT: unreachable
+
+  // CHECK-NOSTRICT: entry:
+  // CHECK-NOSTRICT-NEXT: alloca
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT: ret i32 undef
+}
+
+enum Enum {
+  A, B
+};
+
+// CHECK: @_Z21alwaysOptimizedReturn4Enum
+// CHECK-OPT: @_Z21alwaysOptimizedReturn4Enum
+// CHECK-NOSTRICT: @_Z21alwaysOptimizedReturn4Enum
+// CHECK-NOSTRICT-OPT: @_Z21alwaysOptimizedReturn4Enum
+int alwaysOptimizedReturn(Enum e) {
+  switch (e) {
+  case A: return 0;
+  case B: return 1;
+  }
+  // CHECK-NOSTRICT:  call void @llvm.trap()
+  // CHECK-NOSTRICT-NEXT: unreachable
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT-NEXT: icmp
+  // CHECK-NOSTRICT-OPT-NEXT: zext
+  // CHECK-NOSTRICT-OPT-NEXT: ret i32
+}
+
+// CHECK-NOSTRICT: @_Z23strictlyOptimizedReturn4Enum
+// CHECK-NOSTRICT-OPT: @_Z23strictlyOptimizedReturn4Enum
+int strictlyOptimizedReturn(Enum e) {
+  switch (e) {
+  case A: return 22;
+  }
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT-NEXT: ret i32 22
 }
Index: lib/Sema/Sema.cpp
===
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -1154,7 +1154,7 @@
 }
 
 void Sema::PopFunctionScopeInfo(const AnalysisBasedWarnings::Policy *WP,
-const Decl *D, const BlockExpr *blkExpr) {
+Decl *D, const BlockExpr *blkExpr) {
   FunctionScopeInfo *Scope = FunctionScopes.pop_back_val();
   assert(!FunctionScopes.empty() && "mismatched push/pop!");
 
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -521,13 +521,22 @@
 
 } // anonymous namespace
 
+/// The given declaration \p D is marked as associated with a non-void return
+/// warning.
+static void markHasNonVoidReturnWarning(Decl *D) {
+  if 

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.
arphaman added reviewers: rjmccall, rsmith, mehdi_amini.
arphaman added a subscriber: cfe-commits.
arphaman set the repository for this revision to rL LLVM.

This patch adds a new clang flag called `-f[no-]strict-return`. The purpose of 
this flag is to control how the code generator applies the undefined behaviour 
return optimisation to value returning functions that flow-off without a 
required return:

- If -fstrict-return is on (default for non Darwin targets), then the code 
generator follows the current behaviour: it emits the IR for the undefined 
behaviour (trap with unreachable).

- Otherwise, the code generator emits the IR for the undefined behaviour only 
if the function avoided -Wreturn-type warnings. This avoidance is detected even 
if -Wreturn-type warnings are disabled (-Wno-return-type).




Repository:
  rL LLVM

https://reviews.llvm.org/D27163

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/LangOptions.def
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Sema/AnalysisBasedWarnings.h
  include/clang/Sema/Sema.h
  lib/CodeGen/CodeGenFunction.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/Sema.cpp
  test/CodeGenCXX/return.cpp
  test/Driver/clang_f_opts.c
  test/Driver/darwin-no-strict-return.c

Index: test/Driver/darwin-no-strict-return.c
===
--- /dev/null
+++ test/Driver/darwin-no-strict-return.c
@@ -0,0 +1,7 @@
+// Darwin should have -fno-strict-return turned on by default
+// rdar://13102603
+
+// RUN: %clang -### -S -target x86_64-apple-macosx10.7.0 %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-RETURN %s
+// RUN: %clang -### -S -target x86_64-apple-macosx10.7.0 -fstrict-return %s 2>&1 | FileCheck -check-prefix=CHECK-STRICT-RETURN %s
+// CHECK-NO-STRICT-RETURN: "-fno-strict-return"
+// CHECK-STRICT-RETURN-NOT: "-fno-strict-return"
Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -469,3 +469,8 @@
 // CHECK-WCHAR2: -fshort-wchar
 // CHECK-WCHAR2-NOT: -fno-short-wchar
 // DELIMITERS: {{^ *"}}
+
+// RUN: %clang -### -S -fstrict-return %s 2>&1 | FileCheck -check-prefix=CHECK-STRICT-RETURN %s
+// RUN: %clang -### -S -fno-strict-return %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-RETURN %s
+// CHECK-STRICT-RETURN-NOT: "-fno-strict-return"
+// CHECK-NO-STRICT-RETURN: "-fno-strict-return"
Index: test/CodeGenCXX/return.cpp
===
--- test/CodeGenCXX/return.cpp
+++ test/CodeGenCXX/return.cpp
@@ -1,12 +1,61 @@
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -O -o - %s | FileCheck %s --check-prefix=CHECK-OPT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -fno-strict-return -o - %s | FileCheck %s --check-prefix=CHECK-NOSTRICT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -fno-strict-return -Wno-return-type -o - %s | FileCheck %s --check-prefix=CHECK-NOSTRICT
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -fno-strict-return -O -o - %s | FileCheck %s --check-prefix=CHECK-NOSTRICT-OPT
 
 // CHECK: @_Z9no_return
 // CHECK-OPT: @_Z9no_return
+// CHECK-NOSTRICT: @_Z9no_return
+// CHECK-NOSTRICT-OPT: @_Z9no_return
 int no_return() {
   // CHECK:  call void @llvm.trap
   // CHECK-NEXT: unreachable
 
   // CHECK-OPT-NOT: call void @llvm.trap
   // CHECK-OPT: unreachable
+
+  // CHECK-NOSTRICT: entry:
+  // CHECK-NOSTRICT-NEXT: alloca
+  // CHECK-NOSTRICT-NEXT: load
+  // CHECK-NOSTRICT-NEXT: ret i32
+  // CHECK-NOSTRICT-NEXT: }
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT: ret i32 undef
+}
+
+enum Enum {
+  A, B
+};
+
+// CHECK: @_Z21alwaysOptimizedReturn4Enum
+// CHECK-OPT: @_Z21alwaysOptimizedReturn4Enum
+// CHECK-NOSTRICT: @_Z21alwaysOptimizedReturn4Enum
+// CHECK-NOSTRICT-OPT: @_Z21alwaysOptimizedReturn4Enum
+int alwaysOptimizedReturn(Enum e) {
+  switch (e) {
+  case A: return 0;
+  case B: return 1;
+  }
+  // CHECK-NOSTRICT:  call void @llvm.trap()
+  // CHECK-NOSTRICT-NEXT: unreachable
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT-NEXT: icmp
+  // CHECK-NOSTRICT-OPT-NEXT: zext
+  // CHECK-NOSTRICT-OPT-NEXT: ret i32
+}
+
+// CHECK-NOSTRICT: @_Z23strictlyOptimizedReturn4Enum
+// CHECK-NOSTRICT-OPT: @_Z23strictlyOptimizedReturn4Enum
+int strictlyOptimizedReturn(Enum e) {
+  switch (e) {
+  case A: return 22;
+  }
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
+
+  // CHECK-NOSTRICT-OPT: entry:
+  // CHECK-NOSTRICT-OPT-NEXT: ret i32 22
 }
Index: lib/Sema/Sema.cpp
===
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@