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

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

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

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

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

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

47

Document what's going on in the tests please.

[rjmccall]

test/CodeGenCXX/return.cpp

21

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

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

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

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–94

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

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

1157

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–94

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

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.

1157

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

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

1323

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

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

1157

since then Sema won't warn about the missing return

Ah. Yeah, that makes sense.

[mehdi_amini]

include/clang/Driver/Options.td

1323

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

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

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

1323

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.

include/clang/AST/Decl.h

private:
   /// skipped.
   unsigned HasSkippedBody : 1;
 
   /// Indicates if the function declaration will have a body, once we're done
   /// parsing it.  (We don't set it to false when we're done parsing, in the
   /// hopes this is simpler.)
   unsigned WillHaveBody : 1;
 
+  /// Indicates if analysis has found that control may reach the end of this
+  /// function without executing an appropriate return.
+  unsigned HasNonVoidReturnWarning : 1;
+
   /// \brief End part of this FunctionDecl's source range.
   ///
   /// We could compute the full range in getSourceRange(). However, when we're
   /// dealing with a function definition deserialized from a PCH/AST file,
   /// we can only compute the full range once the function body has been
   /// de-serialized, so it's far better to have the (sometimes-redundant)
   /// EndRangeLoc.
   SourceLocation EndRangeLoc;
FunctionDecl(Kind DK, ASTContext &C, DeclContext *DC, SourceLocation StartLoc,
         DeclContext(DK), redeclarable_base(C), ParamInfo(nullptr), Body(),
         SClass(S), IsInline(isInlineSpecified),
         IsInlineSpecified(isInlineSpecified), IsVirtualAsWritten(false),
         IsPure(false), HasInheritedPrototype(false), HasWrittenPrototype(true),
         IsDeleted(false), IsTrivial(false), IsDefaulted(false),
         IsExplicitlyDefaulted(false), HasImplicitReturnZero(false),
         IsLateTemplateParsed(false), IsConstexpr(isConstexprSpecified),
         UsesSEHTry(false), HasSkippedBody(false), WillHaveBody(false),
-        EndRangeLoc(NameInfo.getEndLoc()), TemplateOrSpecialization(),
-        DNLoc(NameInfo.getInfo()) {}
+        HasNonVoidReturnWarning(false), EndRangeLoc(NameInfo.getEndLoc()),
+        TemplateOrSpecialization(), DNLoc(NameInfo.getInfo()) {}
 
   typedef Redeclarable<FunctionDecl> redeclarable_base;
   FunctionDecl *getNextRedeclarationImpl() override {
     return getNextRedeclaration();
   }
   FunctionDecl *getPreviousDeclImpl() override {
     return getPreviousDecl();
   }
public:
   /// \brief True if the function was a definition but its body was skipped.
   bool hasSkippedBody() const { return HasSkippedBody; }
   void setHasSkippedBody(bool Skipped = true) { HasSkippedBody = Skipped; }
 
   /// True if this function will eventually have a body, once it's fully parsed.
   bool willHaveBody() const { return WillHaveBody; }
   void setWillHaveBody(bool V = true) { WillHaveBody = V; }
 
+  /// True if analysis has found that control may reach the end of this function
+  /// without executing an appropriate return.
+  bool hasNonVoidReturnWarning() const { return HasNonVoidReturnWarning; }
+  void setHasNonVoidReturnWarning(bool V = true) {
+    HasNonVoidReturnWarning = V;
+  }
+
   void setPreviousDeclaration(FunctionDecl * PrevDecl);
 
   FunctionDecl *getCanonicalDecl() override;
   const FunctionDecl *getCanonicalDecl() const {
     return const_cast<FunctionDecl*>(this)->getCanonicalDecl();
   }
 
   unsigned getBuiltinID() const;

include/clang/Basic/LangOptions.def

 LANGOPT(ApplePragmaPack, 1, 0, "Apple gcc-compatible #pragma pack handling")
 
 LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
 
 LANGOPT(SanitizeAddressFieldPadding, 2, 0, "controls how aggressive is ASan "
                                            "field padding (0: none, 1:least "
                                            "aggressive, 2: more aggressive)")
 
+LANGOPT(CheckReturnControlFlow, 1, 0, "Should the control flow for missing"
+                                      "returns be checked even if the"
+                                      "corresponding diagnostic is disabled")
+
 #undef LANGOPT
 #undef COMPATIBLE_LANGOPT
 #undef BENIGN_LANGOPT
 #undef ENUM_LANGOPT
 #undef COMPATIBLE_ENUM_LANGOPT
 #undef BENIGN_ENUM_LANGOPT
 #undef VALUE_LANGOPT
 #undef COMPATIBLE_VALUE_LANGOPT
 #undef BENIGN_VALUE_LANGOPT
 

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<"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<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>,

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
 

include/clang/Sema/AnalysisBasedWarnings.h

private:
   /// a single function.
   unsigned MaxUninitAnalysisBlockVisitsPerFunction;
 
   /// @}
 
 public:
   AnalysisBasedWarnings(Sema &s);
 
-  void IssueWarnings(Policy P, FunctionScopeInfo *fscope,
-                     const Decl *D, const BlockExpr *blkExpr);
+  void IssueWarnings(Policy P, FunctionScopeInfo *fscope, Decl *D,
+                     const BlockExpr *blkExpr);
 
   Policy getDefaultPolicy() { return DefaultPolicy; }
 
   void PrintStats() const;
 };
 
 }} // end namespace clang::sema
 
 #endif

include/clang/Sema/Sema.h

public:
   /// when parsing generic lambda 'auto' parameters.
   void RecordParsingTemplateParameterDepth(unsigned Depth);
   
   void PushCapturedRegionScope(Scope *RegionScope, CapturedDecl *CD,
                                RecordDecl *RD,
                                CapturedRegionKind K);
   void
   PopFunctionScopeInfo(const sema::AnalysisBasedWarnings::Policy *WP = nullptr,
-                       const Decl *D = nullptr,
-                       const BlockExpr *blkExpr = nullptr);
+                       Decl *D = nullptr, const BlockExpr *blkExpr = nullptr);
 
   sema::FunctionScopeInfo *getCurFunction() const {
     return FunctionScopes.back();
   }
   
   sema::FunctionScopeInfo *getEnclosingFunction() const {
     if (FunctionScopes.empty())
       return nullptr;

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;
 }
 
 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);
 
   // Check if we should generate debug info for this function.
   if (FD->hasAttr<NoDebugAttr>())
     DebugInfo = nullptr; // disable debug info indefinitely for this function
 
   SourceRange BodyRange;
   if (Stmt *Body = FD->getBody()) BodyRange = Body->getSourceRange();
   CurEHLocation = BodyRange.getEnd();
 
   // Use the location of the start of the function to determine where
   // the function definition is located. By default use the location
   // of the declaration as the location for the subprogram. A function
   // may lack a declaration in the source code if it is created by code
   // gen. (examples: _GLOBAL__I_a, __cxx_global_array_dtor, thunk).
   SourceLocation Loc = FD->getLocation();
 
   // If this is a function specialization then use the pattern body
   // as the location for the function.
void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
   if (getLangOpts().CPlusPlus && !FD->hasImplicitReturnZero() && !SawAsmBlock &&
       !FD->getReturnType()->isVoidType() && Builder.GetInsertBlock()) {
     if (SanOpts.has(SanitizerKind::Return)) {
       SanitizerScope SanScope(this);
       llvm::Value *IsFalse = Builder.getFalse();
       EmitCheck(std::make_pair(IsFalse, SanitizerKind::Return),
                 "missing_return", EmitCheckSourceLocation(FD->getLocation()),
                 None);
-    } else if (CGM.getCodeGenOpts().OptimizationLevel == 0) {
+      Builder.CreateUnreachable();
+      Builder.ClearInsertionPoint();
+    } else if (CGM.getCodeGenOpts().StrictReturn ||
+               !FD->hasNonVoidReturnWarning()) {
+      if (CGM.getCodeGenOpts().OptimizationLevel == 0)
         EmitTrapCall(llvm::Intrinsic::trap);
-    }
       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);

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");
+  // -fno-strict-return is turned on by default on Darwin.
+  if (!Args.hasFlag(options::OPT_fstrict_return, options::OPT_fno_strict_return,
+                    !getToolChain().getTriple().isOSDarwin()))
+    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");
 

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")
#include "clang/Frontend/LangStandards.def"
 
   // Parse -fsanitize= arguments.
   parseSanitizerKinds("-fsanitize=", Args.getAllArgValues(OPT_fsanitize_EQ),
                       Diags, Opts.Sanitize);
   // -fsanitize-address-field-padding=N has to be a LangOpt, parse it here.
   Opts.SanitizeAddressFieldPadding =
       getLastArgIntValue(Args, OPT_fsanitize_address_field_padding, 0, Diags);
   Opts.SanitizerBlacklistFiles = Args.getAllArgValues(OPT_fsanitize_blacklist);
+  Opts.CheckReturnControlFlow = Args.hasArg(OPT_fno_strict_return);
 }
 
 static void ParsePreprocessorArgs(PreprocessorOptions &Opts, ArgList &Args,
                                   FileManager &FileMgr,
                                   DiagnosticsEngine &Diags) {
   using namespace options;
   Opts.ImplicitPCHInclude = Args.getLastArgValue(OPT_include_pch);
   Opts.ImplicitPTHInclude = Args.getLastArgValue(OPT_include_pth);

lib/Sema/AnalysisBasedWarnings.cpp

bool checkDiagnostics(DiagnosticsEngine &D, bool ReturnsVoid,
     }
     // For blocks / lambdas.
     return ReturnsVoid && !HasNoReturn;
   }
 };
 
 } // anonymous namespace
 
+/// The given declaration \p D is marked as associated with a non-void return
+/// warning.
+static void markHasNonVoidReturnWarning(Decl *D) {
+  if (auto *FD = dyn_cast<FunctionDecl>(D))
+    FD->setHasNonVoidReturnWarning();
+  // Don't need to mark Objective-C methods or blocks since the undefined
+  // behaviour optimization isn't used for them.
+}
+
 /// CheckFallThroughForFunctionDef - Check that we don't fall off the end of a
 /// function that should return a value.  Check that we don't fall off the end
 /// of a noreturn function.  We assume that functions and blocks not marked
 /// noreturn will return.
-static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
+static void CheckFallThroughForBody(Sema &S, Decl *D, const Stmt *Body,
                                     const BlockExpr *blkExpr,
-                                    const CheckFallThroughDiagnostics& CD,
+                                    const CheckFallThroughDiagnostics &CD,
                                     AnalysisDeclContext &AC) {
 
   bool ReturnsVoid = false;
   bool HasNoReturn = false;
 
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     if (const auto *CBody = dyn_cast<CoroutineBodyStmt>(Body))
       ReturnsVoid = CBody->getFallthroughHandler() != nullptr;
if (const FunctionType *FT =
       if (FT->getNoReturnAttr())
         HasNoReturn = true;
     }
   }
 
   DiagnosticsEngine &Diags = S.getDiagnostics();
 
   // Short circuit for compilation speed.
-  if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn))
-      return;
+  if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn) &&
+      !S.getLangOpts().CheckReturnControlFlow)
 
   SourceLocation LBrace = Body->getLocStart(), RBrace = Body->getLocEnd();
   // Either in a function body compound statement, or a function-try-block.
   switch (CheckFallThrough(AC)) {
     case UnknownFallThrough:
       break;
 
     case MaybeFallThrough:
       if (HasNoReturn)
         S.Diag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn);
-      else if (!ReturnsVoid)
+      else if (!ReturnsVoid) {
+        markHasNonVoidReturnWarning(D);
         S.Diag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid);
+      }
       break;
     case AlwaysFallThrough:
       if (HasNoReturn)
         S.Diag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
-      else if (!ReturnsVoid)
+      else if (!ReturnsVoid) {
+        markHasNonVoidReturnWarning(D);
         S.Diag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
+      }
       break;
     case NeverFallThroughOrReturn:
       if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) {
         if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
           S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn) << 0 << FD;
         } else if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
           S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn) << 1 << MD;
         } else {
DefaultPolicy.enableConsumedAnalysis =
     isEnabled(D, warn_use_in_invalid_state);
 }
 
 static void flushDiagnostics(Sema &S, const sema::FunctionScopeInfo *fscope) {
   for (const auto &D : fscope->PossiblyUnreachableDiags)
     S.Diag(D.Loc, D.PD);
 }
 
-void clang::sema::
-AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
-                                     sema::FunctionScopeInfo *fscope,
-                                     const Decl *D, const BlockExpr *blkExpr) {
+void clang::sema::AnalysisBasedWarnings::IssueWarnings(
+    sema::AnalysisBasedWarnings::Policy P, sema::FunctionScopeInfo *fscope,
+    Decl *D, const BlockExpr *blkExpr) {
 
   // We avoid doing analysis-based warnings when there are errors for
   // two reasons:
   // (1) The CFGs often can't be constructed (if the body is invalid), so
   //     don't bother trying.
   // (2) The code already has problems; running the analysis just takes more
   //     time.
   DiagnosticsEngine &Diags = S.getDiagnostics();

lib/Sema/Sema.cpp

if (LambdaScopeInfo *const LSI = getCurLambda()) {
     LSI->AutoTemplateParameterDepth = Depth;
     return;
   } 
   llvm_unreachable( 
       "Remove assertion if intentionally called in a non-lambda context.");
 }
 
 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!");
 
   // Issue any analysis-based warnings.
   if (WP && D)
     AnalysisWarnings.IssueWarnings(*WP, Scope, D, blkExpr);
   else
     for (const auto &PUD : Scope->PossiblyUnreachableDiags)

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 -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
 }

test/Driver/clang_f_opts.c

 // Make sure we don't match the -NOT lines with the linker invocation.
 // Delimiters match the start of the cc1 and the start of the linker lines
 // DELIMITERS: {{^ *"}}
 // CHECK-WCHAR1: -fno-short-wchar
 // CHECK-WCHAR1-NOT: -fshort-wchar
 // 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"

test/Driver/darwin-no-strict-return.c

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