diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index b91ad0f1822955..ab98eda694ed3b 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -11,9 +11,11 @@ #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h" #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 "../utils/Matchers.h" @@ -35,6 +37,9 @@ struct UseAfterMove { // 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 @@ -48,7 +53,7 @@ class UseAfterMoveFinder { // use-after-move is found, writes information about it to 'TheUseAfterMove'. // Returns whether a use-after-move was found. bool find(Stmt *CodeBlock, const Expr *MovingCall, - const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove); + const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove); private: bool findInternal(const CFGBlock *Block, const Expr *MovingCall, @@ -89,7 +94,7 @@ UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext) : Context(TheContext) {} bool UseAfterMoveFinder::find(Stmt *CodeBlock, 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 @@ -108,17 +113,33 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall, Sequence = std::make_unique(TheCFG.get(), CodeBlock, Context); BlockMap = std::make_unique(TheCFG.get(), Context); + auto CFA = std::make_unique(*TheCFG); 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.contains(UseBlock) + : CFA->isReachable(UseBlock, MoveBlock); + } + return Found; } bool UseAfterMoveFinder::findInternal(const CFGBlock *Block, @@ -175,6 +196,10 @@ bool UseAfterMoveFinder::findInternal(const CFGBlock *Block, 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; } } @@ -394,7 +419,7 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg, "there is no guarantee about the order in which they are evaluated", DiagnosticIDs::Note) << IsMove; - } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) { + } else if (Use.UseHappensInLaterLoopIteration) { Check->diag(UseLoc, "the use happens in a later loop iteration than the " "%select{forward|move}0", @@ -495,7 +520,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { for (Stmt *CodeBlock : CodeBlocks) { UseAfterMoveFinder Finder(Result.Context); UseAfterMove Use; - if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use)) + if (Finder.find(CodeBlock, MovingCall, Arg, &Use)) emitDiagnostic(MovingCall, Arg, Use, this, Result.Context, determineMoveType(MoveDecl)); } diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp index 50df451ecfa268..145a5fe378b3e2 100644 --- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp +++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp @@ -55,12 +55,18 @@ 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 llvm::any_of(getParentStmts(Descendant, Context), + [Ancestor, Context](const Stmt *Parent) { + return isDescendantOrEqual(Parent, Ancestor, Context); + }); +} - return false; +bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call, + ASTContext *Context) { + return llvm::any_of(Call->arguments(), + [Descendant, Context](const Expr *Arg) { + return isDescendantOrEqual(Descendant, Arg, Context); + }); } llvm::SmallVector @@ -95,9 +101,59 @@ bool ExprSequence::inSequence(const Stmt *Before, const Stmt *After) const { 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 (is_contained(Call->arguments(), 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 (const 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 && + 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 index 277a6e75da2acc..78c69ce8af0915 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -254,7 +254,10 @@ Changes in existing checks - Improved :doc:`bugprone-use-after-move ` check to also handle - calls to ``std::forward``. + calls to ``std::forward``. Fixed sequencing of designated initializers. Fixed + sequencing of callees: 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. - Improved :doc:`cppcoreguidelines-missing-std-forward ` check by no longer 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 index 7d9f63479a1b4e..6a4e3990e36dc5 100644 --- 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; @@ -135,6 +136,7 @@ class A { A &operator=(A &&); void foo() const; + void bar(int i) const; int getInt() const; operator bool() const; @@ -576,6 +578,19 @@ void useAndMoveInLoop() { 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. { @@ -1352,6 +1367,40 @@ void ifWhileAndSwitchSequenceInitDeclAndCondition() { } } +// 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 @@ -1469,7 +1518,6 @@ class CtorInitOrder { // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved s{std::move(val)} {} // wrong order // CHECK-NOTES: [[@LINE-1]]:9: note: move occurred here - // CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop iteration than the move private: bool a;