Skip to content

Commit

Permalink
ccall: handle Union appearing as a field-type without crashing (#46787)
Browse files Browse the repository at this point in the history
We disallow union as the direct type, so perhaps we should disallow it
as a field-type also, but since we do allow references in those cases
typically, we will allow this also.

Also DRY the emit_global code, since it had bit-rotted relative to the
usual code path through emit_globalref (and apparently could still be
run though for handling the first argument to cfunction).

Fix #46786

(cherry picked from commit ed01ee0)
  • Loading branch information
vtjnash authored and KristofferC committed Oct 27, 2022
1 parent 1a2f7ce commit 3977585
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 28 deletions.
18 changes: 12 additions & 6 deletions src/abi_aarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@ Type *get_llvm_vectype(jl_datatype_t *dt, LLVMContext &ctx) const
// the homogeneity check.
jl_datatype_t *ft0 = (jl_datatype_t*)jl_field_type(dt, 0);
// `ft0` should be a `VecElement` type and the true element type
// should be a primitive type
if (ft0->name != jl_vecelement_typename ||
((jl_datatype_t*)jl_field_type(ft0, 0))->layout->nfields)
// should be a primitive type (nfields == 0)
if (!jl_is_datatype(ft0) || ft0->name != jl_vecelement_typename)
return nullptr;
jl_datatype_t *ft00 = (jl_datatype_t*)jl_field_type(ft0, 0);
if (!jl_is_datatype(ft00) || ft00->layout->nfields)
return nullptr;
for (size_t i = 1; i < nfields; i++) {
if (jl_field_type(dt, i) != (jl_value_t*)ft0) {
Expand Down Expand Up @@ -120,15 +122,17 @@ bool isHFAorHVA(jl_datatype_t *dt, size_t dsz, size_t &nele, ElementType &ele, L
// For composite types, find the first non zero sized member
size_t i;
size_t fieldsz;
for (i = 0;i < nfields;i++) {
for (i = 0; i < nfields; i++) {
if ((fieldsz = jl_field_size(dt, i))) {
break;
}
}
assert(i < nfields);
// If there's only one non zero sized member, try again on this member
// If there's only one non-zero sized member, try again on this member
if (fieldsz == dsz) {
dt = (jl_datatype_t*)jl_field_type(dt, i);
if (!jl_is_datatype(dt))
return false;
continue;
}
if (Type *vectype = get_llvm_vectype(dt, ctx)) {
Expand All @@ -140,11 +144,13 @@ bool isHFAorHVA(jl_datatype_t *dt, size_t dsz, size_t &nele, ElementType &ele, L
return true;
}
// Otherwise, process each members
for (;i < nfields;i++) {
for (; i < nfields; i++) {
size_t fieldsz = jl_field_size(dt, i);
if (fieldsz == 0)
continue;
jl_datatype_t *fieldtype = (jl_datatype_t*)jl_field_type(dt, i);
if (!jl_is_datatype(dt))
return false;
// Check element count.
// This needs to be done after the zero size member check
if (nele > 3 || !isHFAorHVA(fieldtype, fieldsz, nele, ele, ctx)) {
Expand Down
2 changes: 2 additions & 0 deletions src/abi_arm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ size_t isLegalHA(jl_datatype_t *dt, Type *&base, LLVMContext &ctx) const
size_t parent_members = jl_datatype_nfields(dt);
for (size_t i = 0; i < parent_members; ++i) {
jl_datatype_t *fdt = (jl_datatype_t*)jl_field_type(dt,i);
if (!jl_is_datatype(fdt))
return 0;

Type *T = isLegalHAType(fdt, ctx);
if (T)
Expand Down
7 changes: 5 additions & 2 deletions src/abi_ppc64le.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ struct ABI_PPC64leLayout : AbiLayout {
// count the homogeneous floating aggregate size (saturating at max count of 8)
unsigned isHFA(jl_datatype_t *ty, jl_datatype_t **ty0, bool *hva) const
{
if (jl_datatype_size(ty) > 128 || ty->layout->npointers || ty->layout->haspadding)
return 9;

size_t i, l = ty->layout->nfields;
// handle homogeneous float aggregates
if (l == 0) {
Expand All @@ -52,7 +55,7 @@ unsigned isHFA(jl_datatype_t *ty, jl_datatype_t **ty0, bool *hva) const
*hva = false;
if (*ty0 == NULL)
*ty0 = ty;
else if (*hva || ty->size != (*ty0)->size)
else if (*hva || jl_datatype_size(ty) != jl_datatype_size(*ty0))
return 9;
return 1;
}
Expand All @@ -69,7 +72,7 @@ unsigned isHFA(jl_datatype_t *ty, jl_datatype_t **ty0, bool *hva) const
*hva = true;
if (*ty0 == NULL)
*ty0 = ty;
else if (!*hva || ty->size != (*ty0)->size)
else if (!*hva || jl_datatype_size(ty) != jl_datatype_size(*ty0))
return 9;
for (i = 1; i < l; i++) {
jl_datatype_t *fld = (jl_datatype_t*)jl_field_type(ty, i);
Expand Down
4 changes: 4 additions & 0 deletions src/abi_x86_64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ void classifyType(Classification& accum, jl_datatype_t *dt, uint64_t offset) con
jl_value_t *ty = jl_field_type(dt, i);
if (jl_field_isptr(dt, i))
ty = (jl_value_t*)jl_voidpointer_type;
else if (!jl_is_datatype(ty)) { // inline union
accum.addField(offset, Memory);
continue;
}
classifyType(accum, (jl_datatype_t*)ty, offset + jl_field_offset(dt, i));
}
}
Expand Down
21 changes: 1 addition & 20 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3955,25 +3955,6 @@ static jl_cgval_t emit_sparam(jl_codectx_t &ctx, size_t i)
return mark_julia_type(ctx, sp, true, jl_any_type);
}

static jl_cgval_t emit_global(jl_codectx_t &ctx, jl_sym_t *sym)
{
jl_binding_t *jbp = NULL;
Value *bp = global_binding_pointer(ctx, ctx.module, sym, &jbp, false);
assert(bp != NULL);
if (jbp && jbp->value != NULL) {
if (jbp->constp)
return mark_julia_const(ctx, jbp->value);
// double-check that a global variable is actually defined. this
// can be a problem in parallel when a definition is missing on
// one machine.
LoadInst *v = ctx.builder.CreateAlignedLoad(ctx.types().T_prjlvalue, bp, Align(sizeof(void*)));
v->setOrdering(AtomicOrdering::Unordered);
tbaa_decorate(ctx.tbaa().tbaa_binding, v);
return mark_julia_type(ctx, v, true, jl_any_type);
}
return emit_checked_var(ctx, bp, sym, false, ctx.tbaa().tbaa_binding);
}

static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym)
{
Value *isnull = NULL;
Expand Down Expand Up @@ -4626,7 +4607,7 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaval)
{
if (jl_is_symbol(expr)) {
jl_sym_t *sym = (jl_sym_t*)expr;
return emit_global(ctx, sym);
return emit_globalref(ctx, ctx.module, sym, AtomicOrdering::Unordered);
}
if (jl_is_slot(expr) || jl_is_argument(expr)) {
return emit_local(ctx, expr);
Expand Down
26 changes: 26 additions & 0 deletions test/ccall.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1590,6 +1590,32 @@ function caller22734(ptr)
end
@test caller22734(ptr22734) === 32.0

# issue #46786 -- non-isbitstypes passed "by-value"
struct NonBits46786
x::Union{Int16,NTuple{3,UInt8}}
end
let ptr = @cfunction(identity, NonBits46786, (NonBits46786,))
obj1 = NonBits46786((0x01,0x02,0x03))
obj2 = ccall(ptr, NonBits46786, (NonBits46786,), obj1)
@test obj1 === obj2
end
let ptr = @cfunction(identity, Base.RefValue{NonBits46786}, (Base.RefValue{NonBits46786},))
obj1 = Base.RefValue(NonBits46786((0x01,0x02,0x03)))
obj2 = ccall(ptr, Base.RefValue{NonBits46786}, (Base.RefValue{NonBits46786},), obj1)
@test obj1 !== obj2
@test obj1.x === obj2.x
end

mutable struct MutNonBits46786
x::Union{Int16,NTuple{3,UInt8}}
end
let ptr = @cfunction(identity, MutNonBits46786, (MutNonBits46786,))
obj1 = MutNonBits46786((0x01,0x02,0x03))
obj2 = ccall(ptr, MutNonBits46786, (MutNonBits46786,), obj1)
@test obj1 !== obj2
@test obj1.x === obj2.x
end

# 26297#issuecomment-371165725
# test that the first argument to cglobal is recognized as a tuple literal even through
# macro expansion
Expand Down

0 comments on commit 3977585

Please sign in to comment.