Skip to content

Fix a few GCChecker issues #51

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 30, 2024
Merged

Fix a few GCChecker issues #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
32f8368
Minor fix in GCChecker. Args for jl_type_error are unpinned.
qinsoon May 17, 2024
63a35f2
Fix aotcompile.cpp
qinsoon May 17, 2024
7358b40
Properly inherit pinning state for binding variables, jl_propagate_root
qinsoon May 22, 2024
e983899
Add more test cases
qinsoon May 23, 2024
9063221
In a few cases, we also check if value state is pinned. When it is not,
qinsoon May 27, 2024
bedda75
Fix issues in jitlayers
qinsoon May 28, 2024
0525112
Add note about when a value is moved. Fix aotcompile
qinsoon May 28, 2024
8645ccb
Revert code changes to runtime code
qinsoon May 29, 2024
0c8460d
Add JL_REQUIRE_TPIN for arguments
qinsoon May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 99 additions & 19 deletions src/clangsa/GCChecker.cpp
View file
Open in desktop
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
Open in desktop
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