[clang] [clang-tools-extra] Revert "[clang analysis] ExprMutationAnalyzer avoid infinite recursion for recursive forwarding reference" (PR #88765)
Florian Mayer via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 15 10:47:14 PDT 2024
https://github.com/fmayer created https://github.com/llvm/llvm-project/pull/88765
Reverts llvm/llvm-project#87954
Broke sanitizer bots, e.g. https://lab.llvm.org/buildbot/#/builders/239/builds/6587/steps/10/logs/stdio
>From 82b9a06f73df5301ffd950775055304124f63e02 Mon Sep 17 00:00:00 2001
From: Florian Mayer <florian.mayer at bitsrc.org>
Date: Mon, 15 Apr 2024 10:46:21 -0700
Subject: [PATCH] =?UTF-8?q?Revert=20"[clang=20analysis]=20ExprMutationAnal?=
=?UTF-8?q?yzer=20avoid=20infinite=20recursion=20for=20re=E2=80=A6"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This reverts commit 8095b9ce6bf5831a14c72028920708f38d13d0c3.
---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ---
.../misc/const-correctness-templates.cpp | 15 ----------
.../Analysis/Analyses/ExprMutationAnalyzer.h | 28 +++++------------
clang/lib/Analysis/ExprMutationAnalyzer.cpp | 22 +++++---------
.../Analysis/ExprMutationAnalyzerTest.cpp | 30 -------------------
5 files changed, 15 insertions(+), 84 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 7095c564444fe6..4dfbd8ca49ab9b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -221,10 +221,6 @@ 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 248374a71dd40b..9da468128743e9 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,18 +58,3 @@ 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 c4e5d0badb8e58..1ceef944fbc34e 100644
--- a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
+++ b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
@@ -8,10 +8,11 @@
#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 {
@@ -21,15 +22,8 @@ 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)
- : ExprMutationAnalyzer(Stm, Context, std::make_shared<Cache>()) {}
+ : Stm(Stm), Context(Context) {}
bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; }
bool isMutated(const Decl *Dec) { return findMutation(Dec) != nullptr; }
@@ -51,11 +45,6 @@ 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);
@@ -80,7 +69,9 @@ class ExprMutationAnalyzer {
const Stmt &Stm;
ASTContext &Context;
- std::shared_ptr<Cache> CrossAnalysisCache;
+ llvm::DenseMap<const FunctionDecl *,
+ std::unique_ptr<FunctionParmMutationAnalyzer>>
+ FuncParmAnalyzer;
ResultMap Results;
ResultMap PointeeResults;
};
@@ -89,12 +80,7 @@ class ExprMutationAnalyzer {
// params.
class FunctionParmMutationAnalyzer {
public:
- 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);
+ FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context);
bool isMutated(const ParmVarDecl *Parm) {
return findMutation(Parm) != nullptr;
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 503ca4bea99e50..bb042760d297a7 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -638,10 +638,9 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) {
if (!RefType->getPointeeType().getQualifiers() &&
RefType->getPointeeType()->getAs<TemplateTypeParmType>()) {
std::unique_ptr<FunctionParmMutationAnalyzer> &Analyzer =
- CrossAnalysisCache->FuncParmAnalyzer[Func];
+ FuncParmAnalyzer[Func];
if (!Analyzer)
- Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context,
- CrossAnalysisCache));
+ Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context));
if (Analyzer->findMutation(Parm))
return Exp;
continue;
@@ -654,15 +653,13 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) {
}
FunctionParmMutationAnalyzer::FunctionParmMutationAnalyzer(
- const FunctionDecl &Func, ASTContext &Context,
- std::shared_ptr<ExprMutationAnalyzer::Cache> CrossAnalysisCache)
- : BodyAnalyzer(*Func.getBody(), Context, CrossAnalysisCache) {
+ const FunctionDecl &Func, ASTContext &Context)
+ : BodyAnalyzer(*Func.getBody(), Context) {
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,
- CrossAnalysisCache);
+ ExprMutationAnalyzer InitAnalyzer(*Init->getInit(), Context);
for (const ParmVarDecl *Parm : Ctor->parameters()) {
if (Results.contains(Parm))
continue;
@@ -678,14 +675,11 @@ 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];
+
+ return Results[Parm] = nullptr;
}
} // namespace clang
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index 9c1dc1a76db63d..f58ce4aebcbfc8 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -977,36 +977,6 @@ 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) {
More information about the cfe-commits
mailing list