https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From 5fc21145abde56096b607cf123f529f97b458252 Mon Sep 17 00:00:00 2001
From: martinboehme <mboe...@google.com>
Date: Wed, 29 May 2024 07:23:35 +0800
Subject: [PATCH 1/2] [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
---
.../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 64 ++++++++++++++--
.../clang-tidy/utils/ExprSequence.cpp | 73 +++++++++++++++++--
clang-tools-extra/docs/ReleaseNotes.rst | 6 ++
.../checkers/bugprone/use-after-move.cpp | 50 ++++++++++++-
4 files changed, 178 insertions(+), 15 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..b8d2f1ea65d2c 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 auto &Succ : Current->succs()) {
+ if (!Visited.count(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,31 @@ 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.count(UseBlock) != 0
+ : reaches(UseBlock, MoveBlock);
+ }
+
+ return found;
}
bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
@@ -175,6 +219,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 +442,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 +543,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;
>From c1c47daa43f946deb831fc0839d5274f32bae469 Mon Sep 17 00:00:00 2001
From: Kefu Chai <tchai...@gmail.com>
Date: Wed, 29 May 2024 07:19:36 +0800
Subject: [PATCH 2/2] [squash-me] clang-tools-extra
Signed-off-by: Kefu Chai <tchai...@gmail.com>
---
.../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b8d2f1ea65d2c..f357b21b3a967 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -88,8 +88,8 @@ bool reaches(const CFGBlock *Before, const CFGBlock *After) {
Visited.insert(Current);
- for (const auto &Succ : Current->succs()) {
- if (!Visited.count(Succ))
+ for (const CFGBlock *Succ : Current->succs()) {
+ if (Succ && !Visited.contains(Succ))
Stack.push_back(Succ);
}
}
@@ -146,10 +146,10 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
MoveBlock = &TheCFG->getEntry();
}
- bool found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
+ bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
TheUseAfterMove);
- if (found) {
+ if (Found) {
if (const CFGBlock *UseBlock =
BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef))
// Does the use happen in a later loop iteration than the move?
@@ -158,11 +158,10 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
// - 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
+ UseBlock == MoveBlock ? Visited.contains(UseBlock)
: reaches(UseBlock, MoveBlock);
}
-
- return found;
+ return Found;
}
bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
