Le 12/09/2026 à 21:40, Jerry D a écrit :
On 9/3/26 12:27 PM, Mikael Morin wrote:
From: Mikael Morin <[email protected]>
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-
[email protected]/
https://inbox.sourceware.org/fortran/20260829150020.29239-1-morin-
[email protected]/
https://patchwork.sourceware.org/project/gcc/
cover/[email protected]/
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 review. Some answers below. I'll post a v2 patch set in
any case addressing the different review items, after the part 6 patches
are reviewed and pushed.
Thanks for the much needed work.
Regards,
Jerry
PR122521-part7-review.txt
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?
Well, I mention the scalar case because that was my main focus, it was
where the problem was found, and from a quick look arrays don't have the
same problem. But as you note the function has a wide scope, and I
can't completely exclude some array cases being impacted by the change.
The change should be equally safe for arrays though. The function
get_class_declared_type supports array descriptors as _data, so arrays
should be fine, and the rest depends on the result of that function and
is otherwise rank-agnostic.
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.
Ok, I can add a note.
To be complete on the risk of regression between patch 1 and 9, note
that I'm pretty sure that some cases are broken by this patch, that is
cases where using the class descriptor size happens to give the right value.
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?
OK, will do.
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.
Do you mean like this?
/* For classes, ... */
case BT_CLASS:
if (VOID_TYPE_P (etype))
break;
/* Fall through. */
default:
== 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.
The function is only used from gfc_conv_class_to_class, so I'm pretty
sure the input is a polymorphic reference. But admittedly there is no
strong guarantee.
If the invariant does hold after patch 3,
gcc_checking_assert states it without aborting release compilers.
OK, I can do that.
Also class_ref is left uninitialized on the false path, which some
configurations will flag as -Wmaybe-uninitialized.
Ok, will fix.
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.
There is indeed ref walking that is redundant, to avoid creating a new
data reference when the input already has what's needed. I'll see what
can be done; the function has to accept several input patterns, which
doesn't help. Just adding the data to class_ref won't work for assumed
rank scalar inputs, having pattern class->_data.data and for which the
tmp should be the same class->_data.data.
== 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.
Yes, the plan is to remove the span check to fix PR127233, and then
associated.c no longer needs any change.
You've had conflict issues with the part 5 series, so I've decided to
post patches that were sitting on the previous series strictly as
posted, not on a newer rebased variant. Of course I can't both do that
and integrate changes from trunk at the same time.
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.
stride_in_bytes is only used in a loop guarded by rank, so I don't think
it matters.
- 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.
OK, that one is possibly bad, but note that a 0 value isn't much better.
That code looks dubious in any case. It doesn't seem to support span !=
elem_len.
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'm not that sure there is such a convention about 0 span; is it
documented anywhere?. The caf/single.c snippet you quote above doesn't
have it at least.
Either the span is really useless and keeping it undefined is fine, or
it is used and it should to be set to the right value. I would rather
drop the patch than set the span to some wrong value.
Anyway, there seems to be some controversy about this, so maybe it's
best to submit the patch separatedly from the rest. Nothing depends on it.
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 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?
Not really. In fact this patch didn't exist initially. But patch 1 was
so obviously insufficient that I had to add this one.
I have created a new PR:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=127384
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.
Thanks for checking.
What I wondered myself after writing the patch is whether I should guard
with a check on the _vptr or on the _data. The patch wasn't regressing
so I kept the _vptr check without looking any further.
