[clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. #93623
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be 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 If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
from
f95da97 to
c1c47da
Compare
|
@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. We are also using clang-tidy in our project. Since we are using the chained expression like On top of your change, I made following changes
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. |
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
2 times, most recently
from
31ef216 to
3cf88b8
Compare
No, not at all. On the contrary -- thanks for picking up this change that I let go stale.
Yes, those changes look good to me. Thanks! |
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
from
3cf88b8 to
3f1ef81
Compare
|
@llvm/pr-subscribers-clang-tidy Author: Kefu Chai (tchaikov) ChangesThis eliminates false positives in bugprone-use-after-move where a variable 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 Full diff: https://github.com/llvm/llvm-project/pull/93623.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..f357b21b3a967 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -14,6 +14,7 @@
#include "clang/Analysis/CFG.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "../utils/ExprSequence.h"
#include "../utils/Matchers.h"
@@ -35,6 +36,9 @@ struct UseAfterMove {
// Is the order in which the move and the use are evaluated undefined?
bool EvaluationOrderUndefined;
+
+ // Does the use happen in a later loop iteration than the move?
+ bool UseHappensInLaterLoopIteration;
};
/// Finds uses of a variable after a move (and maintains state required by the
@@ -48,7 +52,7 @@ class UseAfterMoveFinder {
// use-after-move is found, writes information about it to 'TheUseAfterMove'.
// Returns whether a use-after-move was found.
bool find(Stmt *CodeBlock, const Expr *MovingCall,
- const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
+ const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
private:
bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
@@ -69,6 +73,30 @@ class UseAfterMoveFinder {
llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
};
+/// Returns whether the `Before` block can reach the `After` block.
+bool reaches(const CFGBlock *Before, const CFGBlock *After) {
+ llvm::SmallVector<const CFGBlock *> Stack;
+ llvm::SmallPtrSet<const CFGBlock *, 1> Visited;
+
+ Stack.push_back(Before);
+ while (!Stack.empty()) {
+ const CFGBlock *Current = Stack.back();
+ Stack.pop_back();
+
+ if (Current == After)
+ return true;
+
+ Visited.insert(Current);
+
+ for (const CFGBlock *Succ : Current->succs()) {
+ if (Succ && !Visited.contains(Succ))
+ Stack.push_back(Succ);
+ }
+ }
+
+ return false;
+}
+
} // namespace
// Matches nodes that are
@@ -89,7 +117,7 @@ UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
: Context(TheContext) {}
bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
- const ValueDecl *MovedVariable,
+ const DeclRefExpr *MovedVariable,
UseAfterMove *TheUseAfterMove) {
// Generate the CFG manually instead of through an AnalysisDeclContext because
// it seems the latter can't be used to generate a CFG for the body of a
@@ -110,15 +138,30 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
Visited.clear();
- const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall);
- if (!Block) {
+ const CFGBlock *MoveBlock = BlockMap->blockContainingStmt(MovingCall);
+ if (!MoveBlock) {
// This can happen if MovingCall is in a constructor initializer, which is
// not included in the CFG because the CFG is built only from the function
// body.
- Block = &TheCFG->getEntry();
+ MoveBlock = &TheCFG->getEntry();
}
- return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove);
+ bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
+ TheUseAfterMove);
+
+ if (Found) {
+ if (const CFGBlock *UseBlock =
+ BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef))
+ // Does the use happen in a later loop iteration than the move?
+ // - If they are in the same CFG block, we know the use happened in a
+ // later iteration if we visited that block a second time.
+ // - Otherwise, we know the use happened in a later iteration if the
+ // move is reachable from the use.
+ TheUseAfterMove->UseHappensInLaterLoopIteration =
+ UseBlock == MoveBlock ? Visited.contains(UseBlock)
+ : reaches(UseBlock, MoveBlock);
+ }
+ return Found;
}
bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
@@ -175,6 +218,10 @@ bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
MovingCall != nullptr &&
Sequence->potentiallyAfter(MovingCall, Use);
+ // We default to false here and change this to true if required in
+ // find().
+ TheUseAfterMove->UseHappensInLaterLoopIteration = false;
+
return true;
}
}
@@ -394,7 +441,7 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
"there is no guarantee about the order in which they are evaluated",
DiagnosticIDs::Note)
<< IsMove;
- } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
+ } else if (Use.UseHappensInLaterLoopIteration) {
Check->diag(UseLoc,
"the use happens in a later loop iteration than the "
"%select{forward|move}0",
@@ -495,7 +542,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
for (Stmt *CodeBlock : CodeBlocks) {
UseAfterMoveFinder Finder(Result.Context);
UseAfterMove Use;
- if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
+ if (Finder.find(CodeBlock, MovingCall, Arg, &Use))
emitDiagnostic(MovingCall, Arg, Use, this, Result.Context,
determineMoveType(MoveDecl));
}
diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
index 50df451ecfa26..d8a8486009dc3 100644
--- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -55,12 +55,23 @@ bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
ASTContext *Context) {
if (Descendant == Ancestor)
return true;
- for (const Stmt *Parent : getParentStmts(Descendant, Context)) {
- if (isDescendantOrEqual(Parent, Ancestor, Context))
- return true;
- }
+ return llvm::any_of(getParentStmts(Descendant, Context),
+ [Ancestor, Context](const Stmt *Parent) {
+ return isDescendantOrEqual(Parent, Ancestor, Context);
+ });
+}
- return false;
+bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call,
+ ASTContext *Context) {
+ return llvm::any_of(Call->arguments(),
+ [Descendant, Context](const Expr *Arg) {
+ return isDescendantOrEqual(Descendant, Arg, Context);
+ });
+}
+
+bool argsContain(const CallExpr *Call, const Stmt *TheStmt) {
+ return std::find(Call->arguments().begin(), Call->arguments().end(),
+ TheStmt) != Call->arguments().end();
}
llvm::SmallVector<const InitListExpr *>
@@ -95,9 +106,59 @@ bool ExprSequence::inSequence(const Stmt *Before, const Stmt *After) const {
return true;
}
+ SmallVector<const Stmt *, 1> BeforeParents = getParentStmts(Before, Context);
+
+ // Since C++17, the callee of a call expression is guaranteed to be sequenced
+ // before all of the arguments.
+ // We handle this as a special case rather than using the general
+ // `getSequenceSuccessor` logic above because the callee expression doesn't
+ // have an unambiguous successor; the order in which arguments are evaluated
+ // is indeterminate.
+ for (const Stmt *Parent : BeforeParents) {
+ // Special case: If the callee is a `MemberExpr` with a `DeclRefExpr` as its
+ // base, we consider it to be sequenced _after_ the arguments. This is
+ // because the variable referenced in the base will only actually be
+ // accessed when the call happens, i.e. once all of the arguments have been
+ // evaluated. This has no basis in the C++ standard, but it reflects actual
+ // behavior that is relevant to a use-after-move scenario:
+ //
+ // ```
+ // a.bar(consumeA(std::move(a));
+ // ```
+ //
+ // In this example, we end up accessing `a` after it has been moved from,
+ // even though nominally the callee `a.bar` is evaluated before the argument
+ // `consumeA(std::move(a))`. Note that this is not specific to C++17, so
+ // we implement this logic unconditionally.
+ if (const auto *call = dyn_cast<CXXMemberCallExpr>(Parent)) {
+ if (argsContain(call, Before) &&
+ isa<DeclRefExpr>(
+ call->getImplicitObjectArgument()->IgnoreParenImpCasts()) &&
+ isDescendantOrEqual(After, call->getImplicitObjectArgument(),
+ Context))
+ return true;
+
+ // We need this additional early exit so that we don't fall through to the
+ // more general logic below.
+ if (auto *Member = dyn_cast<MemberExpr>(Before);
+ Member && call->getCallee() == Member &&
+ isa<DeclRefExpr>(Member->getBase()->IgnoreParenImpCasts()) &&
+ isDescendantOfArgs(After, call, Context))
+ return false;
+ }
+
+ if (!Context->getLangOpts().CPlusPlus17)
+ continue;
+
+ if (const auto *call = dyn_cast<CallExpr>(Parent);
+ call && call->getCallee() == Before &&
+ isDescendantOfArgs(After, call, Context))
+ return true;
+ }
+
// If 'After' is a parent of 'Before' or is sequenced after one of these
// parents, we know that it is sequenced after 'Before'.
- for (const Stmt *Parent : getParentStmts(Before, Context)) {
+ for (const Stmt *Parent : BeforeParents) {
if (Parent == After || inSequence(Parent, After))
return true;
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3e3195f6f6813..96ffe1133a296 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -241,6 +241,12 @@ Changes in existing checks
function with the same prefix as the default argument, e.g. ``std::unique_ptr``
and ``std::unique``, avoiding false positive for assignment operator overloading.
+- Improved :doc:`bugprone-use-after-move
+ <clang-tidy/checks/bugprone/use-after-move>`: fixed sequencing of designated
+ initializers. Fixed sequencing of callees: In C++17 and later, the callee of
+ a function is guaranteed to be sequenced before the arguments, so don't warn
+ if the use happens in the callee and the move happens in one of the arguments.
+
- Improved :doc:`bugprone-use-after-move
<clang-tidy/checks/bugprone/use-after-move>` check to also handle
calls to ``std::forward``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index 7d9f63479a1b4..6a4e3990e36dc 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -1,3 +1,4 @@
+// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
typedef decltype(nullptr) nullptr_t;
@@ -135,6 +136,7 @@ class A {
A &operator=(A &&);
void foo() const;
+ void bar(int i) const;
int getInt() const;
operator bool() const;
@@ -576,6 +578,19 @@ void useAndMoveInLoop() {
std::move(a);
}
}
+ // Same as above, but the use and the move are in different CFG blocks.
+ {
+ A a;
+ for (int i = 0; i < 10; ++i) {
+ if (i < 10)
+ a.foo();
+ // CHECK-NOTES: [[@LINE-1]]:9: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here
+ // CHECK-NOTES: [[@LINE-3]]:9: note: the use happens in a later loop
+ if (i < 10)
+ std::move(a);
+ }
+ }
// However, this case shouldn't be flagged -- the scope of the declaration of
// 'a' is important.
{
@@ -1352,6 +1367,40 @@ void ifWhileAndSwitchSequenceInitDeclAndCondition() {
}
}
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments -- but only in C++17 and later.
+namespace CalleeSequencedBeforeArguments {
+int consumeA(std::unique_ptr<A> a);
+int consumeA(A &&a);
+
+void calleeSequencedBeforeArguments() {
+ {
+ std::unique_ptr<A> a;
+ a->bar(consumeA(std::move(a)));
+ // CHECK-NOTES-CXX11: [[@LINE-1]]:5: warning: 'a' used after it was moved
+ // CHECK-NOTES-CXX11: [[@LINE-2]]:21: note: move occurred here
+ // CHECK-NOTES-CXX11: [[@LINE-3]]:5: note: the use and move are unsequenced
+ }
+ {
+ std::unique_ptr<A> a;
+ std::unique_ptr<A> getArg(std::unique_ptr<A> a);
+ getArg(std::move(a))->bar(a->getInt());
+ // CHECK-NOTES: [[@LINE-1]]:31: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:12: note: move occurred here
+ // CHECK-NOTES-CXX11: [[@LINE-3]]:31: note: the use and move are unsequenced
+ }
+ {
+ A a;
+ // Nominally, the callee `a.bar` is evaluated before the argument
+ // `consumeA(std::move(a))`, but in effect `a` is only accessed after the
+ // call to `A::bar()` happens, i.e. after the argument has been evaluted.
+ a.bar(consumeA(std::move(a)));
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+ }
+}
+} // namespace CalleeSequencedBeforeArguments
+
// Some statements in templates (e.g. null, break and continue statements) may
// be shared between the uninstantiated and instantiated versions of the
// template and therefore have multiple parents. Make sure the sequencing code
@@ -1469,7 +1518,6 @@ class CtorInitOrder {
// CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
s{std::move(val)} {} // wrong order
// CHECK-NOTES: [[@LINE-1]]:9: note: move occurred here
- // CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop iteration than the move
private:
bool a;
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Kefu Chai (tchaikov) ChangesThis eliminates false positives in bugprone-use-after-move where a variable 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 Full diff: https://github.com/llvm/llvm-project/pull/93623.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..f357b21b3a967 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -14,6 +14,7 @@
#include "clang/Analysis/CFG.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "../utils/ExprSequence.h"
#include "../utils/Matchers.h"
@@ -35,6 +36,9 @@ struct UseAfterMove {
// Is the order in which the move and the use are evaluated undefined?
bool EvaluationOrderUndefined;
+
+ // Does the use happen in a later loop iteration than the move?
+ bool UseHappensInLaterLoopIteration;
};
/// Finds uses of a variable after a move (and maintains state required by the
@@ -48,7 +52,7 @@ class UseAfterMoveFinder {
// use-after-move is found, writes information about it to 'TheUseAfterMove'.
// Returns whether a use-after-move was found.
bool find(Stmt *CodeBlock, const Expr *MovingCall,
- const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
+ const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
private:
bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
@@ -69,6 +73,30 @@ class UseAfterMoveFinder {
llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
};
+/// Returns whether the `Before` block can reach the `After` block.
+bool reaches(const CFGBlock *Before, const CFGBlock *After) {
+ llvm::SmallVector<const CFGBlock *> Stack;
+ llvm::SmallPtrSet<const CFGBlock *, 1> Visited;
+
+ Stack.push_back(Before);
+ while (!Stack.empty()) {
+ const CFGBlock *Current = Stack.back();
+ Stack.pop_back();
+
+ if (Current == After)
+ return true;
+
+ Visited.insert(Current);
+
+ for (const CFGBlock *Succ : Current->succs()) {
+ if (Succ && !Visited.contains(Succ))
+ Stack.push_back(Succ);
+ }
+ }
+
+ return false;
+}
+
} // namespace
// Matches nodes that are
@@ -89,7 +117,7 @@ UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
: Context(TheContext) {}
bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
- const ValueDecl *MovedVariable,
+ const DeclRefExpr *MovedVariable,
UseAfterMove *TheUseAfterMove) {
// Generate the CFG manually instead of through an AnalysisDeclContext because
// it seems the latter can't be used to generate a CFG for the body of a
@@ -110,15 +138,30 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
Visited.clear();
- const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall);
- if (!Block) {
+ const CFGBlock *MoveBlock = BlockMap->blockContainingStmt(MovingCall);
+ if (!MoveBlock) {
// This can happen if MovingCall is in a constructor initializer, which is
// not included in the CFG because the CFG is built only from the function
// body.
- Block = &TheCFG->getEntry();
+ MoveBlock = &TheCFG->getEntry();
}
- return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove);
+ bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
+ TheUseAfterMove);
+
+ if (Found) {
+ if (const CFGBlock *UseBlock =
+ BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef))
+ // Does the use happen in a later loop iteration than the move?
+ // - If they are in the same CFG block, we know the use happened in a
+ // later iteration if we visited that block a second time.
+ // - Otherwise, we know the use happened in a later iteration if the
+ // move is reachable from the use.
+ TheUseAfterMove->UseHappensInLaterLoopIteration =
+ UseBlock == MoveBlock ? Visited.contains(UseBlock)
+ : reaches(UseBlock, MoveBlock);
+ }
+ return Found;
}
bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
@@ -175,6 +218,10 @@ bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
MovingCall != nullptr &&
Sequence->potentiallyAfter(MovingCall, Use);
+ // We default to false here and change this to true if required in
+ // find().
+ TheUseAfterMove->UseHappensInLaterLoopIteration = false;
+
return true;
}
}
@@ -394,7 +441,7 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
"there is no guarantee about the order in which they are evaluated",
DiagnosticIDs::Note)
<< IsMove;
- } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
+ } else if (Use.UseHappensInLaterLoopIteration) {
Check->diag(UseLoc,
"the use happens in a later loop iteration than the "
"%select{forward|move}0",
@@ -495,7 +542,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
for (Stmt *CodeBlock : CodeBlocks) {
UseAfterMoveFinder Finder(Result.Context);
UseAfterMove Use;
- if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
+ if (Finder.find(CodeBlock, MovingCall, Arg, &Use))
emitDiagnostic(MovingCall, Arg, Use, this, Result.Context,
determineMoveType(MoveDecl));
}
diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
index 50df451ecfa26..d8a8486009dc3 100644
--- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -55,12 +55,23 @@ bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
ASTContext *Context) {
if (Descendant == Ancestor)
return true;
- for (const Stmt *Parent : getParentStmts(Descendant, Context)) {
- if (isDescendantOrEqual(Parent, Ancestor, Context))
- return true;
- }
+ return llvm::any_of(getParentStmts(Descendant, Context),
+ [Ancestor, Context](const Stmt *Parent) {
+ return isDescendantOrEqual(Parent, Ancestor, Context);
+ });
+}
- return false;
+bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call,
+ ASTContext *Context) {
+ return llvm::any_of(Call->arguments(),
+ [Descendant, Context](const Expr *Arg) {
+ return isDescendantOrEqual(Descendant, Arg, Context);
+ });
+}
+
+bool argsContain(const CallExpr *Call, const Stmt *TheStmt) {
+ return std::find(Call->arguments().begin(), Call->arguments().end(),
+ TheStmt) != Call->arguments().end();
}
llvm::SmallVector<const InitListExpr *>
@@ -95,9 +106,59 @@ bool ExprSequence::inSequence(const Stmt *Before, const Stmt *After) const {
return true;
}
+ SmallVector<const Stmt *, 1> BeforeParents = getParentStmts(Before, Context);
+
+ // Since C++17, the callee of a call expression is guaranteed to be sequenced
+ // before all of the arguments.
+ // We handle this as a special case rather than using the general
+ // `getSequenceSuccessor` logic above because the callee expression doesn't
+ // have an unambiguous successor; the order in which arguments are evaluated
+ // is indeterminate.
+ for (const Stmt *Parent : BeforeParents) {
+ // Special case: If the callee is a `MemberExpr` with a `DeclRefExpr` as its
+ // base, we consider it to be sequenced _after_ the arguments. This is
+ // because the variable referenced in the base will only actually be
+ // accessed when the call happens, i.e. once all of the arguments have been
+ // evaluated. This has no basis in the C++ standard, but it reflects actual
+ // behavior that is relevant to a use-after-move scenario:
+ //
+ // ```
+ // a.bar(consumeA(std::move(a));
+ // ```
+ //
+ // In this example, we end up accessing `a` after it has been moved from,
+ // even though nominally the callee `a.bar` is evaluated before the argument
+ // `consumeA(std::move(a))`. Note that this is not specific to C++17, so
+ // we implement this logic unconditionally.
+ if (const auto *call = dyn_cast<CXXMemberCallExpr>(Parent)) {
+ if (argsContain(call, Before) &&
+ isa<DeclRefExpr>(
+ call->getImplicitObjectArgument()->IgnoreParenImpCasts()) &&
+ isDescendantOrEqual(After, call->getImplicitObjectArgument(),
+ Context))
+ return true;
+
+ // We need this additional early exit so that we don't fall through to the
+ // more general logic below.
+ if (auto *Member = dyn_cast<MemberExpr>(Before);
+ Member && call->getCallee() == Member &&
+ isa<DeclRefExpr>(Member->getBase()->IgnoreParenImpCasts()) &&
+ isDescendantOfArgs(After, call, Context))
+ return false;
+ }
+
+ if (!Context->getLangOpts().CPlusPlus17)
+ continue;
+
+ if (const auto *call = dyn_cast<CallExpr>(Parent);
+ call && call->getCallee() == Before &&
+ isDescendantOfArgs(After, call, Context))
+ return true;
+ }
+
// If 'After' is a parent of 'Before' or is sequenced after one of these
// parents, we know that it is sequenced after 'Before'.
- for (const Stmt *Parent : getParentStmts(Before, Context)) {
+ for (const Stmt *Parent : BeforeParents) {
if (Parent == After || inSequence(Parent, After))
return true;
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3e3195f6f6813..96ffe1133a296 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -241,6 +241,12 @@ Changes in existing checks
function with the same prefix as the default argument, e.g. ``std::unique_ptr``
and ``std::unique``, avoiding false positive for assignment operator overloading.
+- Improved :doc:`bugprone-use-after-move
+ <clang-tidy/checks/bugprone/use-after-move>`: fixed sequencing of designated
+ initializers. Fixed sequencing of callees: In C++17 and later, the callee of
+ a function is guaranteed to be sequenced before the arguments, so don't warn
+ if the use happens in the callee and the move happens in one of the arguments.
+
- Improved :doc:`bugprone-use-after-move
<clang-tidy/checks/bugprone/use-after-move>` check to also handle
calls to ``std::forward``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index 7d9f63479a1b4..6a4e3990e36dc 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -1,3 +1,4 @@
+// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
typedef decltype(nullptr) nullptr_t;
@@ -135,6 +136,7 @@ class A {
A &operator=(A &&);
void foo() const;
+ void bar(int i) const;
int getInt() const;
operator bool() const;
@@ -576,6 +578,19 @@ void useAndMoveInLoop() {
std::move(a);
}
}
+ // Same as above, but the use and the move are in different CFG blocks.
+ {
+ A a;
+ for (int i = 0; i < 10; ++i) {
+ if (i < 10)
+ a.foo();
+ // CHECK-NOTES: [[@LINE-1]]:9: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here
+ // CHECK-NOTES: [[@LINE-3]]:9: note: the use happens in a later loop
+ if (i < 10)
+ std::move(a);
+ }
+ }
// However, this case shouldn't be flagged -- the scope of the declaration of
// 'a' is important.
{
@@ -1352,6 +1367,40 @@ void ifWhileAndSwitchSequenceInitDeclAndCondition() {
}
}
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments -- but only in C++17 and later.
+namespace CalleeSequencedBeforeArguments {
+int consumeA(std::unique_ptr<A> a);
+int consumeA(A &&a);
+
+void calleeSequencedBeforeArguments() {
+ {
+ std::unique_ptr<A> a;
+ a->bar(consumeA(std::move(a)));
+ // CHECK-NOTES-CXX11: [[@LINE-1]]:5: warning: 'a' used after it was moved
+ // CHECK-NOTES-CXX11: [[@LINE-2]]:21: note: move occurred here
+ // CHECK-NOTES-CXX11: [[@LINE-3]]:5: note: the use and move are unsequenced
+ }
+ {
+ std::unique_ptr<A> a;
+ std::unique_ptr<A> getArg(std::unique_ptr<A> a);
+ getArg(std::move(a))->bar(a->getInt());
+ // CHECK-NOTES: [[@LINE-1]]:31: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:12: note: move occurred here
+ // CHECK-NOTES-CXX11: [[@LINE-3]]:31: note: the use and move are unsequenced
+ }
+ {
+ A a;
+ // Nominally, the callee `a.bar` is evaluated before the argument
+ // `consumeA(std::move(a))`, but in effect `a` is only accessed after the
+ // call to `A::bar()` happens, i.e. after the argument has been evaluted.
+ a.bar(consumeA(std::move(a)));
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+ }
+}
+} // namespace CalleeSequencedBeforeArguments
+
// Some statements in templates (e.g. null, break and continue statements) may
// be shared between the uninstantiated and instantiated versions of the
// template and therefore have multiple parents. Make sure the sequencing code
@@ -1469,7 +1518,6 @@ class CtorInitOrder {
// CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
s{std::move(val)} {} // wrong order
// CHECK-NOTES: [[@LINE-1]]:9: note: move occurred here
- // CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop iteration than the move
private:
bool a;
|
|
@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. |
EugeneZelenko
requested review from
PiotrZSL,
SimplyDanny,
5chmidti and
HerrCai0907
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
from
3f1ef81 to
e9e03e8
Compare
|
v2:
|
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
from
e9e03e8 to
14106f8
Compare
| // We default to false here and change this to true if required in | ||
| // find(). | ||
| TheUseAfterMove->UseHappensInLaterLoopIteration = false; | ||
|
|
There was a problem hiding this comment.
The initialization could also happen in the struct itself (we should initialize EvaluationOrderUndefined in the struct as well).
There was a problem hiding this comment.
agreed. but practically, we reference EvaluationOrderUndefined only if UseAfterMoveFinder::find() returns true, which indicates a use-after-move is identified. see
llvm-project/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
Lines 498 to 501 in 4d95850
true in UseAfterMoveFinder::findInternal() are
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<UseAfterMove> 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 instead in a follow-up change, i will do it.
There was a problem hiding this comment.
after a second thought, i am
- removing this initialization, and
- just set the default value in the
UseAfterMovestruct, and - move the accompanied comment there.
for better readability. but the optional<> refactor is still a nice-to-have, IMHO.
There was a problem hiding this comment.
for better readability. but the
optional<>refactor is still a nice-to-have, IMHO.
created #98100
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
from
14106f8 to
81df014
Compare
|
v3:
|
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
from
81df014 to
cb1dfa3
Compare
|
@5chmidti Hi Julian, thanks for your thoughtful review and suggestions! |
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
from
cb1dfa3 to
00df701
Compare
… 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 llvm#57758 Fixes llvm#59612
tchaikov
force-pushed
the
bugprone-use-after-move-sequenced
branch
from
00df701 to
e07681e
Compare
|
v4:
|
|
@5chmidti hi Julian, thank you for your review, suggestions and approval. updated accordingly. |
|
@PiotrZSL @SimplyDanny and @HerrCai0907 Hello, gentlemen. Would you be available to take a look at this at your earliest convenience? |
|
@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. |
|
Normally PR can be merged if someone approved. You can merge it by yourself. |
|
@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 merge this pull request for me. |
|
@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 Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
thank you Piotr for merging this change. |
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:
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