[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
Kefu Chai via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 7 20:45:29 PDT 2024
https://github.com/tchaikov updated https://github.com/llvm/llvm-project/pull/93623
>From 00df70151da03f9a3d3c6ae3ee8078fd6ff654f0 Mon Sep 17 00:00:00 2001
From: martinboehme <mboehme at google.com>
Date: Wed, 29 May 2024 07:23:35 +0800
Subject: [PATCH] [clang-tidy] Let bugprone-use-after-move ignore the moved
variable in callee
In C++17, callee is guaranteed to be sequenced before arguments.
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:
```
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
---
.../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 40 ++++++++---
.../clang-tidy/utils/ExprSequence.cpp | 68 +++++++++++++++++--
clang-tools-extra/docs/ReleaseNotes.rst | 5 +-
.../checkers/bugprone/use-after-move.cpp | 50 +++++++++++++-
4 files changed, 147 insertions(+), 16 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..3072c709b3120 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,11 @@ 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?
+ //
+ // We default to false and change it to true if required in find().
+ bool UseHappensInLaterLoopIteration = false;
};
/// Finds uses of a variable after a move (and maintains state required by the
@@ -48,7 +55,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 +96,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 +117,32 @@ 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.
+ auto CFA = std::make_unique<CFGReverseBlockReachabilityAnalysis>(*TheCFG);
+ TheUseAfterMove->UseHappensInLaterLoopIteration =
+ UseBlock == MoveBlock ? Visited.contains(UseBlock)
+ : CFA->isReachable(UseBlock, MoveBlock);
+ }
+ }
+ return Found;
}
bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
@@ -394,7 +418,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 +519,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..145a5fe378b3e 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<const InitListExpr *>
@@ -95,9 +101,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 (is_contained(Call->arguments(), 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 (const 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 277a6e75da2ac..78c69ce8af091 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
<clang-tidy/checks/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
<clang-tidy/checks/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 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;
More information about the cfe-commits
mailing list