Skip to content

Commit

Permalink
codegen: fix gc rooting bug (#51744)
Browse files Browse the repository at this point in the history
ccall was not creating roots for the contents of struct values which
contained roots on the stack, as expected to align with `GC.@preserve`,
and causing many segfaults for #51319

(cherry picked from commit e36f65f)
  • Loading branch information
vtjnash authored and KristofferC committed Oct 23, 2023
1 parent 516725d commit 8329725
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 25 deletions.
2 changes: 1 addition & 1 deletion src/ccall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1413,7 +1413,7 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs)
if (jl_is_long(argi_root))
continue;
jl_cgval_t arg_root = emit_expr(ctx, argi_root);
Value *gc_root = get_gc_root_for(arg_root);
Value *gc_root = get_gc_root_for(ctx, arg_root);
if (gc_root)
gc_uses.push_back(gc_root);
}
Expand Down
30 changes: 21 additions & 9 deletions src/cgutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -305,19 +305,34 @@ static Value *emit_pointer_from_objref(jl_codectx_t &ctx, Value *V)
return Call;
}

static Value *get_gc_root_for(const jl_cgval_t &x)
static Value *emit_unbox(jl_codectx_t &ctx, Type *to, const jl_cgval_t &x, jl_value_t *jt);
static void emit_unbox_store(jl_codectx_t &ctx, const jl_cgval_t &x, Value* dest, MDNode *tbaa_dest, unsigned alignment, bool isVolatile=false);

static Value *get_gc_root_for(jl_codectx_t &ctx, const jl_cgval_t &x)
{
if (x.Vboxed)
if (x.constant || x.typ == jl_bottom_type)
return nullptr;
if (x.Vboxed) // superset of x.isboxed
return x.Vboxed;
if (x.ispointer() && !x.constant) {
assert(!x.isboxed);
#ifndef NDEBUG
if (x.ispointer()) {
assert(x.V);
if (PointerType *T = dyn_cast<PointerType>(x.V->getType())) {
if (T->getAddressSpace() == AddressSpace::Tracked ||
T->getAddressSpace() == AddressSpace::Derived) {
return x.V;
assert(T->getAddressSpace() != AddressSpace::Tracked);
if (T->getAddressSpace() == AddressSpace::Derived) {
// n.b. this IR would not be valid after LLVM-level inlining,
// since codegen does not have a way to determine the whether
// this argument value needs to be re-rooted
}
}
}
#endif
if (jl_is_concrete_immutable(x.typ) && !jl_is_pointerfree(x.typ)) {
Type *T = julia_type_to_llvm(ctx, x.typ);
return emit_unbox(ctx, T, x, x.typ);
}
// nothing here to root, move along
return nullptr;
}

Expand Down Expand Up @@ -1795,9 +1810,6 @@ static Value *emit_bounds_check(jl_codectx_t &ctx, const jl_cgval_t &ainfo, jl_v
return im1;
}

static Value *emit_unbox(jl_codectx_t &ctx, Type *to, const jl_cgval_t &x, jl_value_t *jt);
static void emit_unbox_store(jl_codectx_t &ctx, const jl_cgval_t &x, Value* dest, MDNode *tbaa_dest, unsigned alignment, bool isVolatile=false);

static void emit_write_barrier(jl_codectx_t&, Value*, ArrayRef<Value*>);
static void emit_write_barrier(jl_codectx_t&, Value*, Value*);
static void emit_write_multibarrier(jl_codectx_t&, Value*, Value*, jl_value_t*);
Expand Down
20 changes: 8 additions & 12 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3028,9 +3028,12 @@ static Value *emit_bits_compare(jl_codectx_t &ctx, jl_cgval_t arg1, jl_cgval_t a
varg2 = emit_pointer_from_objref(ctx, varg2);
Value *gc_uses[2];
int nroots = 0;
if ((gc_uses[nroots] = get_gc_root_for(arg1)))
// these roots may seem a bit overkill, but we want to make sure
// that a!=b implies (a,)!=(b,) even if a and b are unused and
// therefore could be freed and then the memory for a reused for b
if ((gc_uses[nroots] = get_gc_root_for(ctx, arg1)))
nroots++;
if ((gc_uses[nroots] = get_gc_root_for(arg2)))
if ((gc_uses[nroots] = get_gc_root_for(ctx, arg2)))
nroots++;
OperandBundleDef OpBundle("jl_roots", makeArrayRef(gc_uses, nroots));
auto answer = ctx.builder.CreateCall(prepare_call(memcmp_func), {
Expand Down Expand Up @@ -5790,16 +5793,9 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_
}
std::vector<Value*> vals;
for (size_t i = 0; i < nargs; ++i) {
const jl_cgval_t &ai = argv[i];
if (ai.constant || ai.typ == jl_bottom_type)
continue;
if (ai.isboxed) {
vals.push_back(ai.Vboxed);
}
else if (jl_is_concrete_immutable(ai.typ) && !jl_is_pointerfree(ai.typ)) {
Type *at = julia_type_to_llvm(ctx, ai.typ);
vals.push_back(emit_unbox(ctx, at, ai, ai.typ));
}
Value *gc_root = get_gc_root_for(ctx, argv[i]);
if (gc_root)
vals.push_back(gc_root);
}
Value *token = vals.empty()
? (Value*)ConstantTokenNone::get(ctx.builder.getContext())
Expand Down
6 changes: 3 additions & 3 deletions test/compiler/codegen.jl
Original file line number Diff line number Diff line change
Expand Up @@ -574,12 +574,12 @@ end
end
@test occursin("llvm.julia.gc_preserve_begin", get_llvm(f3, Tuple{Bool}, true, false, false))

# unions of immutables (JuliaLang/julia#39501)
# PhiNode of unions of immutables (JuliaLang/julia#39501)
function f2(cond)
val = cond ? 1 : 1f0
val = cond ? 1 : ""
GC.@preserve val begin end
end
@test !occursin("llvm.julia.gc_preserve_begin", get_llvm(f2, Tuple{Bool}, true, false, false))
@test occursin("llvm.julia.gc_preserve_begin", get_llvm(f2, Tuple{Bool}, true, false, false))
# make sure the fix for the above doesn't regress #34241
function f4(cond)
val = cond ? ([1],) : ([1f0],)
Expand Down

0 comments on commit 8329725

Please sign in to comment.