Re: [PATCH, Fortran] [PR libfortran/101317] Bind(c): Improve error checking in CFI_* functions

[Tobias Burnus <tobias@codesourcery.com>]

On 17.07.21 02:49, Sandra Loosemore wrote:

> This patch is for PR101317, one of the bugs uncovered by the TS29113
> testsuite.  Here I'd observed that CFI_establish, etc was not
> diagnosing some invalid-argument situations documented in the
> standard, although it was properly catching others.  After fixing
> those I discovered a couple small mistakes in the test cases and fixed
> those too.

Some first comments – I think I have to read though the file
ISO_Fortran_binding.c itself and not only your patch.

> --- a/libgfortran/runtime/ISO_Fortran_binding.c
> +++ b/libgfortran/runtime/ISO_Fortran_binding.c
> @@ -232,7 +232,16 @@ CFI_allocate (CFI_cdesc_t *dv, const CFI_index_t lower_bounds[],
>     /* If the type is a Fortran character type, the descriptor's element
>        length is replaced by the elem_len argument. */
>     if (dv->type == CFI_type_char || dv->type == CFI_type_ucs4_char)
> -    dv->elem_len = elem_len;
> +    {
> +      if (unlikely (compile_options.bounds_check) && elem_len == 0)
> +     {
> +       fprintf ("CFI_allocate: The supplied elem_len must be "
> +                "greater than zero (elem_len = %d).\n",
> +                (int) elem_len);

I think there is no need to use '(elem_len = %d)' given that it is always zero as stated in the error message itself.

(Appears twice)

However, the check itself is also wrong – cf. below.

  * * *

Talking about CFI_allocatable, there is also another bug in that function,
untouched by your patch:

  /* If the type is a character, the descriptor's element length is replaced
      by the elem_len argument. */
   if (dv->type == CFI_type_char || dv->type == CFI_type_ucs4_char ||
       dv->type == CFI_type_signed_char)
     dv->elem_len = elem_len;

The bug is that CFI_type_signed_char is not a character type.

> +  else if (unlikely (compile_options.bounds_check)
> +        && type < 0)
Pointless line break.
> +           fprintf (stderr, "CFI_establish: Extents must be nonnegative "
> +                    "(extents[%d] = %d).\n", i, (int)extents[i]);
> +           return CFI_INVALID_EXTENT;
> +         }

How about PRIiPTR + ptrdiff_t instead of %d + (int) cast? At least as
positive value, extent may exceed INT_MAX.

(Twice)

>     if (result->type == CFI_type_char || result->type == CFI_type_ucs4_char)
> -    result->elem_len = elem_len;
> +    {
> +      if (unlikely (compile_options.bounds_check) && elem_len == 0)
> +     {
> +       fprintf ("CFI_select_part: The supplied elem_len must be "
> +                "greater than zero (elem_len = %d).\n",
> +                (int) elem_len);

What's wrong with  ["", ""]? Or with:
   character(len=:), allocatable :: str2(:)
   str2 = [str1(5:4)]
both are len(...) == 0 arrays with 1 or 2 elements.

> +       if (source->attribute == CFI_attribute_other
> +           && source->rank > 0
> +           && source->dim[source->rank - 1].extent == -1)
> +         {
> +           fprintf (stderr, "CFI_setpointer: The source is a "
> +                    "nonallocatable nonpointer object that is an "
> +                    "assumed-size array.\n");

I think you could just check for assumed rank – without
CFI_attribute_other in the 'if' and 'nonallocatable nonpointer' in the
error message. Only nonallocatable nonpointer variables can be of
assumed size (in Fortran); I think that makes the message simpler
(focusing on the issue) and if the C user passes an allocatable/pointer,
which is assumed rank, it is also a bug.

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955