Properly guard UpsilonNode unboxed store (#51853)

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.

base/compiler/abstractinterpretation.jl

@@ -2230,31 +2230,31 @@ end
 
 function abstract_eval_special_value(interp::AbstractInterpreter, @nospecialize(e), vtypes::Union{VarTable,Nothing}, sv::AbsIntState)
  if isa(e, QuoteNode)
- return Const(e.value)
+ return RTEffects(Const(e.value), EFFECTS_TOTAL)
  elseif isa(e, SSAValue)
- return abstract_eval_ssavalue(e, sv)
+ return RTEffects(abstract_eval_ssavalue(e, sv), EFFECTS_TOTAL)
  elseif isa(e, SlotNumber)
+ effects = EFFECTS_THROWS
  if vtypes !== nothing
  vtyp = vtypes[slot_id(e)]
- if vtyp.undef
- merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; nothrow=false))
+ if !vtyp.undef
+ effects = EFFECTS_TOTAL
  end
- return vtyp.typ
+ return RTEffects(vtyp.typ, effects)
  end
- merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; nothrow=false))
- return Any
+ return RTEffects(Any, effects)
  elseif isa(e, Argument)
  if vtypes !== nothing
- return vtypes[slot_id(e)].typ
+ return RTEffects(vtypes[slot_id(e)].typ, EFFECTS_TOTAL)
  else
  @assert isa(sv, IRInterpretationState)
- return sv.ir.argtypes[e.n] # TODO frame_argtypes(sv)[e.n] and remove the assertion
+ return RTEffects(sv.ir.argtypes[e.n], EFFECTS_TOTAL) # TODO frame_argtypes(sv)[e.n] and remove the assertion
  end
  elseif isa(e, GlobalRef)
  return abstract_eval_globalref(interp, e, sv)
  end
 
- return Const(e)
+ return RTEffects(Const(e), EFFECTS_TOTAL)
 end
 
 function abstract_eval_value_expr(interp::AbstractInterpreter, e::Expr, vtypes::Union{VarTable,Nothing}, sv::AbsIntState)
@@ -2288,8 +2288,9 @@ function abstract_eval_value(interp::AbstractInterpreter, @nospecialize(e), vtyp
  if isa(e, Expr)
  return abstract_eval_value_expr(interp, e, vtypes, sv)
  else
- typ = abstract_eval_special_value(interp, e, vtypes, sv)
- return collect_limitations!(typ, sv)
+ (;rt, effects) = abstract_eval_special_value(interp, e, vtypes, sv)
+ merge_effects!(interp, sv, effects)
+ return collect_limitations!(rt, sv)
  end
 end
 
@@ -2615,7 +2616,9 @@ function abstract_eval_phi(interp::AbstractInterpreter, phi::PhiNode, vtypes::Un
  for i in 1:length(phi.values)
  isassigned(phi.values, i) || continue
  val = phi.values[i]
- rt = tmerge(typeinf_lattice(interp), rt, abstract_eval_special_value(interp, val, vtypes, sv))
+ # N.B.: Phi arguments are restricted to not have effects, so we can drop
+ # them here safely.
+ rt = tmerge(typeinf_lattice(interp), rt, abstract_eval_special_value(interp, val, vtypes, sv).rt)
  end
  return rt
 end
@@ -2631,53 +2634,55 @@ function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e),
  add_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW)
  return abstract_eval_phi(interp, e, vtypes, sv)
  end
- return abstract_eval_special_value(interp, e, vtypes, sv)
- end
- (; rt, effects) = abstract_eval_statement_expr(interp, e, vtypes, sv)
- if effects.noub === NOUB_IF_NOINBOUNDS
- if !iszero(get_curr_ssaflag(sv) & IR_FLAG_INBOUNDS)
- effects = Effects(effects; noub=ALWAYS_FALSE)
- elseif !propagate_inbounds(sv)
- # The callee read our inbounds flag, but unless we propagate inbounds,
- # we ourselves don't read our parent's inbounds.
- effects = Effects(effects; noub=ALWAYS_TRUE)
+ (; rt, effects) = abstract_eval_special_value(interp, e, vtypes, sv)
+ else
+ (; rt, effects) = abstract_eval_statement_expr(interp, e, vtypes, sv)
+ if effects.noub === NOUB_IF_NOINBOUNDS
+ if !iszero(get_curr_ssaflag(sv) & IR_FLAG_INBOUNDS)
+ effects = Effects(effects; noub=ALWAYS_FALSE)
+ elseif !propagate_inbounds(sv)
+ # The callee read our inbounds flag, but unless we propagate inbounds,
+ # we ourselves don't read our parent's inbounds.
+ effects = Effects(effects; noub=ALWAYS_TRUE)
+ end
+ end
+ e = e::Expr
+ @assert !isa(rt, TypeVar) "unhandled TypeVar"
+ rt = maybe_singleton_const(rt)
+ if !isempty(sv.pclimitations)
+ if rt isa Const || rt === Union{}
+ empty!(sv.pclimitations)
+ else
+ rt = LimitedAccuracy(rt, sv.pclimitations)
+ sv.pclimitations = IdSet{InferenceState}()
+ end
  end
  end
  # N.B.: This only applies to the effects of the statement itself.
  # It is possible for arguments (GlobalRef/:static_parameter) to throw,
  # but these will be recomputed during SSA construction later.
  set_curr_ssaflag!(sv, flags_for_effects(effects), IR_FLAGS_EFFECTS)
  merge_effects!(interp, sv, effects)
- e = e::Expr
- @assert !isa(rt, TypeVar) "unhandled TypeVar"
- rt = maybe_singleton_const(rt)
- if !isempty(sv.pclimitations)
- if rt isa Const || rt === Union{}
- empty!(sv.pclimitations)
- else
- rt = LimitedAccuracy(rt, sv.pclimitations)
- sv.pclimitations = IdSet{InferenceState}()
- end
- end
+
  return rt
 end
 
 function isdefined_globalref(g::GlobalRef)
  return ccall(:jl_globalref_boundp, Cint, (Any,), g) != 0
 end
 
-function abstract_eval_globalref(g::GlobalRef)
+function abstract_eval_globalref_type(g::GlobalRef)
  if isdefined_globalref(g) && isconst(g)
  return Const(ccall(:jl_get_globalref_value, Any, (Any,), g))
  end
  ty = ccall(:jl_get_binding_type, Any, (Any, Any), g.mod, g.name)
  ty === nothing && return Any
  return ty
 end
-abstract_eval_global(M::Module, s::Symbol) = abstract_eval_globalref(GlobalRef(M, s))
+abstract_eval_global(M::Module, s::Symbol) = abstract_eval_globalref_type(GlobalRef(M, s))
 
 function abstract_eval_globalref(interp::AbstractInterpreter, g::GlobalRef, sv::AbsIntState)
- rt = abstract_eval_globalref(g)
+ rt = abstract_eval_globalref_type(g)
  consistent = inaccessiblememonly = ALWAYS_FALSE
  nothrow = false
  if isa(rt, Const)
@@ -2692,8 +2697,7 @@ function abstract_eval_globalref(interp::AbstractInterpreter, g::GlobalRef, sv::
  consistent = inaccessiblememonly = ALWAYS_TRUE
  rt = Union{}
  end
- merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; consistent, nothrow, inaccessiblememonly))
- return rt
+ return RTEffects(rt, Effects(EFFECTS_TOTAL; consistent, nothrow, inaccessiblememonly))
 end
 
 function handle_global_assignment!(interp::AbstractInterpreter, frame::InferenceState, lhs::GlobalRef, @nospecialize(newty))

base/compiler/optimize.jl

@@ -396,7 +396,7 @@ function argextype(
  elseif isa(x, QuoteNode)
  return Const(x.value)
  elseif isa(x, GlobalRef)
- return abstract_eval_globalref(x)
+ return abstract_eval_globalref_type(x)
  elseif isa(x, PhiNode)
  return Any
  elseif isa(x, PiNode)

base/compiler/ssair/slot2ssa.jl

@@ -207,7 +207,7 @@ function typ_for_val(@nospecialize(x), ci::CodeInfo, ir::IRCode, idx::Int, slott
  end
  return (ci.ssavaluetypes::Vector{Any})[idx]
  end
- isa(x, GlobalRef) && return abstract_eval_globalref(x)
+ isa(x, GlobalRef) && return abstract_eval_globalref_type(x)
  isa(x, SSAValue) && return (ci.ssavaluetypes::Vector{Any})[x.id]
  isa(x, Argument) && return slottypes[x.n]
  isa(x, NewSSAValue) && return types(ir)[new_to_regular(x, length(ir.stmts))]

base/compiler/utilities.jl

@@ -450,7 +450,7 @@ function is_throw_call(e::Expr)
  if e.head === :call
  f = e.args[1]
  if isa(f, GlobalRef)
- ff = abstract_eval_globalref(f)
+ ff = abstract_eval_globalref_type(f)
  if isa(ff, Const) && ff.val === Core.throw
  return true
  end

src/cgutils.cpp

@@ -1250,7 +1250,7 @@ static Value *emit_sizeof(jl_codectx_t &ctx, const jl_cgval_t &p)
  BasicBlock *dynloadBB = BasicBlock::Create(ctx.builder.getContext(), "dyn_sizeof", ctx.f);
  BasicBlock *postBB = BasicBlock::Create(ctx.builder.getContext(), "post_sizeof", ctx.f);
  Value *isboxed = ctx.builder.CreateICmpNE(
- ctx.builder.CreateAnd(p.TIndex, ConstantInt::get(getInt8Ty(ctx.builder.getContext()), 0x80)),
+ ctx.builder.CreateAnd(p.TIndex, ConstantInt::get(getInt8Ty(ctx.builder.getContext()), UNION_BOX_MARKER)),
  ConstantInt::get(getInt8Ty(ctx.builder.getContext()), 0));
  ctx.builder.CreateCondBr(isboxed, dynloadBB, postBB);
  ctx.builder.SetInsertPoint(dynloadBB);
@@ -1406,6 +1406,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;
@@ -1544,21 +1547,25 @@ static bool can_optimize_isa_union(jl_uniontype_t *type)
 }
 
 // a simple case of emit_isa that is obvious not to include a safe-point
-static Value *emit_exactly_isa(jl_codectx_t &ctx, const jl_cgval_t &arg, jl_datatype_t *dt)
+static Value *emit_exactly_isa(jl_codectx_t &ctx, const jl_cgval_t &arg, jl_datatype_t *dt, bool could_be_null=false)
 {
  assert(jl_is_concrete_type((jl_value_t*)dt));
  if (arg.TIndex) {
  unsigned tindex = get_box_tindex(dt, arg.typ);
  if (tindex > 0) {
  // optimize more when we know that this is a split union-type where tindex = 0 is invalid
- Value *xtindex = ctx.builder.CreateAnd(arg.TIndex, ConstantInt::get(getInt8Ty(ctx.builder.getContext()), 0x7f));
+ Value *xtindex = ctx.builder.CreateAnd(arg.TIndex, ConstantInt::get(getInt8Ty(ctx.builder.getContext()), ~UNION_BOX_MARKER));
  auto isa = ctx.builder.CreateICmpEQ(xtindex, ConstantInt::get(getInt8Ty(ctx.builder.getContext()), tindex));
  setName(ctx.emission_context, isa, "exactly_isa");
  return isa;
  }
  else if (arg.Vboxed) {
- // test for (arg.TIndex == 0x80 && typeof(arg.V) == type)
- Value *isboxed = ctx.builder.CreateICmpEQ(arg.TIndex, ConstantInt::get(getInt8Ty(ctx.builder.getContext()), 0x80));
+ // test for (arg.TIndex == UNION_BOX_MARKER && typeof(arg.V) == type)
+ Value *isboxed = ctx.builder.CreateICmpEQ(arg.TIndex, ConstantInt::get(getInt8Ty(ctx.builder.getContext()), UNION_BOX_MARKER));
+ if (could_be_null) {
+ isboxed = ctx.builder.CreateAnd(isboxed,
+ ctx.builder.CreateNot(null_pointer_cmp(ctx, arg.Vboxed)));
+ }
  setName(ctx.emission_context, isboxed, "isboxed");
  BasicBlock *currBB = ctx.builder.GetInsertBlock();
  BasicBlock *isaBB = BasicBlock::Create(ctx.builder.getContext(), "isa", ctx.f);
@@ -1579,9 +1586,16 @@ static Value *emit_exactly_isa(jl_codectx_t &ctx, const jl_cgval_t &arg, jl_data
  return ConstantInt::get(getInt1Ty(ctx.builder.getContext()), 0);
  }
  }
- auto isa = ctx.builder.CreateICmpEQ(emit_typeof(ctx, arg, false, true), emit_tagfrom(ctx, dt));
- setName(ctx.emission_context, isa, "exactly_isa");
- return isa;
+ Value *isnull = NULL;
+ if (could_be_null && arg.isboxed) {
+ isnull = null_pointer_cmp(ctx, arg.Vboxed);
+ }
+ Constant *Vfalse = ConstantInt::get(getInt1Ty(ctx.builder.getContext()), 0);
+ return emit_guarded_test(ctx, isnull, Vfalse, [&]{
+ auto isa = ctx.builder.CreateICmpEQ(emit_typeof(ctx, arg, false, true), emit_tagfrom(ctx, dt));
+ setName(ctx.emission_context, isa, "exactly_isa");
+ return isa;
+ });
 }
 
 static std::pair<Value*, bool> emit_isa(jl_codectx_t &ctx, const jl_cgval_t &x,
@@ -3152,7 +3166,7 @@ static AllocaInst *try_emit_union_alloca(jl_codectx_t &ctx, jl_uniontype_t *ut,
  * returning `Constant::getNullValue(ctx.types().T_pjlvalue)` in one of the skipped cases. If `skip` is not empty,
  * skip[0] (corresponding to unknown boxed) must always be set. In that
  * case, the calling code must separately deal with the case where
- * `vinfo` is already an unknown boxed union (union tag 0x80).
+ * `vinfo` is already an unknown boxed union (union tag UNION_BOX_MARKER).
  */
 // Returns ctx.types().T_prjlvalue
 static Value *box_union(jl_codectx_t &ctx, const jl_cgval_t &vinfo, const SmallBitVector &skip)