fix #33954, recursion through field types in is_derived_type
#34223
Conversation
JeffBezanson
added
bugfix
JeffBezanson
force-pushed
the
jb/fix33954
branch
from
558af50 to
3bac205
Compare
src/datatype.c
Outdated
| @@ -523,6 +525,7 @@ void jl_compute_field_offsets(jl_datatype_t *st) | |||
| //else if (st->layout->fielddesc_type != 0) // GC only implements support for this | |||
| // isinlinealloc = 0; | |||
| isinlinealloc = 0; | |||
| isbitstype = 0; | |||
There was a problem hiding this comment.
we just asserted this was zero on the previous line
There was a problem hiding this comment.
Yes I have a fix for this.
There was a problem hiding this comment.
It seemed like there might be a case above where we did set not-inlinealloc (from references-name), but don’t simultaneously also set not-isbitstype
I agree. Let's just delete it then—I don't think it's even correct as written (except perhaps for the trivial case of maxdepth=1). I think it was supposed to permit cases like this: |
JeffBezanson
force-pushed
the
jb/fix33954
branch
from
3bac205 to
cd0848d
Compare
src/datatype.c
Outdated
| @@ -306,6 +306,8 @@ static int references_name(jl_value_t *p, jl_typename_t *name) JL_NOTSAFEPOINT | |||
| if (jl_is_datatype(p)) { | |||
| if (((jl_datatype_t*)p)->name == name) | |||
| return 1; | |||
| if (((jl_datatype_t*)p)->layout && (jl_datatype_nfields(p) == 0 || ((jl_datatype_t*)p)->isbitstype)) | |||
There was a problem hiding this comment.
I’m not sure we’ve always computed layout yet. In theory, this can eventually be changed to search the declared type structure (wrapper) for a use of the TypeVar as the direct field type (or as a Union) and cached on the TypeName
JeffBezanson
force-pushed
the
jb/fix33954
branch
from
cd0848d to
aa51f85
Compare
(cherry picked from commit 8b57f64)
…e` (JuliaLang#34223) (cherry picked from commit 8b57f64)
I'm not fully sure how best to address this. But, I assume looking through field types in
is_derived_typecan't be that important, since we don't do it for any non-bits types. So I just tried something simple and limited the depth of recursion that can happen via that path.fix #33954