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

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

Quuxplusone added a comment.

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.



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


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1173
+    } else if (CGM.getCodeGenOpts().StrictReturn ||
+               shouldUseUndefinedBehaviorReturnOptimization(FD, getContext())) {
+      if (CGM.getCodeGenOpts().OptimizationLevel == 0)
----------------
arphaman wrote:
> 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.
> 
> since then Sema won't warn about the missing return

Ah. Yeah, that makes sense.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1071
+/// Return true if the given function \p FD should use the undefined behavior
+/// return optimization.
+static bool
----------------
Nit: this code comment is not particularly useful to the reader.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163