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

[Sandra Loosemore <sandra@codesourcery.com>]

On 7/21/21 11:26 AM, Tobias Burnus wrote:
> 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.

Yeah, I could fix this.  I'd initially forgotten that elem_len was an 
unsigned type and was trying to test it by passing a negative value.  :-P

> 
> (Appears twice)
> 
> However, the check itself is also wrong – cf. below.

Hmmm.  CFI_establish explicitly says that the elem_len has to be greater 
than zero.  It seems somewhat confusing that it's inconsistent with the 
other functions that take an elem_len argument.

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

Ha!  I noticed the same thing and already posted a separate patch for 
that.  :-P

https://gcc.gnu.org/pipermail/fortran/2021-July/056243.html

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

Hmmm, there are similar problems in existing code in other functions in 
this file (e.g., CFI_section).

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

The wording of the message reflects the language of the standard:
"source shall be a null pointer or the address of a C descriptor for an 
allocated allocatable object, a data pointer object, or a nonallocatable 
nonpointer data object that is not an assumed-size array.

-Sandra