Page MenuHomePhabricator

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

Authored by arphaman on Nov 28 2016, 6:00 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 79395.Nov 28 2016, 6:00 AM
arphaman retitled this revision from to Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation.
arphaman updated this object.
arphaman added reviewers: rjmccall, rsmith, mehdi_amini.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: cfe-commits.
arphaman updated this revision to Diff 79396.Nov 28 2016, 6:04 AM

Fixed comment typo.

mehdi_amini edited edge metadata.Nov 28 2016, 9:56 AM

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

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.

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 added inline comments.
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 added inline comments.Nov 28 2016, 1:38 PM
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 added inline comments.Nov 28 2016, 1:55 PM
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 added inline comments.Nov 28 2016, 2:00 PM
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 edited edge metadata.Nov 28 2016, 2:02 PM

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?

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 edited edge metadata.Nov 28 2016, 2:27 PM

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 added a subscriber: rnk.Nov 28 2016, 3:09 PM

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

arphaman updated this revision to Diff 79556.Nov 29 2016, 6:20 AM
arphaman edited edge metadata.

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 added inline comments.Nov 29 2016, 6:22 AM
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 added inline comments.Nov 29 2016, 6:26 AM
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.

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.

Quuxplusone added inline comments.Dec 15 2016, 3:20 AM
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 updated this revision to Diff 81584.Dec 15 2016, 7:56 AM
arphaman marked 3 inline comments as done.

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.

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 accepted this revision.Dec 15 2016, 9:46 AM
mehdi_amini edited edge metadata.

LGTM.

lib/CodeGen/CodeGenFunction.cpp
1086 ↗(On Diff #81584)

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

This revision is now accepted and ready to land.Dec 15 2016, 9:46 AM

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

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 added inline comments.Dec 15 2016, 10:57 AM
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 added inline comments.Dec 15 2016, 1:15 PM
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.

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 added inline comments.Dec 16 2016, 7:41 AM
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 added inline comments.Dec 16 2016, 7:52 AM
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.

This revision was automatically updated to reflect the committed changes.
arphaman marked 5 inline comments as done.