Re: [PATCH 2/2] libgfortran: Remove empty array descriptor first dimension overwrite [PR112371]

[Harald Anlauf]

Hi Mikael,

Am 06.11.23 um 12:43 schrieb Mikael Morin:

Remove the forced overwrite of the first dimension of the result array
descriptor to set it to zero extent, in the function templates for
transformational functions doing an array reduction along a dimension.  This
overwrite, which happened before early returning in case the result array
was empty, was wrong because an array may have a non-zero extent in the
first dimension and still be empty if it has a zero extent in a higher
dimension.  Overwriting the dimension was resulting in wrong array result
upper bound for the first dimension in that case.

The offending piece of code was present in several places, and this removes
them all.  More precisely, there is only one case to fix for logical
reduction functions, and there are three cases for other reduction
functions, corresponding to non-masked reduction, reduction with array mask,
and reduction with scalar mask.  The impacted m4 files are
ifunction_logical.m4 for logical reduction functions, ifunction.m4 for
regular functions and types, ifunction-s.m4 for character minloc and maxloc,
ifunction-s2.m4 for character minval and maxval, and ifindloc1.m4 for
findloc.

while your fix seems mechanical and correct, I wonder if you looked
at the following pre-existing irregularity which can be seen in
this snippet:

diff --git a/libgfortran/m4/ifunction.m4 b/libgfortran/m4/ifunction.m4
index 480649cf691..abc15b430ab 100644
--- a/libgfortran/m4/ifunction.m4
+++ b/libgfortran/m4/ifunction.m4
@@ -96,12 +96,7 @@ name`'rtype_qual`_'atype_code` ('rtype` * const restrict 
retarray,

        retarray->base_addr = xmallocarray (alloc_size, sizeof (rtype_name));
        if (alloc_size == 0)
-       {
-         /* Make sure we have a zero-sized array.  */
-         GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1);
-         return;
-
-       }
+       return;
      }
    else
      {

This is all enclosed in a block which has
  if (retarray->base_addr == NULL)
but allocates and sets retarray->base_addr, while

@@ -290,11 +285,7 @@ m'name`'rtype_qual`_'atype_code` ('rtype` * const restrict 
retarray,
        retarray->dtype.rank = rank;

        if (alloc_size == 0)
-       {
-         /* Make sure we have a zero-sized array.  */
-         GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1);
-         return;
-       }
+       return;
        else
        retarray->base_addr = xmallocarray (alloc_size, sizeof (rtype_name));

and

@@ -454,11 +445,7 @@ void
        alloc_size = GFC_DESCRIPTOR_STRIDE(retarray,rank-1) * extent[rank-1];

        if (alloc_size == 0)
-       {
-         /* Make sure we have a zero-sized array.  */
-         GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1);
-         return;
-       }
+       return;
        else
        retarray->base_addr = xmallocarray (alloc_size, sizeof (rtype_name));
      }

do not set retarray->base_addr to non-NULL for alloc_size == 0.

Do you know if the first snippet can be safely rewritten to avoid
the (hopefully pointless) xmallocarray for alloc_size == 0?

Thanks,
Harald