Skip to content

Commit a84a058

Browse files
vtjnashvtjnash
authored
Merge pull request #43953 from JuliaLang/jn/43636
impose stricter and earlier validation on ccall/cfunction types
2 parents dc0aa61 + 418fbb3 commit a84a058

File tree

4 files changed

+121
-75
lines changed

4 files changed

+121
-75
lines changed

‎src/ccall.cpp‎

Lines changed: 27 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1026,7 +1026,7 @@ std::string generate_func_sig(const char *fname)
10261026
abi->use_sret(jl_nothing_type, lrt->getContext());
10271027
}
10281028
else {
1029-
if (!jl_is_datatype(rt) || ((jl_datatype_t*)rt)->layout == NULL || jl_is_layout_opaque(((jl_datatype_t*)rt)->layout) || jl_is_cpointer_type(rt) || retboxed) {
1029+
if (retboxed || jl_is_cpointer_type(rt) || lrt->isPointerTy()) {
10301030
prt = lrt; // passed as pointer
10311031
abi->use_sret(jl_voidpointer_type, lrt->getContext());
10321032
}
@@ -1077,8 +1077,8 @@ std::string generate_func_sig(const char *fname)
10771077
}
10781078

10791079
t = _julia_struct_to_llvm(ctx, lrt->getContext(), tti, &isboxed, llvmcall);
1080-
if (t == NULL || t == getVoidTy(lrt->getContext())) {
1081-
return make_errmsg(fname, i + 1, " doesn't correspond to a C type");
1080+
if (t == getVoidTy(lrt->getContext())) {
1081+
return make_errmsg(fname, i + 1, " type doesn't correspond to a C type");
10821082
}
10831083
}
10841084

@@ -1215,14 +1215,21 @@ static const std::string verify_ccall_sig(jl_value_t *&rt, jl_value_t *at,
12151215
JL_TYPECHK(ccall, type, rt);
12161216
JL_TYPECHK(ccall, simplevector, at);
12171217

1218-
if (jl_is_array_type(rt)) {
1219-
// `Array` used as return type just returns a julia object reference
1220-
rt = (jl_value_t*)jl_any_type;
1218+
if (rt == (jl_value_t*)jl_any_type || jl_is_array_type(rt) ||
1219+
(jl_is_datatype(rt) && ((jl_datatype_t*)rt)->layout != NULL &&
1220+
jl_is_layout_opaque(((jl_datatype_t*)rt)->layout))) {
1221+
// n.b. `Array` used as return type just returns a julia object reference
1222+
lrt = JuliaType::get_prjlvalue_ty(ctxt);
1223+
retboxed = true;
1224+
}
1225+
else {
1226+
// jl_type_mappable_to_c should have already ensured that these are valid
1227+
assert(jl_is_structtype(rt) || jl_is_primitivetype(rt) || rt == (jl_value_t*)jl_bottom_type);
1228+
lrt = _julia_struct_to_llvm(ctx, ctxt, rt, &retboxed, llvmcall);
1229+
assert(!retboxed);
1230+
if (CountTrackedPointers(lrt).count != 0)
1231+
return "return type struct fields cannot contain a reference";
12211232
}
1222-
1223-
lrt = _julia_struct_to_llvm(ctx, ctxt, rt, &retboxed, llvmcall);
1224-
if (lrt == NULL)
1225-
return "return type doesn't correspond to a C type";
12261233

12271234
// is return type fully statically known?
12281235
if (unionall_env == NULL) {
@@ -1391,7 +1398,6 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs)
13911398
assert(retboxed ? lrt == ctx.types().T_prjlvalue : lrt == getSizeTy(ctx.builder.getContext()));
13921399
assert(!isVa && !llvmcall && nccallargs == 1);
13931400
jl_value_t *tti = jl_svecref(at, 0);
1394-
Value *ary;
13951401
Type *largty;
13961402
bool isboxed;
13971403
if (jl_is_abstract_ref_type(tti)) {
@@ -1402,29 +1408,21 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs)
14021408
else {
14031409
largty = _julia_struct_to_llvm(&ctx.emission_context, ctx.builder.getContext(), tti, &isboxed, llvmcall);
14041410
}
1411+
Value *retval;
14051412
if (isboxed) {
1406-
ary = boxed(ctx, argv[0]);
1413+
retval = boxed(ctx, argv[0]);
1414+
retval = emit_pointer_from_objref(ctx, emit_bitcast(ctx, retval, ctx.types().T_prjlvalue));
14071415
}
14081416
else {
1409-
ary = emit_unbox(ctx, largty, argv[0], tti);
1417+
retval = emit_unbox(ctx, largty, argv[0], tti);
1418+
retval = emit_inttoptr(ctx, retval, ctx.types().T_pjlvalue);
14101419
}
1420+
// retval is now an untracked jl_value_t*
1421+
if (retboxed)
1422+
// WARNING: this addrspace cast necessarily implies that the value is rooted elsewhere!
1423+
retval = ctx.builder.CreateAddrSpaceCast(retval, ctx.types().T_prjlvalue);
14111424
JL_GC_POP();
1412-
if (!retboxed) {
1413-
return mark_or_box_ccall_result(
1414-
ctx,
1415-
ctx.builder.CreatePtrToInt(
1416-
emit_pointer_from_objref(ctx, emit_bitcast(ctx, ary, ctx.types().T_prjlvalue)),
1417-
getSizeTy(ctx.builder.getContext())),
1418-
retboxed, rt, unionall, static_rt);
1419-
}
1420-
else {
1421-
return mark_or_box_ccall_result(
1422-
ctx,
1423-
ctx.builder.CreateAddrSpaceCast(
1424-
emit_inttoptr(ctx, ary, ctx.types().T_pjlvalue),
1425-
ctx.types().T_prjlvalue), // WARNING: this addrspace cast necessarily implies that the value is rooted elsewhere!
1426-
retboxed, rt, unionall, static_rt);
1427-
}
1425+
return mark_or_box_ccall_result(ctx, retval, retboxed, rt, unionall, static_rt);
14281426
}
14291427
else if (is_libjulia_func(jl_cpu_pause)) {
14301428
// Keep in sync with the julia_threads.h version

‎src/cgutils.cpp‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -573,8 +573,10 @@ static Type *_julia_struct_to_llvm(jl_codegen_params_t *ctx, LLVMContext &ctxt,
573573
bool isTuple = jl_is_tuple_type(jt);
574574
jl_svec_t *ftypes = jl_get_fieldtypes(jst);
575575
size_t i, ntypes = jl_svec_len(ftypes);
576-
if (!jl_struct_try_layout(jst))
577-
return NULL; // caller should have checked jl_type_mappable_to_c already, but we'll be nice
576+
if (!jl_struct_try_layout(jst)) {
577+
assert(0 && "caller should have checked jl_type_mappable_to_c already");
578+
abort();
579+
}
578580
if (ntypes == 0 || jl_datatype_nbits(jst) == 0)
579581
return getVoidTy(ctxt);
580582
Type *_struct_decl = NULL;
@@ -597,7 +599,8 @@ static Type *_julia_struct_to_llvm(jl_codegen_params_t *ctx, LLVMContext &ctxt,
597599
if (jl_field_isatomic(jst, i)) {
598600
// TODO: eventually support this?
599601
// though it's a bit unclear how the implicit load should be interpreted
600-
return NULL;
602+
assert(0 && "caller should have checked jl_type_mappable_to_c already");
603+
abort();
601604
}
602605
Type *lty;
603606
if (jl_field_isptr(jst, i)) {

‎src/jltypes.c‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -242,14 +242,17 @@ int jl_has_fixed_layout(jl_datatype_t *dt)
242242
int jl_type_mappable_to_c(jl_value_t *ty)
243243
{
244244
assert(!jl_is_typevar(ty) && jl_is_type(ty));
245-
if (jl_is_structtype(ty)) {
246-
jl_datatype_t *jst = (jl_datatype_t*)ty;
247-
return jl_has_fixed_layout(jst);
248-
}
249-
ty = jl_unwrap_unionall(ty);
250-
if (jl_is_tuple_type(ty) || jl_is_namedtuple_type(ty))
251-
return 0; // TODO: relax some?
252-
return 1; // as boxed or primitive
245+
if (jl_is_structtype(ty))
246+
return jl_has_fixed_layout((jl_datatype_t*)ty) && ((jl_datatype_t*)ty)->name->atomicfields == NULL;
247+
if (jl_is_primitivetype(ty))
248+
return 1;
249+
if (ty == (jl_value_t*)jl_any_type || ty == (jl_value_t*)jl_bottom_type)
250+
return 1; // as boxed
251+
if (jl_is_abstract_ref_type(ty) || jl_is_array_type(ty) ||
252+
(jl_is_datatype(ty) && ((jl_datatype_t*)ty)->layout != NULL &&
253+
jl_is_layout_opaque(((jl_datatype_t*)ty)->layout)))
254+
return 1; // as boxed
255+
return 0; // refuse to map Union and UnionAll to C
253256
}
254257

255258
// Return true for any type (Integer or Unsigned) that can fit in a

‎test/ccall.jl‎

Lines changed: 77 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -924,61 +924,77 @@ for (t, v) in ((Complex{Int32}, :ci32), (Complex{Int64}, :ci64),
924924
@test b === c
925925
let cf = @cfunction($fname1, Ref{$t}, (Ref{$t},))
926926
b = ccall(cf, Ref{$t}, (Ref{$t},), a)
927+
verbose && println("C: ", b)
928+
@test b == $v
929+
@test b === a
930+
@test b === c
927931
end
928-
verbose && println("C: ", b)
929-
@test b == $v
930-
@test b === a
931-
@test b === c
932932
let cf = @cfunction($fname, $t, ($t,))
933933
b = ccall(cf, $t, ($t,), a)
934-
end
935-
verbose && println("C: ",b)
936-
@test b == $v
937-
if ismutable($v)
938-
@test !(b === c)
939-
@test !(b === a)
934+
verbose && println("C: ",b)
935+
@test b == $v
936+
if ismutable($v)
937+
@test !(b === c)
938+
@test !(b === a)
939+
end
940940
end
941941
let cf = @cfunction($fname1, $t, (Ref{$t},))
942942
b = ccall(cf, $t, (Ref{$t},), a)
943-
end
944-
verbose && println("C: ",b)
945-
@test b == $v
946-
if ismutable($v)
947-
@test !(b === c)
948-
@test !(b === a)
943+
verbose && println("C: ",b)
944+
@test b == $v
945+
if ismutable($v)
946+
@test !(b === c)
947+
@test !(b === a)
948+
end
949949
end
950950
let cf = @cfunction($fname, Ref{$t}, ($t,))
951951
b = ccall(cf, Ref{$t}, ($t,), a)
952-
end
953-
verbose && println("C: ",b)
954-
@test b == $v
955-
@test b === c
956-
if ismutable($v)
957-
@test !(b === a)
952+
verbose && println("C: ",b)
953+
@test b == $v
954+
@test b === c
955+
if ismutable($v)
956+
@test !(b === a)
957+
end
958958
end
959959
let cf = @cfunction($fname, Any, (Ref{$t},))
960960
b = ccall(cf, Any, (Ref{$t},), $v)
961-
end
962-
verbose && println("C: ",b)
963-
@test b == $v
964-
@test b === c
965-
if ismutable($v)
966-
@test !(b === a)
961+
verbose && println("C: ",b)
962+
@test b == $v
963+
@test b === c
964+
if ismutable($v)
965+
@test !(b === a)
966+
end
967967
end
968968
let cf = @cfunction($fname, Any, (Ref{Any},))
969969
b = ccall(cf, Any, (Ref{Any},), $v)
970+
@test b == $v
971+
@test b === c
972+
if ismutable($v)
973+
@test !(b === a)
974+
end
970975
end
971-
@test b == $v
972-
@test b === c
973-
if ismutable($v)
974-
@test !(b === a)
976+
a isa Complex && let cf = @cfunction($fname, Ref{Complex}, (Ref{Any},))
977+
b = ccall(cf, Ref{Complex}, (Ref{Any},), $v)
978+
@test b == $v
979+
@test b === c
980+
if ismutable($v)
981+
@test !(b === a)
982+
end
975983
end
976984
let cf = @cfunction($fname, Ref{AbstractString}, (Ref{Any},))
977985
@test_throws TypeError ccall(cf, Any, (Ref{Any},), $v)
978986
end
979-
let cf = @cfunction($fname, AbstractString, (Ref{Any},))
987+
let cf = @cfunction($fname, Ref{Ptr}, (Ref{Any},))
988+
@test_throws TypeError ccall(cf, Any, (Ref{Any},), $v)
989+
end
990+
let cf = @cfunction($fname, Ref{Complex{UInt32}}, (Ref{Any},))
980991
@test_throws TypeError ccall(cf, Any, (Ref{Any},), $v)
981992
end
993+
if a isa Complex
994+
local mkcb(::Type{T}) where {T} = @cfunction($fname, Ref{Complex{T}}, (Ref{Any},))
995+
@test ccall(mkcb(typeof(a.re)), Any, (Ref{Any},), $v) === $v
996+
end
997+
#FIXME: @test_throws TypeError ccall(mkcb(UInt32), Any, (Ref{Any},), $v)
982998
end
983999
end
9841000

@@ -1484,7 +1500,21 @@ end
14841500
@test_throws(TypeError, @eval if false; ccall(:fn, Some.var, ()); end)
14851501
@test_throws(TypeError, @eval if false; ccall(:fn, Cvoid, (Some.var,), Some(0)); end)
14861502
@test_throws(ErrorException("ccall method definition: Vararg not allowed for argument list"),
1487-
@eval ccall(+, Int, (Vararg{Int},), 1))
1503+
@eval ccall(:fn, Int, (Vararg{Int},), 1))
1504+
@test_throws(ErrorException("ccall method definition: argument 1 type doesn't correspond to a C type"),
1505+
@eval ccall(:fn, Int, (Integer,), 1))
1506+
@test_throws(ErrorException("ccall method definition: argument 1 type doesn't correspond to a C type"),
1507+
@eval ccall(:fn, Int, (Ptr,), C_NULL))
1508+
@test_throws(ErrorException("ccall method definition: return type doesn't correspond to a C type"),
1509+
@eval ccall(:fn, Integer, (Integer,), 1))
1510+
@test_throws(ErrorException("ccall method definition: return type doesn't correspond to a C type"),
1511+
@eval ccall(:fn, Ptr, ()))
1512+
# This is hard to test: @test_throws(ErrorException("ccall argument 1 type doesn't correspond to a C type"),
1513+
# @eval ccall(:fn, Int, (Union{},), 1))
1514+
@test_throws(ErrorException("ccall argument 1 type doesn't correspond to a C type"),
1515+
@eval ccall(:fn, Int, (Nothing,), nothing))
1516+
@test_throws(ErrorException("ccall return type struct fields cannot contain a reference"),
1517+
@eval ccall(:fn, typeof(Ref("")), ()))
14881518

14891519
# test for malformed syntax errors
14901520
@test Expr(:error, "more arguments than types for ccall") == Meta.lower(@__MODULE__, :(ccall(:fn, A, (), x)))
@@ -1525,10 +1555,22 @@ evalf_callback_19805(ci::callinfos_19805{FUNC_FT}) where {FUNC_FT} = ci.f(0.5)::
15251555
@eval () -> @cfunction(+, Int, (Ref{T}, Ref{T})) where T)
15261556
@test_throws(ErrorException("could not evaluate cfunction return type (it might depend on a local variable)"),
15271557
@eval () -> @cfunction(+, Ref{T}, (Int, Int)) where T)
1528-
@test_throws(ErrorException("cfunction argument 2 doesn't correspond to a C type"),
1558+
@test_throws(ErrorException("cfunction argument 2 type doesn't correspond to a C type"),
15291559
@eval @cfunction(+, Int, (Int, Nothing)))
1560+
@test_throws(ErrorException("cfunction argument 2 type doesn't correspond to a C type"),
1561+
@eval @cfunction(+, Int, (Int, Union{})))
15301562
@test_throws(ErrorException("cfunction return type Ref{Any} is invalid. Use Any or Ptr{Any} instead."),
15311563
@eval @cfunction(+, Ref{Any}, (Int, Int)))
1564+
@test_throws(ErrorException("cfunction method definition: argument 1 type doesn't correspond to a C type"),
1565+
@eval @cfunction(+, Int, (Integer, Integer)))
1566+
@test_throws(ErrorException("cfunction method definition: argument 1 type doesn't correspond to a C type"),
1567+
@eval @cfunction(+, Int, (Ptr,)))
1568+
@test_throws(ErrorException("cfunction method definition: return type doesn't correspond to a C type"),
1569+
@eval @cfunction(+, Integer, (Int, Int)))
1570+
@test_throws(ErrorException("cfunction method definition: return type doesn't correspond to a C type"),
1571+
@eval @cfunction(+, Ptr, (Int, Int)))
1572+
@test_throws(ErrorException("cfunction return type struct fields cannot contain a reference"),
1573+
@eval @cfunction(+, typeof(Ref("")), ()))
15321574

15331575
# test Ref{abstract_type} calling parameter passes a heap box
15341576
abstract type Abstract22734 end

0 commit comments

Comments
 (0)