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

[Harald Anlauf <anlauf@gmx.de>]

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