As I state at the bottom of all this, Part 6 is approved when you are ready.
See further below, all good.
On 9/14/26 4:47 AM, Mikael Morin wrote:
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.
--- snip ---
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:
It was not clear to me if the comment applied to the if clause or explaining why
fall through.
case BT_CLASS:
/* For classes, ... */ ?????
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.
I completely get it. Its complicated.
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.
OK
- 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.
No need for another go around on this.>>
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
OK
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.
All good, by the way, I have no comments on part 6 so consider it good to go
when you are ready.
Cheers,
Jerry
