[clang-tools-extra] 8c10256 - clang-tidy: Detect use-after-move in CXXCtorInitializer
via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 23 02:23:57 PDT 2023
Author: MarcoFalke
Date: 2023-03-23T10:19:54+01:00
New Revision: 8c10256734cd47274671fcabe94f24f15ecd6209
URL: https://github.com/llvm/llvm-project/commit/8c10256734cd47274671fcabe94f24f15ecd6209
DIFF: https://github.com/llvm/llvm-project/commit/8c10256734cd47274671fcabe94f24f15ecd6209.diff
LOG: clang-tidy: Detect use-after-move in CXXCtorInitializer
Fixes https://github.com/llvm/llvm-project/issues/51844
Differential Revision: https://reviews.llvm.org/D146288
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b7eadb87b4fcd..c10c3652a153a 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -58,11 +58,11 @@ class UseAfterMoveFinder {
public:
UseAfterMoveFinder(ASTContext *TheContext);
- // Within the given function body, finds the first use of 'MovedVariable' that
+ // Within the given code block, finds the first use of 'MovedVariable' that
// occurs after 'MovingCall' (the expression that performs the move). If a
// use-after-move is found, writes information about it to 'TheUseAfterMove'.
// Returns whether a use-after-move was found.
- bool find(Stmt *FunctionBody, const Expr *MovingCall,
+ bool find(Stmt *CodeBlock, const Expr *MovingCall,
const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
private:
@@ -104,7 +104,7 @@ static StatementMatcher inDecltypeOrTemplateArg() {
UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
: Context(TheContext) {}
-bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall,
+bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
const ValueDecl *MovedVariable,
UseAfterMove *TheUseAfterMove) {
// Generate the CFG manually instead of through an AnalysisDeclContext because
@@ -118,12 +118,11 @@ bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall,
Options.AddImplicitDtors = true;
Options.AddTemporaryDtors = true;
std::unique_ptr<CFG> TheCFG =
- CFG::buildCFG(nullptr, FunctionBody, Context, Options);
+ CFG::buildCFG(nullptr, CodeBlock, Context, Options);
if (!TheCFG)
return false;
- Sequence =
- std::make_unique<ExprSequence>(TheCFG.get(), FunctionBody, Context);
+ Sequence = std::make_unique<ExprSequence>(TheCFG.get(), CodeBlock, Context);
BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
Visited.clear();
@@ -398,20 +397,28 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
}
void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
+ // try_emplace is a common maybe-moving function that returns a
+ // bool to tell callers whether it moved. Ignore std::move inside
+ // try_emplace to avoid false positives as we don't track uses of
+ // the bool.
+ auto TryEmplaceMatcher =
+ cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
auto CallMoveMatcher =
- callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
+ callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))),
hasArgument(0, declRefExpr().bind("arg")),
+ unless(inDecltypeOrTemplateArg()),
+ unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"),
anyOf(hasAncestor(compoundStmt(
hasParent(lambdaExpr().bind("containing-lambda")))),
- hasAncestor(functionDecl().bind("containing-func"))),
- unless(inDecltypeOrTemplateArg()),
- // try_emplace is a common maybe-moving function that returns a
- // bool to tell callers whether it moved. Ignore std::move inside
- // try_emplace to avoid false positives as we don't track uses of
- // the bool.
- unless(hasParent(cxxMemberCallExpr(
- callee(cxxMethodDecl(hasName("try_emplace")))))))
- .bind("call-move");
+ hasAncestor(functionDecl(anyOf(
+ cxxConstructorDecl(
+ hasAnyConstructorInitializer(withInitializer(
+ expr(anyOf(equalsBoundNode("call-move"),
+ hasDescendant(expr(
+ equalsBoundNode("call-move")))))
+ .bind("containing-ctor-init"))))
+ .bind("containing-ctor"),
+ functionDecl().bind("containing-func"))))));
Finder->addMatcher(
traverse(
@@ -434,6 +441,10 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
}
void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *ContainingCtor =
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("containing-ctor");
+ const auto *ContainingCtorInit =
+ Result.Nodes.getNodeAs<Expr>("containing-ctor-init");
const auto *ContainingLambda =
Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda");
const auto *ContainingFunc =
@@ -445,23 +456,38 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
if (!MovingCall || !MovingCall->getExprLoc().isValid())
MovingCall = CallMove;
- Stmt *FunctionBody = nullptr;
- if (ContainingLambda)
- FunctionBody = ContainingLambda->getBody();
- else if (ContainingFunc)
- FunctionBody = ContainingFunc->getBody();
- else
- return;
-
// Ignore the std::move if the variable that was passed to it isn't a local
// variable.
if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod())
return;
- UseAfterMoveFinder Finder(Result.Context);
- UseAfterMove Use;
- if (Finder.find(FunctionBody, MovingCall, Arg->getDecl(), &Use))
- emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
+ // Collect all code blocks that could use the arg after move.
+ llvm::SmallVector<Stmt *> CodeBlocks{};
+ if (ContainingCtor) {
+ CodeBlocks.push_back(ContainingCtor->getBody());
+ if (ContainingCtorInit) {
+ // Collect the constructor initializer expressions.
+ bool BeforeMove{true};
+ for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
+ if (BeforeMove && Init->getInit()->IgnoreImplicit() ==
+ ContainingCtorInit->IgnoreImplicit())
+ BeforeMove = false;
+ if (!BeforeMove)
+ CodeBlocks.push_back(Init->getInit());
+ }
+ }
+ } else if (ContainingLambda) {
+ CodeBlocks.push_back(ContainingLambda->getBody());
+ } else if (ContainingFunc) {
+ CodeBlocks.push_back(ContainingFunc->getBody());
+ }
+
+ for (Stmt *CodeBlock : CodeBlocks) {
+ UseAfterMoveFinder Finder(Result.Context);
+ UseAfterMove Use;
+ if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
+ emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
+ }
}
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 80f5b46681713..89419141cebbd 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -162,6 +162,10 @@ Changes in existing checks
<clang-tidy/checks/bugprone/suspicious-include>` check.
Global options of the same name should be used instead.
+- Improved :doc:`bugprone-use-after-move
+ <clang-tidy/checks/bugprone/use-after-move>` check to also cover constructor
+ initializers.
+
- Deprecated check-local options `HeaderFileExtensions`
in :doc:`google-build-namespaces
<clang-tidy/checks/google/build-namespaces>` check.
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 45cef8abfd1f6..1e0831048dbd4 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
@@ -369,6 +369,18 @@ void lambdas() {
};
a.foo();
}
+ // Don't warn if 'a' is a copy inside a synchronous lambda
+ {
+ A a;
+ A copied{[a] mutable { return std::move(a); }()};
+ a.foo();
+ }
+ // False negative (should warn if 'a' is a ref inside a synchronous lambda)
+ {
+ A a;
+ A moved{[&a] mutable { return std::move(a); }()};
+ a.foo();
+ }
// Warn if the use consists of a capture that happens after a move.
{
A a;
@@ -1367,6 +1379,120 @@ void typeId() {
}
} // namespace UnevalContext
+class CtorInit {
+public:
+ CtorInit(std::string val)
+ : a{val.empty()}, // fine
+ s{std::move(val)},
+ b{val.empty()}
+ // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:9: note: move occurred here
+ {}
+
+private:
+ bool a;
+ std::string s;
+ bool b;
+};
+
+class CtorInitLambda {
+public:
+ CtorInitLambda(std::string val)
+ : a{val.empty()}, // fine
+ s{std::move(val)},
+ b{[&] { return val.empty(); }()},
+ // CHECK-NOTES: [[@LINE-1]]:12: warning: 'val' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:9: note: move occurred here
+ c{[] {
+ std::string str{};
+ std::move(str);
+ return str.empty();
+ // CHECK-NOTES: [[@LINE-1]]:18: warning: 'str' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
+ }()} {
+ std::move(val);
+ // CHECK-NOTES: [[@LINE-1]]:15: warning: 'val' used after it was moved
+ // CHECK-NOTES: [[@LINE-13]]:9: note: move occurred here
+ std::string val2{};
+ std::move(val2);
+ val2.empty();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'val2' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+ }
+
+private:
+ bool a;
+ std::string s;
+ bool b;
+ bool c;
+ bool d{};
+};
+
+class CtorInitOrder {
+public:
+ CtorInitOrder(std::string val)
+ : a{val.empty()}, // fine
+ b{val.empty()},
+ // 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;
+ std::string s;
+ bool b;
+};
+
+struct Obj {};
+struct CtorD {
+ CtorD(Obj b);
+};
+
+struct CtorC {
+ CtorC(Obj b);
+};
+
+struct CtorB {
+ CtorB(Obj &b);
+};
+
+struct CtorA : CtorB, CtorC, CtorD {
+ CtorA(Obj b) : CtorB{b}, CtorC{std::move(b)}, CtorD{b} {}
+ // CHECK-NOTES: [[@LINE-1]]:55: warning: 'b' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:34: note: move occurred here
+};
+
+struct Base {
+ Base(Obj b) : bb{std::move(b)} {}
+ template <typename Call> Base(Call &&c) : bb{c()} {};
+
+ Obj bb;
+};
+
+struct Derived : Base, CtorC {
+ Derived(Obj b)
+ : Base{[&] mutable { return std::move(b); }()},
+ // False negative: The lambda/std::move was executed, so it should warn
+ // below
+ CtorC{b} {}
+};
+
+struct Derived2 : Base, CtorC {
+ Derived2(Obj b)
+ : Base{[&] mutable { return std::move(b); }},
+ // This was a move, but it doesn't warn below, because it can't know if
+ // the lambda/std::move was actually called
+ CtorC{b} {}
+};
+
+struct Derived3 : Base, CtorC {
+ Derived3(Obj b)
+ : Base{[c = std::move(b)] mutable { return std::move(c); }}, CtorC{b} {}
+ // CHECK-NOTES: [[@LINE-1]]:74: warning: 'b' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:19: note: move occurred here
+};
+
class PR38187 {
public:
PR38187(std::string val) : val_(std::move(val)) {
More information about the cfe-commits
mailing list