From 3dacfa0ce90a960c0c4fce4e576d48bee2f99ba9 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 15 Sep 2022 16:54:07 -0400 Subject: [PATCH] ccall: handle Union appearing as a field-type without crashing 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 --- src/abi_aarch64.cpp | 18 ++++++++++++------ src/abi_arm.cpp | 2 ++ src/abi_ppc64le.cpp | 7 +++++-- src/abi_x86_64.cpp | 4 ++++ src/codegen.cpp | 22 +--------------------- test/ccall.jl | 26 ++++++++++++++++++++++++++ 6 files changed, 50 insertions(+), 29 deletions(-) diff --git a/src/abi_aarch64.cpp b/src/abi_aarch64.cpp index 1a3f160329c6c..514c3c5a81a6d 100644 --- a/src/abi_aarch64.cpp +++ b/src/abi_aarch64.cpp @@ -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) { @@ -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)) { @@ -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)) { diff --git a/src/abi_arm.cpp b/src/abi_arm.cpp index 4987d07657ae6..441aa95b1fdf6 100644 --- a/src/abi_arm.cpp +++ b/src/abi_arm.cpp @@ -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) diff --git a/src/abi_ppc64le.cpp b/src/abi_ppc64le.cpp index 016eebd455525..2e18acdbd4f4b 100644 --- a/src/abi_ppc64le.cpp +++ b/src/abi_ppc64le.cpp @@ -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) { @@ -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; } @@ -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); diff --git a/src/abi_x86_64.cpp b/src/abi_x86_64.cpp index 43e539b8386ce..c3d12417e6de8 100644 --- a/src/abi_x86_64.cpp +++ b/src/abi_x86_64.cpp @@ -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)); } } diff --git a/src/codegen.cpp b/src/codegen.cpp index 0346393ea67c3..9165e3e7e222c 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -4260,26 +4260,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); - if (bp == NULL) - return jl_cgval_t(); - 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; @@ -4930,7 +4910,7 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_ { 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); diff --git a/test/ccall.jl b/test/ccall.jl index d302d5e3d75d0..e3d52cc1498db 100644 --- a/test/ccall.jl +++ b/test/ccall.jl @@ -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