[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)

Jan Voung via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 15 18:58:58 PDT 2024


https://github.com/jvoung updated https://github.com/llvm/llvm-project/pull/111006

>From f82e63e470f704f29f4c161579fd592c27301868 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Thu, 3 Oct 2024 15:21:32 +0000
Subject: [PATCH 1/4] [clang][dataflow] Add a lattice to help represent cache
 const accessor methods

By caching const accessor methods we can sometimes treat method call
results as stable (e.g., for issue
https://github.com/llvm/llvm-project/issues/58510).
Users can clear the cache when a non-const method is called that may
modify the state of an object.

This is represented as mixin.
It  will be used in a follow on patch to change
bugprone-unchecked-optional-access's lattice from
NoopLattice to CachedConstAccessorsLattice<NoopLattice>,
along with some additional transfer functions.
---
 .../CachedConstAccessorsLattice.h             | 221 ++++++++++++++++++
 .../Analysis/FlowSensitive/CMakeLists.txt     |   1 +
 .../CachedConstAccessorsLatticeTest.cpp       | 217 +++++++++++++++++
 3 files changed, 439 insertions(+)
 create mode 100644 clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
 create mode 100644 clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp

diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
new file mode 100644
index 00000000000000..b8517fabd1f550
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -0,0 +1,221 @@
+//===-- CachedConstAccessorsLattice.h ---------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+///   const std::optional<Foo>& getFoo() const;
+///   void clear();
+/// };
+//
+/// void func(Bar& s) {
+///   if (s.getFoo().has_value()) {
+///     use(s.getFoo().value()); // safe (checked earlier getFoo())
+///     s.clear();
+///     use(s.getFoo().value()); // unsafe (invalidate cache for s)
+///   }
+/// }
+template <typename Base>
+class CachedConstAccessorsLattice : public Base {
+public:
+  using Base::Base;  // inherit all constructors
+
+  /// Creates or returns a previously created `Value` associated with a const
+  ///  method call `obj.getFoo()` where `RecordLoc` is the
+  /// `RecordStorageLocation` of `obj`.
+  /// Returns nullptr if unable to find or create a value.
+  ///
+  /// Requirements:
+  ///
+  ///  - `CE` should return a value (not a reference or record type)
+  Value *
+  getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+                                    const CallExpr *CE, Environment &Env);
+
+  /// Creates or returns a previously created `StorageLocation` associated a
+  /// const method call `obj.getFoo()` where `RecordLoc` is the
+  /// `RecordStorageLocation` of `obj`.
+  ///
+  /// The callback `Initialize` runs on the storage location if newly created.
+  /// Returns nullptr if unable to find or create a value.
+  ///
+  /// Requirements:
+  ///
+  ///  - `CE` should return a location (GLValue or a record type).
+  StorageLocation *getOrCreateConstMethodReturnStorageLocation(
+      const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+      Environment &Env,
+      llvm::function_ref<void(StorageLocation &)> Initialize);
+
+  void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
+    ConstMethodReturnValues.erase(&RecordLoc);
+  }
+
+  void clearConstMethodReturnStorageLocations(
+      const RecordStorageLocation &RecordLoc) {
+    ConstMethodReturnStorageLocations.erase(&RecordLoc);
+  }
+
+  bool operator==(const CachedConstAccessorsLattice &Other) const {
+    return Base::operator==(Other);
+  }
+
+  LatticeJoinEffect join(const CachedConstAccessorsLattice &Other);
+
+private:
+  // Maps a record storage location and const method to the value to return
+  // from that const method.
+  using ConstMethodReturnValuesType = llvm::SmallDenseMap<
+      const RecordStorageLocation *,
+      llvm::SmallDenseMap<const FunctionDecl *, Value *>>;
+  ConstMethodReturnValuesType ConstMethodReturnValues;
+
+  // Maps a record storage location and const method to the record storage
+  // location to return from that const method.
+  using ConstMethodReturnStorageLocationsType = llvm::SmallDenseMap<
+      const RecordStorageLocation *,
+      llvm::SmallDenseMap<const FunctionDecl *, StorageLocation *>>;
+  ConstMethodReturnStorageLocationsType ConstMethodReturnStorageLocations;
+};
+
+namespace internal {
+
+template <typename T>
+llvm::SmallDenseMap<const RecordStorageLocation *,
+                    llvm::SmallDenseMap<const FunctionDecl *, T *>>
+joinConstMethodMap(
+    const llvm::SmallDenseMap<const RecordStorageLocation *,
+                              llvm::SmallDenseMap<const FunctionDecl *, T *>>
+        &Map1,
+    const llvm::SmallDenseMap<const RecordStorageLocation *,
+                              llvm::SmallDenseMap<const FunctionDecl *, T *>>
+        &Map2,
+    LatticeEffect &Effect) {
+  llvm::SmallDenseMap<const RecordStorageLocation *,
+                      llvm::SmallDenseMap<const FunctionDecl *, T *>>
+      Result;
+  for (auto &[Loc, DeclToT] : Map1) {
+    auto It = Map2.find(Loc);
+    if (It == Map2.end()) {
+      Effect = LatticeJoinEffect::Changed;
+      continue;
+    }
+    const auto &OtherDeclToT = It->second;
+    auto &JoinedDeclToT = Result[Loc];
+    for (auto [Func, Var] : DeclToT) {
+      T *OtherVar = OtherDeclToT.lookup(Func);
+      if (OtherVar == nullptr || OtherVar != Var) {
+        Effect = LatticeJoinEffect::Changed;
+        continue;
+      }
+      JoinedDeclToT.insert({Func, Var});
+    }
+  }
+  return Result;
+}
+
+} // namespace internal
+
+template <typename Base>
+LatticeEffect CachedConstAccessorsLattice<Base>::join(
+    const CachedConstAccessorsLattice<Base> &Other) {
+
+  LatticeEffect Effect = Base::join(Other);
+
+  // For simplicity, we only retain values that are identical, but not ones that
+  // are non-identical but equivalent. This is likely to be sufficient in
+  // practice, and it reduces implementation complexity considerably.
+
+  ConstMethodReturnValues = internal::joinConstMethodMap<Value>(
+      ConstMethodReturnValues, Other.ConstMethodReturnValues, Effect);
+
+  ConstMethodReturnStorageLocations =
+      internal::joinConstMethodMap<StorageLocation>(
+          ConstMethodReturnStorageLocations,
+          Other.ConstMethodReturnStorageLocations, Effect);
+
+  return Effect;
+}
+
+template <typename Base>
+Value *CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnValue(
+    const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+    Environment &Env) {
+  QualType Type = CE->getType();
+  assert(!Type.isNull());
+  assert(!Type->isReferenceType());
+  assert(!Type->isRecordType());
+
+  auto &ObjMap = ConstMethodReturnValues[&RecordLoc];
+  const FunctionDecl *DirectCallee = CE->getDirectCallee();
+  if (DirectCallee == nullptr)
+    return nullptr;
+  auto it = ObjMap.find(DirectCallee);
+  if (it != ObjMap.end())
+    return it->second;
+
+  Value *Val = Env.createValue(Type);
+  if (Val != nullptr)
+    ObjMap.insert({DirectCallee, Val});
+  return Val;
+}
+
+template <typename Base>
+StorageLocation *
+CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
+    const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+    Environment &Env,
+    llvm::function_ref<void(StorageLocation &)> Initialize) {
+  QualType Type = CE->getType();
+  assert(!Type.isNull());
+  assert(CE->isGLValue() || Type->isRecordType());
+  auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
+  const FunctionDecl *DirectCallee = CE->getDirectCallee();
+  if (DirectCallee == nullptr)
+    return nullptr;
+  auto it = ObjMap.find(DirectCallee);
+  if (it != ObjMap.end())
+    return it->second;
+
+  StorageLocation &Loc =
+      Env.createStorageLocation(CE->getType().getNonReferenceType());
+  Initialize(Loc);
+
+  ObjMap.insert({DirectCallee, &Loc});
+  return &Loc;
+}
+
+} // namespace dataflow
+} // namespace clang
+
+#endif  // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
index 12fee5dc2789ce..4e1819bfa166a8 100644
--- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
@@ -7,6 +7,7 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests
   ArenaTest.cpp
   ASTOpsTest.cpp
   CFGMatchSwitchTest.cpp
+  CachedConstAccessorsLatticeTest.cpp
   ChromiumCheckModelTest.cpp
   DataflowAnalysisContextTest.cpp
   DataflowEnvironmentTest.cpp
diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
new file mode 100644
index 00000000000000..eb6f081fd22c70
--- /dev/null
+++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
@@ -0,0 +1,217 @@
+//===- unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp ==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
+
+#include <cassert>
+#include <memory>
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Testing/TestAST.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang::dataflow {
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasName;
+using ast_matchers::match;
+using ast_matchers::selectFirst;
+
+using dataflow::DataflowAnalysisContext;
+using dataflow::Environment;
+using dataflow::LatticeJoinEffect;
+using dataflow::RecordStorageLocation;
+using dataflow::Value;
+using dataflow::WatchedLiteralsSolver;
+
+NamedDecl *lookup(StringRef Name, const DeclContext &DC) {
+  auto Result = DC.lookup(&DC.getParentASTContext().Idents.get(Name));
+  EXPECT_TRUE(Result.isSingleResult()) << Name;
+  return Result.front();
+}
+
+class CachedConstAccessorsLatticeTest : public ::testing::Test {
+protected:
+  using LatticeT = CachedConstAccessorsLattice<NoopLattice>;
+
+  DataflowAnalysisContext DACtx{std::make_unique<WatchedLiteralsSolver>()};
+  Environment Env{DACtx};
+};
+
+// Basic test AST with two const methods (return a value, and return a ref).
+struct CommonTestInputs {
+  CommonTestInputs()
+      : AST(R"cpp(
+    struct S {
+      int *valProperty() const;
+      int &refProperty() const;
+    };
+    void target() {
+      S s;
+      s.valProperty();
+      S s2;
+      s2.refProperty();
+    }
+  )cpp") {
+    auto *SDecl = cast<CXXRecordDecl>(
+        lookup("S", *AST.context().getTranslationUnitDecl()));
+    SType = AST.context().getRecordType(SDecl);
+    CallVal = selectFirst<CallExpr>(
+        "call",
+        match(cxxMemberCallExpr(callee(functionDecl(hasName("valProperty"))))
+                  .bind("call"),
+              AST.context()));
+    assert(CallVal != nullptr);
+
+    CallRef = selectFirst<CallExpr>(
+        "call",
+        match(cxxMemberCallExpr(callee(functionDecl(hasName("refProperty"))))
+                  .bind("call"),
+              AST.context()));
+    assert(CallRef != nullptr);
+}
+
+  TestAST AST;
+  QualType SType;
+  const CallExpr *CallVal;
+  const CallExpr *CallRef;
+};
+
+TEST_F(CachedConstAccessorsLatticeTest, SameValBeforeClearOrDiffAfterClear) {
+  CommonTestInputs Inputs;
+  auto *CE = Inputs.CallVal;
+  RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(),
+                            {});
+
+  LatticeT Lattice;
+  Value *Val1 = Lattice.getOrCreateConstMethodReturnValue(Loc, CE, Env);
+  Value *Val2 = Lattice.getOrCreateConstMethodReturnValue(Loc, CE, Env);
+
+  EXPECT_EQ(Val1, Val2);
+
+  Lattice.clearConstMethodReturnValues(Loc);
+  Value *Val3 = Lattice.getOrCreateConstMethodReturnValue(Loc, CE, Env);
+
+  EXPECT_NE(Val3, Val1);
+  EXPECT_NE(Val3, Val2);
+}
+
+TEST_F(CachedConstAccessorsLatticeTest, SameLocBeforeClearOrDiffAfterClear) {
+  CommonTestInputs Inputs;
+  auto *CE = Inputs.CallRef;
+  RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(),
+                            {});
+
+  LatticeT Lattice;
+  auto NopInit = [](StorageLocation &) {};
+  StorageLocation *Loc1 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+      Loc, CE, Env, NopInit);
+  auto NotCalled = [](StorageLocation &) {
+    ASSERT_TRUE(false) << "Not reached";
+  };
+  StorageLocation *Loc2 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+      Loc, CE, Env, NotCalled);
+
+  EXPECT_EQ(Loc1, Loc2);
+
+  Lattice.clearConstMethodReturnStorageLocations(Loc);
+  StorageLocation *Loc3 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+      Loc, CE, Env, NopInit);
+
+  EXPECT_NE(Loc3, Loc1);
+  EXPECT_NE(Loc3, Loc2);
+}
+
+TEST_F(CachedConstAccessorsLatticeTest, ClearDifferentLocs) {
+  CommonTestInputs Inputs;
+  auto *CE = Inputs.CallRef;
+  RecordStorageLocation LocS1(Inputs.SType, RecordStorageLocation::FieldToLoc(),
+                              {});
+  RecordStorageLocation LocS2(Inputs.SType, RecordStorageLocation::FieldToLoc(),
+                              {});
+
+  LatticeT Lattice;
+  auto NopInit = [](StorageLocation &) {};
+  StorageLocation *RetLoc1 =
+      Lattice.getOrCreateConstMethodReturnStorageLocation(LocS1, CE, Env,
+                                                          NopInit);
+  Lattice.clearConstMethodReturnStorageLocations(LocS2);
+  auto NotCalled = [](StorageLocation &) {
+    ASSERT_TRUE(false) << "Not reached";
+  };
+  StorageLocation *RetLoc2 =
+      Lattice.getOrCreateConstMethodReturnStorageLocation(LocS1, CE, Env,
+                                                          NotCalled);
+
+  EXPECT_EQ(RetLoc1, RetLoc2);
+}
+
+TEST_F(CachedConstAccessorsLatticeTest, JoinSameNoop) {
+  CommonTestInputs Inputs;
+  auto *CE = Inputs.CallVal;
+  RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(),
+                            {});
+
+  LatticeT EmptyLattice;
+  LatticeT EmptyLattice2;
+  EXPECT_EQ(EmptyLattice.join(EmptyLattice2), LatticeJoinEffect::Unchanged);
+
+  LatticeT Lattice1;
+  Lattice1.getOrCreateConstMethodReturnValue(Loc, CE, Env);
+  EXPECT_EQ(Lattice1.join(Lattice1), LatticeJoinEffect::Unchanged);
+}
+
+TEST_F(CachedConstAccessorsLatticeTest, ProducesNewValueAfterJoinDistinct) {
+  CommonTestInputs Inputs;
+  auto *CE = Inputs.CallVal;
+  RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(),
+                            {});
+
+  // L1 w/ v vs L2 empty
+  LatticeT Lattice1;
+  Value *Val1 = Lattice1.getOrCreateConstMethodReturnValue(Loc, CE, Env);
+
+  LatticeT EmptyLattice;
+
+  EXPECT_EQ(Lattice1.join(EmptyLattice), LatticeJoinEffect::Changed);
+  Value *ValAfterJoin =
+      Lattice1.getOrCreateConstMethodReturnValue(Loc, CE, Env);
+
+  EXPECT_NE(ValAfterJoin, Val1);
+
+  // L1 w/ v1 vs L3 w/ v2
+  LatticeT Lattice3;
+  Value *Val3 = Lattice3.getOrCreateConstMethodReturnValue(Loc, CE, Env);
+
+  EXPECT_EQ(Lattice1.join(Lattice3), LatticeJoinEffect::Changed);
+  Value *ValAfterJoin2 =
+      Lattice1.getOrCreateConstMethodReturnValue(Loc, CE, Env);
+
+  EXPECT_NE(ValAfterJoin2, ValAfterJoin);
+  EXPECT_NE(ValAfterJoin2, Val3);
+}
+
+} // namespace
+} // namespace clang::dataflow

>From 4885d657a6ec13f304d851dc18f8b26089738a40 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Thu, 3 Oct 2024 15:32:37 +0000
Subject: [PATCH 2/4] clang-format

---
 .../CachedConstAccessorsLattice.h             | 19 ++++++++-----------
 .../CachedConstAccessorsLatticeTest.cpp       |  2 +-
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index b8517fabd1f550..52114173e21bba 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -45,10 +45,9 @@ namespace dataflow {
 ///     use(s.getFoo().value()); // unsafe (invalidate cache for s)
 ///   }
 /// }
-template <typename Base>
-class CachedConstAccessorsLattice : public Base {
+template <typename Base> class CachedConstAccessorsLattice : public Base {
 public:
-  using Base::Base;  // inherit all constructors
+  using Base::Base; // inherit all constructors
 
   /// Creates or returns a previously created `Value` associated with a const
   ///  method call `obj.getFoo()` where `RecordLoc` is the
@@ -74,8 +73,7 @@ class CachedConstAccessorsLattice : public Base {
   ///  - `CE` should return a location (GLValue or a record type).
   StorageLocation *getOrCreateConstMethodReturnStorageLocation(
       const RecordStorageLocation &RecordLoc, const CallExpr *CE,
-      Environment &Env,
-      llvm::function_ref<void(StorageLocation &)> Initialize);
+      Environment &Env, llvm::function_ref<void(StorageLocation &)> Initialize);
 
   void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
     ConstMethodReturnValues.erase(&RecordLoc);
@@ -95,9 +93,9 @@ class CachedConstAccessorsLattice : public Base {
 private:
   // Maps a record storage location and const method to the value to return
   // from that const method.
-  using ConstMethodReturnValuesType = llvm::SmallDenseMap<
-      const RecordStorageLocation *,
-      llvm::SmallDenseMap<const FunctionDecl *, Value *>>;
+  using ConstMethodReturnValuesType =
+      llvm::SmallDenseMap<const RecordStorageLocation *,
+                          llvm::SmallDenseMap<const FunctionDecl *, Value *>>;
   ConstMethodReturnValuesType ConstMethodReturnValues;
 
   // Maps a record storage location and const method to the record storage
@@ -194,8 +192,7 @@ template <typename Base>
 StorageLocation *
 CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
     const RecordStorageLocation &RecordLoc, const CallExpr *CE,
-    Environment &Env,
-    llvm::function_ref<void(StorageLocation &)> Initialize) {
+    Environment &Env, llvm::function_ref<void(StorageLocation &)> Initialize) {
   QualType Type = CE->getType();
   assert(!Type.isNull());
   assert(CE->isGLValue() || Type->isRecordType());
@@ -218,4 +215,4 @@ CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
 } // namespace dataflow
 } // namespace clang
 
-#endif  // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
index eb6f081fd22c70..de0c9278fae457 100644
--- a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
@@ -91,7 +91,7 @@ struct CommonTestInputs {
                   .bind("call"),
               AST.context()));
     assert(CallRef != nullptr);
-}
+  }
 
   TestAST AST;
   QualType SType;

>From 10e69fd93ffa31f5b6b0e937092f8aab6f9cecc2 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Wed, 16 Oct 2024 01:52:19 +0000
Subject: [PATCH 3/4] Fix comments and add two more tests.

- a test for an accessor returning a record type
- a test for calling accessors of different objects
---
 .../CachedConstAccessorsLattice.h             |  6 +-
 .../CachedConstAccessorsLatticeTest.cpp       | 94 ++++++++++++++++++-
 2 files changed, 96 insertions(+), 4 deletions(-)

diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 52114173e21bba..3c3028eb9452fe 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -50,7 +50,7 @@ template <typename Base> class CachedConstAccessorsLattice : public Base {
   using Base::Base; // inherit all constructors
 
   /// Creates or returns a previously created `Value` associated with a const
-  ///  method call `obj.getFoo()` where `RecordLoc` is the
+  /// method call `obj.getFoo()` where `RecordLoc` is the
   /// `RecordStorageLocation` of `obj`.
   /// Returns nullptr if unable to find or create a value.
   ///
@@ -61,8 +61,8 @@ template <typename Base> class CachedConstAccessorsLattice : public Base {
   getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
                                     const CallExpr *CE, Environment &Env);
 
-  /// Creates or returns a previously created `StorageLocation` associated a
-  /// const method call `obj.getFoo()` where `RecordLoc` is the
+  /// Creates or returns a previously created `StorageLocation` associated with
+  /// a const method call `obj.getFoo()` where `RecordLoc` is the
   /// `RecordStorageLocation` of `obj`.
   ///
   /// The callback `Initialize` runs on the storage location if newly created.
diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
index de0c9278fae457..d88fc684a0504a 100644
--- a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
@@ -32,6 +32,7 @@
 namespace clang::dataflow {
 namespace {
 
+using ast_matchers::BoundNodes;
 using ast_matchers::callee;
 using ast_matchers::cxxMemberCallExpr;
 using ast_matchers::functionDecl;
@@ -46,6 +47,8 @@ using dataflow::RecordStorageLocation;
 using dataflow::Value;
 using dataflow::WatchedLiteralsSolver;
 
+using testing::SizeIs;
+
 NamedDecl *lookup(StringRef Name, const DeclContext &DC) {
   auto Result = DC.lookup(&DC.getParentASTContext().Idents.get(Name));
   EXPECT_TRUE(Result.isSingleResult()) << Name;
@@ -99,7 +102,8 @@ struct CommonTestInputs {
   const CallExpr *CallRef;
 };
 
-TEST_F(CachedConstAccessorsLatticeTest, SameValBeforeClearOrDiffAfterClear) {
+TEST_F(CachedConstAccessorsLatticeTest,
+       SamePrimitiveValBeforeClearOrDiffAfterClear) {
   CommonTestInputs Inputs;
   auto *CE = Inputs.CallVal;
   RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(),
@@ -144,6 +148,51 @@ TEST_F(CachedConstAccessorsLatticeTest, SameLocBeforeClearOrDiffAfterClear) {
   EXPECT_NE(Loc3, Loc2);
 }
 
+TEST_F(CachedConstAccessorsLatticeTest,
+       SameStructValBeforeClearOrDiffAfterClear) {
+  TestAST AST(R"cpp(
+    struct S {
+      S structValProperty() const;
+    };
+    void target() {
+      S s;
+      s.structValProperty();
+    }
+  )cpp");
+  auto *SDecl =
+      cast<CXXRecordDecl>(lookup("S", *AST.context().getTranslationUnitDecl()));
+  QualType SType = AST.context().getRecordType(SDecl);
+  const CallExpr *CE = selectFirst<CallExpr>(
+      "call", match(cxxMemberCallExpr(
+                        callee(functionDecl(hasName("structValProperty"))))
+                        .bind("call"),
+                    AST.context()));
+  ASSERT_NE(CE, nullptr);
+
+  RecordStorageLocation Loc(SType, RecordStorageLocation::FieldToLoc(), {});
+
+  LatticeT Lattice;
+  // Accessors that return a record by value are modeled by a record storage
+  // location (instead of a Value).
+  auto NopInit = [](StorageLocation &) {};
+  StorageLocation *Loc1 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+      Loc, CE, Env, NopInit);
+  auto NotCalled = [](StorageLocation &) {
+    ASSERT_TRUE(false) << "Not reached";
+  };
+  StorageLocation *Loc2 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+      Loc, CE, Env, NotCalled);
+
+  EXPECT_EQ(Loc1, Loc2);
+
+  Lattice.clearConstMethodReturnStorageLocations(Loc);
+  StorageLocation *Loc3 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+      Loc, CE, Env, NopInit);
+
+  EXPECT_NE(Loc3, Loc1);
+  EXPECT_NE(Loc3, Loc1);
+}
+
 TEST_F(CachedConstAccessorsLatticeTest, ClearDifferentLocs) {
   CommonTestInputs Inputs;
   auto *CE = Inputs.CallRef;
@@ -168,6 +217,49 @@ TEST_F(CachedConstAccessorsLatticeTest, ClearDifferentLocs) {
   EXPECT_EQ(RetLoc1, RetLoc2);
 }
 
+TEST_F(CachedConstAccessorsLatticeTest, DifferentValsFromDifferentLocs) {
+  TestAST AST(R"cpp(
+    struct S {
+      int *valProperty() const;
+    };
+    void target() {
+      S s1;
+      s1.valProperty();
+      S s2;
+      s2.valProperty();
+    }
+  )cpp");
+  auto *SDecl =
+      cast<CXXRecordDecl>(lookup("S", *AST.context().getTranslationUnitDecl()));
+  QualType SType = AST.context().getRecordType(SDecl);
+  SmallVector<BoundNodes, 1> valPropertyCalls =
+      match(cxxMemberCallExpr(callee(functionDecl(hasName("valProperty"))))
+                .bind("call"),
+            AST.context());
+  ASSERT_THAT(valPropertyCalls, SizeIs(2));
+
+  const CallExpr *CE1 = selectFirst<CallExpr>(
+      "call", valPropertyCalls);
+  ASSERT_NE(CE1, nullptr);
+
+  valPropertyCalls.erase(valPropertyCalls.begin());
+  const CallExpr *CE2 = selectFirst<CallExpr>(
+      "call", valPropertyCalls);
+  ASSERT_NE(CE2, nullptr);
+  ASSERT_NE(CE1, CE2);
+
+  RecordStorageLocation LocS1(SType, RecordStorageLocation::FieldToLoc(), {});
+  RecordStorageLocation LocS2(SType, RecordStorageLocation::FieldToLoc(), {});
+
+  LatticeT Lattice;
+  Value *Val1 =
+      Lattice.getOrCreateConstMethodReturnValue(LocS1, CE1, Env);
+  Value *Val2 =
+      Lattice.getOrCreateConstMethodReturnValue(LocS2, CE2, Env);
+
+  EXPECT_NE(Val1, Val2);
+}
+
 TEST_F(CachedConstAccessorsLatticeTest, JoinSameNoop) {
   CommonTestInputs Inputs;
   auto *CE = Inputs.CallVal;

>From 5578e3192774fdfc30aa5781ea5ab4aab28b2d12 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Wed, 16 Oct 2024 01:58:26 +0000
Subject: [PATCH 4/4] Fix formatting

---
 .../CachedConstAccessorsLatticeTest.cpp              | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
index d88fc684a0504a..6488833bd14cf2 100644
--- a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
@@ -238,13 +238,11 @@ TEST_F(CachedConstAccessorsLatticeTest, DifferentValsFromDifferentLocs) {
             AST.context());
   ASSERT_THAT(valPropertyCalls, SizeIs(2));
 
-  const CallExpr *CE1 = selectFirst<CallExpr>(
-      "call", valPropertyCalls);
+  const CallExpr *CE1 = selectFirst<CallExpr>("call", valPropertyCalls);
   ASSERT_NE(CE1, nullptr);
 
   valPropertyCalls.erase(valPropertyCalls.begin());
-  const CallExpr *CE2 = selectFirst<CallExpr>(
-      "call", valPropertyCalls);
+  const CallExpr *CE2 = selectFirst<CallExpr>("call", valPropertyCalls);
   ASSERT_NE(CE2, nullptr);
   ASSERT_NE(CE1, CE2);
 
@@ -252,10 +250,8 @@ TEST_F(CachedConstAccessorsLatticeTest, DifferentValsFromDifferentLocs) {
   RecordStorageLocation LocS2(SType, RecordStorageLocation::FieldToLoc(), {});
 
   LatticeT Lattice;
-  Value *Val1 =
-      Lattice.getOrCreateConstMethodReturnValue(LocS1, CE1, Env);
-  Value *Val2 =
-      Lattice.getOrCreateConstMethodReturnValue(LocS2, CE2, Env);
+  Value *Val1 = Lattice.getOrCreateConstMethodReturnValue(LocS1, CE1, Env);
+  Value *Val2 = Lattice.getOrCreateConstMethodReturnValue(LocS2, CE2, Env);
 
   EXPECT_NE(Val1, Val2);
 }



More information about the cfe-commits mailing list