Re: [Patch, fortran] PR fortran/100906/100907/100911/100914/100915/100916

[Sandra Loosemore <sandra@codesourcery.com>]

On 6/13/21 12:36 PM, José Rui Faustino de Sousa via Gcc-patches wrote:
> Hi All!
> 
> Proposed patch to:
> 
> Bug 100906 - Bind(c): failure handling character with len/=1
> Bug 100907 - Bind(c): failure handling wide character
> Bug 100911 - Bind(c): failure handling C_PTR
> Bug 100914 - Bind(c): errors handling complex
> Bug 100915 - Bind(c): failure handling C_FUNPTR
> Bug 100916 - Bind(c): CFI_type_other unimplemented

I've been playing a bit with this patch in conjunction with my TS29113 
testsuite.  It seems to help some things, but it causes a regression in 
library/section-1.f90 that I don't really understand.  :-S  I'm also 
still running up against the long double bug in PR100917; after doing 
some experiments on top of this patch, I suggested some possible 
solutions/workarounds in that issue.

> Patch tested only on x86_64-pc-linux-gnu.

In general, I think it is a good idea to test interoperability features 
on a 32-bit host as well.  I've been building i686-pc-linux-gnu and 
testing both -m32 and -m64 multilibs.  ISO_Fortran_binding.h is pretty 
broken on 32-bit host but I'll have a patch for that in the next day or so.

Anyway, I have a few comments on the libgfortran changes, mostly 
cosmetic things.  I'm not at all familiar with the workings of the 
Fortran front end code that produces descriptors, so I don't think I 
have anything useful to say about that part of the patch.

> diff --git a/libgfortran/runtime/ISO_Fortran_binding.c b/libgfortran/runtime/ISO_Fortran_binding.c
> index 20833ad..3a269d7 100644
> --- a/libgfortran/runtime/ISO_Fortran_binding.c
> +++ b/libgfortran/runtime/ISO_Fortran_binding.c
> @@ -36,31 +36,81 @@ export_proto(cfi_desc_to_gfc_desc);
>  void
>  cfi_desc_to_gfc_desc (gfc_array_void *d, CFI_cdesc_t **s_ptr)
>  {
> +  signed char type, kind;
> +  size_t size;
>    int n;
> -  index_type kind;
>    CFI_cdesc_t *s = *s_ptr;
>  
>    if (!s)
>      return;
>  
> +  /* Verify descriptor.  */
> +  switch(s->attribute)

GNU coding standards:  space before the open paren.

> +    {
> +    case CFI_attribute_pointer:
> +    case CFI_attribute_allocatable:
> +      break;
> +    case CFI_attribute_other:
> +      if (s->base_addr)
> +	break;
> +      /* FALL THROUGH */
> +    default:
> +      internal_error (NULL, "INVALID CFI DESCRIPTOR");

We could have a better message here that doesn't SHOUT.  Maybe "Invalid 
attribute in CFI descriptor"?
>  
> +  kind = _CFI_DECODE_KIND (s->type);
> +  switch(type)

Space before the paren again.

> +    {
> +    case BT_INTEGER:
> +    case BT_LOGICAL:
> +    case BT_REAL:
> +      size = (size_t)kind;
> +      break;
> +    case BT_COMPLEX:
> +      size = (size_t)(kind << 1);
> +      break;
> +    case BT_DERIVED:
> +    case BT_CHARACTER:
> +    case BT_VOID:
> +      size = s->elem_len;
> +      break;
> +    default:
> +      if (type != CFI_type_other)
> +	internal_error(NULL, "TYPE ERROR");

Space before the paren and better error message again.

Section 18.5.4 of the Fortran 2018 standard says:  "If a C type is not 
interoperable with a Fortran type and kind supported by the Fortran 
processor, its macro shall evaluate to a negative value."  And in fact 
I'll have a patch for PR101305 soon that will use a negative value 
distinct from CFI_type_other for that.  If you construct a descriptor 
for such an object in C and try passing it to Fortran, I think that's a 
user error, not an internal error.

> +  if (size <= 0)
> +    internal_error(NULL, "SIZE ERROR");

Same issues here with fixing formatting and better message.

> +  GFC_DESCRIPTOR_SIZE (d) = size;
> +  
>    d->dtype.version = s->version;
> +
> +  if ((s->rank < 0) || (s->rank > CFI_MAX_RANK))

You could delete the inner parentheses here.

> +    internal_error(NULL, "Rank out of range.");

Space before the paren again.  It also looks like other error messages 
in libgfortran don't add punctuation at the end.

There are several more instances of space-before-paren problems and 
fixing the errpr messages, I'm not going to point the rest of them out 
individually.

> @@ -74,14 +124,19 @@ cfi_desc_to_gfc_desc (gfc_array_void *d, CFI_cdesc_t **s_ptr)
>      }
>  
>    d->offset = 0;
> -  for (n = 0; n < GFC_DESCRIPTOR_RANK (d); n++)
> -    {
> -      GFC_DESCRIPTOR_LBOUND(d, n) = (index_type)s->dim[n].lower_bound;
> -      GFC_DESCRIPTOR_UBOUND(d, n) = (index_type)(s->dim[n].extent
> -						+ s->dim[n].lower_bound - 1);
> -      GFC_DESCRIPTOR_STRIDE(d, n) = (index_type)(s->dim[n].sm / s->elem_len);
> -      d->offset -= GFC_DESCRIPTOR_STRIDE(d, n) * GFC_DESCRIPTOR_LBOUND(d, n);
> -    }
> +  if (GFC_DESCRIPTOR_DATA (d))
> +    for (n = 0; n < GFC_DESCRIPTOR_RANK (d); n++)
> +      {
> +	CFI_index_t lb = 1;
> +	
> +	if (s->attribute != CFI_attribute_other)
> +	  lb = s->dim[n].lower_bound;
> +
> +	GFC_DESCRIPTOR_LBOUND(d, n) = (index_type)lb;
> +	GFC_DESCRIPTOR_UBOUND(d, n) = (index_type)(s->dim[n].extent + lb - 1);
> +	GFC_DESCRIPTOR_STRIDE(d, n) = (index_type)(s->dim[n].sm / s->elem_len);
> +	d->offset -= GFC_DESCRIPTOR_STRIDE(d, n) * GFC_DESCRIPTOR_LBOUND(d, n);
> +      }
>  }

You might as well fix the space-before-paren issues in the code block 
you reindented as well.

> @@ -92,32 +147,87 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_ptr, const gfc_array_void *s)
>  {
>    int n;
>    CFI_cdesc_t *d;
> +  signed char type, kind;
>  
>    /* Play it safe with allocation of the flexible array member 'dim'
>       by setting the length to CFI_MAX_RANK. This should not be necessary
>       but valgrind complains accesses after the allocated block.  */
>    if (*d_ptr == NULL)
> -    d = malloc (sizeof (CFI_cdesc_t)
> +    d = calloc (1, sizeof (CFI_cdesc_t)
>  		+ (CFI_type_t)(CFI_MAX_RANK * sizeof (CFI_dim_t)));

This change screwed up the indentation of that last line; the "+" should 
line up with "sizeof".  IIRC the GNU coding standards also recommend 
parenthesizing that binary expression so Emacs will indent it properly 
automatically.

> +  
>    d->rank = (CFI_rank_t)GFC_DESCRIPTOR_RANK (s);
> +  if ((d->rank < 0) || (d->rank > CFI_MAX_RANK))
> +    internal_error(NULL, "Rank out of range.");

Another too-many-parentheses instance.

> +  switch (d->type)
> +    {
> +    case CFI_type_Integer:
> +    case CFI_type_Logical:
> +    case CFI_type_Real:
> +      kind = (signed char)d->elem_len;
> +      break;
> +    case CFI_type_Complex:
> +      kind = (signed char)(d->elem_len >> 1);
> +      break;
> +    case CFI_type_Character:
> +      kind = 1;
> +      break;
> +    case CFI_type_struct:
> +    case CFI_type_cptr:
> +    case CFI_type_other:
> +      kind = 0;
> +      break;
> +    default:
> +      internal_error(NULL, "TYPE ERROR");
> +    }

case CFI_type_cfunptr:  ???

-Sandra