[PATCH 0/9] fortran: array descriptor part 7: Scalar descriptor init tweaks [PR122521]

[Jerry D]

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 <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.

--- 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