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

jvoung · 2024-10-03T15:27:14Z

By caching const accessor methods we can sometimes treat method call results as stable (e.g., for issue #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, along with some additional transfer functions.

…r methods By caching const accessor methods we can sometimes treat method call results as stable (e.g., for issue llvm#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.

github-actions · 2024-10-03T15:30:53Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmbot · 2024-10-03T16:18:30Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Jan Voung (jvoung)

Changes

By caching const accessor methods we can sometimes treat method call results as stable (e.g., for issue #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.

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

3 Files Affected:

(added) clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h (+218)
(modified) clang/unittests/Analysis/FlowSensitive/CMakeLists.txt (+1)
(added) clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp (+217)

diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
new file mode 100644
index 00000000000000..52114173e21bba
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -0,0 +1,218 @@
+//===-- 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..de0c9278fae457
--- /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

jvoung · 2024-10-15T13:00:06Z

Friendly ping =)

Xazax-hun

Overall looks good to me but also wait for a review from the code owners.

Xazax-hun · 2024-10-15T13:16:09Z

clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp

+      : AST(R"cpp(
+    struct S {
+      int *valProperty() const;
+      int &refProperty() const;


I'd like to see a test with a method returning a record.

Thanks! Added a test "SameStructValBeforeClearOrDiffAfterClear"

ymand

modulo the requests for tests. This looks great!

ymand · 2024-10-15T14:38:41Z

clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h

+  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


Suggested change

/// method call `obj.getFoo()` where `RecordLoc` is the

/// method call `obj.getFoo()` where `RecordLoc` is the

ymand · 2024-10-15T14:40:03Z

clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h

+  getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+                                    const CallExpr *CE, Environment &Env);
+
+  /// Creates or returns a previously created `StorageLocation` associated a


Suggested change

/// Creates or returns a previously created `StorageLocation` associated a

/// Creates or returns a previously created `StorageLocation` associated with a

ymand · 2024-10-15T14:53:32Z

clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp

+    void target() {
+      S s;
+      s.valProperty();
+      S s2;


Can we also test: 2 objects, same function called, distinct results? That would catch bad keying on the object (for example).

Thanks! Added "DifferentValsFromDifferentLocs"

- a test for an accessor returning a record type - a test for calling accessors of different objects

Treat calls to zero-param const methods as having stable return values (with a cache) to address issue llvm#58510. The cache is invalidated when non-const methods are called. This uses the infrastructure from PR llvm#111006. For now we cache methods returning: - ref to optional - optional by value - booleans We can extend that to pointers to optional in a next change.

…ess (#112605) Treat calls to zero-param const methods as having stable return values (with a cache) to address issue #58510. The cache is invalidated when non-const methods are called. This uses the infrastructure from PR #111006. For now we cache methods returning: - ref to optional - optional by value - booleans We can extend that to pointers to optional in a next change.

nico · 2024-10-24T16:45:51Z

clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h

+  ConstMethodReturnStorageLocationsType ConstMethodReturnStorageLocations;
+};
+
+namespace internal {


This breaks building on Windows (arguably by exposing an existing bug):

http://45.33.8.238/win/95986/step_3.txt (scroll to the very bottom):

../../clang/include\clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h(157,29): error: reference to 'internal' is ambiguous 157 | ConstMethodReturnValues = internal::joinConstMethodMap<Value>( | ^ ../../clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp(1083,31): note: in instantiation of member function 'clang::dataflow::DataflowAnalysis<clang::dataflow::UncheckedOptionalAccessModel, clang::dataflow::CachedConstAccessorsLattice<clang::dataflow::NoopLattice>>::joinTypeErased' requested here 1083 | UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx, | ^ ../../clang/include\clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h(109,11): note: candidate found by name lookup is 'clang::dataflow::internal' 109 | namespace internal { | ^ ../../clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp(205,1): note: candidate found by name lookup is 'clang::dataflow::(anonymous namespace)::internal' 205 | AST_MATCHER_P(CXXMemberCallExpr, publicReceiverType, | ^ ../../clang/include\clang/ASTMatchers/ASTMatchersMacros.h(131,3): note: expanded from macro 'AST_MATCHER_P' 131 | AST_MATCHER_P_OVERLOAD(Type, DefineMatcher, ParamType, Param, 0) | ^ ../../clang/include\clang/ASTMatchers/ASTMatchersMacros.h(135,13): note: expanded from macro 'AST_MATCHER_P_OVERLOAD' 135 | namespace internal { \ | ^

Please take a look and revert for now if it takes a while to fix.

Ah -- Taking a look!

(Here's a build that needs less scrolling to find the diag: http://45.33.8.238/win/95994/step_3.txt)

Thanks! Created #113601

Though is there a way to validate this, similar to this clang-cl compile?

The functionality in the pointer nullability lattice was extracted into CachedConstAccessorsLattice for reuse in llvm/llvm-project#111006 . So it should be the same, except for some names (get$X vs getOrCreate$X), and a callback for initializing a StorageLocation if created by getOrCreate$X. Perhaps in the future more can be abstracted into the base dataflow transfer functions. PiperOrigin-RevId: 697632107 Change-Id: Ibfcaa741b86ff85352c9abc9901034a8ac47c035

jvoung changed the title ~~[clang][dataflow] Add a lattice to help represent cache const accessor methods~~ [clang][dataflow] Add a lattice to help cache const accessor methods Oct 3, 2024

clang-format

4885d65

jvoung marked this pull request as ready for review October 3, 2024 16:17

jvoung requested a review from martinboehme October 3, 2024 16:18

llvmbot added clang Clang issues not falling into any other category clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:analysis labels Oct 3, 2024

jvoung requested a review from ymand October 3, 2024 16:18

Xazax-hun reviewed Oct 15, 2024

View reviewed changes

ymand approved these changes Oct 15, 2024

View reviewed changes

jvoung added 2 commits October 16, 2024 01:52

Fix comments and add two more tests.

10e69fd

- a test for an accessor returning a record type - a test for calling accessors of different objects

Fix formatting

5578e31

ymand approved these changes Oct 16, 2024

View reviewed changes

Merge branch 'main' into unchecked_optional_cache_lattice

b23b67b

jvoung merged commit 87dd5dc into llvm:main Oct 16, 2024
8 checks passed

jvoung mentioned this pull request Oct 16, 2024

[clang][dataflow] Cache accessors for bugprone-unchecked-optional-access #112605

Merged

nico reviewed Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jvoung commented Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024 •

edited

Loading

llvmbot commented Oct 3, 2024 •

edited

Loading

jvoung commented Oct 15, 2024

Xazax-hun left a comment

Xazax-hun Oct 15, 2024

jvoung Oct 16, 2024

ymand left a comment

ymand Oct 15, 2024

jvoung Oct 16, 2024

ymand Oct 15, 2024

jvoung Oct 16, 2024

ymand Oct 15, 2024

jvoung Oct 16, 2024

nico Oct 24, 2024

jvoung Oct 24, 2024

nico Oct 24, 2024

jvoung Oct 24, 2024

	/// method call `obj.getFoo()` where `RecordLoc` is the
	/// method call `obj.getFoo()` where `RecordLoc` is the

	/// Creates or returns a previously created `StorageLocation` associated a
	/// Creates or returns a previously created `StorageLocation` associated with a

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

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

Conversation

jvoung commented Oct 3, 2024 • edited Loading

github-actions bot commented Oct 3, 2024 • edited Loading

llvmbot commented Oct 3, 2024 • edited Loading

jvoung commented Oct 15, 2024

Xazax-hun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvoung commented Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024 •

edited

Loading

llvmbot commented Oct 3, 2024 •

edited

Loading