Skip to content

Commit 72a28a3

Browse files
jvoungjvoung
authored
[clang][dataflow] Use smart pointer caching in unchecked optional accessor (#120249)
Part 2 (and final part) following #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 d07762e commit 72a28a3

File tree

7 files changed

+302
-14
lines changed

7 files changed

+302
-14
lines changed

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

Lines changed: 40 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,27 @@ 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+
///
87+
/// The callback `Initialize` runs on the storage location if newly created.
88+
///
89+
/// Requirements:
90+
///
91+
/// - `Callee` should return a location (return type is a reference type or a
92+
/// record type).
93+
StorageLocation &getOrCreateConstMethodReturnStorageLocation(
94+
const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
95+
Environment &Env, llvm::function_ref<void(StorageLocation &)> Initialize);
96+
7897
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
7998
ConstMethodReturnValues.erase(&RecordLoc);
8099
}
@@ -212,6 +231,27 @@ CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation(
212231
return &Loc;
213232
}
214233

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

‎clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,13 @@ struct UncheckedOptionalAccessModelOptions {
3737
/// can't identify when their results are used safely (across calls),
3838
/// resulting in false positives in all such cases. Note: this option does not
3939
/// cover access through `operator[]`.
40-
/// FIXME: we currently cache and equate the result of const accessors
41-
/// returning pointers, so cover the case of operator-> followed by
42-
/// operator->, which covers the common case of smart pointers. We also cover
43-
/// some limited cases of returning references (if return type is an optional
44-
/// type), so cover some cases of operator* followed by operator*. We don't
45-
/// cover mixing operator-> and operator*. Once we are confident in this const
46-
/// accessor caching, we shouldn't need the IgnoreSmartPointerDereference
47-
/// option anymore.
40+
///
41+
/// FIXME: we now cache and equate the result of const accessors
42+
/// that look like unique_ptr, have both `->` (returning a pointer type) and
43+
/// `*` (returning a reference type). This includes mixing `->` and
44+
/// `*` in a sequence of calls as long as the object is not modified. Once we
45+
/// are confident in this const accessor caching, we shouldn't need the
46+
/// IgnoreSmartPointerDereference option anymore.
4847
bool IgnoreSmartPointerDereference = false;
4948
};
5049

‎clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,13 @@
2727
#include <cassert>
2828

2929
#include "clang/AST/Decl.h"
30+
#include "clang/AST/Expr.h"
3031
#include "clang/AST/Stmt.h"
3132
#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"
3237

3338
namespace clang::dataflow {
3439

@@ -58,6 +63,107 @@ ast_matchers::StatementMatcher isSmartPointerLikeOperatorArrow();
5863
ast_matchers::StatementMatcher isSmartPointerLikeValueMethodCall();
5964
ast_matchers::StatementMatcher isSmartPointerLikeGetMethodCall();
6065

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 `*` 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 that can be
80+
/// cached. Runs the `InitializeLoc` callback to initialize any new
81+
/// StorageLocations.
82+
///
83+
/// Requirements:
84+
///
85+
/// - LatticeT should use the `CachedConstAccessorsLattice` mixin.
86+
template <typename LatticeT>
87+
void transferSmartPointerLikeCachedDeref(
88+
const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc,
89+
TransferState<LatticeT> &State,
90+
llvm::function_ref<void(StorageLocation &)> InitializeLoc);
91+
92+
/// A transfer function for `operator->` (and `get`) calls that can be cached.
93+
/// Runs the `InitializeLoc` callback to initialize any new StorageLocations.
94+
///
95+
/// Requirements:
96+
///
97+
/// - LatticeT should use the `CachedConstAccessorsLattice` mixin.
98+
template <typename LatticeT>
99+
void transferSmartPointerLikeCachedGet(
100+
const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
101+
TransferState<LatticeT> &State,
102+
llvm::function_ref<void(StorageLocation &)> InitializeLoc);
103+
104+
template <typename LatticeT>
105+
void transferSmartPointerLikeCachedDeref(
106+
const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc,
107+
TransferState<LatticeT> &State,
108+
llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
109+
if (State.Env.getStorageLocation(*DerefExpr) != nullptr)
110+
return;
111+
if (SmartPointerLoc == nullptr)
112+
return;
113+
114+
const FunctionDecl *Callee = DerefExpr->getDirectCallee();
115+
if (Callee == nullptr)
116+
return;
117+
const FunctionDecl *CanonicalCallee =
118+
getCanonicalSmartPointerLikeOperatorCallee(DerefExpr);
119+
// This shouldn't happen, as we should at least find `Callee` itself.
120+
assert(CanonicalCallee != nullptr);
121+
if (CanonicalCallee != Callee) {
122+
// When using the provided matchers, we should always get a reference to
123+
// the same type.
124+
assert(CanonicalCallee->getReturnType()->isReferenceType() &&
125+
Callee->getReturnType()->isReferenceType());
126+
assert(CanonicalCallee->getReturnType()
127+
.getNonReferenceType()
128+
->getCanonicalTypeUnqualified() ==
129+
Callee->getReturnType()
130+
.getNonReferenceType()
131+
->getCanonicalTypeUnqualified());
132+
}
133+
134+
StorageLocation &LocForValue =
135+
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
136+
*SmartPointerLoc, CanonicalCallee, State.Env, InitializeLoc);
137+
State.Env.setStorageLocation(*DerefExpr, LocForValue);
138+
}
139+
140+
template <typename LatticeT>
141+
void transferSmartPointerLikeCachedGet(
142+
const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
143+
TransferState<LatticeT> &State,
144+
llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
145+
if (SmartPointerLoc == nullptr)
146+
return;
147+
148+
const FunctionDecl *CanonicalCallee =
149+
getCanonicalSmartPointerLikeOperatorCallee(GetExpr);
150+
151+
if (CanonicalCallee != nullptr) {
152+
auto &LocForValue =
153+
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
154+
*SmartPointerLoc, CanonicalCallee, State.Env, InitializeLoc);
155+
State.Env.setValue(*GetExpr,
156+
State.Env.template create<PointerValue>(LocForValue));
157+
} else {
158+
// Otherwise, just cache the pointer value as if it was a const accessor.
159+
Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(
160+
*SmartPointerLoc, GetExpr, State.Env);
161+
if (Val == nullptr)
162+
return;
163+
State.Env.setValue(*GetExpr, *Val);
164+
}
165+
}
166+
61167
} // namespace clang::dataflow
62168

63169
#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H

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

Lines changed: 51 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,25 @@ 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, State.Env, [&](StorageLocation &Loc) {
561566
setHasValue(cast<RecordStorageLocation>(Loc),
562567
State.Env.makeAtomicBoolValue(), State.Env);
563568
});
564-
if (Loc == nullptr)
565-
return;
566569
if (CE->isGLValue()) {
567570
// If the call to the const method returns a reference to an optional,
568571
// link the call expression to the cached StorageLocation.
569-
State.Env.setStorageLocation(*CE, *Loc);
572+
State.Env.setStorageLocation(*CE, Loc);
570573
} else {
571574
// If the call to the const method returns an optional by value, we
572575
// need to use CopyRecord to link the optional to the result object
573576
// of the call expression.
574577
auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
575-
copyRecord(*cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
578+
copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
576579
}
577580
return;
578581
}
@@ -1031,6 +1034,48 @@ auto buildTransferMatchSwitch() {
10311034
transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env);
10321035
})
10331036

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

‎clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ ast_matchers::StatementMatcher isSmartPointerLikeOperatorArrow() {
132132
callee(cxxMethodDecl(parameterCountIs(0), returns(pointerType()),
133133
ofClass(smartPointerClassWithGetOrValue()))));
134134
}
135+
135136
ast_matchers::StatementMatcher isSmartPointerLikeValueMethodCall() {
136137
return cxxMemberCallExpr(callee(
137138
cxxMethodDecl(parameterCountIs(0), returns(referenceType()),
@@ -144,4 +145,24 @@ ast_matchers::StatementMatcher isSmartPointerLikeGetMethodCall() {
144145
ofClass(smartPointerClassWithGet()))));
145146
}
146147

148+
const FunctionDecl *
149+
getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE) {
150+
const FunctionDecl *CanonicalCallee = nullptr;
151+
const CXXMethodDecl *Callee =
152+
cast_or_null<CXXMethodDecl>(CE->getDirectCallee());
153+
if (Callee == nullptr)
154+
return nullptr;
155+
const CXXRecordDecl *RD = Callee->getParent();
156+
if (RD == nullptr)
157+
return nullptr;
158+
for (const auto *MD : RD->methods()) {
159+
if (MD->getOverloadedOperator() == OO_Star && MD->isConst() &&
160+
MD->getNumParams() == 0 && MD->getReturnType()->isReferenceType()) {
161+
CanonicalCallee = MD;
162+
break;
163+
}
164+
}
165+
return CanonicalCallee;
166+
}
167+
147168
} // namespace clang::dataflow

‎clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,35 @@ TEST_F(CachedConstAccessorsLatticeTest, SameLocBeforeClearOrDiffAfterClear) {
148148
EXPECT_NE(Loc3, Loc2);
149149
}
150150

151+
TEST_F(CachedConstAccessorsLatticeTest,
152+
SameLocBeforeClearOrDiffAfterClearWithCallee) {
153+
CommonTestInputs Inputs;
154+
auto *CE = Inputs.CallRef;
155+
RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(),
156+
{});
157+
158+
LatticeT Lattice;
159+
auto NopInit = [](StorageLocation &) {};
160+
const FunctionDecl *Callee = CE->getDirectCallee();
161+
ASSERT_NE(Callee, nullptr);
162+
StorageLocation &Loc1 = Lattice.getOrCreateConstMethodReturnStorageLocation(
163+
Loc, Callee, Env, NopInit);
164+
auto NotCalled = [](StorageLocation &) {
165+
ASSERT_TRUE(false) << "Not reached";
166+
};
167+
StorageLocation &Loc2 = Lattice.getOrCreateConstMethodReturnStorageLocation(
168+
Loc, Callee, Env, NotCalled);
169+
170+
EXPECT_EQ(&Loc1, &Loc2);
171+
172+
Lattice.clearConstMethodReturnStorageLocations(Loc);
173+
StorageLocation &Loc3 = Lattice.getOrCreateConstMethodReturnStorageLocation(
174+
Loc, Callee, Env, NopInit);
175+
176+
EXPECT_NE(&Loc3, &Loc1);
177+
EXPECT_NE(&Loc3, &Loc2);
178+
}
179+
151180
TEST_F(CachedConstAccessorsLatticeTest,
152181
SameStructValBeforeClearOrDiffAfterClear) {
153182
TestAST AST(R"cpp(

0 commit comments

Comments
 (0)