[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] 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] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
github-actions[bot] wrote: @tchaikov Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our [build bots](https://lab.llvm.org/buildbot/). If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail [here](https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr). If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of [LLVM development](https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy). You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! 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/PiotrZSL closed 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)
HerrCai0907 wrote: Normally PR can be merged if someone approved PR. You can merge it by yourself. If you don't have access, I'm glad to help you. 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 @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: @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
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
https://github.com/PiotrZSL approved this pull request. LGTM. 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)
@@ -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 = >getEntry();
+MoveBlock = >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)
@@ -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 = >getEntry();
+MoveBlock = >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);
martinboehme wrote:
Drive-by comment: These types are fine to put on the stack. Note that the
elements contained in these containers are stored on the heap in any case. For
example, in the case of `DenseMap`, this storage is allocated by
`DenseMap::allocateBuckets()`. The only things contained in `DenseMap` itself
(and hence stored on the stack) are a pointer to the bucket array `Buckets` and
some counters (`NumEntries`, `NumBuckets`, `NumTombstones`).
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 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)
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)
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 = >getEntry();
+MoveBlock = >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
[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)
@@ -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 = >getEntry();
+MoveBlock = >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)
@@ -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)
@@ -35,6 +37,11 @@ struct UseAfterMove {
// Is the order in which the move and the use are evaluated undefined?
bool EvaluationOrderUndefined;
5chmidti wrote:
While you're at it, please initialize `EvaluationOrderUndefined` to `false` as
well.
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 = >getEntry();
+MoveBlock = >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);
5chmidti wrote:
`CFA` does not need to be a unique_ptr, you can simply create a
`CFGReverseBlockReachabilityAnalysis` on the 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/5chmidti approved this pull request. LGTM after fixing these two comments, but let's wait for others to review as well 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/5chmidti 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 = >getEntry();
+MoveBlock = >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
[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 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 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 = >getEntry();
+MoveBlock = >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
[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)
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)
@@ -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)
@@ -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)
@@ -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; + 5chmidti wrote: The initialization could also happen in the struct itself (we should initialize `EvaluationOrderUndefined` in the struct as well). 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)
@@ -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();
5chmidti wrote:
Please use `llvm::is_contained`
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)
@@ -95,9 +106,59 @@ bool ExprSequence::inSequence(const Stmt *Before, const
Stmt *After) const {
return true;
}
+ SmallVector BeforeParents = getParentStmts(Before, Context);
+
+ // Since C++17, the callee of a call expression is guaranteed to be sequenced
+ // before all of the arguments.
+ // We handle this as a special case rather than using the general
+ // `getSequenceSuccessor` logic above because the callee expression doesn't
+ // have an unambiguous successor; the order in which arguments are evaluated
+ // is indeterminate.
+ for (const Stmt *Parent : BeforeParents) {
+// Special case: If the callee is a `MemberExpr` with a `DeclRefExpr` as
its
+// base, we consider it to be sequenced _after_ the arguments. This is
+// because the variable referenced in the base will only actually be
+// accessed when the call happens, i.e. once all of the arguments have been
+// evaluated. This has no basis in the C++ standard, but it reflects actual
+// behavior that is relevant to a use-after-move scenario:
+//
+// ```
+// a.bar(consumeA(std::move(a));
+// ```
+//
+// In this example, we end up accessing `a` after it has been moved from,
+// even though nominally the callee `a.bar` is evaluated before the
argument
+// `consumeA(std::move(a))`. Note that this is not specific to C++17, so
+// we implement this logic unconditionally.
+if (const auto *call = dyn_cast(Parent)) {
5chmidti wrote:
`call` -> `Call`
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)
@@ -95,9 +106,59 @@ bool ExprSequence::inSequence(const Stmt *Before, const
Stmt *After) const {
return true;
}
+ SmallVector BeforeParents = getParentStmts(Before, Context);
+
+ // Since C++17, the callee of a call expression is guaranteed to be sequenced
+ // before all of the arguments.
+ // We handle this as a special case rather than using the general
+ // `getSequenceSuccessor` logic above because the callee expression doesn't
+ // have an unambiguous successor; the order in which arguments are evaluated
+ // is indeterminate.
+ for (const Stmt *Parent : BeforeParents) {
+// Special case: If the callee is a `MemberExpr` with a `DeclRefExpr` as
its
+// base, we consider it to be sequenced _after_ the arguments. This is
+// because the variable referenced in the base will only actually be
+// accessed when the call happens, i.e. once all of the arguments have been
+// evaluated. This has no basis in the C++ standard, but it reflects actual
+// behavior that is relevant to a use-after-move scenario:
+//
+// ```
+// a.bar(consumeA(std::move(a));
+// ```
+//
+// In this example, we end up accessing `a` after it has been moved from,
+// even though nominally the callee `a.bar` is evaluated before the
argument
+// `consumeA(std::move(a))`. Note that this is not specific to C++17, so
+// we implement this logic unconditionally.
+if (const auto *call = dyn_cast(Parent)) {
+ if (argsContain(call, Before) &&
+ isa(
+ call->getImplicitObjectArgument()->IgnoreParenImpCasts()) &&
+ isDescendantOrEqual(After, call->getImplicitObjectArgument(),
+ Context))
+return true;
+
+ // We need this additional early exit so that we don't fall through to
the
+ // more general logic below.
+ if (const auto *Member = dyn_cast(Before);
+ Member && call->getCallee() == Member &&
+ isa(Member->getBase()->IgnoreParenImpCasts()) &&
+ isDescendantOfArgs(After, call, Context))
+return false;
+}
+
+if (!Context->getLangOpts().CPlusPlus17)
+ continue;
+
+if (const auto *call = dyn_cast(Parent);
+call && call->getCallee() == Before &&
+isDescendantOfArgs(After, call, Context))
+ return true;
+ }
5chmidti wrote:
`call` -> `Call`
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;
+}
+
5chmidti wrote:
What do you think about using
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Analysis/Analyses/CFGReachabilityAnalysis.h?
We are rebuilding the CFG a lot of times and don't cache anything, so the
caching in `CFGReverseBlockReachabilityAnalysis` won't have any worth until we
do, but it is an existing implementation of what you are doing here, so we
should at least consider 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)
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 = >getEntry();
+MoveBlock = >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 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 = >getEntry();
+MoveBlock = >getEntry();
}
-
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
@@ -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 EugeneZelenko wrote: Please merge with existing entry for this check. 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)
@@ -95,9 +106,59 @@ bool ExprSequence::inSequence(const Stmt *Before, const
Stmt *After) const {
return true;
}
+ SmallVector BeforeParents = getParentStmts(Before, Context);
+
+ // Since C++17, the callee of a call expression is guaranteed to be sequenced
+ // before all of the arguments.
+ // We handle this as a special case rather than using the general
+ // `getSequenceSuccessor` logic above because the callee expression doesn't
+ // have an unambiguous successor; the order in which arguments are evaluated
+ // is indeterminate.
+ for (const Stmt *Parent : BeforeParents) {
+// Special case: If the callee is a `MemberExpr` with a `DeclRefExpr` as
its
+// base, we consider it to be sequenced _after_ the arguments. This is
+// because the variable referenced in the base will only actually be
+// accessed when the call happens, i.e. once all of the arguments have been
+// evaluated. This has no basis in the C++ standard, but it reflects actual
+// behavior that is relevant to a use-after-move scenario:
+//
+// ```
+// a.bar(consumeA(std::move(a));
+// ```
+//
+// In this example, we end up accessing `a` after it has been moved from,
+// even though nominally the callee `a.bar` is evaluated before the
argument
+// `consumeA(std::move(a))`. Note that this is not specific to C++17, so
+// we implement this logic unconditionally.
+if (const auto *call = dyn_cast(Parent)) {
+ if (argsContain(call, Before) &&
+ isa(
+ call->getImplicitObjectArgument()->IgnoreParenImpCasts()) &&
+ isDescendantOrEqual(After, call->getImplicitObjectArgument(),
+ Context))
+return true;
+
+ // We need this additional early exit so that we don't fall through to
the
+ // more general logic below.
+ if (auto *Member = dyn_cast(Before);
EugeneZelenko wrote:
```suggestion
if (const auto *Member = dyn_cast(Before);
```
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)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: Kefu Chai (tchaikov)
Changes
This eliminates false positives in bugprone-use-after-move where a variable
is used in the callee and moved from in the arguments.
We introduce one special case: If the callee is a MemberExpr with a DeclRefExpr
as its base, we consider it to be sequenced after the arguments. This is
because the variable referenced in the base will only actually be accessed when
the call happens, i.e. once all of the arguments have been evaluated. This has
no basis in the C++ standard, but it reflects actual behavior that is relevant
to a use-after-move scenario:
```c++
a.bar(consumeA(std::move(a));
```
In this example, we end up accessing a after it has been moved from, even
though nominally the callee a.bar is evaluated before the argument
consumeA(std::move(a)).
Treating this scenario correctly has required rewriting the logic in
bugprone-use-after-move that governs whether the use happens in a later loop
iteration than the move. This was previously based on an unsound heuristic
(does the use come lexically before the move?); we now use a more rigourous
criterion based on reachability in the CFG.
Fixes #57758
Fixes #59612
---
Full diff: https://github.com/llvm/llvm-project/pull/93623.diff
4 Files Affected:
- (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
(+55-8)
- (modified) clang-tools-extra/clang-tidy/utils/ExprSequence.cpp (+67-6)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp (+49-1)
``diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..f357b21b3a967 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -14,6 +14,7 @@
#include "clang/Analysis/CFG.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "../utils/ExprSequence.h"
#include "../utils/Matchers.h"
@@ -35,6 +36,9 @@ struct UseAfterMove {
// Is the order in which the move and the use are evaluated undefined?
bool EvaluationOrderUndefined;
+
+ // Does the use happen in a later loop iteration than the move?
+ bool UseHappensInLaterLoopIteration;
};
/// Finds uses of a variable after a move (and maintains state required by the
@@ -48,7 +52,7 @@ class UseAfterMoveFinder {
// use-after-move is found, writes information about it to 'TheUseAfterMove'.
// Returns whether a use-after-move was found.
bool find(Stmt *CodeBlock, const Expr *MovingCall,
-const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
+const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
private:
bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
@@ -69,6 +73,30 @@ class UseAfterMoveFinder {
llvm::SmallPtrSet 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 = >getEntry();
+MoveBlock = >getEntry();
}
- return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove);
+ bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
+
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
llvmbot wrote:
@llvm/pr-subscribers-clang-tidy
Author: Kefu Chai (tchaikov)
Changes
This eliminates false positives in bugprone-use-after-move where a variable
is used in the callee and moved from in the arguments.
We introduce one special case: If the callee is a MemberExpr with a DeclRefExpr
as its base, we consider it to be sequenced after the arguments. This is
because the variable referenced in the base will only actually be accessed when
the call happens, i.e. once all of the arguments have been evaluated. This has
no basis in the C++ standard, but it reflects actual behavior that is relevant
to a use-after-move scenario:
```c++
a.bar(consumeA(std::move(a));
```
In this example, we end up accessing a after it has been moved from, even
though nominally the callee a.bar is evaluated before the argument
consumeA(std::move(a)).
Treating this scenario correctly has required rewriting the logic in
bugprone-use-after-move that governs whether the use happens in a later loop
iteration than the move. This was previously based on an unsound heuristic
(does the use come lexically before the move?); we now use a more rigourous
criterion based on reachability in the CFG.
Fixes #57758
Fixes #59612
---
Full diff: https://github.com/llvm/llvm-project/pull/93623.diff
4 Files Affected:
- (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
(+55-8)
- (modified) clang-tools-extra/clang-tidy/utils/ExprSequence.cpp (+67-6)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp (+49-1)
``diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..f357b21b3a967 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -14,6 +14,7 @@
#include "clang/Analysis/CFG.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "../utils/ExprSequence.h"
#include "../utils/Matchers.h"
@@ -35,6 +36,9 @@ struct UseAfterMove {
// Is the order in which the move and the use are evaluated undefined?
bool EvaluationOrderUndefined;
+
+ // Does the use happen in a later loop iteration than the move?
+ bool UseHappensInLaterLoopIteration;
};
/// Finds uses of a variable after a move (and maintains state required by the
@@ -48,7 +52,7 @@ class UseAfterMoveFinder {
// use-after-move is found, writes information about it to 'TheUseAfterMove'.
// Returns whether a use-after-move was found.
bool find(Stmt *CodeBlock, const Expr *MovingCall,
-const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
+const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
private:
bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
@@ -69,6 +73,30 @@ class UseAfterMoveFinder {
llvm::SmallPtrSet 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 = >getEntry();
+MoveBlock = >getEntry();
}
- return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove);
+ bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
+
[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)
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 = >getEntry();
+MoveBlock = >getEntry();
}
-
[clang-tools-extra] [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. (PR #93623)
martinboehme 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 the > main branch. Hope you don't mind that I created this PR without your > permissions. No, not at all. On the contrary -- thanks for picking up this change that I let go stale. > 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. These false > alarms reduce the signal-to-noise ratio of the output of this tool. > > On top of your change, I made following changes > > [snip] > > 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, and then mark this PR > ready-for-review. Yes, those changes look good to me. Thanks! 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 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 : 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 = >getEntry();
+MoveBlock = >getEntry();
}
- return
[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 : 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 = >getEntry();
+MoveBlock = >getEntry();
}
- return findInternal(Block,
[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 /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 : 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 = >getEntry();
+MoveBlock = >getEntry();
}
- return findInternal(Block,
[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)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). 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 : Current->succs()) {
+ if (!Visited.count(Succ))
+Stack.push_back(Succ);
+}
+ }
+
+ return false;
+}
+
} // namespace
// Matches nodes that are
@@ -89,7 +117,7 @@
