diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -15,6 +15,7 @@ #include "clang/Analysis/CFG.h" #include "clang/Lex/Lexer.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallPtrSet.h" #include "../utils/ExprSequence.h" #include @@ -50,6 +51,9 @@ // Is the order in which the move and the use are evaluated undefined? bool EvaluationOrderUndefined; + + // Does the use happen in a later loop iteration than the move? + bool UseHappensInLaterLoopIteration; }; /// Finds uses of a variable after a move (and maintains state required by the @@ -63,7 +67,7 @@ // 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); + const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove); private: bool findInternal(const CFGBlock *Block, const Expr *MovingCall, @@ -84,6 +88,30 @@ llvm::SmallPtrSet Visited; }; +/// Returns whether the `Before` block can reach the `After` block. +bool reaches(const CFGBlock *Before, const CFGBlock *After) { + llvm::SmallVector Stack; + llvm::SmallPtrSet Visited; + + Stack.push_back(Before); + while (!Stack.empty()) { + const CFGBlock *Current = Stack.back(); + Stack.pop_back(); + + if (Current == After) + return true; + + Visited.insert(Current); + + for (const auto &Succ : Current->succs()) { + if (!Visited.count(Succ)) + Stack.push_back(Succ); + } + } + + return false; +} + } // namespace @@ -105,7 +133,7 @@ : Context(TheContext) {} bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall, - const ValueDecl *MovedVariable, + const DeclRefExpr *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 @@ -127,15 +155,31 @@ BlockMap = std::make_unique(TheCFG.get(), Context); Visited.clear(); - const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall); - if (!Block) { + const CFGBlock *MoveBlock = BlockMap->blockContainingStmt(MovingCall); + if (!MoveBlock) { // This can happen if MovingCall is in a constructor initializer, which is // not included in the CFG because the CFG is built only from the function // body. - Block = &TheCFG->getEntry(); + MoveBlock = &TheCFG->getEntry(); } - return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove); + bool found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(), + TheUseAfterMove); + + if (found) { + if (const CFGBlock *UseBlock = + BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef)) + // Does the use happen in a later loop iteration than the move? + // - If they are in the same CFG block, we know the use happened in a + // later iteration if we visited that block a second time. + // - Otherwise, we know the use happened in a later iteration if the + // move is reachable from the use. + TheUseAfterMove->UseHappensInLaterLoopIteration = + UseBlock == MoveBlock ? Visited.count(UseBlock) != 0 + : reaches(UseBlock, MoveBlock); + } + + return found; } bool UseAfterMoveFinder::findInternal(const CFGBlock *Block, @@ -192,6 +236,10 @@ MovingCall != nullptr && Sequence->potentiallyAfter(MovingCall, Use); + // We default to false here and change this to true if required in + // find(). + TheUseAfterMove->UseHappensInLaterLoopIteration = false; + return true; } } @@ -390,7 +438,7 @@ "the use and move are unsequenced, i.e. there is no guarantee " "about the order in which they are evaluated", DiagnosticIDs::Note); - } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) { + } else if (Use.UseHappensInLaterLoopIteration) { Check->diag(UseLoc, "the use happens in a later loop iteration than the move", DiagnosticIDs::Note); @@ -460,7 +508,7 @@ UseAfterMoveFinder Finder(Result.Context); UseAfterMove Use; - if (Finder.find(FunctionBody, MovingCall, Arg->getDecl(), &Use)) + if (Finder.find(FunctionBody, MovingCall, Arg, &Use)) emitDiagnostic(MovingCall, Arg, Use, this, Result.Context); } diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp --- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp +++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp @@ -63,6 +63,25 @@ return false; } +bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call, + ASTContext *Context) { + for (const Expr *Arg : Call->arguments()) { + if (isDescendantOrEqual(Descendant, Arg, Context)) + return true; + } + + return false; +} + +bool argsContain(const CallExpr *Call, const Stmt *TheStmt) { + for (const Expr *Arg : Call->arguments()) { + if (Arg == TheStmt) + return true; + } + + return false; +} + llvm::SmallVector getAllInitListForms(const InitListExpr *InitList) { llvm::SmallVector result = {InitList}; @@ -95,9 +114,60 @@ return true; } + SmallVector BeforeParents = getParentStmts(Before, Context); + + // Since C++17, the callee of a call expression is guaranteed to be sequenced + // before all of the arguments. + // We handle this as a special case rather than using the general + // `getSequenceSuccessor` logic above because the callee expression doesn't + // have an unambiguous successor; the order in which arguments are evaluated + // is indeterminate. + for (const Stmt *Parent : BeforeParents) { + // Special case: If the callee is a `MemberExpr` with a `DeclRefExpr` as its + // base, we consider it to be sequenced _after_ the arguments. This is + // because the variable referenced in the base will only actually be + // accessed when the call happens, i.e. once all of the arguments have been + // evaluated. This has no basis in the C++ standard, but it reflects actual + // behavior that is relevant to a use-after-move scenario: + // + // ``` + // a.bar(consumeA(std::move(a)); + // ``` + // + // In this example, we end up accessing `a` after it has been moved from, + // even though nominally the callee `a.bar` is evaluated before the argument + // `consumeA(std::move(a))`. Note that this is not specific to C++17, so + // we implement this logic unconditionally. + if (const auto *call = dyn_cast(Parent)) { + if (argsContain(call, Before) && + isa( + call->getImplicitObjectArgument()->IgnoreParenImpCasts()) && + isDescendantOrEqual(After, call->getImplicitObjectArgument(), + Context)) + return true; + + // We need this additional early exit so that we don't fall through to the + // more general logic below. + if (auto *Member = dyn_cast(Before); + Member && call->getCallee() == Member && + isa(Member->getBase()->IgnoreParenImpCasts()) && + isDescendantOfArgs(After, call, Context)) + return false; + } + + if (!Context->getLangOpts().CPlusPlus17) + continue; + + if (const auto *call = dyn_cast(Parent); + call && call->getCallee() == Before) { + if (isDescendantOfArgs(After, call, 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)) { + for (const Stmt *Parent : BeforeParents) { if (Parent == After || inSequence(Parent, After)) return true; } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -162,6 +162,13 @@ ` check. Global options of the same name should be used instead. +- In :doc:`bugprone-use-after-move + `: + - Fixed handling for designated initializers. + - Fix: In C++17 and later, the callee of a function is guaranteed to be + sequenced before the arguments, so don't warn if the use happens in the + callee and the move happens in one of the arguments. + - Deprecated check-local options `HeaderFileExtensions` in :doc:`google-build-namespaces ` check. @@ -219,10 +226,6 @@ ` check when warning would be emitted in constructor for virtual base class initialization. -- Improved :doc:`bugprone-use-after-move - ` to understand that there is a - sequence point between designated initializers. - Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp @@ -1,3 +1,4 @@ +// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing // RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing typedef decltype(nullptr) nullptr_t; @@ -123,6 +124,7 @@ A &operator=(A &&); void foo() const; + void bar(int i) const; int getInt() const; operator bool() const; @@ -552,6 +554,19 @@ std::move(a); } } + // Same as above, but the use and the move are in different CFG blocks. + { + A a; + for (int i = 0; i < 10; ++i) { + if (i < 10) + a.foo(); + // CHECK-NOTES: [[@LINE-1]]:9: warning: 'a' used after it was moved + // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here + // CHECK-NOTES: [[@LINE-3]]:9: note: the use happens in a later loop + if (i < 10) + std::move(a); + } + } // However, this case shouldn't be flagged -- the scope of the declaration of // 'a' is important. { @@ -1307,6 +1322,40 @@ } } +// In a function call, the expression that determines the callee is sequenced +// before the arguments -- but only in C++17 and later. +namespace CalleeSequencedBeforeArguments { +int consumeA(std::unique_ptr a); +int consumeA(A &&a); + +void calleeSequencedBeforeArguments() { + { + std::unique_ptr a; + a->bar(consumeA(std::move(a))); + // CHECK-NOTES-CXX11: [[@LINE-1]]:5: warning: 'a' used after it was moved + // CHECK-NOTES-CXX11: [[@LINE-2]]:21: note: move occurred here + // CHECK-NOTES-CXX11: [[@LINE-3]]:5: note: the use and move are unsequenced + } + { + std::unique_ptr a; + std::unique_ptr getArg(std::unique_ptr a); + getArg(std::move(a))->bar(a->getInt()); + // CHECK-NOTES: [[@LINE-1]]:31: warning: 'a' used after it was moved + // CHECK-NOTES: [[@LINE-2]]:12: note: move occurred here + // CHECK-NOTES-CXX11: [[@LINE-3]]:31: note: the use and move are unsequenced + } + { + A a; + // Nominally, the callee `a.bar` is evaluated before the argument + // `consumeA(std::move(a))`, but in effect `a` is only accessed after the + // call to `A::bar()` happens, i.e. after the argument has been evaluted. + a.bar(consumeA(std::move(a))); + // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved + // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here + } +} +} // namespace CalleeSequencedBeforeArguments + // Some statements in templates (e.g. null, break and continue statements) may // be shared between the uninstantiated and instantiated versions of the // template and therefore have multiple parents. Make sure the sequencing code