llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: mitchell (zeyi2) <details> <summary>Changes</summary> This PR is not ready for review yet. TODOs: - Add documentation - Enhance testcases - Code cleanup --- Full diff: https://github.com/llvm/llvm-project/pull/173192.diff 2 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp (+69-1) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp (+109) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index 98b0202a87f2d..cd4bf400d3985 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -8,8 +8,10 @@ #include "UseAfterMoveCheck.h" +#include "clang/AST/DeclCXX.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/LambdaCapture.h" #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h" #include "clang/Analysis/CFG.h" @@ -79,7 +81,19 @@ class UseAfterMoveFinder { llvm::SmallPtrSet<const CFGBlock *, 8> Visited; }; -} // namespace +AST_MATCHER_P(CXXRecordDecl, hasCaptureByReference, const ValueDecl *, + TargetDecl) { + if (!Node.isLambda()) + return false; + + for (const auto &Capture : Node.captures()) { + if (Capture.capturesVariable() && Capture.getCaptureKind() == LCK_ByRef && + Capture.getCapturedVar() == TargetDecl) { + return true; + } + } + return false; +} static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) { return anyOf(hasAnyName("::std::move", "::std::forward"), @@ -150,6 +164,29 @@ makeReinitMatcher(const ValueDecl *MovedVariable, .bind("reinit"); } +bool isVariableResetInLambda(const Stmt *Body, const ValueDecl *MovedVariable, + ASTContext *Context, + llvm::ArrayRef<StringRef> InvalidationFunctions) { + if (!Body) + return false; + + // If the variable is not mentioned at all in the lambda body, + // it cannot be reinitialized. + const auto VariableMentionMatcher = stmt(anyOf( + hasDescendant(declRefExpr(hasDeclaration(equalsNode(MovedVariable)))), + hasDescendant(memberExpr(hasDeclaration(equalsNode(MovedVariable)))))); + + if (match(VariableMentionMatcher, *Body, *Context).empty()) + return false; + + const auto ReinitMatcher = + makeReinitMatcher(MovedVariable, InvalidationFunctions); + + return !match(findAll(ReinitMatcher), *Body, *Context).empty(); +} + +} // namespace + // Matches nodes that are // - Part of a decltype argument or class template argument (we check this by // seeing if they are children of a TypeLoc), or @@ -374,6 +411,13 @@ void UseAfterMoveFinder::getReinits( const auto ReinitMatcher = makeReinitMatcher(MovedVariable, InvalidationFunctions); + // Match calls to lambdas that capture the moved variable by reference. + const auto LambdaCallMatcher = + cxxOperatorCallExpr( + hasOverloadedOperatorName("()"), + callee(cxxMethodDecl(ofClass(hasCaptureByReference(MovedVariable))))) + .bind("lambda-call"); + Stmts->clear(); DeclRefs->clear(); for (const auto &Elem : *Block) { @@ -397,6 +441,30 @@ void UseAfterMoveFinder::getReinits( DeclRefs->insert(TheDeclRef); } } + + // Check for calls to lambdas that capture the moved variable + // by reference and reinitialize it within their body. + const SmallVector<BoundNodes, 1> LambdaMatches = + match(findAll(LambdaCallMatcher), *S->getStmt(), *Context); + + for (const auto &Match : LambdaMatches) { + const auto *Operator = + Match.getNodeAs<CXXOperatorCallExpr>("lambda-call"); + + if (Operator && BlockMap->blockContainingStmt(Operator) == Block) { + const auto *MD = + dyn_cast_or_null<CXXMethodDecl>(Operator->getDirectCallee()); + if (!MD) + continue; + + const auto *RD = MD->getParent(); + const auto *LambdaBody = MD->getBody(); + if (RD && RD->isLambda() && LambdaBody && + isVariableResetInLambda(LambdaBody, MovedVariable, Context, + InvalidationFunctions)) + Stmts->insert(Operator); + } + } } } 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 b2df2638106e0..b1ed032cbb27b 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 @@ -1703,3 +1703,112 @@ void Run() { } } // namespace custom_invalidation + +void lambdaReinit() { + { + std::string s; + auto g = [&]() { + s = std::string(); + return true; + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + if (!s.empty()) { + std::move(s); + } + } + } + } + + { + std::string s; + auto g = [&]() { + s.clear(); + return true; + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + if (!s.empty()) { + std::move(s); + } + } + } + } + + { + std::string s; + auto g = [&]() { + s.assign(10, 'a'); + return true; + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + if (!s.empty()) { + std::move(s); + } + } + } + } + + { + std::string s; + auto g = [&]() { + return !s.empty(); + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + std::move(s); + // CHECK-NOTES: [[@LINE-1]]:19: warning: 's' used after it was moved + // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here + // CHECK-NOTES: [[@LINE-3]]:19: note: the use happens in a later loop + } + } + } + + { + std::string s; + auto g = [s]() mutable { + s = std::string(); + return true; + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + std::move(s); + // CHECK-NOTES: [[@LINE-1]]:19: warning: 's' used after it was moved + // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here + // CHECK-NOTES: [[@LINE-3]]:19: note: the use happens in a later loop + } + } + } + + { + std::string s; + while ([&]() { s = std::string(); return true; }()) { + // CHECK-NOTES: [[@LINE-1]]:13: warning: 's' used after it was moved + // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here + // CHECK-NOTES: [[@LINE-3]]:13: note: the use happens in a later loop + if (!s.empty()) { + std::move(s); + } + } + } +} + +void lambdaReinitInDeadCode() { + // FIXME: This is a known False Negative. The check currently + // lacks the ability to do control flow analysis. + { + std::string s; + auto g = [&]() { + if (false) { + s = std::string(); + } + }; + std::move(s); + + g(); + + s.clear(); + // CHECK-NOTES-NOT: [[@LINE-2]]:5: warning: 's' used after it was moved + } +} `````````` </details> https://github.com/llvm/llvm-project/pull/173192 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
