Skip to content

Commit 5b55db2

Browse files
qinsoonqinsoon
committed
Properly inherit pinning state for binding variables, jl_propagate_root
and deriving expr. Raise an error if we attempt to pin a moved value.
1 parent f579da0 commit 5b55db2

File tree

3 files changed

+149
-15
lines changed

3 files changed

+149
-15
lines changed

‎src/clangsa/GCChecker.cpp

Lines changed: 65 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@
33
// Assumptions for pinning:
44
// * args need to be pinned
55
// * JL_ROOTING_ARGUMENT and JL_ROOTED_ARGUMENT will propagate pinning state as well.
6+
// * The checker may not consider alias for derived pointers in some cases.
7+
// * if f(x) returns a derived pointer from x, a = f(x); b = f(x); PTR_PIN(a); The checker will NOT find b as pinned.
8+
// * a = x->y; b = x->y; PTR_PIN(a); The checker will find b as pinned.
9+
// * Need to see if this affects correctness.
610

711
#include "clang/Frontend/FrontendActions.h"
812
#include "clang/StaticAnalyzer/Checkers/SValExplainer.h"
@@ -96,6 +100,7 @@ class GCChecker
96100
llvm::dbgs() << ((P == TransitivelyPinned) ? "TransitivelyPinned"
97101
: (P == Pinned) ? "Pinned"
98102
: (P == NotPinned) ? "NotPinned"
103+
: (P == Moved) ? "Moved"
99104
: "Error");
100105
llvm::dbgs() << ",";
101106
if (S == Rooted)
@@ -325,6 +330,7 @@ class GCChecker
325330
SymbolRef getSymbolForResult(const Expr *Result, const ValueState *OldValS,
326331
ProgramStateRef &State, CheckerContext &C) const;
327332
void validateValue(const GCChecker::ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message) const;
333+
void validateValueRootnessOnly(const GCChecker::ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message) const;
328334
void validateValue(const GCChecker::ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message, SourceRange range) const;
329335
int validateValueInner(const GCChecker::ValueState* VS) const;
330336
GCChecker::ValueState getRootedFromRegion(const MemRegion *Region, const PinState *PS, int Depth) const;
@@ -401,13 +407,16 @@ SymbolRef GCChecker::walkToRoot(callback f, const ProgramStateRef &State,
401407
const MemRegion *Region) {
402408
if (!Region)
403409
return nullptr;
410+
logWithDump("- walkToRoot, Region", Region);
404411
while (true) {
405412
const SymbolicRegion *SR = Region->getSymbolicBase();
406413
if (!SR) {
407414
return nullptr;
408415
}
409416
SymbolRef Sym = SR->getSymbol();
410417
const ValueState *OldVState = State->get<GCValueMap>(Sym);
418+
logWithDump("- walkToRoot, Sym", Sym);
419+
logWithDump("- walkToRoot, OldVState", OldVState);
411420
if (f(Sym, OldVState)) {
412421
if (const SymbolRegionValue *SRV = dyn_cast<SymbolRegionValue>(Sym)) {
413422
Region = SRV->getRegion();
@@ -468,6 +477,15 @@ void GCChecker::validateValue(const ValueState* VS, CheckerContext &C, SymbolRef
468477
}
469478
}
470479

480+
void GCChecker::validateValueRootnessOnly(const ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message) const {
481+
int v = validateValueInner(VS);
482+
if (v == FREED) {
483+
GCChecker::report_value_error(C, Sym, (std::string(message) + " GCed").c_str());
484+
} else if (v == MOVED) {
485+
// We don't care if it is moved
486+
}
487+
}
488+
471489
void GCChecker::validateValue(const ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message) const {
472490
int v = validateValueInner(VS);
473491
if (v == FREED) {
@@ -1106,7 +1124,11 @@ bool GCChecker::processPotentialSafepoint(const CallEvent &Call,
11061124
if (RetSym == I.getKey())
11071125
continue;
11081126
if (I.getData().isNotPinned()) {
1109-
State = State->set<GCValueMap>(I.getKey(), ValueState::getMoved(I.getData()));
1127+
logWithDump("- move unpinned values, Sym", I.getKey());
1128+
logWithDump("- move unpinned values, VS", I.getData());
1129+
auto NewVS = ValueState::getMoved(I.getData());
1130+
State = State->set<GCValueMap>(I.getKey(), NewVS);
1131+
logWithDump("- move unpinned values, NewVS", NewVS);
11101132
DidChange = true;
11111133
}
11121134
}
@@ -1178,6 +1200,8 @@ bool GCChecker::processAllocationOfResult(const CallEvent &Call,
11781200
Call.getOriginExpr(), C.getLocationContext(), QT, C.blockCount());
11791201
State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), S);
11801202
Sym = S.getAsSymbol();
1203+
logWithDump("- conjureSymbolVal, S", S);
1204+
logWithDump("- conjureSymbolVal, Sym", Sym);
11811205
}
11821206
if (isGloballyRootedType(QT))
11831207
State = State->set<GCValueMap>(Sym, ValueState::getRooted(nullptr, ValueState::pinState(isGloballyTransitivelyPinnedType(QT)), -1));
@@ -1231,8 +1255,11 @@ bool GCChecker::processAllocationOfResult(const CallEvent &Call,
12311255
const MemRegion *Region = Test.getAsRegion();
12321256
const ValueState *OldVState =
12331257
getValStateForRegion(C.getASTContext(), State, Region);
1234-
if (OldVState)
1235-
NewVState = *OldVState;
1258+
if (OldVState) {
1259+
NewVState = ValueState::inheritState(*OldVState);
1260+
logWithDump("- jl_propagates_root, OldVState", *OldVState);
1261+
logWithDump("- jl_propagates_root, NewVState", NewVState);
1262+
}
12361263
break;
12371264
}
12381265
}
@@ -1360,6 +1387,7 @@ void GCChecker::checkDerivingExpr(const Expr *Result, const Expr *Parent,
13601387
SymbolRef OldSym = ParentVal.getAsSymbol(true);
13611388
const MemRegion *Region = C.getSVal(Parent).getAsRegion();
13621389
const ValueState *OldValS = OldSym ? State->get<GCValueMap>(OldSym) : nullptr;
1390+
logWithDump("- Region", Region);
13631391
logWithDump("- OldSym", OldSym);
13641392
logWithDump("- OldValS", OldValS);
13651393
SymbolRef NewSym = getSymbolForResult(Result, OldValS, State, C);
@@ -1369,6 +1397,7 @@ void GCChecker::checkDerivingExpr(const Expr *Result, const Expr *Parent,
13691397
logWithDump("- NewSym", NewSym);
13701398
// NewSym might already have a better root
13711399
const ValueState *NewValS = State->get<GCValueMap>(NewSym);
1400+
logWithDump("- NewValS", NewValS);
13721401
if (Region) {
13731402
const VarRegion *VR = Region->getAs<VarRegion>();
13741403
bool inheritedState = false;
@@ -1418,8 +1447,9 @@ void GCChecker::checkDerivingExpr(const Expr *Result, const Expr *Parent,
14181447
validateValue(OldValS, C, OldSym, "Creating derivative of value that may have been");
14191448
if (!OldValS->isPotentiallyFreed() && ResultTracked) {
14201449
logWithDump("- Set as OldValS, Sym", NewSym);
1421-
logWithDump("- Set as OldValS, VS", OldValS);
1422-
C.addTransition(State->set<GCValueMap>(NewSym, *OldValS));
1450+
auto InheritVS = ValueState::inheritState(*OldValS);
1451+
logWithDump("- Set as OldValS, InheritVS", InheritVS);
1452+
C.addTransition(State->set<GCValueMap>(NewSym, InheritVS));
14231453
return;
14241454
}
14251455
}
@@ -1739,6 +1769,13 @@ bool GCChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
17391769
report_error(C, "Can not understand this pin.");
17401770
return true;
17411771
}
1772+
1773+
const ValueState *OldVS = C.getState()->get<GCValueMap>(Sym);
1774+
if (OldVS && OldVS->isMoved()) {
1775+
report_error(C, "Attempt to PIN a value that is already moved.");
1776+
return true;
1777+
}
1778+
17421779
auto MRV = Arg.getAs<loc::MemRegionVal>();
17431780
if (!MRV) {
17441781
report_error(C, "PTR_PIN with something other than a local variable");
@@ -1747,7 +1784,6 @@ bool GCChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
17471784
const MemRegion *Region = MRV->getRegion();
17481785
auto State = C.getState()->set<GCPinMap>(Region, PinState::getPin(CurrentDepth));
17491786
logWithDump("- Pin region", Region);
1750-
const ValueState *OldVS = State->get<GCValueMap>(Sym);
17511787
State = State->set<GCValueMap>(Sym, ValueState::getPinned(*OldVS));
17521788
logWithDump("- Pin value", Sym);
17531789
C.addTransition(State);
@@ -1899,8 +1935,11 @@ void GCChecker::checkBind(SVal LVal, SVal RVal, const clang::Stmt *S,
18991935
const ValueState *ValSP = nullptr;
19001936
ValueState ValS;
19011937
if (rootRegionIfGlobal(R->getBaseRegion(), State, C, &ValS)) {
1938+
logWithDump("- rootRegionIfGlobal, base", R->getBaseRegion());
19021939
ValSP = &ValS;
1940+
logWithDump("- rootRegionIfGlobal ValSP", ValSP);
19031941
} else {
1942+
logWithDump("- getValStateForRegion", R);
19041943
ValSP = getValStateForRegion(C.getASTContext(), State, R);
19051944
}
19061945
if (ValSP && ValSP->isRooted()) {
@@ -1910,8 +1949,9 @@ void GCChecker::checkBind(SVal LVal, SVal RVal, const clang::Stmt *S,
19101949
RValState->RootDepth < ValSP->RootDepth) {
19111950
logWithDump("- No need to set ValState, current ValState", RValState);
19121951
} else {
1913-
logWithDump("- Set ValState, current ValState", ValSP);
1914-
C.addTransition(State->set<GCValueMap>(Sym, *ValSP));
1952+
auto InheritVS = ValueState::inheritState(*ValSP);
1953+
logWithDump("- Set ValState, InheritVS", InheritVS);
1954+
C.addTransition(State->set<GCValueMap>(Sym, InheritVS));
19151955
}
19161956
}
19171957
} else {
@@ -2008,42 +2048,55 @@ void GCChecker::checkLocation(SVal SLoc, bool IsLoad, const Stmt *S,
20082048
// Loading from a root produces a rooted symbol. TODO: Can we do something
20092049
// better than this.
20102050
if (IsLoad && (RS = State->get<GCRootMap>(SLoc.getAsRegion()))) {
2051+
logWithDump("- IsLoad, RS", RS);
20112052
SymbolRef LoadedSym =
20122053
State->getSVal(SLoc.getAs<Loc>().getValue()).getAsSymbol();
20132054
if (LoadedSym) {
20142055
const ValueState *ValS = State->get<GCValueMap>(LoadedSym);
2056+
logWithDump("- IsLoad, LoadedSym", LoadedSym);
2057+
logWithDump("- IsLoad, ValS", ValS);
20152058
if (!ValS || !ValS->isRooted() || ValS->RootDepth > RS->RootedAtDepth) {
2059+
auto NewVS = getRootedFromRegion(SLoc.getAsRegion(), State->get<GCPinMap>(SLoc.getAsRegion()), RS->RootedAtDepth);
2060+
logWithDump("- IsLoad, NewVS", NewVS);
20162061
DidChange = true;
2017-
State = State->set<GCValueMap>(
2018-
LoadedSym,
2019-
getRootedFromRegion(SLoc.getAsRegion(), State->get<GCPinMap>(SLoc.getAsRegion()), RS->RootedAtDepth));
2062+
State = State->set<GCValueMap>(LoadedSym, NewVS);
20202063
}
20212064
}
20222065
}
2066+
logWithDump("- getAsRegion()", SLoc.getAsRegion());
20232067
// If it's just the symbol by itself, let it be. We allow dead pointer to be
20242068
// passed around, so long as they're not accessed. However, we do want to
20252069
// start tracking any globals that may have been accessed.
20262070
if (rootRegionIfGlobal(SLoc.getAsRegion(), State, C)) {
20272071
C.addTransition(State);
2072+
log("- rootRegionIfGlobal");
20282073
return;
20292074
}
20302075
SymbolRef SymByItself = SLoc.getAsSymbol(false);
2076+
logWithDump("- SymByItself", SymByItself);
20312077
if (SymByItself) {
20322078
DidChange &&C.addTransition(State);
20332079
return;
20342080
}
20352081
// This will walk backwards until it finds the base symbol
20362082
SymbolRef Sym = SLoc.getAsSymbol(true);
2083+
logWithDump("- Sym", Sym);
20372084
if (!Sym) {
20382085
DidChange &&C.addTransition(State);
20392086
return;
20402087
}
20412088
const ValueState *VState = State->get<GCValueMap>(Sym);
2089+
logWithDump("- VState", VState);
20422090
if (!VState) {
20432091
DidChange &&C.addTransition(State);
20442092
return;
20452093
}
2046-
validateValue(VState, C, Sym, "Trying to access value which may have been");
2094+
// If this is the sym, we verify both rootness and pinning. Otherwise, it may be the parent sym and we only care about the rootness.
2095+
if (SymByItself == Sym) {
2096+
validateValue(VState, C, Sym, "Trying to access value which may have been");
2097+
} else {
2098+
validateValueRootnessOnly(VState, C, Sym, "Trying to access value which may have been");
2099+
}
20472100
DidChange &&C.addTransition(State);
20482101
}
20492102

‎test/clangsa/MissingPinning.c

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,15 @@ void missing_pin_before_safepoint() {
3636
// expected-note@-1{{Argument value may have been moved}}
3737
}
3838

39+
void pin_after_safepoint() {
40+
jl_svec_t *val = jl_svec1(NULL);
41+
JL_GC_PROMISE_ROOTED(val);
42+
jl_gc_safepoint();
43+
PTR_PIN(val); // expected-warning{{Attempt to PIN a value that is already moved}}
44+
// expected-note@-1{{Attempt to PIN a value that is already moved}}
45+
look_at_value((jl_value_t*) val);
46+
}
47+
3948
void proper_pin_before_safepoint() {
4049
jl_svec_t *val = jl_svec1(NULL);
4150
JL_GC_PROMISE_ROOTED(val);
@@ -60,3 +69,57 @@ void push_no_tpin_value() {
6069
look_at_value((jl_value_t*) val);
6170
JL_GC_POP();
6271
}
72+
73+
void pointer_to_pointer(jl_value_t **v) {
74+
// *v is not pinned.
75+
look_at_value(*v); // expected-warning{{Passing non-pinned value as argument to function that may GC}}
76+
// expected-note@-1{{Passing non-pinned value as argument to function that may GC}}
77+
// expected-note@-2{{Started tracking value here}}
78+
}
79+
80+
void pointer_to_pointer2(jl_value_t* u, jl_value_t **v) {
81+
*v = u;
82+
look_at_value(*v); // expected-warning{{Passing non-pinned value as argument to function that may GC}}
83+
// expected-note@-1{{Passing non-pinned value as argument to function that may GC}}
84+
// expected-note@+1{{Started tracking value here (root was inherited)}}
85+
}
86+
87+
extern jl_value_t *first_array_elem(jl_array_t *a JL_PROPAGATES_ROOT);
88+
89+
void root_propagation(jl_expr_t *expr) {
90+
PTR_PIN(expr->args);
91+
jl_value_t *val = first_array_elem(expr->args); // expected-note{{Started tracking value here}}
92+
PTR_UNPIN(expr->args);
93+
jl_gc_safepoint();
94+
look_at_value(val); // expected-warning{{Argument value may have been moved}}
95+
// expected-note@-1{{Argument value may have been moved}}
96+
}
97+
98+
void derive_ptr_alias(jl_method_instance_t *mi) {
99+
jl_value_t* a = mi->specTypes;
100+
jl_value_t* b = mi->specTypes;
101+
PTR_PIN(a);
102+
look_at_value(b);
103+
PTR_UNPIN(a);
104+
}
105+
106+
void derive_ptr_alias2(jl_method_instance_t *mi) {
107+
PTR_PIN(mi->specTypes);
108+
look_at_value(mi->specTypes);
109+
PTR_UNPIN(mi->specTypes);
110+
}
111+
112+
// Ignore this case for now. The checker conjures new syms for function return values.
113+
// It pins the first return value, but cannot see the second return value is an alias of the first.
114+
// However, we could rewrite the code so the checker can check it.
115+
// void mtable(jl_value_t *f) {
116+
// PTR_PIN((jl_value_t*)jl_gf_mtable(f));
117+
// look_at_value((jl_value_t*)jl_gf_mtable(f));
118+
// }
119+
120+
void mtable(jl_value_t *f) {
121+
jl_value_t* mtable = (jl_value_t*)jl_gf_mtable(f);
122+
PTR_PIN(mtable);
123+
look_at_value(mtable);
124+
PTR_UNPIN(mtable);
125+
}

‎test/clangsa/MissingRoots.c

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,10 +181,15 @@ void globally_rooted() {
181181
}
182182

183183
extern jl_value_t *first_array_elem(jl_array_t *a JL_PROPAGATES_ROOT);
184+
184185
void root_propagation(jl_expr_t *expr) {
186+
PTR_PIN(expr->args);
185187
jl_value_t *val = first_array_elem(expr->args);
188+
PTR_UNPIN(expr->args);
189+
PTR_PIN(val);
186190
jl_gc_safepoint();
187191
look_at_value(val);
192+
PTR_UNPIN(val);
188193
}
189194

190195
void argument_propagation(jl_value_t *a) {
@@ -201,7 +206,9 @@ void argument_propagation(jl_value_t *a) {
201206
void arg_array(jl_value_t **args) {
202207
jl_gc_safepoint();
203208
jl_value_t *val = args[1];
209+
PTR_PIN(val);
204210
look_at_value(val);
211+
PTR_UNPIN(val);
205212
jl_value_t *val2 = NULL;
206213
JL_GC_PUSH1(&val2);
207214
val2 = val;
@@ -260,7 +267,9 @@ void nonconst_loads(jl_svec_t *v)
260267
size_t i = jl_svec_len(v);
261268
jl_method_instance_t **data = (jl_method_instance_t**)jl_svec_data(v);
262269
jl_method_instance_t *mi = data[i];
270+
PTR_PIN(mi->specTypes);
263271
look_at_value(mi->specTypes);
272+
PTR_UNPIN(mi->specTypes);
264273
}
265274

266275
void nonconst_loads2()
@@ -278,10 +287,12 @@ static inline void look_at_value2(jl_value_t *v) {
278287
look_at_value(v);
279288
}
280289
void mtable(jl_value_t *f) {
281-
look_at_value2((jl_value_t*)jl_gf_mtable(f));
290+
jl_value_t* mtable = (jl_value_t*)jl_gf_mtable(f);
291+
PTR_PIN(mtable);
292+
look_at_value2(mtable);
282293
jl_value_t *val = NULL;
283294
JL_GC_PUSH1(&val);
284-
val = (jl_value_t*)jl_gf_mtable(f);
295+
val = mtable;
285296
JL_GC_POP();
286297
}
287298

@@ -293,7 +304,10 @@ void mtable2(jl_value_t **v) {
293304
}
294305

295306
void tparam0(jl_value_t *atype) {
296-
look_at_value(jl_tparam0(atype));
307+
jl_value_t *param0 = jl_tparam0(atype);
308+
PTR_PIN(param0);
309+
look_at_value(param0);
310+
PTR_UNPIN(param0);
297311
}
298312

299313
extern jl_value_t *global_atype JL_GLOBALLY_ROOTED JL_GLOBALLY_TPINNED;
@@ -330,10 +344,14 @@ void module_member(jl_module_t *m)
330344
{
331345
for(int i=(int)m->usings.len-1; i >= 0; --i) {
332346
jl_module_t *imp = propagation(m);
347+
PTR_PIN(imp);
333348
jl_gc_safepoint();
334349
look_at_value((jl_value_t*)imp);
335350
jl_module_t *prop = propagation(imp);
351+
PTR_PIN(prop);
336352
look_at_value((jl_value_t*)prop);
353+
PTR_UNPIN(prop);
354+
PTR_UNPIN(imp);
337355
JL_GC_PUSH1(&imp);
338356
jl_gc_safepoint();
339357
look_at_value((jl_value_t*)imp);

0 commit comments

Comments
 (0)