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)) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
