llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Kefu Chai (tchaikov) <details> <summary>Changes</summary> This eliminates false positives in bugprone-use-after-move where a variable is used in the callee and moved from in the arguments. We introduce one 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: ```c++ 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)). Treating this scenario correctly has required rewriting the logic in bugprone-use-after-move that governs whether the use happens in a later loop iteration than the move. This was previously based on an unsound heuristic (does the use come lexically before the move?); we now use a more rigourous criterion based on reachability in the CFG. Fixes #<!-- -->57758 Fixes #<!-- -->59612 --- Full diff: https://github.com/llvm/llvm-project/pull/93623.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp (+55-8) - (modified) clang-tools-extra/clang-tidy/utils/ExprSequence.cpp (+67-6) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp (+49-1) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index b91ad0f182295..f357b21b3a967 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -14,6 +14,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 "../utils/Matchers.h" @@ -35,6 +36,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 +52,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, @@ -69,6 +73,30 @@ class UseAfterMoveFinder { llvm::SmallPtrSet<const CFGBlock *, 8> Visited; }; +/// Returns whether the `Before` block can reach the `After` block. +bool reaches(const CFGBlock *Before, const CFGBlock *After) { + llvm::SmallVector<const CFGBlock *> Stack; + llvm::SmallPtrSet<const CFGBlock *, 1> 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 CFGBlock *Succ : Current->succs()) { + if (Succ && !Visited.contains(Succ)) + Stack.push_back(Succ); + } + } + + return false; +} + } // namespace // Matches nodes that are @@ -89,7 +117,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 @@ -110,15 +138,30 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall, BlockMap = std::make_unique<StmtToBlockMap>(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.contains(UseBlock) + : reaches(UseBlock, MoveBlock); + } + return Found; } bool UseAfterMoveFinder::findInternal(const CFGBlock *Block, @@ -175,6 +218,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 +441,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 +542,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 50df451ecfa26..d8a8486009dc3 100644 --- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp +++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp @@ -55,12 +55,23 @@ 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); + }); +} + +bool argsContain(const CallExpr *Call, const Stmt *TheStmt) { + return std::find(Call->arguments().begin(), Call->arguments().end(), + TheStmt) != Call->arguments().end(); } llvm::SmallVector<const InitListExpr *> @@ -95,9 +106,59 @@ bool ExprSequence::inSequence(const Stmt *Before, const Stmt *After) const { return true; } + SmallVector<const Stmt *, 1> 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<CXXMemberCallExpr>(Parent)) { + if (argsContain(call, Before) && + isa<DeclRefExpr>( + 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<MemberExpr>(Before); + Member && call->getCallee() == Member && + isa<DeclRefExpr>(Member->getBase()->IgnoreParenImpCasts()) && + isDescendantOfArgs(After, call, Context)) + return false; + } + + if (!Context->getLangOpts().CPlusPlus17) + continue; + + if (const auto *call = dyn_cast<CallExpr>(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 3e3195f6f6813..96ffe1133a296 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -241,6 +241,12 @@ Changes in existing checks function with the same prefix as the default argument, e.g. ``std::unique_ptr`` and ``std::unique``, avoiding false positive for assignment operator overloading. +- Improved :doc:`bugprone-use-after-move + <clang-tidy/checks/bugprone/use-after-move>`: 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:`bugprone-use-after-move <clang-tidy/checks/bugprone/use-after-move>` check to also handle calls to ``std::forward``. 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 7d9f63479a1b4..6a4e3990e36dc 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> a); +int consumeA(A &&a); + +void calleeSequencedBeforeArguments() { + { + std::unique_ptr<A> 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> a; + std::unique_ptr<A> getArg(std::unique_ptr<A> 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; `````````` </details> https://github.com/llvm/llvm-project/pull/93623 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
