Skip to content

Commit

Permalink
clangsa: Check that a value is rooted on return (#37366)
Browse files Browse the repository at this point in the history 
* clangsa: Check that a value is rooted on return

Fixes #37364
Also reverts part of 4ed484f that
broke the clangsa self tests.

* rm outdated comment
  • Loading branch information
@Keno
Keno authored Sep 3, 2020
1 parent f51d4cb commit 46f78d6
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 46 deletions.
32 changes: 17 additions & 15 deletions src/clangsa/GCChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,16 @@ void GCChecker::checkBeginFunction(CheckerContext &C) const {
void GCChecker::checkEndFunction(const clang::ReturnStmt *RS,
CheckerContext &C) const {
ProgramStateRef State = C.getState();

if (RS && gcEnabledHere(C) && RS->getRetValue() && isGCTracked(RS->getRetValue())) {
auto ResultVal = C.getSVal(RS->getRetValue());
SymbolRef Sym = ResultVal.getAsSymbol(true);
const ValueState *ValS = Sym ? State->get<GCValueMap>(Sym) : nullptr;
if (ValS && ValS->isPotentiallyFreed()) {
report_value_error(C, Sym, "Return value may have been GCed", RS->getSourceRange());
}
}

bool Changed = false;
if (State->get<GCDisabledAt>() == C.getStackFrame()->getIndex()) {
State = State->set<GCDisabledAt>((unsigned)-1);
Expand All @@ -695,10 +705,7 @@ void GCChecker::checkEndFunction(const clang::ReturnStmt *RS,
C.addTransition(State);
if (!C.inTopFrame())
return;
// If `RS` is NULL, the function did not return normally.
// This could be either an abort/assertion failure or an exception throw.
// Do not require the GC frame to match in such case.
if (RS && C.getState()->get<GCDepth>() > 0)
if (C.getState()->get<GCDepth>() > 0)
report_error(C, "Non-popped GC frame present at end of function");
}

Expand Down Expand Up @@ -867,12 +874,17 @@ bool GCChecker::processPotentialSafepoint(const CallEvent &Call,
}
}

// Don't free the return value
SymbolRef RetSym = Call.getReturnValue().getAsSymbol();

// Symbolically free all unrooted values.
GCValueMapTy AMap = State->get<GCValueMap>();
for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) {
if (I.getData().isJustAllocated()) {
if (SpeciallyRootedSymbol == I.getKey())
continue;
if (RetSym == I.getKey())
continue;
State = State->set<GCValueMap>(I.getKey(), ValueState::getFreed());
DidChange = true;
}
Expand Down Expand Up @@ -941,17 +953,7 @@ bool GCChecker::processAllocationOfResult(const CallEvent &Call,
State = State->set<GCValueMap>(Sym, ValueState::getRooted(nullptr, -1));
else {
const ValueState *ValS = State->get<GCValueMap>(Sym);
ValueState NewVState = ValueState::getAllocated();
if (ValS) {
// If the call was inlined, we may have accidentally killed the return
// value above. Revive it here.
const ValueState *PrevValState = C.getState()->get<GCValueMap>(Sym);
if (!ValS->isPotentiallyFreed() ||
(PrevValState && PrevValState->isPotentiallyFreed())) {
return false;
}
NewVState = *PrevValState;
}
ValueState NewVState = ValS ? *ValS : ValueState::getAllocated();
auto *Decl = Call.getDecl();
const FunctionDecl *FD = Decl ? Decl->getAsFunction() : nullptr;
if (FD) {
Expand Down
52 changes: 27 additions & 25 deletions src/disasm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,35 +424,37 @@ jl_value_t *jl_dump_function_ir(void *f, char strip_ir_metadata, char dump_modul
std::string code;
raw_string_ostream stream(code);

Function *llvmf = dyn_cast_or_null<Function>((Function*)f);
if (!llvmf || (!llvmf->isDeclaration() && !llvmf->getParent()))
jl_error("jl_dump_function_ir: Expected Function* in a temporary Module");

JL_LOCK(&codegen_lock); // Might GC
LineNumberAnnotatedWriter AAW{debuginfo};
if (!llvmf->getParent()) {
// print the function declaration as-is
llvmf->print(stream, &AAW);
delete llvmf;
}
else {
Module *m = llvmf->getParent();
if (strip_ir_metadata) {
std::string llvmfn(llvmf->getName());
jl_strip_llvm_addrspaces(m);
jl_strip_llvm_debug(m, true, &AAW);
// rewriting the function type creates a new function, so look it up again
llvmf = m->getFunction(llvmfn);
}
if (dump_module) {
m->print(stream, &AAW);
{
Function *llvmf = dyn_cast_or_null<Function>((Function*)f);
if (!llvmf || (!llvmf->isDeclaration() && !llvmf->getParent()))
jl_error("jl_dump_function_ir: Expected Function* in a temporary Module");

JL_LOCK(&codegen_lock); // Might GC
LineNumberAnnotatedWriter AAW{debuginfo};
if (!llvmf->getParent()) {
// print the function declaration as-is
llvmf->print(stream, &AAW);
delete llvmf;
}
else {
llvmf->print(stream, &AAW);
Module *m = llvmf->getParent();
if (strip_ir_metadata) {
std::string llvmfn(llvmf->getName());
jl_strip_llvm_addrspaces(m);
jl_strip_llvm_debug(m, true, &AAW);
// rewriting the function type creates a new function, so look it up again
llvmf = m->getFunction(llvmfn);
}
if (dump_module) {
m->print(stream, &AAW);
}
else {
llvmf->print(stream, &AAW);
}
delete m;
}
delete m;
JL_UNLOCK(&codegen_lock); // Might GC
}
JL_UNLOCK(&codegen_lock); // Might GC

return jl_pchar_to_string(stream.str().data(), stream.str().size());
}
Expand Down
6 changes: 3 additions & 3 deletions src/gf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1170,7 +1170,7 @@ static jl_method_instance_t *cache_method(

static jl_method_match_t *_gf_invoke_lookup(jl_value_t *types JL_PROPAGATES_ROOT, size_t world, size_t *min_valid, size_t *max_valid);

static jl_method_instance_t *jl_mt_assoc_by_type(jl_methtable_t *mt, jl_datatype_t *tt, size_t world)
static jl_method_instance_t *jl_mt_assoc_by_type(jl_methtable_t *mt JL_PROPAGATES_ROOT, jl_datatype_t *tt, size_t world)
{
// caller must hold the mt->writelock
assert(tt->isdispatchtuple || tt->hasfreetypevars);
Expand Down Expand Up @@ -1811,8 +1811,8 @@ jl_method_instance_t *jl_method_lookup(jl_value_t **args, size_t nargs, size_t w
JL_GC_PUSH1(&tt);
JL_LOCK(&mt->writelock);
jl_method_instance_t *sf = jl_mt_assoc_by_type(mt, tt, world);
JL_GC_POP();
JL_UNLOCK(&mt->writelock);
JL_GC_POP();
return sf;
}

Expand Down Expand Up @@ -2342,8 +2342,8 @@ STATIC_INLINE jl_method_instance_t *jl_lookup_generic_(jl_value_t *F, jl_value_t
// cache miss case
JL_TIMING(METHOD_LOOKUP_SLOW);
mfunc = jl_mt_assoc_by_type(mt, tt, world);
JL_GC_POP();
JL_UNLOCK(&mt->writelock);
JL_GC_POP();
if (jl_options.malloc_log)
jl_gc_sync_total_bytes(last_alloc); // discard allocation count from compilation
if (mfunc == NULL) {
Expand Down
14 changes: 11 additions & 3 deletions test/clangsa/MissingRoots.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,18 +239,18 @@ void pushargs_as_args()
JL_GC_POP();
}

static jl_typemap_entry_t *call_cache[10] JL_GLOBALLY_ROOTED;
static jl_typemap_entry_t *this_call_cache[10] JL_GLOBALLY_ROOTED;
void global_array2() {
jl_value_t *val = NULL;
JL_GC_PUSH1(&val);
val = (jl_value_t*)call_cache[1]->func.linfo;
val = (jl_value_t*)this_call_cache[1]->func.linfo;
JL_GC_POP();
}

void global_array3() {
jl_value_t *val = NULL;
jl_typemap_entry_t *tm = NULL;
tm = call_cache[1];
tm = this_call_cache[1];
val = (jl_value_t*)tm->func.linfo;
look_at_value(val);
}
Expand Down Expand Up @@ -416,3 +416,11 @@ void JL_NORETURN throw_internal(jl_value_t *e JL_MAYBE_UNROOTED)
jl_gc_unsafe_enter(ptls);
look_at_value(e);
}

JL_DLLEXPORT jl_value_t *jl_totally_used_function(int i)
{
jl_value_t *v = jl_box_int32(i); // expected-note{{Started tracking value here}}
jl_safepoint(); // expected-note{{Value may have been GCed here}}
return v; // expected-warning{{Return value may have been GCed}}
// expected-note@-1{{Return value may have been GCed}}
}

0 comments on commit 46f78d6

Please sign in to comment.