From: Martin Bohme Date: Wed, 14 Sep 2016 10:29:32 +0000 (+0000) Subject: [clang-tidy] Add check 'misc-use-after-move' X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=42d3839bc5be31b84dc7cee4a47c9fb7f2ad1bde;p=platform%2Fupstream%2Fllvm.git [clang-tidy] Add check 'misc-use-after-move' Summary: The check warns if an object is used after it has been moved, without an intervening reinitialization. See user-facing documentation for details. Reviewers: sbenza, Prazek, alexfh Subscribers: beanz, mgorny, shadeware, omtcyfz, Eugene.Zelenko, Prazek, fowles, ioeric, cfe-commits Differential Revision: https://reviews.llvm.org/D23353 llvm-svn: 281453 --- diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 7b15c32..0a38b15 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -43,6 +43,7 @@ add_clang_library(clangTidyMiscModule UnusedParametersCheck.cpp UnusedRAIICheck.cpp UnusedUsingDeclsCheck.cpp + UseAfterMoveCheck.cpp VirtualNearMissCheck.cpp LINK_LIBS diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index ac6e47e..b952947 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -51,6 +51,7 @@ #include "UnusedParametersCheck.h" #include "UnusedRAIICheck.h" #include "UnusedUsingDeclsCheck.h" +#include "UseAfterMoveCheck.h" #include "VirtualNearMissCheck.h" namespace clang { @@ -139,6 +140,7 @@ public: CheckFactories.registerCheck("misc-unused-raii"); CheckFactories.registerCheck( "misc-unused-using-decls"); + CheckFactories.registerCheck("misc-use-after-move"); CheckFactories.registerCheck( "misc-virtual-near-miss"); } diff --git a/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.cpp new file mode 100644 index 0000000..f1485c8 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.cpp @@ -0,0 +1,643 @@ +//===--- UseAfterMoveCheck.cpp - clang-tidy -------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "UseAfterMoveCheck.h" + +#include "clang/Analysis/CFG.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/SmallPtrSet.h" +#include "llvm/ADT/SmallVector.h" + +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +namespace { + +/// Provides information about the evaluation order of (sub-)expressions within +/// a `CFGBlock`. +/// +/// While a `CFGBlock` does contain individual `CFGElement`s for some +/// sub-expressions, the order in which those `CFGElement`s appear reflects +/// only one possible order in which the sub-expressions may be evaluated. +/// However, we want to warn if any of the potential evaluation orders can lead +/// to a use-after-move, not just the one contained in the `CFGBlock`. +/// +/// This class implements only a simplified version of the C++ sequencing rules +/// that is, however, sufficient for the purposes of this check. The main +/// limitation is that we do not distinguish between value computation and side +/// effect -- see the "Implementation" section for more details. +/// +/// Note: `SequenceChecker` from SemaChecking.cpp does a similar job (and much +/// more thoroughly), but using it would require +/// - Pulling `SequenceChecker` out into a header file (i.e. making it part of +/// the API), +/// - Removing the dependency of `SequenceChecker` on `Sema`, and +/// - (Probably) modifying `SequenceChecker` to make it suitable to be used in +/// this context. +/// For the moment, it seems preferable to re-implement our own version of +/// sequence checking that is special-cased to what we need here. +/// +/// Implementation +/// -------------- +/// +/// `ExprSequence` uses two types of sequencing edges between nodes in the AST: +/// +/// - Every `Stmt` is assumed to be sequenced after its children. This is +/// overly optimistic because the standard only states that value computations +/// of operands are sequenced before the value computation of the operator, +/// making no guarantees about side effects (in general). +/// +/// For our purposes, this rule is sufficient, however, because this check is +/// interested in operations on objects, which are generally performed through +/// function calls (whether explicit and implicit). Function calls guarantee +/// that the value computations and side effects for all function arguments +/// are sequenced before the execution fo the function. +/// +/// - In addition, some `Stmt`s are known to be sequenced before or after +/// their siblings. For example, the `Stmt`s that make up a `CompoundStmt`are +/// all sequenced relative to each other. The function +/// `getSequenceSuccessor()` implements these sequencing rules. +class ExprSequence { +public: + /// Initializes this `ExprSequence` with sequence information for the given + /// `CFG`. + ExprSequence(const CFG *TheCFG, ASTContext *TheContext); + + /// Returns whether \p Before is sequenced before \p After. + bool inSequence(const Stmt *Before, const Stmt *After) const; + + /// Returns whether \p After can potentially be evaluated after \p Before. + /// This is exactly equivalent to `!inSequence(After, Before)` but makes some + /// conditions read more naturally. + bool potentiallyAfter(const Stmt *After, const Stmt *Before) const; + +private: + // Returns the sibling of \p S (if any) that is directly sequenced after \p S, + // or nullptr if no such sibling exists. For example, if \p S is the child of + // a `CompoundStmt`, this would return the Stmt that directly follows \p S in + // the `CompoundStmt`. + // + // As the sequencing of many constructs that change control flow is already + // encoded in the `CFG`, this function only implements the sequencing rules + // for those constructs where sequencing cannot be inferred from the `CFG`. + const Stmt *getSequenceSuccessor(const Stmt *S) const; + + const Stmt *resolveSyntheticStmt(const Stmt *S) const; + + ASTContext *Context; + + llvm::DenseMap SyntheticStmtSourceMap; +}; + +/// Maps `Stmt`s to the `CFGBlock` that contains them. Some `Stmt`s may be +/// contained in more than one `CFGBlock`; in this case, they are mapped to the +/// innermost block (i.e. the one that is furthest from the root of the tree). +class StmtToBlockMap { +public: + /// Initializes the map for the given `CFG`. + StmtToBlockMap(const CFG *TheCFG, ASTContext *TheContext); + + /// Returns the block that \p S is contained in. Some `Stmt`s may be contained + /// in more than one `CFGBlock`; in this case, this function returns the + /// innermost block (i.e. the one that is furthest from the root of the tree). + const CFGBlock *blockContainingStmt(const Stmt *S) const; + +private: + ASTContext *Context; + + llvm::DenseMap Map; +}; + +/// Contains information about a use-after-move. +struct UseAfterMove { + // The DeclRefExpr that constituted the use of the object. + const DeclRefExpr *DeclRef; + + // Is the order in which the move and the use are evaluated undefined? + bool EvaluationOrderUndefined; +}; + +/// Finds uses of a variable after a move (and maintains state required by the +/// various internal helper functions). +class UseAfterMoveFinder { +public: + UseAfterMoveFinder(ASTContext *TheContext); + + // Within the given function body, finds the first use of 'MovedVariable' that + // occurs after 'MovingCall' (the expression that performs the move). If a + // use-after-move is found, writes information about it to 'TheUseAfterMove'. + // Returns whether a use-after-move was found. + bool find(Stmt *FunctionBody, const Expr *MovingCall, + const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove); + +private: + bool findInternal(const CFGBlock *Block, const Expr *MovingCall, + const ValueDecl *MovedVariable, + UseAfterMove *TheUseAfterMove); + void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *MovedVariable, + llvm::SmallVectorImpl *Uses, + llvm::SmallPtrSetImpl *Reinits); + void getDeclRefs(const CFGBlock *Block, const Decl *MovedVariable, + llvm::SmallPtrSetImpl *DeclRefs); + void getReinits(const CFGBlock *Block, const ValueDecl *MovedVariable, + llvm::SmallPtrSetImpl *Stmts, + llvm::SmallPtrSetImpl *DeclRefs); + + ASTContext *Context; + std::unique_ptr Sequence; + std::unique_ptr BlockMap; + llvm::SmallPtrSet Visited; +}; + +} // namespace + +// Returns the Stmt nodes that are parents of 'S', skipping any potential +// intermediate non-Stmt nodes. +// +// In almost all cases, this function returns a single parent or no parents at +// all. +// +// The case that a Stmt has multiple parents is rare but does actually occur in +// the parts of the AST that we're interested in. Specifically, InitListExpr +// nodes cause ASTContext::getParent() to return multiple parents for certain +// nodes in their subtree because RecursiveASTVisitor visits both the syntactic +// and semantic forms of InitListExpr, and the parent-child relationships are +// different between the two forms. +static SmallVector getParentStmts(const Stmt *S, + ASTContext *Context) { + SmallVector Result; + + ASTContext::DynTypedNodeList Parents = Context->getParents(*S); + + SmallVector NodesToProcess(Parents.begin(), + Parents.end()); + + while (!NodesToProcess.empty()) { + ast_type_traits::DynTypedNode Node = NodesToProcess.back(); + NodesToProcess.pop_back(); + + if (const auto *S = Node.get()) { + Result.push_back(S); + } else { + Parents = Context->getParents(Node); + NodesToProcess.append(Parents.begin(), Parents.end()); + } + } + + return Result; +} + +bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor, + ASTContext *Context) { + if (Descendant == Ancestor) + return true; + for (const Stmt *Parent : getParentStmts(Descendant, Context)) { + if (isDescendantOrEqual(Parent, Ancestor, Context)) + return true; + } + + return false; +} + +ExprSequence::ExprSequence(const CFG *TheCFG, ASTContext *TheContext) + : Context(TheContext) { + for (const auto &SyntheticStmt : TheCFG->synthetic_stmts()) { + SyntheticStmtSourceMap[SyntheticStmt.first] = SyntheticStmt.second; + } +} + +bool ExprSequence::inSequence(const Stmt *Before, const Stmt *After) const { + Before = resolveSyntheticStmt(Before); + After = resolveSyntheticStmt(After); + + // If 'After' is in the subtree of the siblings that follow 'Before' in the + // chain of successors, we know that 'After' is sequenced after 'Before'. + for (const Stmt *Successor = getSequenceSuccessor(Before); Successor; + Successor = getSequenceSuccessor(Successor)) { + if (isDescendantOrEqual(After, Successor, Context)) + return true; + } + + // If 'After' is a parent of 'Before' or is sequenced after one of these + // parents, we know that it is sequenced after 'Before'. + for (const Stmt *Parent : getParentStmts(Before, Context)) { + if (Parent == After || inSequence(Parent, After)) + return true; + } + + return false; +} + +bool ExprSequence::potentiallyAfter(const Stmt *After, + const Stmt *Before) const { + return !inSequence(After, Before); +} + +const Stmt *ExprSequence::getSequenceSuccessor(const Stmt *S) const { + for (const Stmt *Parent : getParentStmts(S, Context)) { + if (const auto *BO = dyn_cast(Parent)) { + // Comma operator: Right-hand side is sequenced after the left-hand side. + if (BO->getLHS() == S && BO->getOpcode() == BO_Comma) + return BO->getRHS(); + } else if (const auto *InitList = dyn_cast(Parent)) { + // Initializer list: Each initializer clause is sequenced after the + // clauses that precede it. + for (unsigned I = 1; I < InitList->getNumInits(); ++I) { + if (InitList->getInit(I - 1) == S) + return InitList->getInit(I); + } + } else if (const auto *Compound = dyn_cast(Parent)) { + // Compound statement: Each sub-statement is sequenced after the + // statements that precede it. + const Stmt *Previous = nullptr; + for (const auto *Child : Compound->body()) { + if (Previous == S) + return Child; + Previous = Child; + } + } else if (const auto *TheDeclStmt = dyn_cast(Parent)) { + // Declaration: Every initializer expression is sequenced after the + // initializer expressions that precede it. + const Expr *PreviousInit = nullptr; + for (const Decl *TheDecl : TheDeclStmt->decls()) { + if (const auto *TheVarDecl = dyn_cast(TheDecl)) { + if (const Expr *Init = TheVarDecl->getInit()) { + if (PreviousInit == S) + return Init; + PreviousInit = Init; + } + } + } + } else if (const auto *ForRange = dyn_cast(Parent)) { + // Range-based for: Loop variable declaration is sequenced before the + // body. (We need this rule because these get placed in the same + // CFGBlock.) + if (S == ForRange->getLoopVarStmt()) + return ForRange->getBody(); + } else if (const auto *TheIfStmt = dyn_cast(Parent)) { + // If statement: If a variable is declared inside the condition, the + // expression used to initialize the variable is sequenced before the + // evaluation of the condition. + if (S == TheIfStmt->getConditionVariableDeclStmt()) + return TheIfStmt->getCond(); + } + } + + return nullptr; +} + +const Stmt *ExprSequence::resolveSyntheticStmt(const Stmt *S) const { + if (SyntheticStmtSourceMap.count(S)) + return SyntheticStmtSourceMap.lookup(S); + else + return S; +} + +StmtToBlockMap::StmtToBlockMap(const CFG *TheCFG, ASTContext *TheContext) + : Context(TheContext) { + for (const auto *B : *TheCFG) { + for (const auto &Elem : *B) { + if (Optional S = Elem.getAs()) + Map[S->getStmt()] = B; + } + } +} + +const CFGBlock *StmtToBlockMap::blockContainingStmt(const Stmt *S) const { + while (!Map.count(S)) { + SmallVector Parents = getParentStmts(S, Context); + if (Parents.empty()) + return nullptr; + S = Parents[0]; + } + + return Map.lookup(S); +} + +// Matches nodes that are +// - Part of a decltype argument or class template argument (we check this by +// seeing if they are children of a TypeLoc), or +// - Part of a function template argument (we check this by seeing if they are +// children of a DeclRefExpr that references a function template). +// DeclRefExprs that fulfill these conditions should not be counted as a use or +// move. +static StatementMatcher inDecltypeOrTemplateArg() { + return anyOf(hasAncestor(typeLoc()), + hasAncestor(declRefExpr( + to(functionDecl(ast_matchers::isTemplateInstantiation()))))); +} + +UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext) + : Context(TheContext) {} + +bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall, + const ValueDecl *MovedVariable, + UseAfterMove *TheUseAfterMove) { + // Generate the CFG manually instead of through an AnalysisDeclContext because + // it seems the latter can't be used to generate a CFG for the body of a + // labmda. + // + // We include implicit and temporary destructors in the CFG so that + // destructors marked [[noreturn]] are handled correctly in the control flow + // analysis. (These are used in some styles of assertion macros.) + CFG::BuildOptions Options; + Options.AddImplicitDtors = true; + Options.AddTemporaryDtors = true; + std::unique_ptr TheCFG = + CFG::buildCFG(nullptr, FunctionBody, Context, Options); + if (!TheCFG) + return false; + + Sequence.reset(new ExprSequence(TheCFG.get(), Context)); + BlockMap.reset(new StmtToBlockMap(TheCFG.get(), Context)); + Visited.clear(); + + const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall); + if (!Block) + return false; + + return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove); +} + +bool UseAfterMoveFinder::findInternal(const CFGBlock *Block, + const Expr *MovingCall, + const ValueDecl *MovedVariable, + UseAfterMove *TheUseAfterMove) { + if (Visited.count(Block)) + return false; + + // Mark the block as visited (except if this is the block containing the + // std::move() and it's being visited the first time). + if (!MovingCall) + Visited.insert(Block); + + // Get all uses and reinits in the block. + llvm::SmallVector Uses; + llvm::SmallPtrSet Reinits; + getUsesAndReinits(Block, MovedVariable, &Uses, &Reinits); + + // Ignore all reinitializations where the move potentially comes after the + // reinit. + llvm::SmallVector ReinitsToDelete; + for (const Stmt *Reinit : Reinits) { + if (MovingCall && Sequence->potentiallyAfter(MovingCall, Reinit)) + ReinitsToDelete.push_back(Reinit); + } + for (const Stmt *Reinit : ReinitsToDelete) { + Reinits.erase(Reinit); + } + + // Find all uses that potentially come after the move. + for (const DeclRefExpr *Use : Uses) { + if (!MovingCall || Sequence->potentiallyAfter(Use, MovingCall)) { + // Does the use have a saving reinit? A reinit is saving if it definitely + // comes before the use, i.e. if there's no potential that the reinit is + // after the use. + bool HaveSavingReinit = false; + for (const Stmt *Reinit : Reinits) { + if (!Sequence->potentiallyAfter(Reinit, Use)) + HaveSavingReinit = true; + } + + if (!HaveSavingReinit) { + TheUseAfterMove->DeclRef = Use; + + // Is this a use-after-move that depends on order of evaluation? + // This is the case if the move potentially comes after the use (and we + // already know that use potentially comes after the move, which taken + // together tells us that the ordering is unclear). + TheUseAfterMove->EvaluationOrderUndefined = + MovingCall != nullptr && + Sequence->potentiallyAfter(MovingCall, Use); + + return true; + } + } + } + + // If the object wasn't reinitialized, call ourselves recursively on all + // successors. + if (Reinits.empty()) { + for (const auto &Succ : Block->succs()) { + if (Succ && findInternal(Succ, nullptr, MovedVariable, TheUseAfterMove)) + return true; + } + } + + return false; +} + +void UseAfterMoveFinder::getUsesAndReinits( + const CFGBlock *Block, const ValueDecl *MovedVariable, + llvm::SmallVectorImpl *Uses, + llvm::SmallPtrSetImpl *Reinits) { + llvm::SmallPtrSet DeclRefs; + llvm::SmallPtrSet ReinitDeclRefs; + + getDeclRefs(Block, MovedVariable, &DeclRefs); + getReinits(Block, MovedVariable, Reinits, &ReinitDeclRefs); + + // All references to the variable that aren't reinitializations are uses. + Uses->clear(); + for (const DeclRefExpr *DeclRef : DeclRefs) { + if (!ReinitDeclRefs.count(DeclRef)) + Uses->push_back(DeclRef); + } + + // Sort the uses by their occurrence in the source code. + std::sort(Uses->begin(), Uses->end(), + [](const DeclRefExpr *D1, const DeclRefExpr *D2) { + return D1->getExprLoc() < D2->getExprLoc(); + }); +} + +void UseAfterMoveFinder::getDeclRefs( + const CFGBlock *Block, const Decl *MovedVariable, + llvm::SmallPtrSetImpl *DeclRefs) { + DeclRefs->clear(); + for (const auto &Elem : *Block) { + Optional S = Elem.getAs(); + if (!S) + continue; + + SmallVector Matches = + match(findAll(declRefExpr(hasDeclaration(equalsNode(MovedVariable)), + unless(inDecltypeOrTemplateArg())) + .bind("declref")), + *S->getStmt(), *Context); + + for (const auto &Match : Matches) { + const auto *DeclRef = Match.getNodeAs("declref"); + if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) + DeclRefs->insert(DeclRef); + } + } +} + +void UseAfterMoveFinder::getReinits( + const CFGBlock *Block, const ValueDecl *MovedVariable, + llvm::SmallPtrSetImpl *Stmts, + llvm::SmallPtrSetImpl *DeclRefs) { + auto DeclRefMatcher = + declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref"); + + auto StandardContainerTypeMatcher = hasType(cxxRecordDecl( + hasAnyName("::std::basic_string", "::std::vector", "::std::deque", + "::std::forward_list", "::std::list", "::std::set", + "::std::map", "::std::multiset", "::std::multimap", + "::std::unordered_set", "::std::unordered_map", + "::std::unordered_multiset", "::std::unordered_multimap"))); + + // Matches different types of reinitialization. + auto ReinitMatcher = + stmt(anyOf( + // Assignment. In addition to the overloaded assignment operator, + // test for built-in assignment as well, since template functions + // may be instantiated to use std::move() on built-in types. + binaryOperator(hasOperatorName("="), hasLHS(DeclRefMatcher)), + cxxOperatorCallExpr(hasOverloadedOperatorName("="), + hasArgument(0, DeclRefMatcher)), + // Declaration. We treat this as a type of reinitialization too, + // so we don't need to treat it separately. + declStmt(hasDescendant(equalsNode(MovedVariable))), + // clear() and assign() on standard containers. + cxxMemberCallExpr( + on(allOf(DeclRefMatcher, StandardContainerTypeMatcher)), + // To keep the matcher simple, we check for assign() calls + // on all standard containers, even though only vector, + // deque, forward_list and list have assign(). If assign() + // is called on any of the other containers, this will be + // flagged by a compile error anyway. + callee(cxxMethodDecl(hasAnyName("clear", "assign")))), + // Passing variable to a function as a non-const pointer. + callExpr(forEachArgumentWithParam( + unaryOperator(hasOperatorName("&"), + hasUnaryOperand(DeclRefMatcher)), + unless(parmVarDecl(hasType(pointsTo(isConstQualified())))))), + // Passing variable to a function as a non-const lvalue reference + // (unless that function is std::move()). + callExpr(forEachArgumentWithParam( + DeclRefMatcher, + unless(parmVarDecl(hasType( + references(qualType(isConstQualified())))))), + unless(callee(functionDecl(hasName("::std::move"))))))) + .bind("reinit"); + + Stmts->clear(); + DeclRefs->clear(); + for (const auto &Elem : *Block) { + Optional S = Elem.getAs(); + if (!S) + continue; + + SmallVector Matches = + match(findAll(ReinitMatcher), *S->getStmt(), *Context); + + for (const auto &Match : Matches) { + const auto *TheStmt = Match.getNodeAs("reinit"); + const auto *TheDeclRef = Match.getNodeAs("declref"); + if (TheStmt && BlockMap->blockContainingStmt(TheStmt) == Block) { + Stmts->insert(TheStmt); + + // We count DeclStmts as reinitializations, but they don't have a + // DeclRefExpr associated with them -- so we need to check 'TheDeclRef' + // before adding it to the set. + if (TheDeclRef) + DeclRefs->insert(TheDeclRef); + } + } + } +} + +static void emitDiagnostic(const Expr *MovingCall, + const ValueDecl *MovedVariable, + const UseAfterMove &Use, ClangTidyCheck *Check, + ASTContext *Context) { + Check->diag(Use.DeclRef->getExprLoc(), "'%0' used after it was moved") + << MovedVariable->getName(); + Check->diag(MovingCall->getExprLoc(), "move occurred here", + DiagnosticIDs::Note); + if (Use.EvaluationOrderUndefined) { + Check->diag(Use.DeclRef->getExprLoc(), + "the use and move are unsequenced, i.e. there is no guarantee " + "about the order in which they are evaluated", + DiagnosticIDs::Note); + } +} + +void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus11) + return; + + auto StandardSmartPointerTypeMatcher = hasType( + cxxRecordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr"))); + + auto CallMoveMatcher = + callExpr( + callee(functionDecl(hasName("::std::move"))), argumentCountIs(1), + hasArgument( + 0, + declRefExpr(unless(StandardSmartPointerTypeMatcher)).bind("arg")), + anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")), + hasAncestor(functionDecl().bind("containing-func"))), + unless(inDecltypeOrTemplateArg())) + .bind("call-move"); + + Finder->addMatcher( + // To find the Stmt that we assume performs the actual move, we look for + // the direct ancestor of the std::move() that isn't one of the node + // types ignored by ignoringParenImpCasts(). + stmt(forEach(expr(ignoringParenImpCasts(CallMoveMatcher))), + unless(expr(ignoringParenImpCasts(equalsBoundNode("call-move"))))) + .bind("moving-call"), + this); +} + +void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { + const auto *ContainingLambda = + Result.Nodes.getNodeAs("containing-lambda"); + const auto *ContainingFunc = + Result.Nodes.getNodeAs("containing-func"); + const auto *CallMove = Result.Nodes.getNodeAs("call-move"); + const auto *MovingCall = Result.Nodes.getNodeAs("moving-call"); + const auto *Arg = Result.Nodes.getNodeAs("arg"); + + if (!MovingCall) + MovingCall = CallMove; + + Stmt *FunctionBody = nullptr; + if (ContainingLambda) + FunctionBody = ContainingLambda->getBody(); + else if (ContainingFunc) + FunctionBody = ContainingFunc->getBody(); + else + return; + + const ValueDecl *MovedVariable = Arg->getDecl(); + + // Ignore the std::move if the variable that was passed to it isn't a local + // variable. + if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod()) + return; + + UseAfterMoveFinder finder(Result.Context); + UseAfterMove Use; + if (finder.find(FunctionBody, MovingCall, MovedVariable, &Use)) + emitDiagnostic(MovingCall, MovedVariable, Use, this, Result.Context); +} + +} // namespace misc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.h b/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.h new file mode 100644 index 0000000..2f6be5b --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.h @@ -0,0 +1,36 @@ +//===--- UseAfterMoveCheck.h - clang-tidy ---------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEAFTERMOVECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEAFTERMOVECHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// The check warns if an object is used after it has been moved, without an +/// intervening reinitialization. +/// +/// For details, see the user-facing documentation: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html +class UseAfterMoveCheck : public ClangTidyCheck { +public: + UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEAFTERMOVECHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 8a7ba63..201741b 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -73,6 +73,12 @@ Improvements to clang-tidy Warns when ``std::move`` is applied to a forwarding reference instead of ``std::forward``. +- New `misc-use-after-move + `_ check + + Warns if an object is used after it has been moved, without an intervening + reinitialization. + - New `mpi-buffer-deref `_ check diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2492283..fdf24f40 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -93,6 +93,7 @@ Clang-Tidy Checks misc-unused-parameters misc-unused-raii misc-unused-using-decls + misc-use-after-move misc-virtual-near-miss modernize-avoid-bind modernize-deprecated-headers diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-use-after-move.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-use-after-move.rst new file mode 100644 index 0000000..17f16b5 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-use-after-move.rst @@ -0,0 +1,197 @@ +.. title:: clang-tidy - misc-use-after-move + +misc-use-after-move +=================== + +Warns if an object is used after it has been moved, for example: + + .. code-block:: c++ + + std::string str = "Hello, world!\n"; + std::vector messages; + messages.emplace_back(std::move(str)); + std::cout << str; + +The last line will trigger a warning that ``str`` is used after it has been +moved. + +The check does not trigger a warning if the object is reinitialized after the +move and before the use. For example, no warning will be output for this code: + + .. code-block:: c++ + + messages.emplace_back(std::move(str)); + str = "Greetings, stranger!\n"; + std::cout << str; + +The check takes control flow into account. A warning is only emitted if the use +can be reached from the move. This means that the following code does not +produce a warning: + + .. code-block:: c++ + + if (condition) { + messages.emplace_back(std::move(str)); + } else { + std::cout << str; + } + +On the other hand, the following code does produce a warning: + + .. code-block:: c++ + + for (int i = 0; i < 10; ++i) { + std::cout << str; + messages.emplace_back(std::move(str)); + } + +(The use-after-move happens on the second iteration of the loop.) + +In some cases, the check may not be able to detect that two branches are +mutually exclusive. For example (assuming that ``i`` is an int): + + .. code-block:: c++ + + if (i == 1) { + messages.emplace_back(std::move(str)); + } + if (i == 2) { + std::cout << str; + } + +In this case, the check will erroneously produce a warning, even though it is +not possible for both the move and the use to be executed. + +An erroneous warning can be silenced by reinitializing the object after the +move: + + .. code-block:: c++ + + if (i == 1) { + messages.emplace_back(std::move(str)); + str = ""; + } + if (i == 2) { + std::cout << str; + } + +No warnings are emitted for objects of type ``std::unique_ptr`` and +``std::shared_ptr``, as they have defined move behavior. (Objects of these +classes are guaranteed to be empty after they have been moved from.) + +Subsections below explain more precisely what exactly the check considers to be +a move, use, and reinitialization. + +Unsequenced moves, uses, and reinitializations +---------------------------------------------- + +In many cases, C++ does not make any guarantees about the order in which +sub-expressions of a statement are evaluated. This means that in code like the +following, it is not guaranteed whether the use will happen before or after the +move: + + .. code-block:: c++ + + void f(int i, std::vector v); + std::vector v = { 1, 2, 3 }; + f(v[1], std::move(v)); + +In this kind of situation, the check will note that the use and move are +unsequenced. + +The check will also take sequencing rules into account when reinitializations +occur in the same statement as moves or uses. A reinitialization is only +considered to reinitialize a variable if it is guaranteed to be evaluated after +the move and before the use. + +Move +---- + +The check currently only considers calls of ``std::move`` on local variables or +function parameters. It does not check moves of member variables or global +variables. + +Any call of ``std::move`` on a variable is considered to cause a move of that +variable, even if the result of ``std::move`` is not passed to an rvalue +reference parameter. + +This means that the check will flag a use-after-move even on a type that does +not define a move constructor or move assignment operator. This is intentional. +Developers may use ``std::move`` on such a type in the expectation that the type +will add move semantics in the future. If such a ``std::move`` has the potential +to cause a use-after-move, we want to warn about it even if the type does not +implement move semantics yet. + +Furthermore, if the result of ``std::move`` *is* passed to an rvalue reference +parameter, this will always be considered to cause a move, even if the function +that consumes this parameter does not move from it, or if it does so only +conditionally. For example, in the following situation, the check will assume +that a move always takes place: + + .. code-block:: c++ + + std::vector messages; + void f(std::string &&str) { + // Only remember the message if it isn't empty. + if (!str.empty()) { + messages.emplace_back(std::move(str)); + } + } + std::string str = ""; + f(std::move(str)); + +The check will assume that the last line causes a move, even though, in this +particular case, it does not. Again, this is intentional. + +When analyzing the order in which moves, uses and reinitializations happen (see +section `Unsequenced moves, uses, and reinitializations`_), the move is assumed +to occur in whichever function the result of the ``std::move`` is passed to. + +Use +--- + +Any occurrence of the moved variable that is not a reinitialization (see below) +is considered to be a use. + +If multiple uses occur after a move, only the first of these is flagged. + +Reinitialization +---------------- + +The check considers a variable to be reinitialized in the following cases: + + - The variable occurs on the left-hand side of an assignment. + + - The variable is passed to a function as a non-const pointer or non-const + lvalue reference. (It is assumed that the variable may be an out-parameter + for the function.) + + - ``clear()`` or ``assign()`` is called on the variable and the variable is of + one of the standard container types ``basic_string``, ``vector``, ``deque``, + ``forward_list``, ``list``, ``set``, ``map``, ``multiset``, ``multimap``, + ``unordered_set``, ``unordered_map``, ``unordered_multiset``, + ``unordered_multimap``. + +If the variable in question is a struct and an individual member variable of +that struct is written to, the check does not consider this to be a +reinitialization -- even if, eventually, all member variables of the struct are +written to. For example: + + .. code-block:: c++ + + struct S { + std::string str; + int i; + }; + S s = { "Hello, world!\n", 42 }; + S s_other = std::move(s); + s.str = "Lorem ipsum"; + s.i = 99; + +The check will not consider ``s`` to be reinitialized after the last line; +instead, the line that assigns to ``s.str`` will be flagged as a use-after-move. +This is intentional as this pattern of reinitializing a struct is error-prone. +For example, if an additional member variable is added to ``S``, it is easy to +forget to add the reinitialization for this additional member. Instead, it is +safer to assign to the entire struct in one go, and this will also avoid the +use-after-move warning. diff --git a/clang-tools-extra/test/clang-tidy/misc-use-after-move.cpp b/clang-tools-extra/test/clang-tidy/misc-use-after-move.cpp new file mode 100644 index 0000000..ca3e955 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-use-after-move.cpp @@ -0,0 +1,1039 @@ +// RUN: %check_clang_tidy %s misc-use-after-move %t + +typedef decltype(nullptr) nullptr_t; + +namespace std { +typedef unsigned size_t; + +template +struct unique_ptr { + unique_ptr(); + T *get() const; +}; + +template +struct shared_ptr { + shared_ptr(); + T *get() const; +}; + +#define DECLARE_STANDARD_CONTAINER(name) \ + template \ + struct name { \ + name(); \ + void clear(); \ + bool empty(); \ + } + +#define DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(name) \ + template \ + struct name { \ + name(); \ + void clear(); \ + bool empty(); \ + void assign(size_t, const T &); \ + } + +DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(basic_string); +DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(vector); +DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque); +DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list); +DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list); +DECLARE_STANDARD_CONTAINER(set); +DECLARE_STANDARD_CONTAINER(map); +DECLARE_STANDARD_CONTAINER(multiset); +DECLARE_STANDARD_CONTAINER(multimap); +DECLARE_STANDARD_CONTAINER(unordered_set); +DECLARE_STANDARD_CONTAINER(unordered_map); +DECLARE_STANDARD_CONTAINER(unordered_multiset); +DECLARE_STANDARD_CONTAINER(unordered_multimap); + +typedef basic_string string; + +template +struct remove_reference; + +template +struct remove_reference { + typedef _Tp type; +}; + +template +struct remove_reference<_Tp &> { + typedef _Tp type; +}; + +template +struct remove_reference<_Tp &&> { + typedef _Tp type; +}; + +template +constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept { + return static_cast::type &&>(__t); +} + +} // namespace std + +class A { +public: + A(); + A(const A &); + A(A &&); + + A &operator=(const A &); + A &operator=(A &&); + + void foo() const; + int getInt() const; + + operator bool() const; + + int i; +}; + +//////////////////////////////////////////////////////////////////////////////// +// General tests. + +// Simple case. +void simple() { + A a; + a.foo(); + A other_a = std::move(a); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here +} + +// A warning should only be emitted for one use-after-move. +void onlyFlagOneUseAfterMove() { + A a; + a.foo(); + A other_a = std::move(a); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here + a.foo(); +} + +void moveAfterMove() { + // Move-after-move also counts as a use. + { + A a; + std::move(a); + std::move(a); + // CHECK-MESSAGES: [[@LINE-1]]:15: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + // This is also true if the move itself turns into the use on the second loop + // iteration. + { + A a; + for (int i = 0; i < 10; ++i) { + std::move(a); + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here + } + } +} + +// Checks also works on function parameters that have a use-after move. +void parameters(A a) { + std::move(a); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here +} + +void uniquePtrAndSharedPtr() { + // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged. + { + std::unique_ptr ptr; + std::move(ptr); + ptr.get(); + } + { + std::shared_ptr ptr; + std::move(ptr); + ptr.get(); + } + // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped + // in a typedef. + { + typedef std::unique_ptr PtrToA; + PtrToA ptr; + std::move(ptr); + ptr.get(); + } + { + typedef std::shared_ptr PtrToA; + PtrToA ptr; + std::move(ptr); + ptr.get(); + } + // And it's also true if the template argument is a little more involved. + { + struct B { + typedef A AnotherNameForA; + }; + std::unique_ptr ptr; + std::move(ptr); + ptr.get(); + } +} + +// The check also works in member functions. +class Container { + void useAfterMoveInMemberFunction() { + A a; + std::move(a); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } +}; + +// We see the std::move() if it's inside a declaration. +void moveInDeclaration() { + A a; + A another_a(std::move(a)); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here +} + +// We see the std::move if it's inside an initializer list. Initializer lists +// are a special case because they cause ASTContext::getParents() to return +// multiple parents for certain nodes in their subtree. This is because +// RecursiveASTVisitor visits both the syntactic and semantic forms of +// InitListExpr, and the parent-child relationships are different between the +// two forms. +void moveInInitList() { + struct S { + A a; + }; + A a; + S s{std::move(a)}; + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:6: note: move occurred here +} + +void lambdas() { + // Use-after-moves inside a lambda should be detected. + { + A a; + auto lambda = [a] { + std::move(a); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:7: note: move occurred here + }; + } + // This is just as true if the variable was declared inside the lambda. + { + auto lambda = [] { + A a; + std::move(a); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:7: note: move occurred here + }; + } + // But don't warn if the move happened inside the lambda but the use happened + // outside -- because + // - the 'a' inside the lambda is a copy, and + // - we don't know when the lambda will get called anyway + { + A a; + auto lambda = [a] { + std::move(a); + }; + a.foo(); + } + // Warn if the use consists of a capture that happens after a move. + { + A a; + std::move(a); + auto lambda = [a]() { a.foo(); }; + // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + // ...even if the capture was implicit. + { + A a; + std::move(a); + auto lambda = [=]() { a.foo(); }; + // CHECK-MESSAGES: [[@LINE-1]]:27: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + // Same tests but for capture by reference. + { + A a; + std::move(a); + auto lambda = [&a]() { a.foo(); }; + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + { + A a; + std::move(a); + auto lambda = [&]() { a.foo(); }; + // CHECK-MESSAGES: [[@LINE-1]]:27: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + // But don't warn if the move happened after the capture. + { + A a; + auto lambda = [a]() { a.foo(); }; + std::move(a); + } + // ...and again, same thing with an implicit move. + { + A a; + auto lambda = [=]() { a.foo(); }; + std::move(a); + } + // Same tests but for capture by reference. + { + A a; + auto lambda = [&a]() { a.foo(); }; + std::move(a); + } + { + A a; + auto lambda = [&]() { a.foo(); }; + std::move(a); + } +} + +// Use-after-moves are detected in uninstantiated templates if the moved type +// is not a dependent type. +template +void movedTypeIsNotDependentType() { + T t; + A a; + std::move(a); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here +} + +// And if the moved type is a dependent type, the use-after-move is detected if +// the template is instantiated. +template +void movedTypeIsDependentType() { + T t; + std::move(t); + t.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 't' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here +} +template void movedTypeIsDependentType(); + +// Using decltype on an expression is not a use. +void decltypeIsNotUse() { + A a; + std::move(a); + decltype(a) other_a; +} + +// Ignore moves or uses that occur as part of template arguments. +template +class ClassTemplate { +public: + void foo(A a); +}; +template +void functionTemplate(A a); +void templateArgIsNotUse() { + { + // A pattern like this occurs in the EXPECT_EQ and ASSERT_EQ macros in + // Google Test. + A a; + ClassTemplate().foo(std::move(a)); + } + { + A a; + functionTemplate(std::move(a)); + } +} + +// Ignore moves of global variables. +A global_a; +void ignoreGlobalVariables() { + std::move(global_a); + global_a.foo(); +} + +// Ignore moves of member variables. +class IgnoreMemberVariables { + A a; + static A static_a; + + void f() { + std::move(a); + a.foo(); + + std::move(static_a); + static_a.foo(); + } +}; + +//////////////////////////////////////////////////////////////////////////////// +// Tests involving control flow. + +void useAndMoveInLoop() { + // Warn about use-after-moves if they happen in a later loop iteration than + // the std::move(). + { + A a; + for (int i = 0; i < 10; ++i) { + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE+1]]:7: note: move occurred here + std::move(a); + } + } + // However, this case shouldn't be flagged -- the scope of the declaration of + // 'a' is important. + { + for (int i = 0; i < 10; ++i) { + A a; + a.foo(); + std::move(a); + } + } + // Same as above, except that we have an unrelated variable being declared in + // the same declaration as 'a'. This case is interesting because it tests that + // the synthetic DeclStmts generated by the CFG are sequenced correctly + // relative to the other statements. + { + for (int i = 0; i < 10; ++i) { + A a, other; + a.foo(); + std::move(a); + } + } + // Don't warn if we return after the move. + { + A a; + for (int i = 0; i < 10; ++i) { + a.foo(); + if (a.getInt() > 0) { + std::move(a); + return; + } + } + } +} + +void differentBranches(int i) { + // Don't warn if the use is in a different branch from the move. + { + A a; + if (i > 0) { + std::move(a); + } else { + a.foo(); + } + } + // Same thing, but with a ternary operator. + { + A a; + i > 0 ? (void)std::move(a) : a.foo(); + } + // A variation on the theme above. + { + A a; + a.getInt() > 0 ? a.getInt() : A(std::move(a)).getInt(); + } + // Same thing, but with a switch statement. + { + A a; + switch (i) { + case 1: + std::move(a); + break; + case 2: + a.foo(); + break; + } + } + // However, if there's a fallthrough, we do warn. + { + A a; + switch (i) { + case 1: + std::move(a); + case 2: + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-4]]:7: note: move occurred here + break; + } + } +} + +// False positive: A use-after-move is flagged even though the "if (b)" and +// "if (!b)" are mutually exclusive. +void mutuallyExclusiveBranchesFalsePositive(bool b) { + A a; + if (b) { + std::move(a); + } + if (!b) { + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-5]]:5: note: move occurred here + } +} + +// Destructors marked [[noreturn]] are handled correctly in the control flow +// analysis. (These are used in some styles of assertion macros.) +class FailureLogger { +public: + FailureLogger(); + [[noreturn]] ~FailureLogger(); + void log(const char *); +}; +#define ASSERT(x) \ + while (x) \ + FailureLogger().log(#x) +bool operationOnA(A); +void noreturnDestructor() { + A a; + // The while loop in the ASSERT() would ordinarily have the potential to cause + // a use-after-move because the second iteration of the loop would be using a + // variable that had been moved from in the first iteration. Check that the + // CFG knows that the second iteration of the loop is never reached because + // the FailureLogger destructor is marked [[noreturn]]. + ASSERT(operationOnA(std::move(a))); +} +#undef ASSERT + +//////////////////////////////////////////////////////////////////////////////// +// Tests for reinitializations + +template +void swap(T &a, T &b) { + T tmp = std::move(a); + a = std::move(b); + b = std::move(tmp); +} +void assignments(int i) { + // Don't report a use-after-move if the variable was assigned to in the + // meantime. + { + A a; + std::move(a); + a = A(); + a.foo(); + } + // The assignment should also be recognized if move, assignment and use don't + // all happen in the same block (but the assignment is still guaranteed to + // prevent a use-after-move). + { + A a; + if (i == 1) { + std::move(a); + a = A(); + } + if (i == 2) { + a.foo(); + } + } + { + A a; + if (i == 1) { + std::move(a); + } + if (i == 2) { + a = A(); + a.foo(); + } + } + // The built-in assignment operator should also be recognized as a + // reinitialization. (std::move() may be called on built-in types in template + // code.) + { + int a1 = 1, a2 = 2; + swap(a1, a2); + } + // A std::move() after the assignment makes the variable invalid again. + { + A a; + std::move(a); + a = A(); + std::move(a); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + // Report a use-after-move if we can't be sure that the variable was assigned + // to. + { + A a; + std::move(a); + if (i < 10) { + a = A(); + } + if (i > 5) { + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-7]]:5: note: move occurred here + } + } +} + +// Passing the object to a function through a non-const pointer or refernce +// counts as a re-initialization. +void passByNonConstPointer(A *); +void passByNonConstReference(A &); +void passByNonConstPointerIsReinit() { + { + A a; + std::move(a); + passByNonConstPointer(&a); + a.foo(); + } + { + A a; + std::move(a); + passByNonConstReference(a); + a.foo(); + } +} + +// Passing the object through a const pointer or reference counts as a use -- +// since the called function cannot reinitialize the object. +void passByConstPointer(const A *); +void passByConstReference(const A &); +void passByConstPointerIsUse() { + { + // Declaring 'a' as const so that no ImplicitCastExpr is inserted into the + // AST -- we wouldn't want the check to rely solely on that to detect a + // const pointer argument. + const A a; + std::move(a); + passByConstPointer(&a); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + const A a; + std::move(a); + passByConstReference(a); + // CHECK-MESSAGES: [[@LINE-1]]:24: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here +} + +// Clearing a standard container using clear() is treated as a +// re-initialization. +void standardContainerClearIsReinit() { + { + std::string container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::vector container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::deque container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::forward_list container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::list container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::set container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::map container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::multiset container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::multimap container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::unordered_set container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::unordered_map container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::unordered_multiset container; + std::move(container); + container.clear(); + container.empty(); + } + { + std::unordered_multimap container; + std::move(container); + container.clear(); + container.empty(); + } + // This should also work for typedefs of standard containers. + { + typedef std::vector IntVector; + IntVector container; + std::move(container); + container.clear(); + container.empty(); + } + // But it shouldn't work for non-standard containers. + { + // This might be called "vector", but it's not in namespace "std". + struct vector { + void clear() {} + } container; + std::move(container); + container.clear(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'container' used after it was + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + // An intervening clear() on a different container does not reinitialize. + { + std::vector container1, container2; + std::move(container1); + container2.clear(); + container1.empty(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'container1' used after it was + // CHECK-MESSAGES: [[@LINE-4]]:5: note: move occurred here + } +} + +// Clearing a standard container using assign() is treated as a +// re-initialization. +void standardContainerAssignIsReinit() { + { + std::string container; + std::move(container); + container.assign(0, ' '); + container.empty(); + } + { + std::vector container; + std::move(container); + container.assign(0, 0); + container.empty(); + } + { + std::deque container; + std::move(container); + container.assign(0, 0); + container.empty(); + } + { + std::forward_list container; + std::move(container); + container.assign(0, 0); + container.empty(); + } + { + std::list container; + std::move(container); + container.clear(); + container.empty(); + } + // But it doesn't work for non-standard containers. + { + // This might be called "vector", but it's not in namespace "std". + struct vector { + void assign(std::size_t, int) {} + } container; + std::move(container); + container.assign(0, 0); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'container' used after it was + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + // An intervening assign() on a different container does not reinitialize. + { + std::vector container1, container2; + std::move(container1); + container2.assign(0, 0); + container1.empty(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'container1' used after it was + // CHECK-MESSAGES: [[@LINE-4]]:5: note: move occurred here + } +} + +//////////////////////////////////////////////////////////////////////////////// +// Tests related to order of evaluation within expressions + +// Relative sequencing of move and use. +void passByRvalueReference(int i, A &&a); +void passByValue(int i, A a); +void passByValue(A a, int i); +A g(A, A &&); +int intFromA(A &&); +int intFromInt(int); +void sequencingOfMoveAndUse() { + // This case is fine because the move only happens inside + // passByRvalueReference(). At this point, a.getInt() is guaranteed to have + // been evaluated. + { + A a; + passByRvalueReference(a.getInt(), std::move(a)); + } + // However, if we pass by value, the move happens when the move constructor is + // called to create a temporary, and this happens before the call to + // passByValue(). Because the order in which arguments are evaluated isn't + // defined, the move may happen before the call to a.getInt(). + // + // Check that we warn about a potential use-after move for both orderings of + // a.getInt() and std::move(a), independent of the order in which the + // arguments happen to get evaluated by the compiler. + { + A a; + passByValue(a.getInt(), std::move(a)); + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:29: note: move occurred here + // CHECK-MESSAGES: [[@LINE-3]]:17: note: the use and move are unsequenced + } + { + A a; + passByValue(std::move(a), a.getInt()); + // CHECK-MESSAGES: [[@LINE-1]]:31: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:17: note: move occurred here + // CHECK-MESSAGES: [[@LINE-3]]:31: note: the use and move are unsequenced + } + // An even more convoluted example. + { + A a; + g(g(a, std::move(a)), g(a, std::move(a))); + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:27: note: move occurred here + // CHECK-MESSAGES: [[@LINE-3]]:9: note: the use and move are unsequenced + // CHECK-MESSAGES: [[@LINE-4]]:29: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-5]]:7: note: move occurred here + // CHECK-MESSAGES: [[@LINE-6]]:29: note: the use and move are unsequenced + } + // This case is fine because the actual move only happens inside the call to + // operator=(). a.getInt(), by necessity, is evaluated before that call. + { + A a; + A vec[1]; + vec[a.getInt()] = std::move(a); + } + // However, in the following case, the move happens before the assignment, and + // so the order of evaluation is not guaranteed. + { + A a; + int v[3]; + v[a.getInt()] = intFromA(std::move(a)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:21: note: move occurred here + // CHECK-MESSAGES: [[@LINE-3]]:7: note: the use and move are unsequenced + } + { + A a; + int v[3]; + v[intFromA(std::move(a))] = intFromInt(a.i); + // CHECK-MESSAGES: [[@LINE-1]]:44: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here + // CHECK-MESSAGES: [[@LINE-3]]:44: note: the use and move are unsequenced + } +} + +// Relative sequencing of move and reinitialization. If the two are unsequenced, +// we conservatively assume that the move happens after the reinitialization, +// i.e. the that object does not get reinitialized after the move. +A MutateA(A a); +void passByValue(A a1, A a2); +void sequencingOfMoveAndReinit() { + // Move and reinitialization as function arguments (which are indeterminately + // sequenced). Again, check that we warn for both orderings. + { + A a; + passByValue(std::move(a), (a = A())); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:17: note: move occurred here + } + { + A a; + passByValue((a = A()), std::move(a)); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:28: note: move occurred here + } + // Common usage pattern: Move the object to a function that mutates it in some + // way, then reassign the result to the object. This pattern is fine. + { + A a; + a = MutateA(std::move(a)); + a.foo(); + } +} + +// Relative sequencing of reinitialization and use. If the two are unsequenced, +// we conservatively assume that the reinitialization happens after the use, +// i.e. that the object is not reinitialized at the point in time when it is +// used. +void sequencingOfReinitAndUse() { + // Reinitialization and use in function arguments. Again, check both possible + // orderings. + { + A a; + std::move(a); + passByValue(a.getInt(), (a = A())); + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + { + A a; + std::move(a); + passByValue((a = A()), a.getInt()); + // CHECK-MESSAGES: [[@LINE-1]]:28: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } +} + +// The comma operator sequences its operands. +void commaOperatorSequences() { + { + A a; + A(std::move(a)) + , (a = A()); + a.foo(); + } + { + A a; + (a = A()), A(std::move(a)); + a.foo(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:16: note: move occurred here + } +} + +// An initializer list sequences its initialization clauses. +void initializerListSequences() { + { + struct S1 { + int i; + A a; + }; + A a; + S1 s1{a.getInt(), std::move(a)}; + } + { + struct S2 { + A a; + int i; + }; + A a; + S2 s2{std::move(a), a.getInt()}; + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:11: note: move occurred here + } +} + +// A declaration statement containing multiple declarations sequences the +// initializer expressions. +void declarationSequences() { + { + A a; + A a1 = a, a2 = std::move(a); + } + { + A a; + A a1 = std::move(a), a2 = a; + // CHECK-MESSAGES: [[@LINE-1]]:31: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:12: note: move occurred here + } +} + +// The logical operators && and || sequence their operands. +void logicalOperatorsSequence() { + { + A a; + if (a.getInt() > 0 && A(std::move(a)).getInt() > 0) { + A().foo(); + } + } + // A variation: Negate the result of the && (which pushes the && further down + // into the AST). + { + A a; + if (!(a.getInt() > 0 && A(std::move(a)).getInt() > 0)) { + A().foo(); + } + } + { + A a; + if (A(std::move(a)).getInt() > 0 && a.getInt() > 0) { + // CHECK-MESSAGES: [[@LINE-1]]:41: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:9: note: move occurred here + A().foo(); + } + } + { + A a; + if (a.getInt() > 0 || A(std::move(a)).getInt() > 0) { + A().foo(); + } + } + { + A a; + if (A(std::move(a)).getInt() > 0 || a.getInt() > 0) { + // CHECK-MESSAGES: [[@LINE-1]]:41: warning: 'a' used after it was moved + // CHECK-MESSAGES: [[@LINE-2]]:9: note: move occurred here + A().foo(); + } + } +} + +// A range-based for sequences the loop variable declaration before the body. +void forRangeSequences() { + A v[2] = {A(), A()}; + for (A &a : v) { + std::move(a); + } +} + +// If a variable is declared in an if statement, the declaration of the variable +// (which is treated like a reinitialization by the check) is sequenced before +// the evaluation of the condition (which constitutes a use). +void ifStmtSequencesDeclAndCondition() { + for (int i = 0; i < 10; ++i) { + if (A a = A()) { + std::move(a); + } + } +}