New warning -Wpessimizing-move to catch when removing calls to std::move prevent optimization

[rtrieu]

Summary

This warning will catch:

1. Using std::move in a return where the moved object is a local variable
2. Using std::move on a pr-value object

struct A {}
A test() {
  A a = std::move(A());  // warn since A() is a pr-value
  return std::move(a);  // warn since a is a local variable
}

[rsmith]

I don't think it's appropriate for this warning to look at whether we would have actually performed NRVO. Instead, I would suggest modeling this as follows: when we perform initialization, and the source expression is std::move(x), and x is of class type, and either x is a prvalue or {we're initializing a return object and x is the name of a local variable or parameter of the returned type ignoring cv-qualification}, emit a warning.

[rtrieu]

So then something like this:

struct S{};
S test(bool cond) {
  S s1, s2;
  if (cond)
    return std::move(s1);
  return std::move(s2);
}

would be better with the std::move's removed:

struct S{};
S test(bool cond) {
  S s1, s2;
  if (cond)
    return s1;
  return s2;
}

even though NRVO would not be triggered since different local variables are used?

[rtrieu]

Forgot to add the tests to the patch.

[rsmith]

lib/Sema/SemaInit.cpp

3245–3246 ↗

(On Diff #23981)

This check is not especially cheap, and the code below will be cheap in most cases; it's probably better to omit this.

3249 ↗

(On Diff #23981)

Maybe IgnoreParens before the cast?

3259 ↗

(On Diff #23981)

Maybe IgnoreParens here too.

3392–3393 ↗

(On Diff #23981)

You should defer the diagnostic until we perform the InitializationSequence. We might select a different initialization sequence or overload candidate and not actually call this constructor.

lib/Sema/SemaStmt.cpp

2900

This looks familiar. Can some commonality be factored out here?

Or, better, can you implement both checks in a single place? When performing the initialization sequence, you can ask the initialized entity whether it's the returned object in a return statement.

2931–2932

If the types don't match, you aren't likely to have a pessimizing move (elision would only occur if your return type's constructor took its argument by value), but perhaps we should still produce some kind of "redundant move" warning. (Per http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579, you get an implicit move no matter what the types are.)

[rtrieu]

Consolidate the checking to SemaInit.cpp
Ass warning for redundant move on return

[rtrieu]

lib/Sema/SemaInit.cpp

3245–3246 ↗

(On Diff #23981)

isIgnored check removed.

3249 ↗

(On Diff #23981)

More parens removed.

3259 ↗

(On Diff #23981)

More parens removed. Plus test cases for parens.

3392–3393 ↗

(On Diff #23981)

Moved check to after InitializationSequence in PerformCopyInitiailization.

lib/Sema/SemaStmt.cpp

2900

Moved to a single checking function in SemaInit

2931–2932

Added new warning for redundant moves on return.

[rsmith]

lib/Sema/SemaInit.cpp

7346 ↗

(On Diff #24262)

The reference type check here is redundant: a record type is never a reference type.

7352–7353 ↗

(On Diff #24262)

This check seems to prevent your redundant_move_on_return warning from firing at the right times.

7377–7378 ↗

(On Diff #24262)

You should also check that VD is from the current function/lambda/whatever and not an enclosing one (using DRE->refersToEnclosingVariableOrCapture).

7420–7423 ↗

(On Diff #24262)

Can you put this inside Perform so that all the users of InitializationSequence are guaranteed to reach it? There's lots of places that perform copy elision but don't use this convenience wrapper.

test/SemaCXX/warn-pessmizing-move.cpp

38–43 ↗

(On Diff #24262)

We should give the "redundant move" warning for this case, but we won't because we didn't use a copy or move constructor.

[rtrieu]

Moved the check into InitiailzationSequence::Perform
Add macro handling for the fix-it hints for common cases
Capture additional cases for the redundant move

[rtrieu]

lib/Sema/SemaInit.cpp

7346 ↗

(On Diff #24262)

Removed reference check.

7352–7353 ↗

(On Diff #24262)

Peeled back more nodes in some cases of redundant move, including MaterializeTemporaryExpr's and additional CXXConstructorExpr's.

7377–7378 ↗

(On Diff #24262)

Check added.

7420–7423 ↗

(On Diff #24262)

Moved check to InitializationSequence::Perform.

test/SemaCXX/warn-pessmizing-move.cpp

38–43 ↗

(On Diff #24262)

Added this test case to the redundant move test.

[rsmith]

LGTM with a couple of minor changes.

lib/Sema/SemaInit.cpp

5768–5769 ↗

(On Diff #24587)

If CCE is a CXXTemporaryObjectExpr (which derives from CXXConstructExpr), you shouldn't step through it here -- CXXTemporaryObjectExpr is always modeling explicit syntax.

5770 ↗

(On Diff #24587)

It would be marginally better to check

CCE->getNumArgs() == 0 ||
(CCE->getNumArgs() > 1 && !isa<CXXDefaultArgExpr>(CCE->getArg(1)))

in case the constructor has a default argument.

[rtrieu]

Submitted with changes.

include/clang/AST/Stmt.h

 /// return a value, and it allows returning a value in functions declared to
 /// return void.  We explicitly model this in the AST, which means you can't
 /// depend on the return type of the function and the presence of an argument.
 ///
 class ReturnStmt : public Stmt {
   Stmt *RetExpr;
   SourceLocation RetLoc;
   const VarDecl *NRVOCandidate;
+  bool PessimizingMoveCandidate;
 
 public:
   ReturnStmt(SourceLocation RL)
     : Stmt(ReturnStmtClass), RetExpr(nullptr), RetLoc(RL),
-      NRVOCandidate(nullptr) {}
+      NRVOCandidate(nullptr), PessimizingMoveCandidate(false) {}
 
   ReturnStmt(SourceLocation RL, Expr *E, const VarDecl *NRVOCandidate)
     : Stmt(ReturnStmtClass), RetExpr((Stmt*) E), RetLoc(RL),
-      NRVOCandidate(NRVOCandidate) {}
+      NRVOCandidate(NRVOCandidate), PessimizingMoveCandidate(false) {}
 
   /// \brief Build an empty return expression.
   explicit ReturnStmt(EmptyShell Empty) : Stmt(ReturnStmtClass, Empty) { }
 
   const Expr *getRetValue() const;
   Expr *getRetValue();
   void setRetValue(Expr *E) { RetExpr = reinterpret_cast<Stmt*>(E); }
 
   SourceLocation getReturnLoc() const { return RetLoc; }
   void setReturnLoc(SourceLocation L) { RetLoc = L; }
 
   /// \brief Retrieve the variable that might be used for the named return
   /// value optimization.
   ///
   /// The optimization itself can only be performed if the variable is
   /// also marked as an NRVO object.
   const VarDecl *getNRVOCandidate() const { return NRVOCandidate; }
   void setNRVOCandidate(const VarDecl *Var) { NRVOCandidate = Var; }
 
+  bool isPessimizingMoveCandidate() { return PessimizingMoveCandidate; }
+  void setPessimizingMoveCandidate() { PessimizingMoveCandidate = true; }
+
   SourceLocation getLocStart() const LLVM_READONLY { return RetLoc; }
   SourceLocation getLocEnd() const LLVM_READONLY {
     return RetExpr ? RetExpr->getLocEnd() : RetLoc;
   }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == ReturnStmtClass;
   }

include/clang/Basic/DiagnosticGroups.td

 def ObjCRootClass : DiagGroup<"objc-root-class">;
 def ObjCPointerIntrospectPerformSelector : DiagGroup<"deprecated-objc-pointer-introspection-performSelector">;
 def ObjCPointerIntrospect : DiagGroup<"deprecated-objc-pointer-introspection", [ObjCPointerIntrospectPerformSelector]>;
 def ObjCMultipleMethodNames : DiagGroup<"objc-multiple-method-names">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def Packed : DiagGroup<"packed">;
 def Padded : DiagGroup<"padded">;
+def PessimizingMove : DiagGroup<"pessimizing-move">;
 def PointerArith : DiagGroup<"pointer-arith">;
 def PoundWarning : DiagGroup<"#warnings">;
 def PoundPragmaMessage : DiagGroup<"#pragma-messages">,
                          DiagCategory<"#pragma message Directive">;
 def : DiagGroup<"pointer-to-int-cast">;
 def : DiagGroup<"redundant-decls">;
 def RedeclaredClassMember : DiagGroup<"redeclared-class-member">;
 def GNURedeclaredEnum : DiagGroup<"gnu-redeclared-enum">;

include/clang/Basic/DiagnosticSemaKinds.td

 
 def warn_self_assignment : Warning<
   "explicitly assigning value of variable of type %0 to itself">,
   InGroup<SelfAssignment>, DefaultIgnore;
 def warn_self_move : Warning<
   "explicitly moving variable of type %0 to itself">,
   InGroup<SelfMove>, DefaultIgnore;
 
+def warn_pessimizing_move : Warning<
+  "return value optimization for function %0 prevented by call to "
+  "std::move on return statement">,
+  InGroup<PessimizingMove>, DefaultIgnore;
+def note_remove_move : Note<
+  "remove call to std::move here to allow return value optimization">;
+
 def warn_string_plus_int : Warning<
   "adding %0 to a string does not append to the string">,
   InGroup<StringPlusInt>;
 def warn_string_plus_char : Warning<
   "adding %0 to a string pointer does not append to the string">,
   InGroup<StringPlusChar>;
 def note_string_plus_scalar_silence : Note<
   "use array indexing to silence this warning">;

include/clang/Sema/ScopeInfo.h

public:
   /// True when this is an initializer method not marked as a designated
   /// initializer within a class that has at least one initializer marked as a
   /// designated initializer.
   bool ObjCIsSecondaryInit;
   /// This starts true for a secondary initializer method and will be set to
   /// false if there is an invocation of an initializer on 'self'.
   bool ObjCWarnForNoInitDelegation;
 
+  /// True when function may contain pessimizing moves in return statements.
+  bool HasPossiblePessimizingMove;
+
   /// First C++ 'try' statement in the current function.
   SourceLocation FirstCXXTryLoc;
 
   /// First SEH '__try' statement in the current function.
   SourceLocation FirstSEHTryLoc;
 
   /// \brief Used to determine if errors occurred in this function or block.
   DiagnosticErrorTrap ErrorTrap;
public:
   /// \brief The stack of currently active compound stamement scopes in the
   /// function.
   SmallVector<CompoundScopeInfo, 4> CompoundScopes;
 
   /// \brief A list of PartialDiagnostics created but delayed within the
   /// current function scope.  These diagnostics are vetted for reachability
   /// prior to being emitted.
   SmallVector<PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags;
-  
   /// \brief A list of parameters which have the nonnull attribute and are
   /// modified in the function.
   llvm::SmallPtrSet<const ParmVarDecl*, 8>  ModifiedNonNullParams;
 
 public:
   /// Represents a simple identification of a weak object.
   ///
   /// Part of the implementation of -Wrepeated-use-of-weak.
void setHasSEHTry(SourceLocation TryLoc) {
     FirstSEHTryLoc = TryLoc;
   }
 
   bool NeedsScopeChecking() const {
     return !HasDroppedStmt &&
         (HasIndirectGoto ||
           (HasBranchProtectedScope && HasBranchIntoScope));
   }
-  
   FunctionScopeInfo(DiagnosticsEngine &Diag)
     : Kind(SK_Function),
       HasBranchProtectedScope(false),
       HasBranchIntoScope(false),
       HasIndirectGoto(false),
       HasDroppedStmt(false),
       ObjCShouldCallSuper(false),
       ObjCIsDesignatedInit(false),
       ObjCWarnForNoDesignatedInitChain(false),
       ObjCIsSecondaryInit(false),
       ObjCWarnForNoInitDelegation(false),
+      HasPossiblePessimizingMove(true),
       ErrorTrap(Diag) { }
 
   virtual ~FunctionScopeInfo();
 
   /// \brief Clear out the information in this function scope, making it
   /// suitable for reuse.
   void Clear();
 };

include/clang/Sema/Sema.h

public:
   /// definition, assuming we don't care about analyzing its body or emitting
   /// code for that function.
   ///
   /// This will be \c false only if we may need the body of the function in
   /// order to parse the rest of the program (for instance, if it is
   /// \c constexpr in C++11 or has an 'auto' return type in C++14).
   bool canSkipFunctionBody(Decl *D);
 
-  void computeNRVO(Stmt *Body, sema::FunctionScopeInfo *Scope);
+  void computeNRVO(Stmt *Body, sema::FunctionScopeInfo *Scope,
+                   FunctionDecl *FD = nullptr);
   Decl *ActOnFinishFunctionBody(Decl *Decl, Stmt *Body);
   Decl *ActOnFinishFunctionBody(Decl *Decl, Stmt *Body, bool IsInstantiation);
   Decl *ActOnSkippedFunctionBody(Decl *Decl);
   void ActOnFinishInlineMethodDef(CXXMethodDecl *D);
 
   /// ActOnFinishDelayedAttribute - Invoked when we have finished parsing an
   /// attribute for which parsing is delayed.
   void ActOnFinishDelayedAttribute(Scope *S, Decl *D, ParsedAttributes &Attrs);

lib/Sema/ScopeInfo.cpp

void FunctionScopeInfo::Clear() {
   HasBranchIntoScope = false;
   HasIndirectGoto = false;
   HasDroppedStmt = false;
   ObjCShouldCallSuper = false;
   ObjCIsDesignatedInit = false;
   ObjCWarnForNoDesignatedInitChain = false;
   ObjCIsSecondaryInit = false;
   ObjCWarnForNoInitDelegation = false;
+  HasPossiblePessimizingMove = true;
   FirstCXXTryLoc = SourceLocation();
   FirstSEHTryLoc = SourceLocation();
 
   SwitchStack.clear();
   Returns.clear();
   ErrorTrap.reset();
   PossiblyUnreachableDiags.clear();
   WeakObjectUses.clear();

lib/Sema/SemaDecl.cpp

if (getCurLexicalContext()->isObjCContainer() &&
       getCurLexicalContext()->getDeclKind() != Decl::ObjCCategoryImpl &&
       getCurLexicalContext()->getDeclKind() != Decl::ObjCImplementation)
     Diag(FD->getLocation(), diag::warn_function_def_in_objc_container);
     
   return D;
 }
 
 /// \brief Given the set of return statements within a function body,
-/// compute the variables that are subject to the named return value 
+/// compute the variables that are subject to the named return value
 /// optimization.
 ///
-/// Each of the variables that is subject to the named return value 
+/// Each of the variables that is subject to the named return value
 /// optimization will be marked as NRVO variables in the AST, and any
 /// return statement that has a marked NRVO variable as its NRVO candidate can
 /// use the named return value optimization.
 ///
 /// This function applies a very simplistic algorithm for NRVO: if every return
 /// statement in the scope of a variable has the same NRVO candidate, that
 /// candidate is an NRVO variable.
-void Sema::computeNRVO(Stmt *Body, FunctionScopeInfo *Scope) {
+void Sema::computeNRVO(Stmt *Body, FunctionScopeInfo *Scope, FunctionDecl *FD) {
   ReturnStmt **Returns = Scope->Returns.data();
 
+  bool FoundPessimizingMove = false;
+  const VarDecl *VD = nullptr;
+  bool HasVD = false;
   for (unsigned I = 0, E = Scope->Returns.size(); I != E; ++I) {
     if (const VarDecl *NRVOCandidate = Returns[I]->getNRVOCandidate()) {
-      if (!NRVOCandidate->isNRVOVariable())
+      if (!HasVD) {
+        VD = NRVOCandidate;
+        HasVD = true;
+      }
+      if (VD != NRVOCandidate)
+        VD = nullptr;
+      if (!NRVOCandidate->isNRVOVariable()) {
         Returns[I]->setNRVOCandidate(nullptr);
+        if (Returns[I]->isPessimizingMoveCandidate()) {
+          FoundPessimizingMove = true;
+        }
+      }
+    }
+  }
+
+  // Only diagnose in functions
+  if (!FD) return;
+  // No pessimizing moves found in return statements
+  if (!FoundPessimizingMove) return;
+  // No single VarDecl across all return statements
+  if (!VD) return;
+  // Check the scope status of pessimizing move
+  if (!Scope->HasPossiblePessimizingMove) return;
+  // Don't warn in template instantiations
+  if (!ActiveTemplateInstantiations.empty()) return;
+
+  Diag(FD->getLocation(), diag::warn_pessimizing_move) << FD;
+
+  for (unsigned I = 0, E = Scope->Returns.size(); I != E; ++I) {
+    if (Returns[I]->isPessimizingMoveCandidate()) {
+      Diag(Returns[I]->getLocStart(), diag::note_remove_move)
+          << FixItHint::CreateReplacement(
+              Returns[I]->getRetValue()->getSourceRange(), VD->getName());
     }
   }
 }
 
 bool Sema::canDelayFunctionBody(const Declarator &D) {
   // We can't delay parsing the body of a constexpr function template (yet).
   if (D.getDeclSpec().isConstexprSpecified())
     return false;
if (!FD->isInvalidDecl()) {
       DiagnoseSizeOfParametersAndReturnValue(FD->param_begin(), FD->param_end(),
                                              FD->getReturnType(), FD);
 
       // If this is a structor, we need a vtable.
       if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(FD))
         MarkVTableUsed(FD->getLocation(), Constructor->getParent());
       else if (CXXDestructorDecl *Destructor = dyn_cast<CXXDestructorDecl>(FD))
         MarkVTableUsed(FD->getLocation(), Destructor->getParent());
-      
       // Try to apply the named return value optimization. We have to check
       // if we can do this here because lambdas keep return statements around
       // to deduce an implicit return type.
       if (getLangOpts().CPlusPlus && FD->getReturnType()->isRecordType() &&
           !FD->isDependentContext())
-        computeNRVO(Body, getCurFunction());
+        computeNRVO(Body, getCurFunction(), FD);
     }
-    
     assert((FD == getCurFunctionDecl() || getCurLambda()->CallOperator == FD) &&
            "Function parsing confused");
   } else if (ObjCMethodDecl *MD = dyn_cast_or_null<ObjCMethodDecl>(dcl)) {
     assert(MD == getCurMethodDecl() && "Method parsing confused");
     MD->setBody(Body);
     if (!MD->isInvalidDecl()) {
       DiagnoseUnusedParameters(MD->param_begin(), MD->param_end());
       DiagnoseSizeOfParametersAndReturnValue(MD->param_begin(), MD->param_end(),

lib/Sema/SemaStmt.cpp

bool Sema::DeduceFunctionTypeFromReturnExpr(FunctionDecl *FD,
   } else if (!FD->isInvalidDecl()) {
     // Update all declarations of the function to have the deduced return type.
     Context.adjustDeducedFunctionResultType(FD, Deduced);
   }
 
   return false;
 }
 
 StmtResult
 Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
                       Scope *CurScope) {
   StmtResult R = BuildReturnStmt(ReturnLoc, RetValExp);
   if (R.isInvalid()) {
     return R;
   }
 
-  if (VarDecl *VD =
-      const_cast<VarDecl*>(cast<ReturnStmt>(R.get())->getNRVOCandidate())) {
+  ReturnStmt *RetStmt = cast<ReturnStmt>(R.get());
+  VarDecl *VD = const_cast<VarDecl *>(RetStmt->getNRVOCandidate());
+  // Only set NRVO on true NRVO candidates.  Ignore pessimizing move candidates.
+  if (VD && !RetStmt->isPessimizingMoveCandidate()) {
     CurScope->addNRVOCandidate(VD);
   } else {
     CurScope->setNoNRVO();
   }
 
   return R;
 }
 
+// If the RetExpr is a function call in the form of std::move(x), return
+// the VarDecl for x if "return x;" could possibly be an NRVO.
+static VarDecl *GetPessimizingMoveDecl(Sema &SemaRef, QualType RetType,
+                                       Expr *RetExpr) {
+  if (!RetExpr || RetType->isReferenceType()) return nullptr;
+
+  CallExpr *CE = dyn_cast<CallExpr>(RetExpr);
+  if (!CE || CE->getNumArgs() != 1) return nullptr;
+
+  FunctionDecl *FD = CE->getDirectCallee();
+  if (!FD || !FD->isInStdNamespace() || !FD->getIdentifier() ||
+      !FD->getIdentifier()->isStr("move"))
+    return nullptr;
+
+  DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(CE->getArg(0));
+  if (!DRE || !DRE->getDecl()) return nullptr;
+
+  VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl());
+  if (!VD) return nullptr;
+
+  if (!SemaRef.isCopyElisionCandidate(RetType, VD, false)) return nullptr;
+
+  if (!SemaRef.Context.hasSameType(RetType, VD->getType())) return nullptr;
+
+  return VD;
+}
+
 StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
   // Check for unexpanded parameter packs.
   if (RetValExp && DiagnoseUnexpandedParameterPack(RetValExp))
     return StmtError();
 
   if (isa<CapturingScopeInfo>(getCurFunction()))
     return ActOnCapScopeReturnStmt(ReturnLoc, RetValExp);
 
   QualType FnRetType;
   QualType RelatedRetType;
   const AttrVec *Attrs = nullptr;
   bool isObjCMethod = false;
 
+  // Hold the VarDecl being moved in the return statement.
+  VarDecl* MovedDecl = nullptr;
+
   if (const FunctionDecl *FD = getCurFunctionDecl()) {
     FnRetType = FD->getReturnType();
     if (FD->hasAttrs())
       Attrs = &FD->getAttrs();
     if (FD->isNoReturn())
       Diag(ReturnLoc, diag::warn_noreturn_function_has_return_expr)
         << FD->getDeclName();
+    MovedDecl = GetPessimizingMoveDecl(*this, FnRetType, RetValExp);
   } else if (ObjCMethodDecl *MD = getCurMethodDecl()) {
     FnRetType = MD->getReturnType();
     isObjCMethod = true;
     if (MD->hasAttrs())
       Attrs = &MD->getAttrs();
     if (MD->hasRelatedResultType() && MD->getClassInterface()) {
       // In the implementation of a method with a related return type, the
       // type used to type-check the validity of return statements within the
if (RetValExp) {
       ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc);
       if (ER.isInvalid())
         return StmtError();
       RetValExp = ER.get();
     }
     Result = new (Context) ReturnStmt(ReturnLoc, RetValExp, NRVOCandidate);
   }
 
+  if (Result->getNRVOCandidate()) {
     // If we need to check for the named return value optimization, save the
     // return statement in our scope for later processing.
-  if (Result->getNRVOCandidate())
     FunctionScopes.back()->Returns.push_back(Result);
+  } else if (MovedDecl) {
+    // If the return expression is "return std::move(x);" and "return x;" could
+    // be a NRVO, mark it as a pessimizing move for later diagnostics.
+    Result->setNRVOCandidate(MovedDecl);
+    Result->setPessimizingMoveCandidate();
+    FunctionScopes.back()->Returns.push_back(Result);
+  } else {
+    // Return statement is neither an NRVO candidate or a pessimizing move.
+    FunctionScopes.back()->HasPossiblePessimizingMove = false;
+  }
 
   return Result;
 }
 
 StmtResult
 Sema::ActOnObjCAtCatchStmt(SourceLocation AtLoc,
                            SourceLocation RParen, Decl *Parm,
                            Stmt *Body) {