Skip to content

Commit

Permalink
Properly guard UpsilonNode unboxed store
Browse files Browse the repository at this point in the history
In #51852, we are coercing a boxed `Union{@NamedTuple{progress::String}, @NamedTuple{progress::Float64}}`
to `@NamedTuple{progress::String}` via convert_julia_type. This results in a jl_cgval_t that has
a Vboxed that points to a boxed `@NamedTuple{progress::Float64}` but with a `@NamedTuple{progress::String}`
type tag that the up upsilonnode code then tries to unbox into the typed PhiC slot. This ends up treating
the Float64 as a pointer and crashing in GC. Avoid this by adding a runtime check that the converted value
is actually compatible (we already had this kind of check for the non-boxed cases) and then making the
unboxing runtime-conditional on the type of the union matching the type of the phic.

Fixes #51852
  • Loading branch information
Keno authored and staticfloat committed Oct 25, 2023
1 parent ab8637f commit bb95366
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 7 deletions.
3 changes: 3 additions & 0 deletions src/cgutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,9 @@ static void null_pointer_check(jl_codectx_t &ctx, Value *v, Value **nullcheck =
template<typename Func>
static Value *emit_guarded_test(jl_codectx_t &ctx, Value *ifnot, Value *defval, Func &&func)
{
if (!ifnot) {
return func();
}
if (auto Cond = dyn_cast<ConstantInt>(ifnot)) {
if (Cond->isZero())
return defval;
Expand Down
29 changes: 22 additions & 7 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2321,6 +2321,9 @@ static jl_cgval_t convert_julia_type(jl_codectx_t &ctx, const jl_cgval_t &v, jl_
if (v.TIndex && !jl_is_pointerfree(typ)) {
// discovered that this union-split type must actually be isboxed
if (v.Vboxed) {
if (skip) {
*skip = ctx.builder.CreateNot(emit_exactly_isa(ctx, v, (jl_datatype_t*)typ));
}
return jl_cgval_t(v.Vboxed, true, typ, NULL, best_tbaa(ctx.tbaa(), typ));
}
else {
Expand Down Expand Up @@ -5194,9 +5197,13 @@ static void emit_varinfo_assign(jl_codectx_t &ctx, jl_varinfo_t &vi, jl_cgval_t
// If allow_mismatch is set, type mismatches will not result in traps.
// This is used for upsilon nodes, where the destination can have a narrower
// type than the store, if inference determines that the store is never read.
Value *dummy = NULL;
Value **skip = allow_mismatch ? &dummy : NULL;
rval_info = convert_julia_type(ctx, rval_info, slot_type, skip);
Value *skip = NULL;
rval_info = convert_julia_type(ctx, rval_info, slot_type, &skip);
if (!allow_mismatch && skip) {
CreateTrap(ctx.builder);
return;
}

if (rval_info.typ == jl_bottom_type)
return;

Expand Down Expand Up @@ -5241,8 +5248,13 @@ static void emit_varinfo_assign(jl_codectx_t &ctx, jl_varinfo_t &vi, jl_cgval_t

// store unboxed variables
if (!vi.boxroot || (vi.pTIndex && rval_info.TIndex)) {
emit_vi_assignment_unboxed(ctx, vi, isboxed, rval_info);
emit_guarded_test(ctx, skip ? ctx.builder.CreateNot(skip) : nullptr, nullptr, [&]{
emit_vi_assignment_unboxed(ctx, vi, isboxed, rval_info);
return nullptr;
});
}

return;
}

static void emit_assignment(jl_codectx_t &ctx, jl_value_t *l, jl_value_t *r, ssize_t ssaval)
Expand All @@ -5254,7 +5266,8 @@ static void emit_assignment(jl_codectx_t &ctx, jl_value_t *l, jl_value_t *r, ssi
int sl = jl_slot_number(l) - 1;
// it's a local variable
jl_varinfo_t &vi = ctx.slots[sl];
return emit_varinfo_assign(ctx, vi, rval_info, l);
emit_varinfo_assign(ctx, vi, rval_info, l);
return;
}

jl_module_t *mod;
Expand Down Expand Up @@ -5285,15 +5298,17 @@ static void emit_upsilonnode(jl_codectx_t &ctx, ssize_t phic, jl_value_t *val)
// upsilon node is not dynamically observed.
if (val) {
jl_cgval_t rval_info = emit_expr(ctx, val);
if (rval_info.typ == jl_bottom_type)
if (rval_info.typ == jl_bottom_type) {
// as a special case, PhiC nodes are allowed to use undefined
// values, since they are just copy operations, so we need to
// ignore the store (it will not by dynamically observed), while
// normally, for any other operation result, we'd assume this store
// was unreachable and dead
val = NULL;
else
}
else {
emit_varinfo_assign(ctx, vi, rval_info, NULL, true);
}
}
if (!val) {
if (vi.boxroot) {
Expand Down
16 changes: 16 additions & 0 deletions test/compiler/inference.jl
Original file line number Diff line number Diff line change
Expand Up @@ -5363,3 +5363,19 @@ function phic_type3()
end
@test Base.return_types(phic_type3) |> only === Union{Int, Float64}
@test phic_type3() === 2

# Issue #51852
function phic_type4()
a = (;progress = "a")
try
may_error(false)
let b = Base.inferencebarrier(true) ? (;progress = 1.0) : a
a = b
end
catch
end
GC.gc()
return a
end
@test Base.return_types(phic_type4) |> only === Union{@NamedTuple{progress::Float64}, @NamedTuple{progress::String}}
@test phic_type4() === (;progress = 1.0)

0 comments on commit bb95366

Please sign in to comment.