[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: @5chmidti @PiotrZSL Hi Julian and Piotr, I hope this message finds you well. I'm following up on my previous comment regarding the PR I submitted two weeks ago. I understand you both might be busy, but I wanted to check if there's been any progress or if we are expecting more inputs from other reviewers to move forward with the merge. If that's not the case, could you please help merge this change? 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: @HerrCai0907 Hello Congcong, thank you for your message. I appreciate your willingness to help. As I don't have write access to the repository, I would be grateful if you could review and merge this pull request for me. Please let me know if you need any additional information or if you have any questions about the changes. 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
@@ -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; + tchaikov wrote: agreed. but practically, we reference `EvaluationOrderUndefined` only if `UseAfterMoveFinder::find()` returns `true`, which indicates a use-after-move is identified. see https://github.com/llvm/llvm-project/blob/4d95850d052336a785651030eafa0b24651801a0/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp#L498-L501, and the only two places where we return `true` in `UseAfterMoveFinder::findInternal()` are 1. https://github.com/llvm/llvm-project/blob/4d95850d052336a785651030eafa0b24651801a0/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp#L178 2. https://github.com/llvm/llvm-project/blob/4d95850d052336a785651030eafa0b24651801a0/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp#L188 the 2nd case is a recursive call to `UseAfterMoveFinder::findInternal()`. and the 1st case is where we set `UseHappensInLaterLoopIteration` to `false`. and we may set this member variable to `true` later on. probably a more readable implementation is to return an `optional` from `UseAfterMoveFinder::find()`, so that it's obvious that `UseAfterMove` is always initialized and returned if we find a use-after-move. but that'd be a cleanup, and not directly related to this PR. if it acceptable to piggy back it into this PR, i will do it. 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
@@ -69,6 +73,30 @@ class UseAfterMoveFinder {
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 CFGBlock *Succ : Current->succs()) {
+ if (Succ && !Visited.contains(Succ))
+Stack.push_back(Succ);
+}
+ }
+
+ return false;
+}
+
tchaikov wrote:
yeah, it's equivalent to `reaches()`. will use it instead in the next revision.
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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: v3: - trade `reaches()` helper for `CFGReverseBlockReachabilityAnalysis`. less repeating this way. - replace `argsContain()` helper with `llvm::is_contained()`. less repeating this way. - s/call/Call/. more consistent with the naming convention in LLVM. - initialize `EvaluationOrderUndefined` in-class 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
@@ -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; + tchaikov wrote: after a second thought, i am removing this initialization, and just set the default value in the `UseAfterMove`. and move the accompanied comment there. 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From cb1dfa3c776e6c1c327acc6ec7f02c4bceb64069 Mon Sep 17 00:00:00 2001
From: martinboehme
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 | 39 ---
.../clang-tidy/utils/ExprSequence.cpp | 68 +--
clang-tools-extra/docs/ReleaseNotes.rst | 5 +-
.../checkers/bugprone/use-after-move.cpp | 50 +-
4 files changed, 146 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..0130f4f17f5cc 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
@@ -108,17 +115,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))
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: @5chmidti Thanks for your thoughtful review and suggestions! I've incorporated them into the latest revision, which I'd appreciate you taking another look at. 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From 00df70151da03f9a3d3c6ae3ee8078fd6ff654f0 Mon Sep 17 00:00:00 2001
From: martinboehme
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(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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
@@ -35,6 +37,11 @@ struct UseAfterMove {
// Is the order in which the move and the use are evaluated undefined?
bool EvaluationOrderUndefined;
tchaikov wrote:
sure. but for the reason explained at
https://github.com/llvm/llvm-project/pull/93623#discussion_r1631614642. but
please note, it's just for the sake of readability / consistency, not for the
correctness.
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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
@@ -110,15 +117,32 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
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.
+ auto CFA =
std::make_unique(*TheCFG);
+ TheUseAfterMove->UseHappensInLaterLoopIteration =
+ UseBlock == MoveBlock ? Visited.contains(UseBlock)
+: CFA->isReachable(UseBlock, MoveBlock);
tchaikov wrote:
`CFGReverseBlockReachabilityAnalysis` comes with two member variables of
`llvm::BitVector` and `llvm::DenseMap<>`, which could be potentially too large
to put on the stack, i thought. that's why i allocated it on heap. but if you
believe it's safe to do so. will allocate it on stack.
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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From e07681e21f01c56a9e2705f6380838047886598a Mon Sep 17 00:00:00 2001
From: martinboehme
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 | 42 +---
.../clang-tidy/utils/ExprSequence.cpp | 68 +--
clang-tools-extra/docs/ReleaseNotes.rst | 5 +-
.../checkers/bugprone/use-after-move.cpp | 50 +-
4 files changed, 148 insertions(+), 17 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..c90c92b5f660a 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"
@@ -34,7 +36,12 @@ struct UseAfterMove {
const DeclRefExpr *DeclRef;
// Is the order in which the move and the use are evaluated undefined?
- bool EvaluationOrderUndefined;
+ bool EvaluationOrderUndefined = false;
+
+ // 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(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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: v3: - allocate `CFGReverseBlockReachabilityAnalysis` on stack not on heap, as it's small enough and can be fit in the stack. - initialize `EvaluationOrderUndefined` in-class to be more consistent. please note, before this change, it's always initialized if it's going to be referenced. 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: @5chmidti hi Julian, thank you for your review, suggestions and approval. updated accordingly. 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
@@ -110,15 +117,32 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
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.
+ auto CFA =
std::make_unique(*TheCFG);
+ TheUseAfterMove->UseHappensInLaterLoopIteration =
+ UseBlock == MoveBlock ? Visited.contains(UseBlock)
+: CFA->isReachable(UseBlock, MoveBlock);
tchaikov wrote:
thank you Martin! for the detailed explanation. i concur with you and Julian
now.
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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: @PiotrZSL @SimplyDanny and @HerrCai0907 Hello, gentlemen. Would you be available to take a look at this at your earliest convenience? 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov created
https://github.com/llvm/llvm-project/pull/93623
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.
>From d787a496e2f18f32a8b90f762f7d8f649714f6e7 Mon Sep 17 00:00:00 2001
From: martinboehme
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 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
// Matches nodes that are
@@ -89,7 +117,7 @@ UseAfterMoveFinder::UseAft
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From 5fc21145abde56096b607cf123f529f97b458252 Mon Sep 17 00:00:00 2001
From: martinboehme
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 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
// 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(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 find
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: @martinboehme hello Martin, I resurrected your change at https://reviews.llvm.org/D145581?id=503330#inline-1406063 and posted here in hope that we can continue your efforts and finally land the change in main branch. Hope you don't mind that I created this without your permission. we are also using clang-tidy in our project, since we are using the chained expression like `v.foo(x, y).then(std::move(x)).then(std::move(v))` a lot, it's a little bit annoying that clang-tidy warns at seeing this. this reduces the signal-to-noise ratio of its output. On top of your change, I made following changes - rebase onto the latest main HEAD - s/const auto &Succ/const CFGBlock *Succ/, so it is more explicit that the element type is a pointer. - use `Visited.Contains(e)` instead of `!Visited.count(e)`, for better readability - rename `found` to `Found`, to be more consistent with the naming convention in LLVM - check for null before pushing a new successor node to `Stack`, otherwise we could be iterating a "null" node's successors. these changes are collected in a separate commit in this PR, if you are good with them, I will fold it into the first commit. 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From 5fc21145abde56096b607cf123f529f97b458252 Mon Sep 17 00:00:00 2001
From: martinboehme
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 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
// 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(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 find
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From 5cbb966128b63212dcf9f2284b9f58fbcd12cee6 Mon Sep 17 00:00:00 2001
From: martinboehme
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
Fixes #59612
---
.../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 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
// 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(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();
}
-
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov edited 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From 3f1ef816ae2bfca3ec253f0aad5b4bb69984d60d Mon Sep 17 00:00:00 2001
From: martinboehme
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 | 63 ++--
.../clang-tidy/utils/ExprSequence.cpp | 73 +--
clang-tools-extra/docs/ReleaseNotes.rst | 6 ++
.../checkers/bugprone/use-after-move.cpp | 50 -
4 files changed, 177 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..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 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 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(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(
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov ready_for_review 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: @martinboehme thank you! from now on, i will try to address the upcoming comments from reviewers if this PR is fortunate enough to get more attentions, and to follow it up. 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From e9e03e8c07c7b8deaba7ac11a593dfb63b4c9354 Mon Sep 17 00:00:00 2001
From: martinboehme
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 | 63 ++--
.../clang-tidy/utils/ExprSequence.cpp | 73 +--
clang-tools-extra/docs/ReleaseNotes.rst | 5 +-
.../checkers/bugprone/use-after-move.cpp | 50 -
4 files changed, 175 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..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 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 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(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(
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: v2: - s/auto *Member/const auto *Member/ - merge into the existing ReleaseNote entry of the same check, instead of creating another one 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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/93623
>From 14106f8d990c068f9f75e1ea6a1f10c4be5930f6 Mon Sep 17 00:00:00 2001
From: martinboehme
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 | 63 ++--
.../clang-tidy/utils/ExprSequence.cpp | 73 +--
clang-tools-extra/docs/ReleaseNotes.rst | 5 +-
.../checkers/bugprone/use-after-move.cpp | 50 -
4 files changed, 175 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..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 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 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(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(
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: thank you Piotr for merging this change. 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
[clang-tools-extra] [clang-tidy] let UseAfterMoveFinder::find() return an optional (PR #98100)
https://github.com/tchaikov created
https://github.com/llvm/llvm-project/pull/98100
before this change, we use an output parameter so
`UseAfterMoveFinder::find()` can return the found `UseAfterMove`, and addition
to it, `UseAfterMoveFinder::find()` return a bool, so we can tell if a
use-after-free is identified. this arrangement could be confusing when one
needs to understand when the each member variable of the returned
`UseAfterMove` instance is initialized.
in this change, we return an `std::optional` instead of a bool,
so that it's more obvious on when/where the returned `UseAfterMove` is
initialized.
this change is a cleanup. so it does not change the behavior of this check.
>From 3537fe828bac5678970e106ac89306e465446163 Mon Sep 17 00:00:00 2001
From: Kefu Chai
Date: Tue, 9 Jul 2024 07:58:23 +0800
Subject: [PATCH] [clang-tidy] let UseAfterMoveFinder::find() return an
optional
before this change, we use an output parameter so
`UseAfterMoveFinder::find()` can return the found `UseAfterMove`, and
addition to it, `UseAfterMoveFinder::find()` return a bool, so we can
tell if a use-after-free is identified. this arrangement could be
confusing when one needs to understand when the each member variable of
the returned `UseAfterMove` instance is initialized.
in this change, we return an `std::optional` instead of
a bool, so that it's more obvious on when/where the returned
`UseAfterMove` is initialized.
this change is a cleanup. so it does not change the behavior of this
check.
Signed-off-by: Kefu Chai
---
.../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 53 +--
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index c90c92b5f660a..a740b602af12b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -54,13 +54,12 @@ class UseAfterMoveFinder {
// occurs after 'MovingCall' (the expression that performs the move). If a
// 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 DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
+ std::optional find(Stmt *CodeBlock, const Expr *MovingCall,
+ const DeclRefExpr *MovedVariable);
private:
- bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
-const ValueDecl *MovedVariable,
-UseAfterMove *TheUseAfterMove);
+ std::optional findInternal(const CFGBlock *Block, const Expr
*MovingCall,
+ const ValueDecl *MovedVariable);
void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *MovedVariable,
llvm::SmallVectorImpl *Uses,
llvm::SmallPtrSetImpl *Reinits);
@@ -95,9 +94,8 @@ static StatementMatcher inDecltypeOrTemplateArg() {
UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
: Context(TheContext) {}
-bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
- const DeclRefExpr *MovedVariable,
- UseAfterMove *TheUseAfterMove) {
+std::optional UseAfterMoveFinder::find(Stmt *CodeBlock, const
Expr *MovingCall,
+ const DeclRefExpr
*MovedVariable) {
// 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
// lambda.
@@ -111,7 +109,7 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
std::unique_ptr TheCFG =
CFG::buildCFG(nullptr, CodeBlock, Context, Options);
if (!TheCFG)
-return false;
+return std::nullopt;
Sequence = std::make_unique(TheCFG.get(), CodeBlock, Context);
BlockMap = std::make_unique(TheCFG.get(), Context);
@@ -125,10 +123,9 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
MoveBlock = &TheCFG->getEntry();
}
- bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
-TheUseAfterMove);
+ auto TheUseAfterMove = findInternal(MoveBlock, MovingCall,
MovedVariable->getDecl());
- if (Found) {
+ if (TheUseAfterMove) {
if (const CFGBlock *UseBlock =
BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef)) {
// Does the use happen in a later loop iteration than the move?
@@ -142,15 +139,14 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
: CFA.isReachable(UseBlock, MoveBlock);
}
}
- return Found;
+ return TheUseAfterMove;
}
-bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
-
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
@@ -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; + tchaikov wrote: > for better readability. but the `optional<>` refactor is still a > nice-to-have, IMHO. created #98100 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
[clang-tools-extra] [clang-tidy] let UseAfterMoveFinder::find() return an optional (PR #98100)
https://github.com/tchaikov updated
https://github.com/llvm/llvm-project/pull/98100
>From 1a2d88917db32253514fc7a533f5cb39d465a9c7 Mon Sep 17 00:00:00 2001
From: Kefu Chai
Date: Tue, 9 Jul 2024 07:58:23 +0800
Subject: [PATCH] [clang-tidy] let UseAfterMoveFinder::find() return an
optional
before this change, we use an output parameter so
`UseAfterMoveFinder::find()` can return the found `UseAfterMove`, and
addition to it, `UseAfterMoveFinder::find()` return a bool, so we can
tell if a use-after-free is identified. this arrangement could be
confusing when one needs to understand when the each member variable of
the returned `UseAfterMove` instance is initialized.
in this change, we return an `std::optional` instead of
a bool, so that it's more obvious on when/where the returned
`UseAfterMove` is initialized.
this change is a cleanup. so it does not change the behavior of this
check.
Signed-off-by: Kefu Chai
---
.../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 56 ++-
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index c90c92b5f660a..8f4b5e8092dda 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -54,13 +54,13 @@ class UseAfterMoveFinder {
// occurs after 'MovingCall' (the expression that performs the move). If a
// 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 DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
+ std::optional find(Stmt *CodeBlock, const Expr *MovingCall,
+ const DeclRefExpr *MovedVariable);
private:
- bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
-const ValueDecl *MovedVariable,
-UseAfterMove *TheUseAfterMove);
+ std::optional findInternal(const CFGBlock *Block,
+ const Expr *MovingCall,
+ const ValueDecl *MovedVariable);
void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *MovedVariable,
llvm::SmallVectorImpl *Uses,
llvm::SmallPtrSetImpl *Reinits);
@@ -95,9 +95,9 @@ static StatementMatcher inDecltypeOrTemplateArg() {
UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
: Context(TheContext) {}
-bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
- const DeclRefExpr *MovedVariable,
- UseAfterMove *TheUseAfterMove) {
+std::optional
+UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
+ const DeclRefExpr *MovedVariable) {
// 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
// lambda.
@@ -111,7 +111,7 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
std::unique_ptr TheCFG =
CFG::buildCFG(nullptr, CodeBlock, Context, Options);
if (!TheCFG)
-return false;
+return std::nullopt;
Sequence = std::make_unique(TheCFG.get(), CodeBlock, Context);
BlockMap = std::make_unique(TheCFG.get(), Context);
@@ -125,10 +125,10 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
MoveBlock = &TheCFG->getEntry();
}
- bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
-TheUseAfterMove);
+ auto TheUseAfterMove =
+ findInternal(MoveBlock, MovingCall, MovedVariable->getDecl());
- if (Found) {
+ if (TheUseAfterMove) {
if (const CFGBlock *UseBlock =
BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef)) {
// Does the use happen in a later loop iteration than the move?
@@ -142,15 +142,14 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr
*MovingCall,
: CFA.isReachable(UseBlock, MoveBlock);
}
}
- return Found;
+ return TheUseAfterMove;
}
-bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
- const Expr *MovingCall,
- const ValueDecl *MovedVariable,
- UseAfterMove *TheUseAfterMove) {
+std::optional
+UseAfterMoveFinder::findInternal(const CFGBlock *Block, const Expr *MovingCall,
+ const ValueDecl *MovedVariable) {
if (Visited.count(Block))
-return false;
+return std::nullopt;
// Mark the block as visited (except if this is the block containing the
// std::move() and it's being visited the first time).
@@ -189,17 +188,18 @@ bool Us
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
tchaikov wrote: @5chmidti @PiotrZSL Hi Julian and Piotr, thanks for taking the time to review my PR. I'm eager to get it merged. Is there anything else I can do to help facilitate the merge process? Or we can merge it now? 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
