[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]

arphaman added inline comments.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1078
+  QualType T = FD->getReturnType();
+  if (T.isTriviallyCopyableType(Context)) {
+    // Avoid the optimization for functions that return trivially copyable
----------------
rjmccall wrote:
> arphaman wrote:
> > 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.
> 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.
I see what you mean, this kind of reasoning makes sense. I'll use just the `hasTrivialDestructor` check when committing.


Repository:
  rL LLVM

https://reviews.llvm.org/D27163