Skip to content

Commit

Permalink
Fix a few GCChecker issues (#51)
Browse files Browse the repository at this point in the history 
This PR fixes a few issues in the GCChecker. See the new test cases for what are fixed in the PR.
  • Loading branch information
@qinsoon
qinsoon authored May 30, 2024
1 parent b1f61bb commit 12a958d
Show file tree
Hide file tree
Showing 6 changed files with 277 additions and 30 deletions.
118 changes: 99 additions & 19 deletions src/clangsa/GCChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
// Assumptions for pinning:
// * args need to be pinned
// * JL_ROOTING_ARGUMENT and JL_ROOTED_ARGUMENT will propagate pinning state as well.
// * The checker may not consider alias for derived pointers in some cases.
// * 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.
// * a = x->y; b = x->y; PTR_PIN(a); The checker will find b as pinned.
// * Need to see if this affects correctness.
// * The checker may report some vals as moved even if there is a new load for the val after safepoint.
// * f(x->a); jl_safepoint(); f(x->a); x->a is loaded after a safepoint, but the checker may report errors. This seems fine, as the compiler may hoist the load.
// * a = x->a; f(a); jl_safepoint(); f(a); a may be moved in a safepoint, and the checker will report errors.

#include "clang/Frontend/FrontendActions.h"
#include "clang/StaticAnalyzer/Checkers/SValExplainer.h"
Expand Down Expand Up @@ -96,6 +103,7 @@ class GCChecker
llvm::dbgs() << ((P == TransitivelyPinned) ? "TransitivelyPinned"
: (P == Pinned) ? "Pinned"
: (P == NotPinned) ? "NotPinned"
: (P == Moved) ? "Moved"
: "Error");
llvm::dbgs() << ",";
if (S == Rooted)
Expand Down Expand Up @@ -193,8 +201,13 @@ class GCChecker
VS.FD = FD;
return VS;
}
// Assume arguments are pinned
return getRooted(nullptr, ValueState::Pinned, -1);
bool require_tpin = declHasAnnotation(PVD, "julia_require_tpin");
if (require_tpin) {
return getRooted(nullptr, ValueState::TransitivelyPinned, -1);
} else {
// Assume arguments are pinned
return getRooted(nullptr, ValueState::Pinned, -1);
}
}
};

Expand Down Expand Up @@ -230,7 +243,6 @@ class GCChecker
: "Error");
llvm::dbgs() << ",Depth=";
llvm::dbgs() << RootedAtDepth;
llvm::dbgs() << "\n";
}
};

Expand Down Expand Up @@ -325,6 +337,7 @@ class GCChecker
SymbolRef getSymbolForResult(const Expr *Result, const ValueState *OldValS,
ProgramStateRef &State, CheckerContext &C) const;
void validateValue(const GCChecker::ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message) const;
void validateValueRootnessOnly(const GCChecker::ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message) const;
void validateValue(const GCChecker::ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message, SourceRange range) const;
int validateValueInner(const GCChecker::ValueState* VS) const;
GCChecker::ValueState getRootedFromRegion(const MemRegion *Region, const PinState *PS, int Depth) const;
Expand Down Expand Up @@ -401,13 +414,16 @@ SymbolRef GCChecker::walkToRoot(callback f, const ProgramStateRef &State,
const MemRegion *Region) {
if (!Region)
return nullptr;
logWithDump("- walkToRoot, Region", Region);
while (true) {
const SymbolicRegion *SR = Region->getSymbolicBase();
if (!SR) {
return nullptr;
}
SymbolRef Sym = SR->getSymbol();
const ValueState *OldVState = State->get<GCValueMap>(Sym);
logWithDump("- walkToRoot, Sym", Sym);
logWithDump("- walkToRoot, OldVState", OldVState);
if (f(Sym, OldVState)) {
if (const SymbolRegionValue *SRV = dyn_cast<SymbolRegionValue>(Sym)) {
Region = SRV->getRegion();
Expand Down Expand Up @@ -468,6 +484,15 @@ void GCChecker::validateValue(const ValueState* VS, CheckerContext &C, SymbolRef
}
}

void GCChecker::validateValueRootnessOnly(const ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message) const {
int v = validateValueInner(VS);
if (v == FREED) {
GCChecker::report_value_error(C, Sym, (std::string(message) + " GCed").c_str());
} else if (v == MOVED) {
// We don't care if it is moved
}
}

void GCChecker::validateValue(const ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message) const {
int v = validateValueInner(VS);
if (v == FREED) {
Expand Down Expand Up @@ -717,6 +742,8 @@ PDP GCChecker::GCValueBugVisitor::VisitNode(const ExplodedNode *N,
return MakePDP(Pos, "Root was released here.");
} else if (NewValueState->RootDepth != OldValueState->RootDepth) {
return MakePDP(Pos, "Rooting Depth changed here.");
} else if (NewValueState->isMoved() && !OldValueState->isMoved()) {
return MakePDP(Pos, "Value was moved here.");
}
return nullptr;
}
Expand Down Expand Up @@ -833,6 +860,7 @@ void GCChecker::checkBeginFunction(CheckerContext &C) const {
const auto *FD = dyn_cast<FunctionDecl>(LCtx->getDecl());
if (!FD)
return;
logWithDump("checkBeginFunction", FD);
ProgramStateRef State = C.getState();
bool Change = false;
if (C.inTopFrame()) {
Expand Down Expand Up @@ -861,7 +889,10 @@ void GCChecker::checkBeginFunction(CheckerContext &C) const {
auto Param = State->getLValue(P, LCtx);
const MemRegion *Root = State->getSVal(Param).getAsRegion();
State = State->set<GCRootMap>(Root, RootState::getRoot(-1));
State = State->set<GCPinMap>(Root, PinState::getPin(-1));
if (declHasAnnotation(P, "julia_require_tpin"))
State = State->set<GCPinMap>(Root, PinState::getTransitivePin(-1));
else
State = State->set<GCPinMap>(Root, PinState::getTransitivePin(-1));
} else if (isGCTrackedType(P->getType())) {
auto Param = State->getLValue(P, LCtx);
SymbolRef AssignedSym = State->getSVal(Param).getAsSymbol();
Expand All @@ -880,6 +911,7 @@ void GCChecker::checkBeginFunction(CheckerContext &C) const {

void GCChecker::checkEndFunction(const clang::ReturnStmt *RS,
CheckerContext &C) const {
log("checkEndFunction");
ProgramStateRef State = C.getState();

if (RS && gcEnabledHere(C) && RS->getRetValue() && isGCTracked(RS->getRetValue())) {
Expand Down Expand Up @@ -1104,7 +1136,11 @@ bool GCChecker::processPotentialSafepoint(const CallEvent &Call,
if (RetSym == I.getKey())
continue;
if (I.getData().isNotPinned()) {
State = State->set<GCValueMap>(I.getKey(), ValueState::getMoved(I.getData()));
logWithDump("- move unpinned values, Sym", I.getKey());
logWithDump("- move unpinned values, VS", I.getData());
auto NewVS = ValueState::getMoved(I.getData());
State = State->set<GCValueMap>(I.getKey(), NewVS);
logWithDump("- move unpinned values, NewVS", NewVS);
DidChange = true;
}
}
Expand Down Expand Up @@ -1153,7 +1189,7 @@ bool GCChecker::processArgumentRooting(const CallEvent &Call, CheckerContext &C,
const ValueState *CurrentVState = State->get<GCValueMap>(RootedSymbol);
ValueState NewVState = *OldVState;
// If the old state is pinned, the new state is not pinned.
if (OldVState->isPinned() && ((CurrentVState && CurrentVState->isPinnedByAnyway()) || !CurrentVState)) {
if (OldVState->isPinned() && ((CurrentVState && !CurrentVState->isPinnedByAnyway()) || !CurrentVState)) {
NewVState = ValueState::getNotPinned(*OldVState);
}
logWithDump("- Rooted set to", NewVState);
Expand All @@ -1176,6 +1212,8 @@ bool GCChecker::processAllocationOfResult(const CallEvent &Call,
Call.getOriginExpr(), C.getLocationContext(), QT, C.blockCount());
State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), S);
Sym = S.getAsSymbol();
logWithDump("- conjureSymbolVal, S", S);
logWithDump("- conjureSymbolVal, Sym", Sym);
}
if (isGloballyRootedType(QT))
State = State->set<GCValueMap>(Sym, ValueState::getRooted(nullptr, ValueState::pinState(isGloballyTransitivelyPinnedType(QT)), -1));
Expand Down Expand Up @@ -1229,8 +1267,11 @@ bool GCChecker::processAllocationOfResult(const CallEvent &Call,
const MemRegion *Region = Test.getAsRegion();
const ValueState *OldVState =
getValStateForRegion(C.getASTContext(), State, Region);
if (OldVState)
NewVState = *OldVState;
if (OldVState) {
NewVState = ValueState::inheritState(*OldVState);
logWithDump("- jl_propagates_root, OldVState", *OldVState);
logWithDump("- jl_propagates_root, NewVState", NewVState);
}
break;
}
}
Expand All @@ -1245,8 +1286,11 @@ bool GCChecker::processAllocationOfResult(const CallEvent &Call,
void GCChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
logWithDump("checkPostCall", Call);
ProgramStateRef State = C.getState();
log("- processArgmentRooting");
bool didChange = processArgumentRooting(Call, C, State);
log("- processPotentialsafepoint");
didChange |= processPotentialSafepoint(Call, C, State);
log("- processAllocationOfResult");
didChange |= processAllocationOfResult(Call, C, State);
if (didChange)
C.addTransition(State);
Expand All @@ -1255,6 +1299,7 @@ void GCChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
// Implicitly root values that were casted to globally rooted values
void GCChecker::checkPostStmt(const CStyleCastExpr *CE,
CheckerContext &C) const {
logWithDump("checkpostStmt(CStyleCastExpr)", CE);
if (!isGloballyRootedType(CE->getTypeAsWritten()))
return;
SymbolRef Sym = C.getSVal(CE).getAsSymbol();
Expand Down Expand Up @@ -1354,6 +1399,7 @@ void GCChecker::checkDerivingExpr(const Expr *Result, const Expr *Parent,
SymbolRef OldSym = ParentVal.getAsSymbol(true);
const MemRegion *Region = C.getSVal(Parent).getAsRegion();
const ValueState *OldValS = OldSym ? State->get<GCValueMap>(OldSym) : nullptr;
logWithDump("- Region", Region);
logWithDump("- OldSym", OldSym);
logWithDump("- OldValS", OldValS);
SymbolRef NewSym = getSymbolForResult(Result, OldValS, State, C);
Expand All @@ -1363,6 +1409,7 @@ void GCChecker::checkDerivingExpr(const Expr *Result, const Expr *Parent,
logWithDump("- NewSym", NewSym);
// NewSym might already have a better root
const ValueState *NewValS = State->get<GCValueMap>(NewSym);
logWithDump("- NewValS", NewValS);
if (Region) {
const VarRegion *VR = Region->getAs<VarRegion>();
bool inheritedState = false;
Expand Down Expand Up @@ -1412,8 +1459,9 @@ void GCChecker::checkDerivingExpr(const Expr *Result, const Expr *Parent,
validateValue(OldValS, C, OldSym, "Creating derivative of value that may have been");
if (!OldValS->isPotentiallyFreed() && ResultTracked) {
logWithDump("- Set as OldValS, Sym", NewSym);
logWithDump("- Set as OldValS, VS", OldValS);
C.addTransition(State->set<GCValueMap>(NewSym, *OldValS));
auto InheritVS = ValueState::inheritState(*OldValS);
logWithDump("- Set as OldValS, InheritVS", InheritVS);
C.addTransition(State->set<GCValueMap>(NewSym, InheritVS));
return;
}
}
Expand Down Expand Up @@ -1481,6 +1529,7 @@ void GCChecker::checkPostStmt(const MemberExpr *ME, CheckerContext &C) const {

void GCChecker::checkPostStmt(const UnaryOperator *UO,
CheckerContext &C) const {
logWithDump("checkPostStmt(UnaryOperator)", UO);
if (UO->getOpcode() == UO_Deref) {
checkDerivingExpr(UO, UO->getSubExpr(), true, C);
}
Expand Down Expand Up @@ -1590,6 +1639,11 @@ void GCChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
report_value_error(C, Sym, "Passing non-pinned value as argument to function that may GC", range);
}
}
if (FD && idx < FD->getNumParams() && declHasAnnotation(FD->getParamDecl(idx), "julia_require_tpin")) {
if (!ValState->isTransitivelyPinned()) {
report_value_error(C, Sym, "Passing non-tpinned argument to function that requires a tpin argument.");
}
}
}
}

Expand Down Expand Up @@ -1732,6 +1786,13 @@ bool GCChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
report_error(C, "Can not understand this pin.");
return true;
}

const ValueState *OldVS = C.getState()->get<GCValueMap>(Sym);
if (OldVS && OldVS->isMoved()) {
report_error(C, "Attempt to PIN a value that is already moved.");
return true;
}

auto MRV = Arg.getAs<loc::MemRegionVal>();
if (!MRV) {
report_error(C, "PTR_PIN with something other than a local variable");
Expand All @@ -1740,7 +1801,6 @@ bool GCChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
const MemRegion *Region = MRV->getRegion();
auto State = C.getState()->set<GCPinMap>(Region, PinState::getPin(CurrentDepth));
logWithDump("- Pin region", Region);
const ValueState *OldVS = State->get<GCValueMap>(Sym);
State = State->set<GCValueMap>(Sym, ValueState::getPinned(*OldVS));
logWithDump("- Pin value", Sym);
C.addTransition(State);
Expand Down Expand Up @@ -1885,16 +1945,21 @@ void GCChecker::checkBind(SVal LVal, SVal RVal, const clang::Stmt *S,
log("- No Sym");
return;
}
logWithDump("- Sym", Sym);
const auto *RootState = State->get<GCRootMap>(R);
logWithDump("- R", R);
logWithDump("- RootState for R", RootState);
if (!RootState) {
const ValueState *ValSP = nullptr;
ValueState ValS;
if (rootRegionIfGlobal(R->getBaseRegion(), State, C, &ValS)) {
logWithDump("- rootRegionIfGlobal, base", R->getBaseRegion());
ValSP = &ValS;
logWithDump("- rootRegionIfGlobal ValSP", ValSP);
} else {
logWithDump("- getValStateForRegion", R);
ValSP = getValStateForRegion(C.getASTContext(), State, R);
logWithDump("- getValStateForRegion", ValSP);
}
if (ValSP && ValSP->isRooted()) {
logWithDump("- Found base region that is rooted", ValSP);
Expand All @@ -1903,8 +1968,9 @@ void GCChecker::checkBind(SVal LVal, SVal RVal, const clang::Stmt *S,
RValState->RootDepth < ValSP->RootDepth) {
logWithDump("- No need to set ValState, current ValState", RValState);
} else {
logWithDump("- Set ValState, current ValState", ValSP);
C.addTransition(State->set<GCValueMap>(Sym, *ValSP));
auto InheritVS = ValueState::inheritState(*ValSP);
logWithDump("- Set ValState, InheritVS", InheritVS);
C.addTransition(State->set<GCValueMap>(Sym, InheritVS));
}
}
} else {
Expand All @@ -1929,7 +1995,7 @@ void GCChecker::checkBind(SVal LVal, SVal RVal, const clang::Stmt *S,
} else {
logWithDump("- Found ValState for Sym", RValState);
validateValue(RValState, C, Sym, "Trying to root value which may have been");
if (!RValState->isRooted() ||
if (!RValState->isRooted() || !RValState->isPinnedByAnyway() ||
RValState->RootDepth > RootState->RootedAtDepth) {
auto NewVS = getRootedFromRegion(R, State->get<GCPinMap>(R), RootState->RootedAtDepth);
logWithDump("- getRootedFromRegion", NewVS);
Expand Down Expand Up @@ -1994,48 +2060,62 @@ bool GCChecker::rootRegionIfGlobal(const MemRegion *R, ProgramStateRef &State,

void GCChecker::checkLocation(SVal SLoc, bool IsLoad, const Stmt *S,
CheckerContext &C) const {
logWithDump("checkLocation", SLoc);
ProgramStateRef State = C.getState();
bool DidChange = false;
const RootState *RS = nullptr;
// Loading from a root produces a rooted symbol. TODO: Can we do something
// better than this.
if (IsLoad && (RS = State->get<GCRootMap>(SLoc.getAsRegion()))) {
logWithDump("- IsLoad, RS", RS);
SymbolRef LoadedSym =
State->getSVal(SLoc.getAs<Loc>().getValue()).getAsSymbol();
if (LoadedSym) {
const ValueState *ValS = State->get<GCValueMap>(LoadedSym);
if (!ValS || !ValS->isRooted() || ValS->RootDepth > RS->RootedAtDepth) {
logWithDump("- IsLoad, LoadedSym", LoadedSym);
logWithDump("- IsLoad, ValS", ValS);
if (!ValS || !ValS->isRooted() || !ValS->isPinnedByAnyway() || ValS->RootDepth > RS->RootedAtDepth) {
auto NewVS = getRootedFromRegion(SLoc.getAsRegion(), State->get<GCPinMap>(SLoc.getAsRegion()), RS->RootedAtDepth);
logWithDump("- IsLoad, NewVS", NewVS);
DidChange = true;
State = State->set<GCValueMap>(
LoadedSym,
getRootedFromRegion(SLoc.getAsRegion(), State->get<GCPinMap>(SLoc.getAsRegion()), RS->RootedAtDepth));
State = State->set<GCValueMap>(LoadedSym, NewVS);
}
}
}
logWithDump("- getAsRegion()", SLoc.getAsRegion());
// If it's just the symbol by itself, let it be. We allow dead pointer to be
// passed around, so long as they're not accessed. However, we do want to
// start tracking any globals that may have been accessed.
if (rootRegionIfGlobal(SLoc.getAsRegion(), State, C)) {
C.addTransition(State);
log("- rootRegionIfGlobal");
return;
}
SymbolRef SymByItself = SLoc.getAsSymbol(false);
logWithDump("- SymByItself", SymByItself);
if (SymByItself) {
DidChange &&C.addTransition(State);
return;
}
// This will walk backwards until it finds the base symbol
SymbolRef Sym = SLoc.getAsSymbol(true);
logWithDump("- Sym", Sym);
if (!Sym) {
DidChange &&C.addTransition(State);
return;
}
const ValueState *VState = State->get<GCValueMap>(Sym);
logWithDump("- VState", VState);
if (!VState) {
DidChange &&C.addTransition(State);
return;
}
validateValue(VState, C, Sym, "Trying to access value which may have been");
// 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.
if (SymByItself == Sym) {
validateValue(VState, C, Sym, "Trying to access value which may have been");
} else {
validateValueRootnessOnly(VState, C, Sym, "Trying to access value which may have been");
}
DidChange &&C.addTransition(State);
}

Expand Down
8 changes: 4 additions & 4 deletions src/julia.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1871,12 +1871,12 @@ JL_DLLEXPORT void JL_NORETURN jl_exceptionf(jl_datatype_t *ty,
JL_DLLEXPORT void JL_NORETURN jl_too_few_args(const char *fname, int min);
JL_DLLEXPORT void JL_NORETURN jl_too_many_args(const char *fname, int max);
JL_DLLEXPORT void JL_NORETURN jl_type_error(const char *fname,
jl_value_t *expected JL_MAYBE_UNROOTED,
jl_value_t *got JL_MAYBE_UNROOTED);
jl_value_t *expected JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED,
jl_value_t *got JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED);
JL_DLLEXPORT void JL_NORETURN jl_type_error_rt(const char *fname,
const char *context,
jl_value_t *ty JL_MAYBE_UNROOTED,
jl_value_t *got JL_MAYBE_UNROOTED);
jl_value_t *ty JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED,
jl_value_t *got JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED);
JL_DLLEXPORT void JL_NORETURN jl_undefined_var_error(jl_sym_t *var);
JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_sym_t *type_name, jl_sym_t *var);
JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str);
Expand Down
Loading

0 comments on commit 12a958d

Please sign in to comment.