[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation
Alex Lorenz via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 15 07:57:05 PST 2016
arphaman added inline comments.
================
Comment at: include/clang/Sema/AnalysisBasedWarnings.h:93
- void IssueWarnings(Policy P, FunctionScopeInfo *fscope,
- const Decl *D, const BlockExpr *blkExpr);
+ void IssueWarnings(Policy P, FunctionScopeInfo *fscope, Decl *D,
+ const BlockExpr *blkExpr);
----------------
Quuxplusone wrote:
> 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.
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.
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1078
+ QualType T = FD->getReturnType();
+ if (T.isTriviallyCopyableType(Context)) {
+ // Avoid the optimization for functions that return trivially copyable
----------------
Quuxplusone wrote:
> Peanut-gallery question: Does `isTriviallyCopyableType` also check that the type is trivially destructible and/or default-constructible? Do those things matter?
Yes for trivially destructible, since it calls `CXXRecordDecl::isTriviallyCopyable()` which checks. No for trivially default-constructible, I fixed that.
I think that for us it doesn't really matter that much, since we're mostly worried about C code compiled in C++ mode.
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1173
+ } else if (CGM.getCodeGenOpts().StrictReturn ||
+ shouldUseUndefinedBehaviorReturnOptimization(FD, getContext())) {
+ if (CGM.getCodeGenOpts().OptimizationLevel == 0)
----------------
Quuxplusone wrote:
> 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.
> 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.
Repository:
rL LLVM
https://reviews.llvm.org/D27163
More information about the cfe-commits
mailing list