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

[arphaman]

Summary

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).

[arphaman]

Fixed comment typo.

[mehdi_amini]

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

[arphaman]

In 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.

[mehdi_amini]

In D27163#606744, @arphaman wrote:

In 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?

[• Quuxplusone]

lib/Sema/AnalysisBasedWarnings.cpp

530 ↗

(On Diff #79396)

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?

test/CodeGenCXX/return.cpp

21 ↗

(On Diff #79396)

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.

[mehdi_amini]

lib/Sema/AnalysisBasedWarnings.cpp

530 ↗

(On Diff #79396)

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).

test/CodeGenCXX/return.cpp

21 ↗

(On Diff #79396)

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.

35 ↗

(On Diff #79396)

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

47 ↗

(On Diff #79396)

Document what's going on in the tests please.

[rjmccall]

test/CodeGenCXX/return.cpp

21 ↗

(On Diff #79396)

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.

[mehdi_amini]

test/CodeGenCXX/return.cpp

21 ↗

(On Diff #79396)

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).

[rsmith]

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?

[rsmith]

In 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?

[rjmccall]

In D27163#607100, @rsmith wrote:

In 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).

[rnk]

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

[arphaman]

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.

[arphaman]

lib/Sema/AnalysisBasedWarnings.cpp

530 ↗

(On Diff #79396)

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.

[arphaman]

test/CodeGenCXX/return.cpp

47 ↗

(On Diff #79396)

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.

[arphaman]

In 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.

[arphaman]

Ping.

[• Quuxplusone]

include/clang/Sema/AnalysisBasedWarnings.h

93 ↗

(On Diff #79556)

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.

lib/CodeGen/CodeGenFunction.cpp

1078 ↗

(On Diff #79556)

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

1173 ↗

(On Diff #79556)

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.

[arphaman]

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.

include/clang/Sema/AnalysisBasedWarnings.h

93 ↗

(On Diff #79556)

That's a fair point, I've thought about it a bit more and decided to get rid of the "-Wreturn-type" heuristic. If anything I can try a follow-up patch later if we really end up needing it, but I don't think so.

lib/CodeGen/CodeGenFunction.cpp

1078 ↗

(On Diff #79556)

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.

1173 ↗

(On Diff #79556)

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?

I understand your suggestion, but I'd prefer not to set hasImplicitReturnZero for all functions, since then Sema won't warn about the missing return, which we still want.

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.

I don't think a name like "-fimplicit-return-zero" is appropriate, since it implies that the compiler guarantees a return of a zero value. This might mislead users as they might think that they can use the return value without triggering undefined behaviour, which isn't true.

[majnemer]

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.

[mehdi_amini]

LGTM.

lib/CodeGen/CodeGenFunction.cpp

1086 ↗

(On Diff #81584)

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

[mehdi_amini]

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

[• Quuxplusone]

LGTM. I wonder if rsmith is happy with the exact semantics of "shouldUseUndefinedBehaviorReturnOptimization"... but that seems like a tiny nit that's fixable post-commit anyway.

include/clang/Driver/Options.td

1324 ↗

(On Diff #81584)

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.)

lib/CodeGen/CodeGenFunction.cpp

1071 ↗

(On Diff #81584)

Nit: this code comment is not particularly useful to the reader.

1173 ↗

(On Diff #79556)

since then Sema won't warn about the missing return

Ah. Yeah, that makes sense.

[mehdi_amini]

include/clang/Driver/Options.td

1324 ↗

(On Diff #81584)

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.

[rjmccall]

lib/CodeGen/CodeGenFunction.cpp

1078 ↗

(On Diff #79556)

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.

[arphaman]

In 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.

[arphaman]

lib/CodeGen/CodeGenFunction.cpp

1078 ↗

(On Diff #79556)

I see what you mean, this kind of reasoning makes sense. I'll use just the hasTrivialDestructor check when committing.

[arphaman]

include/clang/Driver/Options.td

1324 ↗

(On Diff #81584)

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.

cfe/trunk/include/clang/Driver/Options.td

def fno_data_sections : Flag <["-"], "fno-data-sections">, Group<f_Group>,
   Flags<[CC1Option]>;
 
 def funique_section_names : Flag <["-"], "funique-section-names">,
   Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use unique names for text and data sections (ELF Only)">;
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group<f_Group>, Flags<[CC1Option]>;
 
+def fstrict_return : Flag<["-"], "fstrict-return">, Group<f_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<f_Group>,
+  Flags<[CC1Option]>;
 
 def fdebug_types_section: Flag <["-"], "fdebug-types-section">, Group<f_Group>,
   Flags<[CC1Option]>, HelpText<"Place debug types in their own section (ELF Only)">;
 def fno_debug_types_section: Flag<["-"], "fno-debug-types-section">, Group<f_Group>,
   Flags<[CC1Option]>;
 def fsplit_dwarf_inlining: Flag <["-"], "fsplit-dwarf-inlining">, Group<f_Group>,
   Flags<[CC1Option]>, HelpText<"Place debug types in their own section (ELF Only)">;
 def fno_split_dwarf_inlining: Flag<["-"], "fno-split-dwarf-inlining">, Group<f_Group>,

cfe/trunk/include/clang/Frontend/CodeGenOptions.def

 VALUE_CODEGENOPT(EmitCheckPathComponentsToStrip, 32, 0)
 
 /// Whether to report the hotness of the code region for optimization remarks.
 CODEGENOPT(DiagnosticsWithHotness, 1, 0)
 
 /// 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
 

cfe/trunk/lib/CodeGen/CodeGenFunction.cpp

QualType CodeGenFunction::BuildFunctionArgList(GlobalDecl GD,
   }
 
   if (MD && (isa<CXXConstructorDecl>(MD) || isa<CXXDestructorDecl>(MD)))
     CGM.getCXXABI().addImplicitStructorParams(*this, ResTy, Args);
 
   return ResTy;
 }
 
+static bool
+shouldUseUndefinedBehaviorReturnOptimization(const FunctionDecl *FD,
+                                             const ASTContext &Context) {
+  QualType T = FD->getReturnType();
+  // Avoid the optimization for functions that return a record type with a
+  // trivial destructor or another trivially copyable type.
+  if (const RecordType *RT = T.getCanonicalType()->getAs<RecordType>()) {
+    if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl()))
+      return !ClassDecl->hasTrivialDestructor();
+  }
+  return !T.isTriviallyCopyableType(Context);
+}
+
 void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
                                    const CGFunctionInfo &FnInfo) {
   const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
   CurGD = GD;
 
   FunctionArgList Args;
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
   // 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 (getLangOpts().CPlusPlus && !FD->hasImplicitReturnZero() && !SawAsmBlock &&
       !FD->getReturnType()->isVoidType() && Builder.GetInsertBlock()) {
+    bool ShouldEmitUnreachable =
+        CGM.getCodeGenOpts().StrictReturn ||
+        shouldUseUndefinedBehaviorReturnOptimization(FD, getContext());
     if (SanOpts.has(SanitizerKind::Return)) {
       SanitizerScope SanScope(this);
       llvm::Value *IsFalse = Builder.getFalse();
       EmitCheck(std::make_pair(IsFalse, SanitizerKind::Return),
                 SanitizerHandler::MissingReturn,
                 EmitCheckSourceLocation(FD->getLocation()), None);
-    } else if (CGM.getCodeGenOpts().OptimizationLevel == 0) {
+    } else if (ShouldEmitUnreachable) {
+      if (CGM.getCodeGenOpts().OptimizationLevel == 0)
         EmitTrapCall(llvm::Intrinsic::trap);
     }
+    if (SanOpts.has(SanitizerKind::Return) || ShouldEmitUnreachable) {
       Builder.CreateUnreachable();
       Builder.ClearInsertionPoint();
     }
+  }
 
   // Emit the standard function epilogue.
   FinishFunction(BodyRange.getEnd());
 
   // If we haven't marked the function nothrow through other means, do
   // a quick pass now to see if we can.
   if (!CurFn->doesNotThrow())
     TryMarkNoThrow(CurFn);

cfe/trunk/lib/Driver/Tools.cpp

if (!Args.hasFlag(options::OPT_fstrict_aliasing, StrictAliasingAliasOption,
                     options::OPT_fno_strict_aliasing, TBAAOnByDefault))
     CmdArgs.push_back("-relaxed-aliasing");
   if (!Args.hasFlag(options::OPT_fstruct_path_tbaa,
                     options::OPT_fno_struct_path_tbaa))
     CmdArgs.push_back("-no-struct-path-tbaa");
   if (Args.hasFlag(options::OPT_fstrict_enums, options::OPT_fno_strict_enums,
                    false))
     CmdArgs.push_back("-fstrict-enums");
+  if (!Args.hasFlag(options::OPT_fstrict_return, options::OPT_fno_strict_return,
+                    true))
+    CmdArgs.push_back("-fno-strict-return");
   if (Args.hasFlag(options::OPT_fstrict_vtable_pointers,
                    options::OPT_fno_strict_vtable_pointers,
                    false))
     CmdArgs.push_back("-fstrict-vtable-pointers");
   if (!Args.hasFlag(options::OPT_foptimize_sibling_calls,
                     options::OPT_fno_optimize_sibling_calls))
     CmdArgs.push_back("-mdisable-tail-calls");
 

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

Opts.IncrementalLinkerCompatible =
       Args.hasArg(OPT_mincremental_linker_compatible);
   Opts.PIECopyRelocations =
       Args.hasArg(OPT_mpie_copy_relocations);
   Opts.OmitLeafFramePointer = Args.hasArg(OPT_momit_leaf_frame_pointer);
   Opts.SaveTempLabels = Args.hasArg(OPT_msave_temp_labels);
   Opts.NoDwarfDirectoryAsm = Args.hasArg(OPT_fno_dwarf_directory_asm);
   Opts.SoftFloat = Args.hasArg(OPT_msoft_float);
   Opts.StrictEnums = Args.hasArg(OPT_fstrict_enums);
+  Opts.StrictReturn = !Args.hasArg(OPT_fno_strict_return);
   Opts.StrictVTablePointers = Args.hasArg(OPT_fstrict_vtable_pointers);
   Opts.UnsafeFPMath = Args.hasArg(OPT_menable_unsafe_fp_math) ||
                       Args.hasArg(OPT_cl_unsafe_math_optimizations) ||
                       Args.hasArg(OPT_cl_fast_relaxed_math);
   Opts.UnwindTables = Args.hasArg(OPT_munwind_tables);
   Opts.RelocationModel = Args.getLastArgValue(OPT_mrelocation_model, "pic");
   Opts.ThreadModel = Args.getLastArgValue(OPT_mthread_model, "posix");
   if (Opts.ThreadModel != "posix" && Opts.ThreadModel != "single")

cfe/trunk/test/CodeGenCXX/return.cpp

-// 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;
+
+  NonTrivialDefaultConstructor() { }
+};
+
+// CHECK-NOSTRICT-LABEL: @_Z28nonTrivialDefaultConstructorv
+NonTrivialDefaultConstructor nonTrivialDefaultConstructor() {
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
+}
+
+// Functions that return records with non-trivial destructors should always use
+// the -fstrict-return optimization.
+
+struct NonTrivialDestructor {
+  ~NonTrivialDestructor();
+};
+
+// CHECK-NOSTRICT-LABEL: @_Z20nonTrivialDestructorv
+NonTrivialDestructor nonTrivialDestructor() {
+  // CHECK-NOSTRICT: call void @llvm.trap
+  // CHECK-NOSTRICT-NEXT: unreachable
+}
+
+// The behavior for lambdas should be identical to functions.
+// CHECK-COMMON-LABEL: @_Z10lambdaTestv
+void lambdaTest() {
+  auto lambda1 = []() -> int {
+  };
+  lambda1();
+
+  // CHECK: call void @llvm.trap
+  // CHECK-NEXT: unreachable
+
+  // CHECK-NOSTRICT-NOT: call void @llvm.trap
+  // CHECK-NOSTRICT-NOT: unreachable
 }

cfe/trunk/test/CodeGenObjCXX/return.mm

+// 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

cfe/trunk/test/Driver/clang_f_opts.c

 
 // RUN: %clang -### -fno-experimental-new-pass-manager -fexperimental-new-pass-manager %s 2>&1 | FileCheck --check-prefix=CHECK-PM --check-prefix=CHECK-NEW-PM %s
 // RUN: %clang -### -fexperimental-new-pass-manager -fno-experimental-new-pass-manager %s 2>&1 | FileCheck --check-prefix=CHECK-PM --check-prefix=CHECK-NO-NEW-PM %s
 // CHECK-PM-NOT: argument unused
 // CHECK-NEW-PM: -fexperimental-new-pass-manager
 // CHECK-NEW-PM-NOT: -fno-experimental-new-pass-manager
 // CHECK-NO-NEW-PM: -fno-experimental-new-pass-manager
 // CHECK-NO-NEW-PM-NOT: -fexperimental-new-pass-manager
+
+// 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"