Skip to content

Commit c526263

Browse files
committed
[clang][dataflow] Use smart pointer caching in unchecked optional accessor
Part 2 (and final part) following llvm#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.
1 parent 3c3094b commit c526263

File tree

9 files changed

+692
-6
lines changed

9 files changed

+692
-6
lines changed

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@
1313
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
1414
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
1515

16+
#include "clang/AST/Decl.h"
1617
#include "clang/AST/Expr.h"
18+
#include "clang/AST/Type.h"
1719
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
1820
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
1921
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -71,10 +73,28 @@ template <typename Base> class CachedConstAccessorsLattice : public Base {
7173
/// Requirements:
7274
///
7375
/// - `CE` should return a location (GLValue or a record type).
76+
///
77+
/// DEPRECATED: switch users to the below overload which takes Callee and Type
78+
/// directly.
7479
StorageLocation *getOrCreateConstMethodReturnStorageLocation(
7580
const RecordStorageLocation &RecordLoc, const CallExpr *CE,
7681
Environment &Env, llvm::function_ref<void(StorageLocation &)> Initialize);
7782

83+
/// Creates or returns a previously created `StorageLocation` associated with
84+
/// a const method call `obj.getFoo()` where `RecordLoc` is the
85+
/// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
86+
/// and `Type` is the return type of `getFoo`.
87+
///
88+
/// The callback `Initialize` runs on the storage location if newly created.
89+
///
90+
/// Requirements:
91+
///
92+
/// - `Type` should return a location (GLValue or a record type).
93+
StorageLocation &getOrCreateConstMethodReturnStorageLocation(
94+
const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
95+
QualType Type, Environment &Env,
96+
llvm::function_ref<void(StorageLocation &)> Initialize);
97+
7898
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
7999
ConstMethodReturnValues.erase(&RecordLoc);
80100
}
@@ -212,6 +232,27 @@ CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
212232
return &Loc;
213233
}
214234

235+
template <typename Base>
236+
StorageLocation &
237+
CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
238+
const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
239+
QualType Type, Environment &Env,
240+
llvm::function_ref<void(StorageLocation &)> Initialize) {
241+
assert(Callee != nullptr);
242+
assert(!Type.isNull());
243+
assert(Type->isReferenceType() || Type->isRecordType());
244+
auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
245+
auto it = ObjMap.find(Callee);
246+
if (it != ObjMap.end())
247+
return *it->second;
248+
249+
StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType());
250+
Initialize(Loc);
251+
252+
ObjMap.insert({Callee, &Loc});
253+
return Loc;
254+
}
255+
215256
} // namespace dataflow
216257
} // namespace clang
217258

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
//===-- SmartPointerAccessorCaching.h ---------------------------*- C++ -*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
//
9+
// This file defines utilities to help cache accessors for smart pointer
10+
// like objects.
11+
//
12+
// These should be combined with CachedConstAccessorsLattice.
13+
// Beyond basic const accessors, smart pointers may have the following two
14+
// additional issues:
15+
//
16+
// 1) There may be multiple accessors for the same underlying object, e.g.
17+
// `operator->`, `operator*`, and `get`. Users may use a mixture of these
18+
// accessors, so the cache should unify them.
19+
//
20+
// 2) There may be non-const overloads of accessors. They are still safe to
21+
// cache, as they don't modify the container object.
22+
//===----------------------------------------------------------------------===//
23+
24+
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
25+
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
26+
27+
#include <cassert>
28+
29+
#include "clang/AST/Decl.h"
30+
#include "clang/AST/Expr.h"
31+
#include "clang/AST/Stmt.h"
32+
#include "clang/ASTMatchers/ASTMatchers.h"
33+
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
34+
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
35+
#include "clang/Analysis/FlowSensitive/Value.h"
36+
#include "llvm/ADT/STLFunctionalExtras.h"
37+
38+
namespace clang::dataflow {
39+
40+
/// Matchers:
41+
/// For now, these match on any class with an `operator*` or `operator->`
42+
/// where the return types have a similar shape as std::unique_ptr
43+
/// and std::optional.
44+
///
45+
/// - `*` returns a reference to a type `T`
46+
/// - `->` returns a pointer to `T`
47+
/// - `get` returns a pointer to `T`
48+
/// - `value` returns a reference `T`
49+
///
50+
/// (1) The `T` should all match across the accessors (ignoring qualifiers).
51+
///
52+
/// (2) The specific accessor used in a call isn't required to be const,
53+
/// but the class must have a const overload of each accessor.
54+
///
55+
/// For now, we don't have customization to ignore certain classes.
56+
/// For example, if writing a ClangTidy check for `std::optional`, these
57+
/// would also match `std::optional`. In order to have special handling
58+
/// for `std::optional`, we assume the (Matcher, TransferFunction) case
59+
/// with custom handling is ordered early so that these generic cases
60+
/// do not trigger.
61+
ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar();
62+
ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorArrow();
63+
ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeValueMethodCall();
64+
ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeGetMethodCall();
65+
66+
// Common transfer functions.
67+
68+
/// Returns the "canonical" callee for smart pointer operators (`*` and `->`)
69+
/// as a key for caching.
70+
///
71+
/// We choose `operator *` as the canonical one, since it needs a
72+
/// StorageLocation anyway.
73+
///
74+
/// Note: there may be multiple `operator*` (one const, one non-const)
75+
/// we pick the const one, which the above provided matchers require to exist.
76+
const FunctionDecl *
77+
getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE);
78+
79+
/// A transfer function for `operator*` (and `value`) calls.
80+
///
81+
/// Requirements:
82+
///
83+
/// - LatticeT should use the `CachedConstAccessorsLattice` mixin.
84+
template <typename LatticeT>
85+
void transferSmartPointerLikeDeref(
86+
const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc,
87+
TransferState<LatticeT> &State,
88+
llvm::function_ref<void(StorageLocation &)> InitializeLoc);
89+
90+
/// A transfer function for `operator->` (and `get`) calls.
91+
///
92+
/// Requirements:
93+
///
94+
/// - LatticeT should use the `CachedConstAccessorsLattice` mixin.
95+
template <typename LatticeT>
96+
void transferSmartPointerLikeGet(
97+
const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
98+
TransferState<LatticeT> &State,
99+
llvm::function_ref<void(StorageLocation &)> InitializeLoc);
100+
101+
template <typename LatticeT>
102+
void transferSmartPointerLikeDeref(
103+
const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc,
104+
TransferState<LatticeT> &State,
105+
llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
106+
if (State.Env.getStorageLocation(*DerefExpr) != nullptr)
107+
return;
108+
if (SmartPointerLoc == nullptr)
109+
return;
110+
111+
const FunctionDecl *Callee = DerefExpr->getDirectCallee();
112+
if (Callee == nullptr)
113+
return;
114+
const FunctionDecl *CanonicalCallee =
115+
getCanonicalSmartPointerLikeOperatorCallee(DerefExpr);
116+
// This shouldn't really happen, as we should at least find `Callee` itself.
117+
assert(CanonicalCallee != nullptr);
118+
if (CanonicalCallee != Callee) {
119+
// When using the provided matchers, we should always get a reference to
120+
// the same type.
121+
assert(CanonicalCallee->getReturnType()->isReferenceType() &&
122+
Callee->getReturnType()->isReferenceType());
123+
assert(CanonicalCallee->getReturnType()
124+
.getNonReferenceType()
125+
->getCanonicalTypeUnqualified() ==
126+
Callee->getReturnType()
127+
.getNonReferenceType()
128+
->getCanonicalTypeUnqualified());
129+
}
130+
131+
StorageLocation &LocForValue =
132+
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
133+
*SmartPointerLoc, CanonicalCallee, CanonicalCallee->getReturnType(),
134+
State.Env, InitializeLoc);
135+
State.Env.setStorageLocation(*DerefExpr, LocForValue);
136+
}
137+
138+
template <typename LatticeT>
139+
void transferSmartPointerLikeGet(
140+
const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
141+
TransferState<LatticeT> &State,
142+
llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
143+
if (SmartPointerLoc == nullptr)
144+
return;
145+
146+
const FunctionDecl *CanonicalCallee =
147+
getCanonicalSmartPointerLikeOperatorCallee(GetExpr);
148+
149+
if (CanonicalCallee != nullptr) {
150+
auto &LocForValue =
151+
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
152+
*SmartPointerLoc, CanonicalCallee, CanonicalCallee->getReturnType(),
153+
State.Env, InitializeLoc);
154+
State.Env.setValue(*GetExpr,
155+
State.Env.template create<PointerValue>(LocForValue));
156+
} else {
157+
// Otherwise, just cache the pointer value as if it was a const accessor.
158+
Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(
159+
*SmartPointerLoc, GetExpr, State.Env);
160+
if (Val == nullptr)
161+
return;
162+
State.Env.setValue(*GetExpr, *Val);
163+
}
164+
}
165+
166+
} // namespace clang::dataflow
167+
168+
#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H

clang/lib/Analysis/FlowSensitive/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ add_clang_library(clangAnalysisFlowSensitive
1010
Logger.cpp
1111
RecordOps.cpp
1212
SimplifyConstraints.cpp
13+
SmartPointerAccessorCaching.cpp
1314
Transfer.cpp
1415
TypeErasedDataflowAnalysis.cpp
1516
Value.cpp

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,10 @@
2525
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
2626
#include "clang/Analysis/FlowSensitive/Formula.h"
2727
#include "clang/Analysis/FlowSensitive/RecordOps.h"
28+
#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
2829
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
2930
#include "clang/Analysis/FlowSensitive/Value.h"
31+
#include "clang/Basic/OperatorKinds.h"
3032
#include "clang/Basic/SourceLocation.h"
3133
#include "llvm/ADT/StringRef.h"
3234
#include "llvm/Support/Casting.h"
@@ -555,24 +557,26 @@ void handleConstMemberCall(const CallExpr *CE,
555557
LatticeTransferState &State) {
556558
// If the const method returns an optional or reference to an optional.
557559
if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
558-
StorageLocation *Loc =
560+
const FunctionDecl *DirectCallee = CE->getDirectCallee();
561+
if (DirectCallee == nullptr)
562+
return;
563+
StorageLocation &Loc =
559564
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
560-
*RecordLoc, CE, State.Env, [&](StorageLocation &Loc) {
565+
*RecordLoc, DirectCallee, CE->getType(), State.Env,
566+
[&](StorageLocation &Loc) {
561567
setHasValue(cast<RecordStorageLocation>(Loc),
562568
State.Env.makeAtomicBoolValue(), State.Env);
563569
});
564-
if (Loc == nullptr)
565-
return;
566570
if (CE->isGLValue()) {
567571
// If the call to the const method returns a reference to an optional,
568572
// link the call expression to the cached StorageLocation.
569-
State.Env.setStorageLocation(*CE, *Loc);
573+
State.Env.setStorageLocation(*CE, Loc);
570574
} else {
571575
// If the call to the const method returns an optional by value, we
572576
// need to use CopyRecord to link the optional to the result object
573577
// of the call expression.
574578
auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
575-
copyRecord(*cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
579+
copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
576580
}
577581
return;
578582
}
@@ -1031,6 +1035,48 @@ auto buildTransferMatchSwitch() {
10311035
transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env);
10321036
})
10331037

1038+
// Smart-pointer-like operator* and operator-> calls that may look like
1039+
// const accessors (below) but need special handling to allow mixing
1040+
// the accessor calls.
1041+
.CaseOfCFGStmt<CXXOperatorCallExpr>(
1042+
isSmartPointerLikeOperatorStar(),
1043+
[](const CXXOperatorCallExpr *E,
1044+
const MatchFinder::MatchResult &Result,
1045+
LatticeTransferState &State) {
1046+
transferSmartPointerLikeDeref(
1047+
E,
1048+
dyn_cast_or_null<RecordStorageLocation>(
1049+
getLocBehindPossiblePointer(*E->getArg(0), State.Env)),
1050+
State, [](StorageLocation &Loc) {});
1051+
})
1052+
.CaseOfCFGStmt<CXXOperatorCallExpr>(
1053+
isSmartPointerLikeOperatorArrow(),
1054+
[](const CXXOperatorCallExpr *E,
1055+
const MatchFinder::MatchResult &Result,
1056+
LatticeTransferState &State) {
1057+
transferSmartPointerLikeGet(
1058+
E,
1059+
dyn_cast_or_null<RecordStorageLocation>(
1060+
getLocBehindPossiblePointer(*E->getArg(0), State.Env)),
1061+
State, [](StorageLocation &Loc) {});
1062+
})
1063+
.CaseOfCFGStmt<CXXMemberCallExpr>(
1064+
isSmartPointerLikeValueMethodCall(),
1065+
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result,
1066+
LatticeTransferState &State) {
1067+
transferSmartPointerLikeDeref(
1068+
E, getImplicitObjectLocation(*E, State.Env), State,
1069+
[](StorageLocation &Loc) {});
1070+
})
1071+
.CaseOfCFGStmt<CXXMemberCallExpr>(
1072+
isSmartPointerLikeGetMethodCall(),
1073+
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result,
1074+
LatticeTransferState &State) {
1075+
transferSmartPointerLikeGet(
1076+
E, getImplicitObjectLocation(*E, State.Env), State,
1077+
[](StorageLocation &Loc) {});
1078+
})
1079+
10341080
// const accessor calls
10351081
.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
10361082
transferValue_ConstMemberCall)

0 commit comments

Comments
 (0)