Skip to content

Commit

Permalink
No longer require arguments to be pinned in GCChecker
Browse files Browse the repository at this point in the history
  • Loading branch information
@qinsoon
qinsoon committed Jun 6, 2024
1 parent 095dfdd commit a223989
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 116 deletions.
52 changes: 29 additions & 23 deletions src/clangsa/GCChecker.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// This file is a part of Julia. License is MIT: https://julialang.org/license

// 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.
Expand Down Expand Up @@ -49,7 +47,7 @@ static const Stmt *getStmtForDiagnostics(const ExplodedNode *N)
}

// Turn on/off the log here
#define DEBUG_LOG 1
#define DEBUG_LOG 0

class GCChecker
: public Checker<
Expand Down Expand Up @@ -175,6 +173,9 @@ class GCChecker
} else if (parent.isPinned()) {
// If parent is pinned, the child is not pinned.
return getNotPinned(parent);
} else if (parent.isMoved()) {
// If parent is moved, the child is not pinned.
return getNotPinned(parent);
} else {
// For other cases, the children have the same state as the parent.
return parent;
Expand All @@ -200,19 +201,20 @@ class GCChecker
const ParmVarDecl *PVD) {
bool isFunctionSafepoint = !isFDAnnotatedNotSafepoint(FD);
bool maybeUnrooted = declHasAnnotation(PVD, "julia_maybe_unrooted");
bool maybeUnpinned = declHasAnnotation(PVD, "julia_maybe_unpinned");
if (!isFunctionSafepoint || maybeUnrooted || maybeUnpinned) {
if (!isFunctionSafepoint || maybeUnrooted) {
ValueState VS = getAllocated();
VS.PVD = PVD;
VS.FD = FD;
return VS;
}
bool require_tpin = declHasAnnotation(PVD, "julia_require_tpin");
bool require_pin = declHasAnnotation(PVD, "julia_require_pin");
if (require_tpin) {
return getRooted(nullptr, ValueState::TransitivelyPinned, -1);
} else {
// Assume arguments are pinned
} else if (require_pin) {
return getRooted(nullptr, ValueState::Pinned, -1);
} else {
return getRooted(nullptr, ValueState::NotPinned, -1);
}
}
};
Expand Down Expand Up @@ -345,6 +347,7 @@ class GCChecker
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;
void validateValueRootnessOnly(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;
template <typename T>
Expand Down Expand Up @@ -481,6 +484,15 @@ static const VarRegion *walk_back_to_global_VR(const MemRegion *Region) {
#define FREED 1
#define MOVED 2

void GCChecker::validateValueRootnessOnly(const ValueState* VS, CheckerContext &C, SymbolRef Sym, const char *message, SourceRange range) const {
int v = validateValueInner(VS);
if (v == FREED) {
GCChecker::report_value_error(C, Sym, (std::string(message) + " GCed").c_str(), range);
} 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, SourceRange range) const {
int v = validateValueInner(VS);
if (v == FREED) {
Expand Down Expand Up @@ -1139,10 +1151,10 @@ bool GCChecker::processPotentialSafepoint(const CallEvent &Call,
// Symbolically move all unpinned values.
GCValueMapTy AMap2 = State->get<GCValueMap>();
for (auto I = AMap2.begin(), E = AMap2.end(); I != E; ++I) {
logWithDump("- check Sym", I.getKey());
if (RetSym == I.getKey())
continue;
if (I.getData().isNotPinned()) {
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);
Expand Down Expand Up @@ -1195,9 +1207,9 @@ 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)) {
NewVState = ValueState::getNotPinned(*OldVState);
}
// if (OldVState->isPinned() && ((CurrentVState && !CurrentVState->isPinnedByAnyway()) || !CurrentVState)) {
// NewVState = ValueState::getNotPinned(*OldVState);
// }
logWithDump("- Rooted set to", NewVState);
State = State->set<GCValueMap>(RootedSymbol, NewVState);
return true;
Expand Down Expand Up @@ -1633,23 +1645,16 @@ void GCChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
range);
}
}
if (ValState->isNotPinned()) {
bool MaybeUnpinned = false;
if (FD) {
if (idx < FD->getNumParams()) {
MaybeUnpinned =
declHasAnnotation(FD->getParamDecl(idx), "julia_maybe_unpinned");
}
}
if (!MaybeUnpinned && isCalleeSafepoint) {
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.");
}
}
if (FD && idx < FD->getNumParams() && declHasAnnotation(FD->getParamDecl(idx), "julia_require_pin")) {
if (!ValState->isPinnedByAnyway()) {
report_value_error(C, Sym, "Passing non-pinned argument to function that requires a pin argument.");
}
}
}
}

Expand Down Expand Up @@ -1810,6 +1815,7 @@ bool GCChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
}

const ValueState *OldVS = C.getState()->get<GCValueMap>(Sym);
logWithDump("- PTR_PIN OldVS", OldVS);
if (OldVS && OldVS->isMoved()) {
report_error(C, "Attempt to PIN a value that is already moved.");
return true;
Expand Down
32 changes: 4 additions & 28 deletions src/julia.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,6 @@ extern void _JL_GC_PUSHARGS(jl_value_t **, size_t) JL_NOTSAFEPOINT;

extern void JL_GC_POP() JL_NOTSAFEPOINT;

#ifdef MMTK_GC
extern void JL_GC_PUSH1_NO_TPIN(void *) JL_NOTSAFEPOINT;
extern void JL_GC_PUSH2_NO_TPIN(void *, void *) JL_NOTSAFEPOINT;
extern void JL_GC_PUSH3_NO_TPIN(void *, void *, void *) JL_NOTSAFEPOINT;
Expand All @@ -931,8 +930,6 @@ extern void _JL_GC_PUSHARGS_NO_TPIN(jl_value_t **, size_t) JL_NOTSAFEPOINT;
memset(rts_var, 0, sizeof(void*) * (n)); \
_JL_GC_PUSHARGS_NO_TPIN(rts_var, (n));

#endif

#else

#define JL_GC_PUSH1(arg1) \
Expand Down Expand Up @@ -976,9 +973,6 @@ extern void _JL_GC_PUSHARGS_NO_TPIN(jl_value_t **, size_t) JL_NOTSAFEPOINT;

#define JL_GC_POP() (jl_pgcstack = jl_pgcstack->prev)

#endif

#ifdef MMTK_GC
// these are pinning roots: only the root object needs to be pinned as opposed to
// the functions above which are transitively pinning
#define JL_GC_PUSH1_NO_TPIN(arg1) \
Expand Down Expand Up @@ -1019,25 +1013,7 @@ extern void _JL_GC_PUSHARGS_NO_TPIN(jl_value_t **, size_t) JL_NOTSAFEPOINT;
((void**)rts_var)[-1] = jl_pgcstack; \
memset((void*)rts_var, 0, (n)*sizeof(jl_value_t*)); \
jl_pgcstack = (jl_gcframe_t*)&(((void**)rts_var)[-2])
#else
// When not using MMTk, default to the stock functions
#define JL_GC_PUSH1_NO_TPIN(arg1) JL_GC_PUSH1(arg1)

#define JL_GC_PUSH2_NO_TPIN(arg1, arg2) JL_GC_PUSH2(arg1, arg2)

#define JL_GC_PUSH3_NO_TPIN(arg1, arg2, arg3) JL_GC_PUSH3(arg1, arg2, arg3)

#define JL_GC_PUSH4_NO_TPIN(arg1, arg2, arg3, arg4) JL_GC_PUSH4(arg1, arg2, arg3, arg4)

#define JL_GC_PUSH5_NO_TPIN(arg1, arg2, arg3, arg4, arg5) JL_GC_PUSH5(arg1, arg2, arg3, arg4, arg5)

#define JL_GC_PUSH6_NO_TPIN(arg1, arg2, arg3, arg4, arg5, arg6) JL_GC_PUSH6(arg1, arg2, arg3, arg4, arg5, arg6)

#define JL_GC_PUSH7_NO_TPIN(arg1, arg2, arg3, arg4, arg5, arg6, arg7) JL_GC_PUSH7(arg1, arg2, arg3, arg4, arg5, arg6, arg7)

#define JL_GC_PUSH8_NO_TPIN(arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8) JL_GC_PUSH8(arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8)

#define JL_GC_PUSHARGS_NO_TPIN(rts_var,n) JL_GC_PUSHARGS(rts_var,n)
#endif

JL_DLLEXPORT int jl_gc_enable(int on);
Expand Down Expand Up @@ -1871,12 +1847,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_MAYBE_UNPINNED,
jl_value_t *got JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED);
jl_value_t *expected JL_MAYBE_UNROOTED,
jl_value_t *got JL_MAYBE_UNROOTED);
JL_DLLEXPORT void JL_NORETURN jl_type_error_rt(const char *fname,
const char *context,
jl_value_t *ty JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED,
jl_value_t *got JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED);
jl_value_t *ty JL_MAYBE_UNROOTED,
jl_value_t *got JL_MAYBE_UNROOTED);
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
8 changes: 4 additions & 4 deletions src/rtutils.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ JL_DLLEXPORT void JL_NORETURN jl_too_many_args(const char *fname, int max)

// with function name / location description, plus extra context
JL_DLLEXPORT void JL_NORETURN jl_type_error_rt(const char *fname, const char *context,
jl_value_t *expected JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED,
jl_value_t *got JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED)
jl_value_t *expected JL_MAYBE_UNROOTED,
jl_value_t *got JL_MAYBE_UNROOTED)
{
jl_value_t *ctxt=NULL;
JL_GC_PUSH3(&ctxt, &expected, &got);
Expand All @@ -121,8 +121,8 @@ JL_DLLEXPORT void JL_NORETURN jl_type_error_rt(const char *fname, const char *co

// with function name or description only
JL_DLLEXPORT void JL_NORETURN jl_type_error(const char *fname,
jl_value_t *expected JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED,
jl_value_t *got JL_MAYBE_UNROOTED JL_MAYBE_UNPINNED)
jl_value_t *expected JL_MAYBE_UNROOTED,
jl_value_t *got JL_MAYBE_UNROOTED)
{
jl_type_error_rt(fname, "", expected, got);
}
Expand Down
4 changes: 2 additions & 2 deletions src/support/analyzer_annotations.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#define JL_PROPAGATES_ROOT __attribute__((annotate("julia_propagates_root")))
#define JL_NOTSAFEPOINT __attribute__((annotate("julia_not_safepoint")))
#define JL_MAYBE_UNROOTED __attribute__((annotate("julia_maybe_unrooted")))
#define JL_MAYBE_UNPINNED __attribute__((annotate("julia_maybe_unpinned")))
#define JL_GLOBALLY_ROOTED __attribute__((annotate("julia_globally_rooted")))
#define JL_GLOBALLY_PINNED __attribute__((annotate("julia_globally_pinned")))
#define JL_GLOBALLY_TPINNED __attribute__((annotate("julia_globally_tpinned")))
Expand All @@ -24,6 +23,7 @@
#define JL_ROOTS_TEMPORARILY __attribute__((annotate("julia_temporarily_roots")))
#define JL_REQUIRE_ROOTED_SLOT __attribute__((annotate("julia_require_rooted_slot")))
#define JL_REQUIRE_TPIN __attribute__((annotate("julia_require_tpin")))
#define JL_REQUIRE_PIN __attribute__((annotate("julia_require_pin")))
#ifdef __cplusplus
extern "C" {
#endif
Expand All @@ -38,7 +38,6 @@ extern "C" {
#define JL_PROPAGATES_ROOT
#define JL_NOTSAFEPOINT
#define JL_MAYBE_UNROOTED
#define JL_MAYBE_UNPINNED
// The runtime may mark any object that is reachable from a global root as globally rooted.
// So JL_GLOBALLY_ROOTED does not need to an actual root. Thus we don't know anything
// about pining state.
Expand All @@ -54,6 +53,7 @@ extern "C" {
#define JL_ROOTS_TEMPORARILY
#define JL_REQUIRE_ROOTED_SLOT
#define JL_REQUIRE_TPIN
#define JL_REQUIRE_PIN
#define JL_GC_PROMISE_ROOTED(x) (void)(x)
#define jl_may_leak(x) (void)(x)

Expand Down
Loading

0 comments on commit a223989

Please sign in to comment.