[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
Jan Voung via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 23 11:19:49 PST 2024
https://github.com/jvoung updated https://github.com/llvm/llvm-project/pull/120249
>From c526263a7accc434dbf6e93c2995ceb2f95873b8 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Tue, 17 Dec 2024 15:38:19 +0000
Subject: [PATCH 1/6] [clang][dataflow] Use smart pointer caching in unchecked
optional accessor
Part 2 (and final part) following https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:
```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.
A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.
---
.../CachedConstAccessorsLattice.h | 41 ++++
.../SmartPointerAccessorCaching.h | 168 +++++++++++++++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../Models/UncheckedOptionalAccessModel.cpp | 58 +++++-
.../SmartPointerAccessorCaching.cpp | 153 ++++++++++++++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 32 +++
.../SmartPointerAccessorCachingTest.cpp | 194 ++++++++++++++++++
.../UncheckedOptionalAccessModelTest.cpp | 50 +++++
9 files changed, 692 insertions(+), 6 deletions(-)
create mode 100644 clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644 clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644 clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 48c5287367739a..6b5dacf9f66d2d 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -13,7 +13,9 @@
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -71,10 +73,28 @@ template <typename Base> class CachedConstAccessorsLattice : public Base {
/// Requirements:
///
/// - `CE` should return a location (GLValue or a record type).
+ ///
+ /// DEPRECATED: switch users to the below overload which takes Callee and Type
+ /// directly.
StorageLocation *getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const CallExpr *CE,
Environment &Env, llvm::function_ref<void(StorageLocation &)> Initialize);
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
+ /// and `Type` is the return type of `getFoo`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ ///
+ /// Requirements:
+ ///
+ /// - `Type` should return a location (GLValue or a record type).
+ StorageLocation &getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+ QualType Type, Environment &Env,
+ llvm::function_ref<void(StorageLocation &)> Initialize);
+
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
ConstMethodReturnValues.erase(&RecordLoc);
}
@@ -212,6 +232,27 @@ CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
return &Loc;
}
+template <typename Base>
+StorageLocation &
+CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+ QualType Type, Environment &Env,
+ llvm::function_ref<void(StorageLocation &)> Initialize) {
+ assert(Callee != nullptr);
+ assert(!Type.isNull());
+ assert(Type->isReferenceType() || Type->isRecordType());
+ auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
+ auto it = ObjMap.find(Callee);
+ if (it != ObjMap.end())
+ return *it->second;
+
+ StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType());
+ Initialize(Loc);
+
+ ObjMap.insert({Callee, &Loc});
+ return Loc;
+}
+
} // namespace dataflow
} // namespace clang
diff --git a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00000000000000..e89e82c893d8cb
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,168 @@
+//===-- SmartPointerAccessorCaching.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 utilities to help cache accessors for smart pointer
+// like objects.
+//
+// These should be combined with CachedConstAccessorsLattice.
+// Beyond basic const accessors, smart pointers may have the following two
+// additional issues:
+//
+// 1) There may be multiple accessors for the same underlying object, e.g.
+// `operator->`, `operator*`, and `get`. Users may use a mixture of these
+// accessors, so the cache should unify them.
+//
+// 2) There may be non-const overloads of accessors. They are still safe to
+// cache, as they don't modify the container object.
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+
+#include <cassert>
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang::dataflow {
+
+/// Matchers:
+/// For now, these match on any class with an `operator*` or `operator->`
+/// where the return types have a similar shape as std::unique_ptr
+/// and std::optional.
+///
+/// - `*` returns a reference to a type `T`
+/// - `->` returns a pointer to `T`
+/// - `get` returns a pointer to `T`
+/// - `value` returns a reference `T`
+///
+/// (1) The `T` should all match across the accessors (ignoring qualifiers).
+///
+/// (2) The specific accessor used in a call isn't required to be const,
+/// but the class must have a const overload of each accessor.
+///
+/// For now, we don't have customization to ignore certain classes.
+/// For example, if writing a ClangTidy check for `std::optional`, these
+/// would also match `std::optional`. In order to have special handling
+/// for `std::optional`, we assume the (Matcher, TransferFunction) case
+/// with custom handling is ordered early so that these generic cases
+/// do not trigger.
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar();
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorArrow();
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeValueMethodCall();
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeGetMethodCall();
+
+// Common transfer functions.
+
+/// Returns the "canonical" callee for smart pointer operators (`*` and `->`)
+/// as a key for caching.
+///
+/// We choose `operator *` as the canonical one, since it needs a
+/// StorageLocation anyway.
+///
+/// Note: there may be multiple `operator*` (one const, one non-const)
+/// we pick the const one, which the above provided matchers require to exist.
+const FunctionDecl *
+getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE);
+
+/// A transfer function for `operator*` (and `value`) calls.
+///
+/// Requirements:
+///
+/// - LatticeT should use the `CachedConstAccessorsLattice` mixin.
+template <typename LatticeT>
+void transferSmartPointerLikeDeref(
+ const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc,
+ TransferState<LatticeT> &State,
+ llvm::function_ref<void(StorageLocation &)> InitializeLoc);
+
+/// A transfer function for `operator->` (and `get`) calls.
+///
+/// Requirements:
+///
+/// - LatticeT should use the `CachedConstAccessorsLattice` mixin.
+template <typename LatticeT>
+void transferSmartPointerLikeGet(
+ const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
+ TransferState<LatticeT> &State,
+ llvm::function_ref<void(StorageLocation &)> InitializeLoc);
+
+template <typename LatticeT>
+void transferSmartPointerLikeDeref(
+ const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc,
+ TransferState<LatticeT> &State,
+ llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
+ if (State.Env.getStorageLocation(*DerefExpr) != nullptr)
+ return;
+ if (SmartPointerLoc == nullptr)
+ return;
+
+ const FunctionDecl *Callee = DerefExpr->getDirectCallee();
+ if (Callee == nullptr)
+ return;
+ const FunctionDecl *CanonicalCallee =
+ getCanonicalSmartPointerLikeOperatorCallee(DerefExpr);
+ // This shouldn't really happen, as we should at least find `Callee` itself.
+ assert(CanonicalCallee != nullptr);
+ if (CanonicalCallee != Callee) {
+ // When using the provided matchers, we should always get a reference to
+ // the same type.
+ assert(CanonicalCallee->getReturnType()->isReferenceType() &&
+ Callee->getReturnType()->isReferenceType());
+ assert(CanonicalCallee->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified() ==
+ Callee->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified());
+ }
+
+ StorageLocation &LocForValue =
+ State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+ *SmartPointerLoc, CanonicalCallee, CanonicalCallee->getReturnType(),
+ State.Env, InitializeLoc);
+ State.Env.setStorageLocation(*DerefExpr, LocForValue);
+}
+
+template <typename LatticeT>
+void transferSmartPointerLikeGet(
+ const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
+ TransferState<LatticeT> &State,
+ llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
+ if (SmartPointerLoc == nullptr)
+ return;
+
+ const FunctionDecl *CanonicalCallee =
+ getCanonicalSmartPointerLikeOperatorCallee(GetExpr);
+
+ if (CanonicalCallee != nullptr) {
+ auto &LocForValue =
+ State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+ *SmartPointerLoc, CanonicalCallee, CanonicalCallee->getReturnType(),
+ State.Env, InitializeLoc);
+ State.Env.setValue(*GetExpr,
+ State.Env.template create<PointerValue>(LocForValue));
+ } else {
+ // Otherwise, just cache the pointer value as if it was a const accessor.
+ Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(
+ *SmartPointerLoc, GetExpr, State.Env);
+ if (Val == nullptr)
+ return;
+ State.Env.setValue(*GetExpr, *Val);
+ }
+}
+
+} // namespace clang::dataflow
+
+#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
diff --git a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
index 05cdaa7e27823d..0c30df8b4b194f 100644
--- a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
@@ -10,6 +10,7 @@ add_clang_library(clangAnalysisFlowSensitive
Logger.cpp
RecordOps.cpp
SimplifyConstraints.cpp
+ SmartPointerAccessorCaching.cpp
Transfer.cpp
TypeErasedDataflowAnalysis.cpp
Value.cpp
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index da5dda063344f9..ce54b450a2121c 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -25,8 +25,10 @@
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/Formula.h"
#include "clang/Analysis/FlowSensitive/RecordOps.h"
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
#include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Basic/OperatorKinds.h"
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
@@ -555,24 +557,26 @@ void handleConstMemberCall(const CallExpr *CE,
LatticeTransferState &State) {
// If the const method returns an optional or reference to an optional.
if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
- StorageLocation *Loc =
+ const FunctionDecl *DirectCallee = CE->getDirectCallee();
+ if (DirectCallee == nullptr)
+ return;
+ StorageLocation &Loc =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
- *RecordLoc, CE, State.Env, [&](StorageLocation &Loc) {
+ *RecordLoc, DirectCallee, CE->getType(), State.Env,
+ [&](StorageLocation &Loc) {
setHasValue(cast<RecordStorageLocation>(Loc),
State.Env.makeAtomicBoolValue(), State.Env);
});
- if (Loc == nullptr)
- return;
if (CE->isGLValue()) {
// If the call to the const method returns a reference to an optional,
// link the call expression to the cached StorageLocation.
- State.Env.setStorageLocation(*CE, *Loc);
+ State.Env.setStorageLocation(*CE, Loc);
} else {
// If the call to the const method returns an optional by value, we
// need to use CopyRecord to link the optional to the result object
// of the call expression.
auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
- copyRecord(*cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
+ copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
}
return;
}
@@ -1031,6 +1035,48 @@ auto buildTransferMatchSwitch() {
transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env);
})
+ // Smart-pointer-like operator* and operator-> calls that may look like
+ // const accessors (below) but need special handling to allow mixing
+ // the accessor calls.
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isSmartPointerLikeOperatorStar(),
+ [](const CXXOperatorCallExpr *E,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferSmartPointerLikeDeref(
+ E,
+ dyn_cast_or_null<RecordStorageLocation>(
+ getLocBehindPossiblePointer(*E->getArg(0), State.Env)),
+ State, [](StorageLocation &Loc) {});
+ })
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isSmartPointerLikeOperatorArrow(),
+ [](const CXXOperatorCallExpr *E,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferSmartPointerLikeGet(
+ E,
+ dyn_cast_or_null<RecordStorageLocation>(
+ getLocBehindPossiblePointer(*E->getArg(0), State.Env)),
+ State, [](StorageLocation &Loc) {});
+ })
+ .CaseOfCFGStmt<CXXMemberCallExpr>(
+ isSmartPointerLikeValueMethodCall(),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferSmartPointerLikeDeref(
+ E, getImplicitObjectLocation(*E, State.Env), State,
+ [](StorageLocation &Loc) {});
+ })
+ .CaseOfCFGStmt<CXXMemberCallExpr>(
+ isSmartPointerLikeGetMethodCall(),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferSmartPointerLikeGet(
+ E, getImplicitObjectLocation(*E, State.Env), State,
+ [](StorageLocation &Loc) {});
+ })
+
// const accessor calls
.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
transferValue_ConstMemberCall)
diff --git a/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
new file mode 100644
index 00000000000000..12d0bdb82cb538
--- /dev/null
+++ b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
@@ -0,0 +1,153 @@
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/AST/CanonicalType.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/OperatorKinds.h"
+
+namespace clang::dataflow {
+
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxOperatorCallExpr;
+using ast_matchers::hasName;
+using ast_matchers::hasOverloadedOperatorName;
+using ast_matchers::ofClass;
+using ast_matchers::parameterCountIs;
+using ast_matchers::pointerType;
+using ast_matchers::referenceType;
+using ast_matchers::returns;
+
+bool hasSmartPointerClassShape(const CXXRecordDecl &RD, bool &HasGet,
+ bool &HasValue) {
+ // We may want to cache this search, but in current profiles it hasn't shown
+ // up as a hot spot (possibly because there aren't many hits, relatively).
+ bool HasArrow = false;
+ bool HasStar = false;
+ CanQualType StarReturnType, ArrowReturnType, GetReturnType, ValueReturnType;
+ for (const auto *MD : RD.methods()) {
+ // We only consider methods that are const and have zero parameters.
+ // It may be that there is a non-const overload for the method, but
+ // there should at least be a const overload as well.
+ if (!MD->isConst() || MD->getNumParams() != 0)
+ continue;
+ if (MD->getOverloadedOperator() == OO_Star &&
+ MD->getReturnType()->isReferenceType()) {
+ HasStar = true;
+ StarReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+ } else if (MD->getOverloadedOperator() == OO_Arrow &&
+ MD->getReturnType()->isPointerType()) {
+ HasArrow = true;
+ ArrowReturnType =
+ MD->getReturnType()->getPointeeType()->getCanonicalTypeUnqualified();
+ } else {
+ IdentifierInfo *II = MD->getIdentifier();
+ if (II == nullptr)
+ continue;
+ if (II->isStr("get") && MD->getReturnType()->isPointerType()) {
+ HasGet = true;
+ GetReturnType = MD->getReturnType()
+ ->getPointeeType()
+ ->getCanonicalTypeUnqualified();
+ } else if (II->isStr("value") && MD->getReturnType()->isReferenceType()) {
+ HasValue = true;
+ ValueReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+ }
+ }
+ }
+
+ if (!HasStar || !HasArrow || StarReturnType != ArrowReturnType)
+ return false;
+ HasGet = HasGet && (GetReturnType == StarReturnType);
+ HasValue = HasValue && (ValueReturnType == StarReturnType);
+ return true;
+}
+
+} // namespace
+} // namespace clang::dataflow
+
+// AST_MATCHER macros create an "internal" namespace, so we put it in
+// its own anonymous namespace instead of in clang::dataflow.
+namespace {
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGet) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasGet;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasValue;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGetOrValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && (HasGet || HasValue);
+}
+
+} // namespace
+
+namespace clang::dataflow {
+
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar() {
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName("*"),
+ callee(cxxMethodDecl(parameterCountIs(0), returns(referenceType()),
+ ofClass(smartPointerClassWithGetOrValue()))));
+}
+
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorArrow() {
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName("->"),
+ callee(cxxMethodDecl(parameterCountIs(0), returns(pointerType()),
+ ofClass(smartPointerClassWithGetOrValue()))));
+}
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeValueMethodCall() {
+ return cxxMemberCallExpr(callee(
+ cxxMethodDecl(parameterCountIs(0), returns(referenceType()),
+ hasName("value"), ofClass(smartPointerClassWithValue()))));
+}
+
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeGetMethodCall() {
+ return cxxMemberCallExpr(callee(
+ cxxMethodDecl(parameterCountIs(0), returns(pointerType()), hasName("get"),
+ ofClass(smartPointerClassWithGet()))));
+}
+
+const FunctionDecl *
+getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE) {
+ const FunctionDecl *CanonicalCallee = nullptr;
+ const CXXMethodDecl *Callee =
+ cast_or_null<CXXMethodDecl>(CE->getDirectCallee());
+ if (Callee == nullptr)
+ return nullptr;
+ const CXXRecordDecl *RD = Callee->getParent();
+ if (RD == nullptr)
+ return nullptr;
+ for (const auto *MD : RD->methods()) {
+ if (MD->getOverloadedOperator() == OO_Star && MD->isConst() &&
+ MD->getNumParams() == 0 && MD->getReturnType()->isReferenceType()) {
+ CanonicalCallee = MD;
+ break;
+ }
+ }
+ return CanonicalCallee;
+}
+
+} // namespace clang::dataflow
diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
index 4e1819bfa166a8..6c01ae8fc2e541 100644
--- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
@@ -21,6 +21,7 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests
SignAnalysisTest.cpp
SimplifyConstraintsTest.cpp
SingleVarConstantPropagationTest.cpp
+ SmartPointerAccessorCachingTest.cpp
TestingSupport.cpp
TestingSupportTest.cpp
TransferBranchTest.cpp
diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
index 6488833bd14cf2..62704b0e94b39a 100644
--- a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
@@ -148,6 +148,38 @@ TEST_F(CachedConstAccessorsLatticeTest, SameLocBeforeClearOrDiffAfterClear) {
EXPECT_NE(Loc3, Loc2);
}
+TEST_F(CachedConstAccessorsLatticeTest,
+ SameLocBeforeClearOrDiffAfterClearWithCalleeAndType) {
+ CommonTestInputs Inputs;
+ auto *CE = Inputs.CallRef;
+ RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(),
+ {});
+
+ LatticeT Lattice;
+ auto NopInit = [](StorageLocation &) {};
+ const FunctionDecl *Callee = CE->getDirectCallee();
+ ASSERT_NE(Callee, nullptr);
+ QualType Type = Callee->getReturnType();
+ Type.dump();
+ CE->dump();
+ StorageLocation &Loc1 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+ Loc, Callee, Type, Env, NopInit);
+ auto NotCalled = [](StorageLocation &) {
+ ASSERT_TRUE(false) << "Not reached";
+ };
+ StorageLocation &Loc2 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+ Loc, Callee, Type, Env, NotCalled);
+
+ EXPECT_EQ(&Loc1, &Loc2);
+
+ Lattice.clearConstMethodReturnStorageLocations(Loc);
+ StorageLocation &Loc3 = Lattice.getOrCreateConstMethodReturnStorageLocation(
+ Loc, Callee, Type, Env, NopInit);
+
+ EXPECT_NE(&Loc3, &Loc1);
+ EXPECT_NE(&Loc3, &Loc2);
+}
+
TEST_F(CachedConstAccessorsLatticeTest,
SameStructValBeforeClearOrDiffAfterClear) {
TestAST AST(R"cpp(
diff --git a/clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp b/clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
new file mode 100644
index 00000000000000..a69d606cdb85ea
--- /dev/null
+++ b/clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
@@ -0,0 +1,194 @@
+//===- unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.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/SmartPointerAccessorCaching.h"
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Testing/TestAST.h"
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+namespace clang::dataflow {
+namespace {
+
+using clang::ast_matchers::match;
+
+template <typename MatcherT>
+bool matches(llvm::StringRef Decls, llvm::StringRef TestInput,
+ MatcherT Matcher) {
+ TestAST InputAST(Decls.str() + TestInput.str());
+ return !match(Matcher, InputAST.context()).empty();
+}
+
+TEST(SmartPointerAccessorCachingTest, MatchesClassWithStarArrowGet) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct unique_ptr {
+ T* operator->() const;
+ T& operator*() const;
+ T* get() const;
+ };
+ } // namespace std
+
+ template <class T>
+ using UniquePtrAlias = std::unique_ptr<T>;
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return (*P).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P.get()->i; }",
+ isSmartPointerLikeGetMethodCall()));
+
+ EXPECT_TRUE(matches(Decls, "int target(UniquePtrAlias<S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfUnexpectedReturnTypes) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ // unique_ptr isn't really like this, but we aren't matching by name
+ template <class T, class U>
+ struct unique_ptr {
+ U* operator->() const;
+ T& operator*() const;
+ T* get() const;
+ };
+ } // namespace std
+
+ struct S { int i; };
+ struct T { int j; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S, T> P) { return (*P).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S, T> P) { return P->j; }",
+ isSmartPointerLikeOperatorArrow()));
+ // The class matching arguably accidentally matches, just because the
+ // instantiation is with S, S. Hopefully doesn't happen too much in real code
+ // with such operator* and operator-> overloads.
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr<S, S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfBinaryStar) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct unique_ptr {
+ T* operator->() const;
+ T& operator*(int x) const;
+ T* get() const;
+ };
+ } // namespace std
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(
+ matches(Decls, "int target(std::unique_ptr<S> P) { return (P * 10).i; }",
+ isSmartPointerLikeOperatorStar()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfNoConstOverloads) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct unique_ptr {
+ T* operator->();
+ T& operator*();
+ T* get();
+ };
+ } // namespace std
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return (*P).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_FALSE(
+ matches(Decls, "int target(std::unique_ptr<S> P) { return P.get()->i; }",
+ isSmartPointerLikeGetMethodCall()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfNoStarMethod) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct unique_ptr {
+ T* operator->();
+ T* get();
+ };
+ } // namespace std
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P->i; }",
+ isSmartPointerLikeGetMethodCall()));
+}
+
+TEST(SmartPointerAccessorCachingTest, MatchesWithValueAndNonConstOverloads) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct optional {
+ const T* operator->() const;
+ T* operator->();
+ const T& operator*() const;
+ T& operator*();
+ const T& value() const;
+ T& value();
+ };
+ } // namespace std
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_TRUE(matches(
+ Decls, "int target(std::optional<S> &NonConst) { return (*NonConst).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_TRUE(matches(
+ Decls, "int target(const std::optional<S> &Const) { return (*Const).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_TRUE(matches(
+ Decls, "int target(std::optional<S> &NonConst) { return NonConst->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_TRUE(matches(
+ Decls, "int target(const std::optional<S> &Const) { return Const->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_TRUE(matches(
+ Decls,
+ "int target(std::optional<S> &NonConst) { return NonConst.value().i; }",
+ isSmartPointerLikeValueMethodCall()));
+ EXPECT_TRUE(matches(
+ Decls,
+ "int target(const std::optional<S> &Const) { return Const.value().i; }",
+ isSmartPointerLikeValueMethodCall()));
+}
+
+} // namespace
+} // namespace clang::dataflow
\ No newline at end of file
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index de16f6be8eedbc..6f590a54f6da9b 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3771,6 +3771,56 @@ TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessorWithModInBetween) {
/*IgnoreSmartPointerDereference=*/false);
}
+TEST_P(UncheckedOptionalAccessTest, SmartPointerAccessorMixed) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ $ns::$optional<int> x;
+ };
+
+ namespace absl {
+ template<typename T>
+ class StatusOr {
+ public:
+ bool ok() const;
+
+ const T& operator*() const&;
+ T& operator*() &;
+ const T&& operator*() const&&;
+ T&& operator*() &&;
+
+ const T* operator->() const;
+ T* operator->();
+
+ const T& value() const;
+ T& value();
+ };
+ }
+
+ void target(absl::StatusOr<A> &mut, const absl::StatusOr<A> &imm) {
+ if (!mut.ok() || !imm.ok())
+ return;
+
+ if (mut->x.has_value()) {
+ mut->x.value();
+ ((*mut).x).value();
+ (mut.value().x).value();
+
+ // check flagged after modifying
+ mut = imm;
+ mut->x.value(); // [[unsafe]]
+ }
+ if (imm->x.has_value()) {
+ imm->x.value();
+ ((*imm).x).value();
+ (imm.value().x).value();
+ }
+ }
+ )cc",
+ /*IgnoreSmartPointerDereference=*/false);
+}
+
TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
>From 526d739741d18e862ed830d5e92e36d7b94fafc5 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Fri, 20 Dec 2024 16:43:24 +0000
Subject: [PATCH 2/6] Update a comment
---
.../Models/UncheckedOptionalAccessModel.h | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 713494178b97bd..fb11c2e230e328 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -37,14 +37,13 @@ struct UncheckedOptionalAccessModelOptions {
/// can't identify when their results are used safely (across calls),
/// resulting in false positives in all such cases. Note: this option does not
/// cover access through `operator[]`.
- /// FIXME: we currently cache and equate the result of const accessors
- /// returning pointers, so cover the case of operator-> followed by
- /// operator->, which covers the common case of smart pointers. We also cover
- /// some limited cases of returning references (if return type is an optional
- /// type), so cover some cases of operator* followed by operator*. We don't
- /// cover mixing operator-> and operator*. Once we are confident in this const
- /// accessor caching, we shouldn't need the IgnoreSmartPointerDereference
- /// option anymore.
+ ///
+ /// FIXME: we now cache and equate the result of const accessors
+ /// that look like unique_ptr, have both `->` (returning a pointer type) and
+ /// `*` (returning a reference type). This includes mixing `->` and
+ /// `*` in a sequence of calls as long as the object is not modified. Once we
+ /// are confident in this const accessor caching, we shouldn't need the
+ /// IgnoreSmartPointerDereference option anymore.
bool IgnoreSmartPointerDereference = false;
};
>From 2dab9fdc6a669593b7edeab892a9f19c2b436d23 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Fri, 20 Dec 2024 18:34:58 +0000
Subject: [PATCH 3/6] Misc cleanups
---
.../SmartPointerAccessorCaching.h | 22 ++++++++++---------
.../Models/UncheckedOptionalAccessModel.cpp | 8 +++----
.../CachedConstAccessorsLatticeTest.cpp | 2 --
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
index dff21256ddb6f1..2abd4caac01f87 100644
--- a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -68,38 +68,40 @@ ast_matchers::StatementMatcher isSmartPointerLikeGetMethodCall();
/// Returns the "canonical" callee for smart pointer operators (`*` and `->`)
/// as a key for caching.
///
-/// We choose `operator *` as the canonical one, since it needs a
+/// We choose `*` as the canonical one, since it needs a
/// StorageLocation anyway.
///
-/// Note: there may be multiple `operator*` (one const, one non-const)
-/// we pick the const one, which the above provided matchers require to exist.
+/// Note: there may be multiple `operator*` (one const, one non-const).
+/// We pick the const one, which the above provided matchers require to exist.
const FunctionDecl *
getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE);
-/// A transfer function for `operator*` (and `value`) calls.
+/// A transfer function for `operator*` (and `value`) calls
+/// that can be cached.
///
/// Requirements:
///
/// - LatticeT should use the `CachedConstAccessorsLattice` mixin.
template <typename LatticeT>
-void transferSmartPointerLikeDeref(
+void transferSmartPointerLikeCachedDeref(
const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc,
TransferState<LatticeT> &State,
llvm::function_ref<void(StorageLocation &)> InitializeLoc);
-/// A transfer function for `operator->` (and `get`) calls.
+/// A transfer function for `operator->` (and `get`) calls
+/// that can be cached.
///
/// Requirements:
///
/// - LatticeT should use the `CachedConstAccessorsLattice` mixin.
template <typename LatticeT>
-void transferSmartPointerLikeGet(
+void transferSmartPointerLikeCachedGet(
const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
TransferState<LatticeT> &State,
llvm::function_ref<void(StorageLocation &)> InitializeLoc);
template <typename LatticeT>
-void transferSmartPointerLikeDeref(
+void transferSmartPointerLikeCachedDeref(
const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc,
TransferState<LatticeT> &State,
llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
@@ -113,7 +115,7 @@ void transferSmartPointerLikeDeref(
return;
const FunctionDecl *CanonicalCallee =
getCanonicalSmartPointerLikeOperatorCallee(DerefExpr);
- // This shouldn't really happen, as we should at least find `Callee` itself.
+ // This shouldn't happen, as we should at least find `Callee` itself.
assert(CanonicalCallee != nullptr);
if (CanonicalCallee != Callee) {
// When using the provided matchers, we should always get a reference to
@@ -136,7 +138,7 @@ void transferSmartPointerLikeDeref(
}
template <typename LatticeT>
-void transferSmartPointerLikeGet(
+void transferSmartPointerLikeCachedGet(
const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
TransferState<LatticeT> &State,
llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index ce54b450a2121c..d287d25910c873 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -1043,7 +1043,7 @@ auto buildTransferMatchSwitch() {
[](const CXXOperatorCallExpr *E,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
- transferSmartPointerLikeDeref(
+ transferSmartPointerLikeCachedDeref(
E,
dyn_cast_or_null<RecordStorageLocation>(
getLocBehindPossiblePointer(*E->getArg(0), State.Env)),
@@ -1054,7 +1054,7 @@ auto buildTransferMatchSwitch() {
[](const CXXOperatorCallExpr *E,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
- transferSmartPointerLikeGet(
+ transferSmartPointerLikeCachedGet(
E,
dyn_cast_or_null<RecordStorageLocation>(
getLocBehindPossiblePointer(*E->getArg(0), State.Env)),
@@ -1064,7 +1064,7 @@ auto buildTransferMatchSwitch() {
isSmartPointerLikeValueMethodCall(),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
- transferSmartPointerLikeDeref(
+ transferSmartPointerLikeCachedDeref(
E, getImplicitObjectLocation(*E, State.Env), State,
[](StorageLocation &Loc) {});
})
@@ -1072,7 +1072,7 @@ auto buildTransferMatchSwitch() {
isSmartPointerLikeGetMethodCall(),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
- transferSmartPointerLikeGet(
+ transferSmartPointerLikeCachedGet(
E, getImplicitObjectLocation(*E, State.Env), State,
[](StorageLocation &Loc) {});
})
diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
index 62704b0e94b39a..9944d44832b831 100644
--- a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
@@ -160,8 +160,6 @@ TEST_F(CachedConstAccessorsLatticeTest,
const FunctionDecl *Callee = CE->getDirectCallee();
ASSERT_NE(Callee, nullptr);
QualType Type = Callee->getReturnType();
- Type.dump();
- CE->dump();
StorageLocation &Loc1 = Lattice.getOrCreateConstMethodReturnStorageLocation(
Loc, Callee, Type, Env, NopInit);
auto NotCalled = [](StorageLocation &) {
>From 18c099294ff2905e15e8e00fb8b1cecbd5c1d140 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Fri, 20 Dec 2024 19:30:08 +0000
Subject: [PATCH 4/6] Simplify test
---
.../Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp | 2 --
1 file changed, 2 deletions(-)
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 6f590a54f6da9b..19c3ff49eab27e 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3787,8 +3787,6 @@ TEST_P(UncheckedOptionalAccessTest, SmartPointerAccessorMixed) {
const T& operator*() const&;
T& operator*() &;
- const T&& operator*() const&&;
- T&& operator*() &&;
const T* operator->() const;
T* operator->();
>From f3eaa466a3d82a5e498c91f34ae4f3e254823a68 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 23 Dec 2024 19:17:17 +0000
Subject: [PATCH 5/6] Simplify API to get type from FunctionDecl's return type
and update a use
---
.../FlowSensitive/CachedConstAccessorsLattice.h | 13 ++++++-------
.../FlowSensitive/SmartPointerAccessorCaching.h | 6 ++----
.../Models/UncheckedOptionalAccessModel.cpp | 2 +-
.../CachedConstAccessorsLatticeTest.cpp | 9 ++++-----
4 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 6b5dacf9f66d2d..aaf89f4e94d4a7 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -82,18 +82,17 @@ template <typename Base> class CachedConstAccessorsLattice : public Base {
/// Creates or returns a previously created `StorageLocation` associated with
/// a const method call `obj.getFoo()` where `RecordLoc` is the
- /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
- /// and `Type` is the return type of `getFoo`.
+ /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`.
///
/// The callback `Initialize` runs on the storage location if newly created.
///
/// Requirements:
///
- /// - `Type` should return a location (GLValue or a record type).
+ /// - `Callee` should return a location (return type is a reference type or a
+ /// record type).
StorageLocation &getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
- QualType Type, Environment &Env,
- llvm::function_ref<void(StorageLocation &)> Initialize);
+ Environment &Env, llvm::function_ref<void(StorageLocation &)> Initialize);
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
ConstMethodReturnValues.erase(&RecordLoc);
@@ -236,9 +235,9 @@ template <typename Base>
StorageLocation &
CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
- QualType Type, Environment &Env,
- llvm::function_ref<void(StorageLocation &)> Initialize) {
+ Environment &Env, llvm::function_ref<void(StorageLocation &)> Initialize) {
assert(Callee != nullptr);
+ QualType Type = Callee->getReturnType();
assert(!Type.isNull());
assert(Type->isReferenceType() || Type->isRecordType());
auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
diff --git a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
index 2abd4caac01f87..641a3120b93437 100644
--- a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -132,8 +132,7 @@ void transferSmartPointerLikeCachedDeref(
StorageLocation &LocForValue =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
- *SmartPointerLoc, CanonicalCallee, CanonicalCallee->getReturnType(),
- State.Env, InitializeLoc);
+ *SmartPointerLoc, CanonicalCallee, State.Env, InitializeLoc);
State.Env.setStorageLocation(*DerefExpr, LocForValue);
}
@@ -151,8 +150,7 @@ void transferSmartPointerLikeCachedGet(
if (CanonicalCallee != nullptr) {
auto &LocForValue =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
- *SmartPointerLoc, CanonicalCallee, CanonicalCallee->getReturnType(),
- State.Env, InitializeLoc);
+ *SmartPointerLoc, CanonicalCallee, State.Env, InitializeLoc);
State.Env.setValue(*GetExpr,
State.Env.template create<PointerValue>(LocForValue));
} else {
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index d287d25910c873..2a6ad5d7c0d369 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -562,7 +562,7 @@ void handleConstMemberCall(const CallExpr *CE,
return;
StorageLocation &Loc =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
- *RecordLoc, DirectCallee, CE->getType(), State.Env,
+ *RecordLoc, DirectCallee, State.Env,
[&](StorageLocation &Loc) {
setHasValue(cast<RecordStorageLocation>(Loc),
State.Env.makeAtomicBoolValue(), State.Env);
diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
index 9944d44832b831..d27f6a6d27e710 100644
--- a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
@@ -149,7 +149,7 @@ TEST_F(CachedConstAccessorsLatticeTest, SameLocBeforeClearOrDiffAfterClear) {
}
TEST_F(CachedConstAccessorsLatticeTest,
- SameLocBeforeClearOrDiffAfterClearWithCalleeAndType) {
+ SameLocBeforeClearOrDiffAfterClearWithCallee) {
CommonTestInputs Inputs;
auto *CE = Inputs.CallRef;
RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(),
@@ -159,20 +159,19 @@ TEST_F(CachedConstAccessorsLatticeTest,
auto NopInit = [](StorageLocation &) {};
const FunctionDecl *Callee = CE->getDirectCallee();
ASSERT_NE(Callee, nullptr);
- QualType Type = Callee->getReturnType();
StorageLocation &Loc1 = Lattice.getOrCreateConstMethodReturnStorageLocation(
- Loc, Callee, Type, Env, NopInit);
+ Loc, Callee, Env, NopInit);
auto NotCalled = [](StorageLocation &) {
ASSERT_TRUE(false) << "Not reached";
};
StorageLocation &Loc2 = Lattice.getOrCreateConstMethodReturnStorageLocation(
- Loc, Callee, Type, Env, NotCalled);
+ Loc, Callee, Env, NotCalled);
EXPECT_EQ(&Loc1, &Loc2);
Lattice.clearConstMethodReturnStorageLocations(Loc);
StorageLocation &Loc3 = Lattice.getOrCreateConstMethodReturnStorageLocation(
- Loc, Callee, Type, Env, NopInit);
+ Loc, Callee, Env, NopInit);
EXPECT_NE(&Loc3, &Loc1);
EXPECT_NE(&Loc3, &Loc2);
>From 0b92eba48bc74936d066dec04c7e454cdf8712e9 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 23 Dec 2024 19:19:29 +0000
Subject: [PATCH 6/6] clang-format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 2a6ad5d7c0d369..e1394e28cd49a7 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -562,8 +562,7 @@ void handleConstMemberCall(const CallExpr *CE,
return;
StorageLocation &Loc =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
- *RecordLoc, DirectCallee, State.Env,
- [&](StorageLocation &Loc) {
+ *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
setHasValue(cast<RecordStorageLocation>(Loc),
State.Env.makeAtomicBoolValue(), State.Env);
});
More information about the cfe-commits
mailing list