Skip to content

recursive types: fix implementation of references_name #50963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 21, 2023
Merged

recursive types: fix implementation of references_name #50963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 35 additions & 23 deletions src/builtins.c
View file
Open in desktop
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
Open in desktop
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