Author: Congcong Cai Date: 2024-04-15T19:38:50+08:00 New Revision: 8095b9ce6bf5831a14c72028920708f38d13d0c3
URL: https://github.com/llvm/llvm-project/commit/8095b9ce6bf5831a14c72028920708f38d13d0c3 DIFF: https://github.com/llvm/llvm-project/commit/8095b9ce6bf5831a14c72028920708f38d13d0c3.diff LOG: [clang analysis] ExprMutationAnalyzer avoid infinite recursion for recursive forwarding reference (#87954) Added: Modified: clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h clang/lib/Analysis/ExprMutationAnalyzer.cpp clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 1405fb0df1f8dd..de0a7b79bb2cf0 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -221,6 +221,10 @@ Changes in existing checks <clang-tidy/checks/llvm/header-guard>` check by replacing the local option `HeaderFileExtensions` by the global option of the same name. +- Improved :doc:`misc-const-correctness + <clang-tidy/checks/misc/const-correctness>` check by avoiding infinite recursion + for recursive forwarding reference. + - Improved :doc:`misc-definitions-in-headers <clang-tidy/checks/misc/definitions-in-headers>` check by replacing the local option `HeaderFileExtensions` by the global option of the same name. diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp index 9da468128743e9..248374a71dd40b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp @@ -58,3 +58,18 @@ void concatenate3(Args... args) (..., (stream << args)); } } // namespace gh70323 + +namespace gh60895 { + +template <class T> void f1(T &&a); +template <class T> void f2(T &&a); +template <class T> void f1(T &&a) { f2<T>(a); } +template <class T> void f2(T &&a) { f1<T>(a); } +void f() { + int x = 0; + // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'x' of type 'int' can be declared 'const' + // CHECK-FIXES: int const x = 0; + f1(x); +} + +} // namespace gh60895 diff --git a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h index 1ceef944fbc34e..c4e5d0badb8e58 100644 --- a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h +++ b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h @@ -8,11 +8,10 @@ #ifndef LLVM_CLANG_ANALYSIS_ANALYSES_EXPRMUTATIONANALYZER_H #define LLVM_CLANG_ANALYSIS_ANALYSES_EXPRMUTATIONANALYZER_H -#include <type_traits> - #include "clang/AST/AST.h" #include "clang/ASTMatchers/ASTMatchers.h" #include "llvm/ADT/DenseMap.h" +#include <variant> namespace clang { @@ -22,8 +21,15 @@ class FunctionParmMutationAnalyzer; /// a given statement. class ExprMutationAnalyzer { public: + friend class FunctionParmMutationAnalyzer; + struct Cache { + llvm::SmallDenseMap<const FunctionDecl *, + std::unique_ptr<FunctionParmMutationAnalyzer>> + FuncParmAnalyzer; + }; + ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context) - : Stm(Stm), Context(Context) {} + : ExprMutationAnalyzer(Stm, Context, std::make_shared<Cache>()) {} bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; } bool isMutated(const Decl *Dec) { return findMutation(Dec) != nullptr; } @@ -45,6 +51,11 @@ class ExprMutationAnalyzer { using MutationFinder = const Stmt *(ExprMutationAnalyzer::*)(const Expr *); using ResultMap = llvm::DenseMap<const Expr *, const Stmt *>; + ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context, + std::shared_ptr<Cache> CrossAnalysisCache) + : Stm(Stm), Context(Context), + CrossAnalysisCache(std::move(CrossAnalysisCache)) {} + const Stmt *findMutationMemoized(const Expr *Exp, llvm::ArrayRef<MutationFinder> Finders, ResultMap &MemoizedResults); @@ -69,9 +80,7 @@ class ExprMutationAnalyzer { const Stmt &Stm; ASTContext &Context; - llvm::DenseMap<const FunctionDecl *, - std::unique_ptr<FunctionParmMutationAnalyzer>> - FuncParmAnalyzer; + std::shared_ptr<Cache> CrossAnalysisCache; ResultMap Results; ResultMap PointeeResults; }; @@ -80,7 +89,12 @@ class ExprMutationAnalyzer { // params. class FunctionParmMutationAnalyzer { public: - FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context); + FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context) + : FunctionParmMutationAnalyzer( + Func, Context, std::make_shared<ExprMutationAnalyzer::Cache>()) {} + FunctionParmMutationAnalyzer( + const FunctionDecl &Func, ASTContext &Context, + std::shared_ptr<ExprMutationAnalyzer::Cache> CrossAnalysisCache); bool isMutated(const ParmVarDecl *Parm) { return findMutation(Parm) != nullptr; diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp index bb042760d297a7..503ca4bea99e50 100644 --- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp +++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp @@ -638,9 +638,10 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) { if (!RefType->getPointeeType().getQualifiers() && RefType->getPointeeType()->getAs<TemplateTypeParmType>()) { std::unique_ptr<FunctionParmMutationAnalyzer> &Analyzer = - FuncParmAnalyzer[Func]; + CrossAnalysisCache->FuncParmAnalyzer[Func]; if (!Analyzer) - Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context)); + Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context, + CrossAnalysisCache)); if (Analyzer->findMutation(Parm)) return Exp; continue; @@ -653,13 +654,15 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) { } FunctionParmMutationAnalyzer::FunctionParmMutationAnalyzer( - const FunctionDecl &Func, ASTContext &Context) - : BodyAnalyzer(*Func.getBody(), Context) { + const FunctionDecl &Func, ASTContext &Context, + std::shared_ptr<ExprMutationAnalyzer::Cache> CrossAnalysisCache) + : BodyAnalyzer(*Func.getBody(), Context, CrossAnalysisCache) { if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(&Func)) { // CXXCtorInitializer might also mutate Param but they're not part of // function body, check them eagerly here since they're typically trivial. for (const CXXCtorInitializer *Init : Ctor->inits()) { - ExprMutationAnalyzer InitAnalyzer(*Init->getInit(), Context); + ExprMutationAnalyzer InitAnalyzer(*Init->getInit(), Context, + CrossAnalysisCache); for (const ParmVarDecl *Parm : Ctor->parameters()) { if (Results.contains(Parm)) continue; @@ -675,11 +678,14 @@ FunctionParmMutationAnalyzer::findMutation(const ParmVarDecl *Parm) { const auto Memoized = Results.find(Parm); if (Memoized != Results.end()) return Memoized->second; - + // To handle call A -> call B -> call A. Assume parameters of A is not mutated + // before analyzing parameters of A. Then when analyzing the second "call A", + // FunctionParmMutationAnalyzer can use this memoized value to avoid infinite + // recursion. + Results[Parm] = nullptr; if (const Stmt *S = BodyAnalyzer.findMutation(Parm)) return Results[Parm] = S; - - return Results[Parm] = nullptr; + return Results[Parm]; } } // namespace clang diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp index f58ce4aebcbfc8..9c1dc1a76db63d 100644 --- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp +++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp @@ -977,6 +977,36 @@ TEST(ExprMutationAnalyzerTest, FollowFuncArgModified) { "void f() { int x; g(x); }"); Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)")); + + AST = buildASTFromCode( + StdRemoveReference + StdForward + + "template <class T> void f1(T &&a);" + "template <class T> void f2(T &&a);" + "template <class T> void f1(T &&a) { f2<T>(std::forward<T>(a)); }" + "template <class T> void f2(T &&a) { f1<T>(std::forward<T>(a)); }" + "void f() { int x; f1(x); }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); + + AST = buildASTFromCode( + StdRemoveReference + StdForward + + "template <class T> void f1(T &&a);" + "template <class T> void f2(T &&a);" + "template <class T> void f1(T &&a) { f2<T>(std::forward<T>(a)); }" + "template <class T> void f2(T &&a) { f1<T>(std::forward<T>(a)); a++; }" + "void f() { int x; f1(x); }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("f1(x)")); + + AST = buildASTFromCode( + StdRemoveReference + StdForward + + "template <class T> void f1(T &&a);" + "template <class T> void f2(T &&a);" + "template <class T> void f1(T &&a) { f2<T>(std::forward<T>(a)); a++; }" + "template <class T> void f2(T &&a) { f1<T>(std::forward<T>(a)); }" + "void f() { int x; f1(x); }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("f1(x)")); } TEST(ExprMutationAnalyzerTest, FollowFuncArgNotModified) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
