[clang] [clang-tools-extra] [clang analysis] ExprMutationAnalyzer avoid infinite recursion for recursive forwarding reference (PR #87954)

Congcong Cai via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 9 23:09:11 PDT 2024


https://github.com/HerrCai0907 updated https://github.com/llvm/llvm-project/pull/87954

>From 19f66851204547232d586288fba79d8770837350 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Mon, 8 Apr 2024 09:20:58 +0800
Subject: [PATCH 1/3] [clang analysis] ExprMutationAnalyzer support recursive
 forwarding reference

Partialy fixes: #60895
---
 .../Analysis/Analyses/ExprMutationAnalyzer.h  | 40 +++++++++++++++----
 clang/lib/Analysis/ExprMutationAnalyzer.cpp   | 22 ++++++----
 .../Analysis/ExprMutationAnalyzerTest.cpp     | 30 ++++++++++++++
 3 files changed, 77 insertions(+), 15 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
index 1ceef944fbc34e..af6106fe303afd 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::DenseMap<const FunctionDecl *,
+                   std::unique_ptr<FunctionParmMutationAnalyzer>>
+        FuncParmAnalyzer;
+  };
+
   ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context)
-      : Stm(Stm), Context(Context) {}
+      : ExprMutationAnalyzer(Stm, Context, nullptr) {}
 
   bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; }
   bool isMutated(const Decl *Dec) { return findMutation(Dec) != nullptr; }
@@ -45,6 +51,19 @@ class ExprMutationAnalyzer {
   using MutationFinder = const Stmt *(ExprMutationAnalyzer::*)(const Expr *);
   using ResultMap = llvm::DenseMap<const Expr *, const Stmt *>;
 
+  ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context, Cache *ParentCache)
+      : Stm(Stm), Context(Context) {
+    if (ParentCache != nullptr) {
+      CrossAnalysisCache = ParentCache;
+    } else {
+      CrossAnalysisCache = std::make_unique<Cache>();
+    }
+  }
+  ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context,
+                       std::unique_ptr<Cache> CrossAnalysisCache)
+      : Stm(Stm), Context(Context),
+        CrossAnalysisCache(std::move(CrossAnalysisCache)) {}
+
   const Stmt *findMutationMemoized(const Expr *Exp,
                                    llvm::ArrayRef<MutationFinder> Finders,
                                    ResultMap &MemoizedResults);
@@ -67,11 +86,15 @@ class ExprMutationAnalyzer {
   const Stmt *findReferenceMutation(const Expr *Exp);
   const Stmt *findFunctionArgMutation(const Expr *Exp);
 
+  Cache *getCache() {
+    return std::holds_alternative<Cache *>(CrossAnalysisCache)
+               ? std::get<Cache *>(CrossAnalysisCache)
+               : std::get<std::unique_ptr<Cache>>(CrossAnalysisCache).get();
+  }
+
   const Stmt &Stm;
   ASTContext &Context;
-  llvm::DenseMap<const FunctionDecl *,
-                 std::unique_ptr<FunctionParmMutationAnalyzer>>
-      FuncParmAnalyzer;
+  std::variant<Cache *, std::unique_ptr<Cache>> CrossAnalysisCache;
   ResultMap Results;
   ResultMap PointeeResults;
 };
@@ -80,7 +103,10 @@ class ExprMutationAnalyzer {
 // params.
 class FunctionParmMutationAnalyzer {
 public:
-  FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context);
+  FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context)
+      : FunctionParmMutationAnalyzer(Func, Context, nullptr) {}
+  FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context,
+                               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..dba6f2a3c02112 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];
+            getCache()->FuncParmAnalyzer[Func];
         if (!Analyzer)
-          Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context));
+          Analyzer.reset(
+              new FunctionParmMutationAnalyzer(*Func, Context, getCache()));
         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,
+    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) {

>From c5d0580b736eafc20c081e9b36f48ea606f5df11 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Mon, 8 Apr 2024 12:11:45 +0800
Subject: [PATCH 2/3] add release-note

---
 clang-tools-extra/docs/ReleaseNotes.rst           |  4 ++++
 .../checkers/misc/const-correctness-templates.cpp | 15 +++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 309b844615a121..19ab5bc4eae882 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -214,6 +214,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

>From 111009710d56d1dd514c8458063f3e033aee152b Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Wed, 10 Apr 2024 14:08:58 +0800
Subject: [PATCH 3/3] use shared_ptr

---
 .../Analysis/Analyses/ExprMutationAnalyzer.h  | 28 ++++++-------------
 clang/lib/Analysis/ExprMutationAnalyzer.cpp   |  8 +++---
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
index af6106fe303afd..ae62135a075717 100644
--- a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
+++ b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
@@ -29,7 +29,7 @@ class ExprMutationAnalyzer {
   };
 
   ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context)
-      : ExprMutationAnalyzer(Stm, Context, nullptr) {}
+      : 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; }
@@ -51,16 +51,8 @@ class ExprMutationAnalyzer {
   using MutationFinder = const Stmt *(ExprMutationAnalyzer::*)(const Expr *);
   using ResultMap = llvm::DenseMap<const Expr *, const Stmt *>;
 
-  ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context, Cache *ParentCache)
-      : Stm(Stm), Context(Context) {
-    if (ParentCache != nullptr) {
-      CrossAnalysisCache = ParentCache;
-    } else {
-      CrossAnalysisCache = std::make_unique<Cache>();
-    }
-  }
   ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context,
-                       std::unique_ptr<Cache> CrossAnalysisCache)
+                       std::shared_ptr<Cache> CrossAnalysisCache)
       : Stm(Stm), Context(Context),
         CrossAnalysisCache(std::move(CrossAnalysisCache)) {}
 
@@ -86,15 +78,9 @@ class ExprMutationAnalyzer {
   const Stmt *findReferenceMutation(const Expr *Exp);
   const Stmt *findFunctionArgMutation(const Expr *Exp);
 
-  Cache *getCache() {
-    return std::holds_alternative<Cache *>(CrossAnalysisCache)
-               ? std::get<Cache *>(CrossAnalysisCache)
-               : std::get<std::unique_ptr<Cache>>(CrossAnalysisCache).get();
-  }
-
   const Stmt &Stm;
   ASTContext &Context;
-  std::variant<Cache *, std::unique_ptr<Cache>> CrossAnalysisCache;
+  std::shared_ptr<Cache> CrossAnalysisCache;
   ResultMap Results;
   ResultMap PointeeResults;
 };
@@ -104,9 +90,11 @@ class ExprMutationAnalyzer {
 class FunctionParmMutationAnalyzer {
 public:
   FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context)
-      : FunctionParmMutationAnalyzer(Func, Context, nullptr) {}
-  FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context,
-                               ExprMutationAnalyzer::Cache *CrossAnalysisCache);
+      : 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 dba6f2a3c02112..503ca4bea99e50 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -638,10 +638,10 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) {
       if (!RefType->getPointeeType().getQualifiers() &&
           RefType->getPointeeType()->getAs<TemplateTypeParmType>()) {
         std::unique_ptr<FunctionParmMutationAnalyzer> &Analyzer =
-            getCache()->FuncParmAnalyzer[Func];
+            CrossAnalysisCache->FuncParmAnalyzer[Func];
         if (!Analyzer)
-          Analyzer.reset(
-              new FunctionParmMutationAnalyzer(*Func, Context, getCache()));
+          Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context,
+                                                          CrossAnalysisCache));
         if (Analyzer->findMutation(Parm))
           return Exp;
         continue;
@@ -655,7 +655,7 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) {
 
 FunctionParmMutationAnalyzer::FunctionParmMutationAnalyzer(
     const FunctionDecl &Func, ASTContext &Context,
-    ExprMutationAnalyzer::Cache *CrossAnalysisCache)
+    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



More information about the cfe-commits mailing list