Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[analysis] assume expr is not mutated after analysis to avoid recursive #90581

Merged
merged 2 commits into from
May 1, 2024

Conversation

HerrCai0907
Copy link
Contributor

Fixes: #89376.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:analysis labels Apr 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 30, 2024

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Congcong Cai (HerrCai0907)

Changes

Fixes: #89376.


Full diff: https://github.com/llvm/llvm-project/pull/90581.diff

2 Files Affected:

  • (modified) clang/lib/Analysis/ExprMutationAnalyzer.cpp (+4-2)
  • (modified) clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp (+24-2)
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 941322be8f870b..3b3782fa1db9a0 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -235,15 +235,17 @@ const Stmt *ExprMutationAnalyzer::Analyzer::findMutationMemoized(
   if (Memoized != MemoizedResults.end())
     return Memoized->second;
 
+  // Assume Exp is not mutated before analyzing Exp.
+  MemoizedResults[Exp] = nullptr;
   if (isUnevaluated(Exp))
-    return MemoizedResults[Exp] = nullptr;
+    return nullptr;
 
   for (const auto &Finder : Finders) {
     if (const Stmt *S = (this->*Finder)(Exp))
       return MemoizedResults[Exp] = S;
   }
 
-  return MemoizedResults[Exp] = nullptr;
+  return nullptr;
 }
 
 const Stmt *
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index 9c1dc1a76db63d..79ccb024283c35 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -10,8 +10,8 @@
 #include "clang/AST/TypeLoc.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
 #include "clang/Tooling/Tooling.h"
-#include "llvm/ADT/SmallString.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cctype>
@@ -43,7 +43,7 @@ std::unique_ptr<ASTUnit> buildASTFromCode(const Twine &Code) {
 }
 
 ExprMatcher declRefTo(StringRef Name) {
-  return declRefExpr(to(namedDecl(hasName(Name))));
+  return declRefExpr(to(namedDecl(hasName(Name)).bind("decl")));
 }
 
 StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
@@ -57,6 +57,13 @@ bool isMutated(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) {
   return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
 }
 
+bool isDeclMutated(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) {
+  const auto *const S = selectFirst<Stmt>("stmt", Results);
+  const auto *const D = selectFirst<Decl>("decl", Results);
+  TraversalKindScope RAII(AST->getASTContext(), TK_AsIs);
+  return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(D);
+}
+
 SmallVector<std::string, 1>
 mutatedBy(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) {
   const auto *const S = selectFirst<Stmt>("stmt", Results);
@@ -1552,6 +1559,21 @@ TEST(ExprMutationAnalyzerTest, UniquePtr) {
 
 // section: complex problems detected on real code
 
+TEST(ExprMutationAnalyzerTest, SelfRef) {
+  std::unique_ptr<ASTUnit> AST{};
+  SmallVector<BoundNodes, 1> Results{};
+
+  AST = buildASTFromCodeWithArgs("void f() { int &x = x; }",
+                                 {"-Wno-unused-value", "-Wno-uninitialized"});
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isDeclMutated(Results, AST.get()));
+
+  AST = buildASTFromCodeWithArgs("void f() { int &x = x; x = 1; }",
+                                 {"-Wno-unused-value", "-Wno-uninitialized"});
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_TRUE(isDeclMutated(Results, AST.get()));
+}
+
 TEST(ExprMutationAnalyzerTest, UnevaluatedContext) {
   const std::string Example =
       "template <typename T>"

@HerrCai0907 HerrCai0907 requested a review from PiotrZSL April 30, 2024 12:57
Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, check if release notes for misc-const-correctness doesn't need to be updated.

@HerrCai0907 HerrCai0907 merged commit 6e31714 into llvm:main May 1, 2024
6 checks passed
@HerrCai0907 HerrCai0907 deleted the fix/mutation branch May 1, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang Clang issues not falling into any other category clang-tools-extra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-tidy][crash] Crash in misc-const-correctness
3 participants