[PATCH 0/9] fortran: array descriptor part 7: Scalar descriptor init tweaks [PR122521]
Jerry D
jvdelisle2@gmail.com
Sat Sep 12 19:40:20 GMT 2026
On 9/3/26 12:27 PM, Mikael Morin wrote:
> From: Mikael Morin <mikael@gcc.gnu.org>
>
> Hello,
>
> this is the seventh part of the array descriptor series. The previous part,
> still awaiting review, isolated the initialization of scalar descriptors to
> three dedicated functions. These three functions do more or less the same
> thing, but with subtle differences, either caused by bugs, useless code, or
> different requirements or assumptions about their input.
>
> This part fixes the bugs, removes useless differences and unifies the three
> implementations.
>
> Patches 1, 4 and 9 fix bugs in the descriptor initialization. They are
> affecting either the elem_len value (patches 1 and 9) or the type value
> (patch 4).
> Patch 3 fixes a possible ICE bug.
> Patch 6 removes the initialization of the span.
> Patches 2 and 7 remove useless work in the descriptor initialization
> generation.
> Patches 5 and 8 factor common code to a shared function.
>
> Fortran-tested on aarch64-unknown-linux-gnu. OK for mainline?
>
> --
>
> Previous parts in the array descriptor series:
>
> part 6 (awaiting review): fortran: Move scalar descriptor init
> https://inbox.sourceware.org/gcc-patches/20260829150020.29239-1-morin-mikael@orange.fr/
> https://inbox.sourceware.org/fortran/20260829150020.29239-1-morin-mikael@orange.fr/
> https://patchwork.sourceware.org/project/gcc/cover/20260829150020.29239-1-morin-mikael@orange.fr/
>
> part 5 (pushed): fortran: Move null- or default-initialization
> part 4 (pushed): fortran: Move dtype fields constants
> part 3 (dropped): fortran: Ensure read-only getters
> part 2 (pushed): fortran: Add getters and setters
> part 1 (pushed): fortran: Move existing functions to a separate file
>
> Mikael Morin (9):
> fortran: array descriptor: Guess size from declared type [PR122521]
> fortran: array descriptor: Simplify scalar dtype initialization
> [PR122521]
> fortran: array descriptor: Add a check for class container type
> [PR122521]
> fortran: array descriptor: Mark polymorphic descriptors as such
> [PR122521]
> fortran: array descriptor: Factor scalar descriptor init 1/2
> [PR122521]
> fortran: array descriptor: Don't set the span in the scalar case
> [PR122521]
> fortran: array descriptor: Remove redundant array type unwrapping
> [PR122521]
> fortran: array descriptor: Factor scalar descriptor init 2/2
> [PR122521]
> fortran: array descriptor: Set polymorphic elem_len value [PR122521]
>
> gcc/fortran/trans-descriptor.cc | 149 +++++++++++++-----
> gcc/fortran/trans-descriptor.h | 6 +-
> gcc/fortran/trans-expr.cc | 4 +-
> gcc/fortran/trans-types.cc | 46 +++++-
> gcc/testsuite/gfortran.dg/assumed_rank_26.f90 | 26 +++
> .../gfortran.dg/coarray_collectives_18.f90 | 2 +-
> gcc/testsuite/gfortran.dg/sizeof_7.f90 | 30 ++++
> gcc/testsuite/gfortran.dg/sizeof_8.f90 | 33 ++++
> libgfortran/intrinsics/associated.c | 5 +-
> 9 files changed, 252 insertions(+), 49 deletions(-)
> create mode 100644 gcc/testsuite/gfortran.dg/assumed_rank_26.f90
> create mode 100644 gcc/testsuite/gfortran.dg/sizeof_7.f90
> create mode 100644 gcc/testsuite/gfortran.dg/sizeof_8.f90
>
Mikael,
This patch set looks mostly good to go. See the attached review report for a few
minor comments.
With those addressed Part 7 OK to go.
Thanks for the much needed work.
Regards,
Jerry
-------------- next part --------------
Review of "array descriptor part 7: Scalar descriptor init tweaks"
[PR122521], patches 1-9.
Applied on top of part 6 and regression-tested on
x86_64-pc-linux-gnu: 77278 passes, 349 expected failures, no
unexpected failures. The three new tests pass at all optimization
levels, and assumed_rank_26 also passes under -fsanitize=address.
Overall this is a clear improvement and unifying the three
implementations is the right endpoint. Two items need resolving
before it goes in (the ordering on part 6 below, and patch 6's stale
libgfortran hunk); the rest are questions.
== Series: depends on part 6 ==
The series depends on part 6 ("Move scalar descriptor init",
2026-08-29), still unreviewed. Part 6 does apply cleanly to current
mainline, so this is purely a question of taking it first.
== Patch 1: three questions and a nit ==
1. Scope. gfc_get_dtype_rank_type builds the dtype for every
descriptor, not only scalar ones. Making BT_CLASS take its size
from the declared type changes elem_len for any descriptor whose
element type is a class container. The ChangeLog mentions only
the scalar assumed-rank case -- is the wider change intended?
2. Bisection window. For unlimited polymorphic, etype now comes out
void, size stays NULL, and the dtype constructor omits elem_len,
so it reads 0 until patch 9 sets it at run time. Harmless if the
series lands as a unit, but worth a note in patch 1.
3. Dropped assert. BT_VOID went from
gcc_assert (TREE_CODE (ptype) == POINTER_TYPE);
size = size_in_bytes (ptype);
to leaving size NULL when ptype is not a pointer. FUNCTION_TYPE
also maps to BT_VOID, so procedure-typed descriptors now silently
get elem_len 0 instead of tripping the assert. Would allowing
only VOID_TYPE through, and still asserting otherwise, be better?
Nit, same function: the explanatory comment sits between the break
and the fall-through marker, reading as if it belonged to the break:
case BT_CLASS:
if (VOID_TYPE_P (etype))
break;
/* For classes, the element length isn't a known constant, ... */
/* Fall through. */
default:
Clearer above the case label.
== Patch 2: OK ==
Dropping gfc_get_scalar_to_descriptor_type also drops the
gfc_expr_attr() input, which looked like it might lose information.
It does not: gfc_build_dtype_constructor never populates the
attribute field, and the only other effect of the round trip was
caching GFC_TYPE_ARRAY_DTYPE on a throwaway type. Good removal.
== Patch 3: OK ==
Good minimal reproducer, and dg-do compile is right for an ICE fix.
See patch 4 for a follow-up cleanup of the check it adds.
== Patch 4: gcc_unreachable, and duplicated ref walking ==
1. gcc_unreachable on the unexpected shape:
tree class_ref;
if (!is_polymorphic_ref (scalar, &class_ref))
gcc_unreachable ();
Patch 3, immediately before, exists precisely because the input is
not always a class container reference. Turning the remaining
unexpected shapes into a hard abort trades a wrong-but-working
result for an ICE. If the invariant does hold after patch 3,
gcc_checking_assert states it without aborting release compilers.
Also class_ref is left uninitialized on the false path, which some
configurations will flag as -Wmaybe-uninitialized.
2. Duplicated ref walking. After this patch, is_polymorphic_ref has
already located the container, yet the test patch 3 added
re-derives it:
if (GFC_CLASS_TYPE_P (TREE_TYPE (tmp))
|| (POINTER_TYPE_P (TREE_TYPE (tmp))
&& GFC_CLASS_TYPE_P (TREE_TYPE (TREE_TYPE (tmp)))))
tmp = gfc_class_data_get (tmp);
Deriving tmp from class_ref would drop it. Keeping patch 3
separate makes sense if it may want backporting, but patch 4 could
clean up.
== Patch 5: no comments ==
== Patch 6: stale libgfortran hunk; span left indeterminate ==
1. The libgfortran hunk is stale (needs resolving). Mainline
5e8f400f12c (PR126964, 2026-09-01) rewrote that check two days
before this posting. associated() now reads:
/* Require that the storage sequences are the same. */
if (GFC_DESCRIPTOR_SIZE (pointer) != GFC_DESCRIPTOR_SIZE (target)
&& GFC_DESCRIPTOR_SPAN (pointer) != GFC_DESCRIPTOR_SPAN (target))
return 0;
The line the patch deletes no longer exists, so patch 6 does not
apply. I tested with that hunk dropped; nothing in the testsuite
depended on it.
2. The span is left indeterminate, not zeroed. This is my main
concern. Removing the initialization does not set the span to
anything -- it leaves whatever the descriptor variable held.
gfc_conv_scalar_to_descriptor builds it with gfc_create_var, so
that is indeterminate stack contents, not zero. Confirmed by tree
dump for a scalar passed to an assumed-rank dummy:
before: desc.data = (void * restrict) &i;
desc.span = (integer(kind=8)) desc.0.dtype.elem_len;
after: desc.data = (void * restrict) &i;
Two reasons this matters more than "a scalar has one element":
- The library already defines a meaning for an unset span. See
stride_in_bytes() in associated.c:
index_type span = GFC_DESCRIPTOR_SPAN (desc);
if (span == 0)
span = GFC_DESCRIPTOR_SIZE (desc);
That contract wants 0, and 0 is exactly what the patch does
not write.
- There is a rank-0 span read that is not guarded by rank. In
libgfortran/caf/single.c:696:
size_t dsize = opt_dst_desc->span;
for (int i = 0; i < GFC_DESCRIPTOR_RANK (opt_dst_desc); ++i)
dsize *= GFC_DESCRIPTOR_EXTENT (opt_dst_desc, i);
memcpy (old_dst_data_ptr, opt_dst_desc->base_addr, dsize);
For rank 0 the loop body never executes, so dsize *is* the
span and becomes a memcpy length directly.
Also worth noting the blast radius: gfc_conv_scalar_to_descriptor
has 19 call sites across trans.cc, trans-array.cc, trans-expr.cc,
trans-decl.cc, trans-intrinsic.cc and trans-openmp.cc, so this is
not confined to the polymorphic-scalar case.
Suggestion: set the span explicitly to 0 in the scalar case rather
than not setting it. That honours the convention the library
already implements, costs one store, and makes the libgfortran
hunk in item 1 unnecessary.
I did not manage to build a failing testcase -- scalar and class
coarray transfers behave identically before and after in both
-fcoarray=single and -fcoarray=lib -- so this is latent rather
than demonstrated.
== Patch 7: OK ==
The unwrapping really is redundant. For pointer-to-array-of-character
both forms leave ptype as the ARRAY_TYPE, so the BT_CHARACTER assert
and length still hold.
== Patch 8: OK ==
Straightforward.
== Patch 9: unlimited polymorphic character ==
gfc_vptr_size_get gives the vptr _size with no _len scaling, so for
CLASS(*) holding a character the element length is the kind size, not
the character length:
character(len=7) :: c
class(*), allocatable :: y; allocate (y, source=c)
! passed on to a CLASS(*) :: a(..) dummy
sizeof(a) => 1, expected 7
This is pre-existing -- I measured the same value with and without
the series, so it is not a regression -- but since patch 9's purpose
is to get the polymorphic elem_len right, it looks like an
incompleteness. gfc_resize_class_size_with_len (trans.cc) exists for
this, and gfc_get_span already uses it on the equivalent path. Was
the unlimited polymorphic case meant to be covered here?
One thing I checked and withdrew: I initially thought the vptr load
could fault for an absent optional polymorphic actual, since the guard
tests the vptr value and loading it dereferences the container. It
cannot: gfc_conv_class_to_class wraps the whole block in a presence
COND_EXPR, so the load is already guarded. Verified at run time with
an absent optional class scalar passed to an assumed-rank dummy.
More information about the Fortran
mailing list