[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
Jan Voung via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 17 07:46:39 PST 2024
https://github.com/jvoung created https://github.com/llvm/llvm-project/pull/120249
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.
>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] [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"
More information about the cfe-commits
mailing list