From 49661a1274cb8fc552bdbf24b993f3506a4835bd Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 17 Aug 2023 19:20:39 +0000 Subject: [PATCH] recursive types: fix implementation of references_name To do the old version correctly, we would need to carefully track whether we end up using that parameter as a parameter of any apply_type call in the fieldtype computation of any of the fields, recursively. Since any of those might cause recursion in the constructor graph, resulting in trying to layout some concrete type before we have finished computing the rest of the fieldtypes. That seems unnecessarily difficult to do well, so instead we go back to just doing the trivial cases. --- src/builtins.c | 58 ++++++++++++++++++++++++++++++-------------------- test/core.jl | 38 +++++++++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 25 deletions(-) diff --git a/src/builtins.c b/src/builtins.c index 4f75fd79eadcc..81afa8bae482a 100644 --- a/src/builtins.c +++ b/src/builtins.c @@ -1694,36 +1694,48 @@ static int equiv_field_types(jl_value_t *old, jl_value_t *ft) // inline it. The only way fields can reference this type (due to // syntax-enforced restrictions) is via being passed as a type parameter. Thus // we can conservatively check this by examining only the parameters of the -// dependent types. -// affects_layout is a hack introduced by #35275 to workaround a problem -// introduced by #34223: it checks whether we will potentially need to -// compute the layout of the object before we have fully computed the types of -// the fields during recursion over the allocation of the parameters for the -// field types (of the concrete subtypes) -static int references_name(jl_value_t *p, jl_typename_t *name, int affects_layout) JL_NOTSAFEPOINT -{ - if (jl_is_uniontype(p)) - return references_name(((jl_uniontype_t*)p)->a, name, affects_layout) || - references_name(((jl_uniontype_t*)p)->b, name, affects_layout); - if (jl_is_unionall(p)) - return references_name((jl_value_t*)((jl_unionall_t*)p)->var->lb, name, 0) || - references_name((jl_value_t*)((jl_unionall_t*)p)->var->ub, name, 0) || - references_name(((jl_unionall_t*)p)->body, name, affects_layout); +// dependent types. Additionally, a field might have already observed this +// object for layout purposes before we got around to deciding if inlining +// would be possible, so we cannot change the layout now if so. +// affects_layout is a (conservative) analysis of layout_uses_free_typevars +// freevars is a (conservative) analysis of what calling jl_has_bound_typevars from name->wrapper gives (TODO: just call this instead?) +static int references_name(jl_value_t *p, jl_typename_t *name, int affects_layout, int freevars) JL_NOTSAFEPOINT +{ + if (freevars && !jl_has_free_typevars(p)) + freevars = 0; + while (jl_is_unionall(p)) { + if (references_name((jl_value_t*)((jl_unionall_t*)p)->var->lb, name, 0, freevars) || + references_name((jl_value_t*)((jl_unionall_t*)p)->var->ub, name, 0, freevars)) + return 1; + p = ((jl_unionall_t*)p)->body; + } + if (jl_is_uniontype(p)) { + return references_name(((jl_uniontype_t*)p)->a, name, affects_layout, freevars) || + references_name(((jl_uniontype_t*)p)->b, name, affects_layout, freevars); + } if (jl_is_typevar(p)) return 0; // already checked by unionall, if applicable if (jl_is_datatype(p)) { jl_datatype_t *dp = (jl_datatype_t*)p; if (affects_layout && dp->name == name) return 1; - // affects_layout checks whether we will need to attempt to layout this - // type (based on whether all copies of it have the same layout) in - // that case, we still need to check the recursive parameters for - // layout recursion happening also, but we know it won't itself cause - // problems for the layout computation affects_layout = ((jl_datatype_t*)jl_unwrap_unionall(dp->name->wrapper))->layout == NULL; + // and even if it has a layout, the fields themselves might trigger layouts if they use tparam i + // rather than checking this for each field, we just assume it applies + if (!affects_layout && freevars && jl_field_names(dp) != jl_emptysvec) { + jl_svec_t *types = ((jl_datatype_t*)jl_unwrap_unionall(dp->name->wrapper))->types; + size_t i, l = jl_svec_len(types); + for (i = 0; i < l; i++) { + jl_value_t *ft = jl_svecref(types, i); + if (!jl_is_typevar(ft) && jl_has_free_typevars(ft)) { + affects_layout = 1; + break; + } + } + } size_t i, l = jl_nparams(p); for (i = 0; i < l; i++) { - if (references_name(jl_tparam(p, i), name, affects_layout)) + if (references_name(jl_tparam(p, i), name, affects_layout, freevars)) return 1; } } @@ -1759,12 +1771,12 @@ JL_CALLABLE(jl_f__typebody) // able to compute the layout of the object before needing to // publish it, so we must assume it cannot be inlined, if that // check passes, then we also still need to check the fields too. - if (!dt->name->mutabl && (nf == 0 || !references_name((jl_value_t*)dt->super, dt->name, 1))) { + if (!dt->name->mutabl && (nf == 0 || !references_name((jl_value_t*)dt->super, dt->name, 0, 1))) { int mayinlinealloc = 1; size_t i; for (i = 0; i < nf; i++) { jl_value_t *fld = jl_svecref(ft, i); - if (references_name(fld, dt->name, 1)) { + if (references_name(fld, dt->name, 1, 1)) { mayinlinealloc = 0; break; } diff --git a/test/core.jl b/test/core.jl index f90a3d968c19b..e65cf08be9e15 100644 --- a/test/core.jl +++ b/test/core.jl @@ -374,8 +374,8 @@ let ft = Base.datatype_fieldtypes @test ft(elT2.body)[1].parameters[1] === elT2 @test Base.isconcretetype(ft(elT2.body)[1]) end -#struct S22624{A,B,C} <: Ref{S22624{Int64,A}}; end -@test_broken @isdefined S22624 +struct S22624{A,B,C} <: Ref{S22624{Int,A}}; end +@test sizeof(S22624) == sizeof(S22624{Int,Int,Int}) == 0 # issue #42297 mutable struct Node42297{T, V} @@ -414,6 +414,18 @@ mutable struct FooFoo{A,B} y::FooFoo{A} end @test FooFoo{Int} <: FooFoo{Int,AbstractString}.types[1] +# make sure this self-referential struct doesn't crash type layout +struct SelfTyA{V} + a::Base.RefValue{V} +end +struct SelfTyB{T} + a::T + b::SelfTyA{SelfTyB{T}} +end +let T = Base.RefValue{SelfTyB{Int}} + @test sizeof(T) === sizeof(Int) + @test sizeof(T.types[1]) === 2 * sizeof(Int) +end let x = (2,3) @test +(x...) == 5 @@ -4110,7 +4122,29 @@ end let z1 = Z14477() @test isa(z1, Z14477) @test isa(z1.fld, Z14477) + @test isdefined(z1, :fld) + @test !isdefined(z1.fld, :fld) +end +struct Z14477B + fld::Union{Nothing,Z14477B} + Z14477B() = new(new(nothing)) end +let z1 = Z14477B() + @test isa(z1, Z14477B) + @test isa(z1.fld, Z14477B) + @test isa(z1.fld.fld, Nothing) +end +struct Z14477C{T} + fld::Z14477C{Int8} + Z14477C() = new{Int16}(new{Int8}()) +end +let z1 = Z14477C() + @test isa(z1, Z14477C) + @test isa(z1.fld, Z14477C) + @test isdefined(z1, :fld) + @test !isdefined(z1.fld, :fld) +end + # issue #8846, generic macros macro m8846(a, b=0)