Skip to content

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

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

Merged
merged 5 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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 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.
/// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah -- Taking a look!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Created #113601

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


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
1 change: 1 addition & 0 deletions clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests
ArenaTest.cpp
ASTOpsTest.cpp
CFGMatchSwitchTest.cpp
CachedConstAccessorsLatticeTest.cpp
ChromiumCheckModelTest.cpp
DataflowAnalysisContextTest.cpp
DataflowEnvironmentTest.cpp
Expand Down
Loading
Loading