Skip to content

Commit

Permalink
recursive types: fix implementation of references_name (#50963)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@vtjnash
vtjnash authored Aug 21, 2023
1 parent 6fd82d7 commit 1182003
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 25 deletions.
58 changes: 35 additions & 23 deletions src/builtins.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}
Expand Down
38 changes: 36 additions & 2 deletions test/core.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down

1 comment on commit 1182003

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(isdaily = true)

Please sign in to comment.