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 <jvo...@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 <jvo...@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 <jvo...@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 <jvo...@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); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
