[Patch, Fortran] libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

[Patch, Fortran] libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

This is a 12/13 regression as come changes to fix the GFC/CFI descriptor
that went into GCC 12 fail with the (bogus) descriptor passed via by a
GCC-11-compiled program.

As later GCC 12 changes moved the descriptor to the front end, those
functions are only in libgomp.so to cater for old program. Richard
suggested in the PR that the best way is to move to the GCC 11 version,
such that libgfortran.so won't regress.

I now did so - except for three fixes (cf. changelog). See also
PR: https://gcc.gnu.org/PR108056

There is no testcase as it needs to be compiled by GCC <= 11 and then
run with linking (dynamically) to a GCC 12 or 13 libgfortran.

OK for mainline and GCC 12?

  * * *

Note: It is strongly recommended to use GCC 12 (or 13) with array-descriptor
C interop as many issues were fixed. Like for the testcase in the PR; in GCC 11
the type arriving in libgomp is BT_ASSUME ('type(*)'). But as the effective
argument is passed as array descriptor through out, the 'float' (real(4)) type
info is actually preservable (as GCC 12 cf. testcase of comment 0 and my comment
in the PR for the C part of the testcase).(*)

Tobias

((*) This is not possible if using a scalar 'type(*)' or a non-array-descriptor
array in between. I think GCC 12 uses 'CFI_other' in the information-is-lost case.)
-----------------
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

[-- Attachment #2: libgfortran-ISO-C-gcc11.diff --]
[-- Type: text/x-patch, Size: 7574 bytes --]

libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

Since GCC 12, the conversion between the array descriptors formats - the
internal (GFC) and the C binding one (CFI) moved to the compiler itself
such that the cfi_desc_to_gfc_desc/gfc_desc_to_cfi_desc functions are only
used with older code (GCC 9 to 11).  The newly added checks caused asserts
as older code did not pass the proper values (e.g. real(4) as effective
argument arrived as BT_ASSUME type as the effective type got lost inbetween).

As proposed in the PR, revert to the GCC 11 version - known bugs is better
than some fixes and new issues. Still, GCC 12 is much better in terms of
TS29113 support and should really be used.

This patch uses the current libgomp version of the GCC 11 branch, except
it fixes the GFC version number (which is 0), uses calloc instead of malloc,
and sets the lower bound to 1 instead of keeping it as is for
CFI_attribute_other.

libgfortran/ChangeLog:

	PR libfortran/108056
	* runtime/ISO_Fortran_binding.c (cfi_desc_to_gfc_desc,
	gfc_desc_to_cfi_desc): Mostly revert to GCC 11 version for
	those backward-compatiblity-only functions.

diff --git a/libgfortran/runtime/ISO_Fortran_binding.c b/libgfortran/runtime/ISO_Fortran_binding.c
index 342df4275b9..e63a717a69b 100644
--- a/libgfortran/runtime/ISO_Fortran_binding.c
+++ b/libgfortran/runtime/ISO_Fortran_binding.c
@@ -39,60 +39,31 @@ 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;
-  size_t size;
   int n;
+  index_type kind;
   CFI_cdesc_t *s = *s_ptr;
 
   if (!s)
     return;
 
-  /* Verify descriptor.  */
-  switch (s->attribute)
-    {
-    case CFI_attribute_pointer:
-    case CFI_attribute_allocatable:
-      break;
-    case CFI_attribute_other:
-      if (s->base_addr)
-	break;
-      runtime_error ("Nonallocatable, nonpointer actual argument to BIND(C) "
-		     "dummy argument where the effective argument is either "
-		     "not allocated or not associated");
-      break;
-    default:
-      runtime_error ("Invalid attribute type %d in CFI_cdesc_t descriptor",
-		     (int) s->attribute);
-      break;
-    }
   GFC_DESCRIPTOR_DATA (d) = s->base_addr;
+  GFC_DESCRIPTOR_TYPE (d) = (signed char)(s->type & CFI_type_mask);
+  kind = (index_type)((s->type - (s->type & CFI_type_mask)) >> CFI_type_kind_shift);
 
   /* Correct the unfortunate difference in order with types.  */
-  type = (signed char)(s->type & CFI_type_mask);
-  switch (type)
-    {
-    case CFI_type_Character:
-      type = BT_CHARACTER;
-      break;
-    case CFI_type_struct:
-      type = BT_DERIVED;
-      break;
-    case CFI_type_cptr:
-      /* FIXME: PR 100915.  GFC descriptors do not distinguish between
-	 CFI_type_cptr and CFI_type_cfunptr.  */
-      type = BT_VOID;
-      break;
-    default:
-      break;
-    }
-
-  GFC_DESCRIPTOR_TYPE (d) = type;
-  GFC_DESCRIPTOR_SIZE (d) = s->elem_len;
+  if (GFC_DESCRIPTOR_TYPE (d) == BT_CHARACTER)
+    GFC_DESCRIPTOR_TYPE (d) = BT_DERIVED;
+  else if (GFC_DESCRIPTOR_TYPE (d) == BT_DERIVED)
+    GFC_DESCRIPTOR_TYPE (d) = BT_CHARACTER;
+
+  if (!s->rank || s->dim[0].sm == (CFI_index_t)s->elem_len)
+    GFC_DESCRIPTOR_SIZE (d) = s->elem_len;
+  else if (GFC_DESCRIPTOR_TYPE (d) != BT_DERIVED)
+    GFC_DESCRIPTOR_SIZE (d) = kind;
+  else
+    GFC_DESCRIPTOR_SIZE (d) = s->elem_len;
 
   d->dtype.version = 0;
-
-  if (s->rank < 0 || s->rank > CFI_MAX_RANK)
-    internal_error (NULL, "Invalid rank in descriptor");
   GFC_DESCRIPTOR_RANK (d) = (signed char)s->rank;
 
   d->dtype.attribute = (signed short)s->attribute;
@@ -131,7 +102,6 @@ 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
@@ -142,99 +112,22 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_ptr, const gfc_array_void *s)
   else
     d = *d_ptr;
 
-  /* Verify descriptor.  */
-  switch (s->dtype.attribute)
-    {
-    case CFI_attribute_pointer:
-    case CFI_attribute_allocatable:
-      break;
-    case CFI_attribute_other:
-      if (s->base_addr)
-	break;
-      runtime_error ("Nonallocatable, nonpointer actual argument to BIND(C) "
-		     "dummy argument where the effective argument is either "
-		     "not allocated or not associated");
-      break;
-    default:
-      internal_error (NULL, "Invalid attribute in gfc_array descriptor");
-      break;
-    }
   d->base_addr = GFC_DESCRIPTOR_DATA (s);
   d->elem_len = GFC_DESCRIPTOR_SIZE (s);
-  if (d->elem_len <= 0)
-    internal_error (NULL, "Invalid size in descriptor");
-
   d->version = CFI_VERSION;
-
   d->rank = (CFI_rank_t)GFC_DESCRIPTOR_RANK (s);
-  if (d->rank < 0 || d->rank > CFI_MAX_RANK)
-    internal_error (NULL, "Invalid rank in descriptor");
-
   d->attribute = (CFI_attribute_t)s->dtype.attribute;
 
-  type = GFC_DESCRIPTOR_TYPE (s);
-  switch (type)
-    {
-    case BT_CHARACTER:
-      d->type = CFI_type_Character;
-      break;
-    case BT_DERIVED:
-      d->type = CFI_type_struct;
-      break;
-    case BT_VOID:
-      /* FIXME: PR 100915.  GFC descriptors do not distinguish between
-	 CFI_type_cptr and CFI_type_cfunptr.  */
-      d->type = CFI_type_cptr;
-      break;
-    default:
-      d->type = (CFI_type_t)type;
-      break;
-    }
-
-  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:
-      /* FIXME: we can't distinguish between kind/len because
-	 the GFC descriptor only encodes the elem_len..
-	 Until PR92482 is fixed, assume elem_len refers to the
-	 character size and not the string length.  */
-      kind = (signed char)d->elem_len;
-      break;
-    case CFI_type_struct:
-    case CFI_type_cptr:
-    case CFI_type_other:
-      /* FIXME: PR 100915.  GFC descriptors do not distinguish between
-	 CFI_type_cptr and CFI_type_cfunptr.  */
-      kind = 0;
-      break;
-    default:
-      internal_error (NULL, "Invalid type in descriptor");
-    }
-
-  if (kind < 0)
-    internal_error (NULL, "Invalid kind in descriptor");
-
-  /* FIXME: This is PR100917.  Because the GFC descriptor encodes only the
-     elem_len and not the kind, we get into trouble with long double kinds
-     that do not correspond directly to the elem_len, specifically the
-     kind 10 80-bit long double on x86 targets.  On x86_64, this has size
-     16 and cannot be differentiated from true _Float128.  Prefer the
-     standard long double type over the GNU extension in that case.  */
-  if (d->type == CFI_type_Real && kind == sizeof (long double))
-    d->type = CFI_type_long_double;
-  else if (d->type == CFI_type_Complex && kind == sizeof (long double))
-    d->type = CFI_type_long_double_Complex;
+  if (GFC_DESCRIPTOR_TYPE (s) == BT_CHARACTER)
+    d->type = CFI_type_Character;
+  else if (GFC_DESCRIPTOR_TYPE (s) == BT_DERIVED)
+    d->type = CFI_type_struct;
   else
+    d->type = (CFI_type_t)GFC_DESCRIPTOR_TYPE (s);
+
+  if (GFC_DESCRIPTOR_TYPE (s) != BT_DERIVED)
     d->type = (CFI_type_t)(d->type
-			   + ((CFI_type_t)kind << CFI_type_kind_shift));
+		+ ((CFI_type_t)d->elem_len << CFI_type_kind_shift));
 
   if (d->base_addr)
     /* Full pointer or allocatable arrays retain their lower_bounds.  */

Re: [Patch, Fortran] libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

[Harald Anlauf]

Hi Tobias,

Am 13.12.22 um 17:29 schrieb Tobias Burnus:
> This is a 12/13 regression as come changes to fix the GFC/CFI descriptor
> that went into GCC 12 fail with the (bogus) descriptor passed via by a
> GCC-11-compiled program.
>
> As later GCC 12 changes moved the descriptor to the front end, those
> functions are only in libgomp.so to cater for old program. Richard
> suggested in the PR that the best way is to move to the GCC 11 version,
> such that libgfortran.so won't regress.

that appears to be the most reasonable & practical way to go.

> I now did so - except for three fixes (cf. changelog). See also
> PR: https://gcc.gnu.org/PR108056
>
> There is no testcase as it needs to be compiled by GCC <= 11 and then
> run with linking (dynamically) to a GCC 12 or 13 libgfortran.

I've verified that the testcase in the PR does not crash with
the re-modified libgfortran.

I've looked at the resulting ISO_Fortran_binding.c vs. the 11-branch
version and am still trying to understand the resulting differences
in the code, in what respect they might be relevant or not.

Given that this is a somewhat delicate situation we're in, is there
a set of tests that I could run *manually* (i.e. compile with gcc-11
and link with gcc-12/13) to verify that this best-effort fix should
be good enough for the common user?

Just a suggestion of a few "randomly" chosen tests?

> OK for mainline and GCC 12?
>
>   * * *
>
> Note: It is strongly recommended to use GCC 12 (or 13) with
> array-descriptor
> C interop as many issues were fixed. Like for the testcase in the PR; in
> GCC 11
> the type arriving in libgomp is BT_ASSUME ('type(*)'). But as the effective
> argument is passed as array descriptor through out, the 'float'
> (real(4)) type
> info is actually preservable (as GCC 12 cf. testcase of comment 0 and my
> comment
> in the PR for the C part of the testcase).(*)

Well, in the real world there are larger installations with large
software stacks, and it is easier said to "compile each component
with the same compiler version" than done...

(I've just had another personal experience during my daytime job.)

Thanks for your effort so far!

Harald

> Tobias
>
> ((*) This is not possible if using a scalar 'type(*)' or a
> non-array-descriptor
> array in between. I think GCC 12 uses 'CFI_other' in the
> information-is-lost case.)
> -----------------
> 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

Re: [Patch, Fortran] libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

[Tobias Burnus]

Hi Harald,

On 13.12.22 21:53, Harald Anlauf via Gcc-patches wrote:

>> I now did so - except for three fixes (cf. changelog). See also
>> PR: https://gcc.gnu.org/PR108056
>>
>> There is no testcase as it needs to be compiled by GCC <= 11 and then
>> run with linking (dynamically) to a GCC 12 or 13 libgfortran.
>
> I've looked at the resulting ISO_Fortran_binding.c vs. the 11-branch
> version and am still trying to understand the resulting differences
> in the code, in what respect they might be relevant or not.

Hmm, if I run a diff, I do not see much differences.

Note: We only talk about those two functions, the other functions are used
by both GCC <= 11 and GCC >= 12.

Fortunately, these functions matter most as they map GFC internals to CFI
internals or vice versa. Most other functions are user callable and there
incompatibilities are less likely to show up and GCC 11 users also could
profit from fixes there. It looks as if CFI_section and CFI_select_part had
some larger changes, likewise CFI_setpointer.

Back to differences: 'diff -U0 -p -w' against the last GCC 11 branch shows:

...
@@ -35,0 +37,2 @@ export_proto(cfi_desc_to_gfc_desc);
+/* NOTE: Since GCC 12, the FE generates code to do the conversion
+   directly without calling this function.  */
@@ -63 +66 @@ cfi_desc_to_gfc_desc (gfc_array_void *d,
-  d->dtype.version = s->version;
+  d->dtype.version = 0;
@@ -76,0 +80 @@ cfi_desc_to_gfc_desc (gfc_array_void *d,
+  if (GFC_DESCRIPTOR_DATA (d))
@@ -79,3 +83,7 @@ cfi_desc_to_gfc_desc (gfc_array_void *d,
-      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);
+       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);
@@ -89,0 +98,2 @@ export_proto(gfc_desc_to_cfi_desc);
+/* NOTE: Since GCC 12, the FE generates code to do the conversion
+   directly without calling this function.  */
@@ -100,2 +110,2 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_pt
-    d = malloc (sizeof (CFI_cdesc_t)
-               + (CFI_type_t)(CFI_MAX_RANK * sizeof (CFI_dim_t)));
+    d = calloc (1, (sizeof (CFI_cdesc_t)
+                   + (CFI_type_t)(CFI_MAX_RANK * sizeof (CFI_dim_t))));
@@ -107 +117 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_pt
-  d->version = s->dtype.version;
+  d->version = CFI_VERSION;
@@ -153 +163 @@ void *CFI_address (const CFI_cdesc_t *dv
...

> Given that this is a somewhat delicate situation we're in, is there
> a set of tests that I could run *manually* (i.e. compile with gcc-11
> and link with gcc-12/13) to verify that this best-effort fix should
> be good enough for the common user?
>
> Just a suggestion of a few "randomly" chosen tests?

Probably yes. I don't have a better suggestion. The problem is that it
usually only matters in some corner cases, like in the PR where a not
some argument is passed to the GFC→CFI conversion but first a Fortran
function is called with TYPE(*) any only then it is passed on. – Such
cases are usually not in the testsuite. (With GCC 12 we have a rather
complex testsuite, but obviously it also does not cover everything.)


>> Note: It is strongly recommended to use GCC 12 (or 13) with
>> array-descriptor
>> C interop as many issues were fixed. [...]
>
> Well, in the real world there are larger installations with large
> software stacks, and it is easier said to "compile each component
> with the same compiler version" than done...

I concur – but there were really many fixes for the array descriptor /
TS29113 in GCC 12.

It is simply not possible to fix tons of bugs and be 100% compatible
with the working bits of the old version – especially if they only work
if one does not look sharply at the result. (Like here, were 'type' is
wrong, which does not matter unless in programs which use them.)

Thanks,

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

Re: [Patch, Fortran] libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

[Harald Anlauf]

Hi Tobias,

Am 13.12.22 um 22:41 schrieb Tobias Burnus:
> Note: We only talk about those two functions, the other functions are used
> by both GCC <= 11 and GCC >= 12.

yes.

> Fortunately, these functions matter most as they map GFC internals to CFI
> internals or vice versa. Most other functions are user callable and there
> incompatibilities are less likely to show up and GCC 11 users also could
> profit from fixes there. It looks as if CFI_section and CFI_select_part had
> some larger changes, likewise CFI_setpointer.
>
> Back to differences: 'diff -U0 -p -w' against the last GCC 11 branch shows:
>
> ...
> @@ -35,0 +37,2 @@ export_proto(cfi_desc_to_gfc_desc);
> +/* NOTE: Since GCC 12, the FE generates code to do the conversion
> +   directly without calling this function.  */
> @@ -63 +66 @@ cfi_desc_to_gfc_desc (gfc_array_void *d,
> -  d->dtype.version = s->version;
> +  d->dtype.version = 0;

I was wondering what the significance of "version" is.
In ISO_Fortran_binding.h we seem to always have

#define CFI_VERSION 1

and it did not change with gcc-12.

If it is just decoration (for now), then it is not important.
I just didn't see where it is used.

> @@ -76,0 +80 @@ cfi_desc_to_gfc_desc (gfc_array_void *d,
> +  if (GFC_DESCRIPTOR_DATA (d))
> @@ -79,3 +83,7 @@ cfi_desc_to_gfc_desc (gfc_array_void *d,
> -      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);
> +       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);

Oh, now I see that on 11-branch in trans-expr.c we set a hard-coded
attribute = 2 instead of using CFI_attribute_other, even if that was
available as a macro defining the very same value.  Thus it is ok.

> @@ -89,0 +98,2 @@ export_proto(gfc_desc_to_cfi_desc);
> +/* NOTE: Since GCC 12, the FE generates code to do the conversion
> +   directly without calling this function.  */
> @@ -100,2 +110,2 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_pt
> -    d = malloc (sizeof (CFI_cdesc_t)
> -               + (CFI_type_t)(CFI_MAX_RANK * sizeof (CFI_dim_t)));
> +    d = calloc (1, (sizeof (CFI_cdesc_t)
> +                   + (CFI_type_t)(CFI_MAX_RANK * sizeof (CFI_dim_t))));
> @@ -107 +117 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_pt
> -  d->version = s->dtype.version;
> +  d->version = CFI_VERSION;

This treatment of "version" was the equivalent to the above that
confused me.  Assuming we were to change CFI_VERSION in gcc-13+,
is this the right choice here regarding backward compatibility?

> Probably yes. I don't have a better suggestion. The problem is that it
> usually only matters in some corner cases, like in the PR where a not
> some argument is passed to the GFC→CFI conversion but first a Fortran
> function is called with TYPE(*) any only then it is passed on. – Such
> cases are usually not in the testsuite. (With GCC 12 we have a rather
> complex testsuite, but obviously it also does not cover everything.)

Well, I understand we have to draw a line here, whether we
reproduce every bug in <= gcc-11 where the user may have
attempted to implement a workaround.  That might be tough.

>> Well, in the real world there are larger installations with large
>> software stacks, and it is easier said to "compile each component
>> with the same compiler version" than done...
>
> I concur – but there were really many fixes for the array descriptor /
> TS29113 in GCC 12.
>
> It is simply not possible to fix tons of bugs and be 100% compatible
> with the working bits of the old version – especially if they only work
> if one does not look sharply at the result. (Like here, were 'type' is
> wrong, which does not matter unless in programs which use them.)

True.  I was only thinking of the 90+ percent of valid and common
uses that we really consider important.

So besides the "version" question ok from my side.

Thanks,
Harald

> Thanks,
>
> 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
>

Re: [Patch, Fortran] libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

[Tobias Burnus]

Hi Harald,

On 13.12.22 23:27, Harald Anlauf wrote:
> Am 13.12.22 um 22:41 schrieb Tobias Burnus:
>> Back to differences: 'diff -U0 -p -w' against the last GCC 11 branch
>> shows:
>>
>> ...
>> @@ -35,0 +37,2 @@ export_proto(cfi_desc_to_gfc_desc);
>> +/* NOTE: Since GCC 12, the FE generates code to do the conversion
>> +   directly without calling this function.  */
>> @@ -63 +66 @@ cfi_desc_to_gfc_desc (gfc_array_void *d,
>> -  d->dtype.version = s->version;
>> +  d->dtype.version = 0;
>
> I was wondering what the significance of "version" is.
> In ISO_Fortran_binding.h we seem to always have
>    #define CFI_VERSION 1
> and it did not change with gcc-12.

The version is 1 for CFI but it is 0 for GFC. However, as we do not
check the GFC version anywhere and it is not publicly exposed, it does
not really matter. Still, "d->dtype.version = 0;" matches what the
compiler itself produces – and for consistency, setting it to 0 is
better than setting it to 1 (via CFI's version field).

Actually 'dtype.version' is not really set anywhere; at least
gfc_get_dtype_rank_type(...) does not set it; zero initialization is
most common but it could be also some random value. In libgfortran,
GFC_DTYPE_CLEAR explicitly sets it to 0.
>> @@ -100,2 +110,2 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_pt
>> -    d = malloc (sizeof (CFI_cdesc_t)
>> -               + (CFI_type_t)(CFI_MAX_RANK * sizeof (CFI_dim_t)));
>> +    d = calloc (1, (sizeof (CFI_cdesc_t)
>> +                   + (CFI_type_t)(CFI_MAX_RANK * sizeof (CFI_dim_t))));
>> @@ -107 +117 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_pt
>> -  d->version = s->dtype.version;
>> +  d->version = CFI_VERSION;
>
> This treatment of "version" was the equivalent to the above that
> confused me.  Assuming we were to change CFI_VERSION in gcc-13+,
> is this the right choice here regarding backward compatibility?

I don't think we will change CFI version any time soon as we rather
closely follow the Fortran standard and I do not see any changes which
are required there.

NOTE: As s->dtype.version is either 0 or some random value, setting
version in the CFI / ISO C descriptor to 1, be it as literal or as macro
constant, makes it the same as CFI_VERSION.

And: I don't think we will change CFI_VERSION or the structure of the
CFI array descriptor any time soon; there does not seem to be any need
for it, it matches the Fortran standard one well (and no plans seem to
be planed on that side) and, finally, changing an array descriptor is
painful!

However, using '1;  /* CFI_VERSION in GCC 11 and at time of writing. */'
would also work – but I would expect that we will go through all CFI
users if we ever change the descriptor (and bump the version), possibly
adding version-number dependent code.

> So besides the "version" question ok from my side.

I hope I could answer the latter.

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

Re: [Patch, Fortran] libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

[Harald Anlauf]

Hi Tobias,

thanks for your explanation!  Now things are clearer.

Am 14.12.22 um 08:57 schrieb Tobias Burnus:
>>> @@ -63 +66 @@ cfi_desc_to_gfc_desc (gfc_array_void *d,
>>> -  d->dtype.version = s->version;
>>> +  d->dtype.version = 0;
>>
>> I was wondering what the significance of "version" is.
>> In ISO_Fortran_binding.h we seem to always have
>>    #define CFI_VERSION 1
>> and it did not change with gcc-12.
>
> The version is 1 for CFI but it is 0 for GFC. However, as we do not
> check the GFC version anywhere and it is not publicly exposed, it does
> not really matter. Still, "d->dtype.version = 0;" matches what the
> compiler itself produces – and for consistency, setting it to 0 is
> better than setting it to 1 (via CFI's version field).
>
> Actually 'dtype.version' is not really set anywhere; at least
> gfc_get_dtype_rank_type(...) does not set it; zero initialization is
> most common but it could be also some random value. In libgfortran,
> GFC_DTYPE_CLEAR explicitly sets it to 0.

OK, that was not easy to find.

>>> @@ -107 +117 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_pt
>>> -  d->version = s->dtype.version;
>>> +  d->version = CFI_VERSION;
>>
>> This treatment of "version" was the equivalent to the above that
>> confused me.  Assuming we were to change CFI_VERSION in gcc-13+,
>> is this the right choice here regarding backward compatibility?
>
> I don't think we will change CFI version any time soon as we rather
> closely follow the Fortran standard and I do not see any changes which
> are required there.
>
> NOTE: As s->dtype.version is either 0 or some random value, setting
> version in the CFI / ISO C descriptor to 1, be it as literal or as macro
> constant, makes it the same as CFI_VERSION.

OK

> And: I don't think we will change CFI_VERSION or the structure of the
> CFI array descriptor any time soon; there does not seem to be any need
> for it, it matches the Fortran standard one well (and no plans seem to
> be planed on that side) and, finally, changing an array descriptor is
> painful!

Agreed.

> However, using '1;  /* CFI_VERSION in GCC 11 and at time of writing. */'
> would also work – but I would expect that we will go through all CFI
> users if we ever change the descriptor (and bump the version), possibly
> adding version-number dependent code.

Agreed.  Searching for a string that can be guessed is
easier that looking for a magic '1'.

>> So besides the "version" question ok from my side.
>
> I hope I could answer the latter.

Yes, and thanks for the patch!

Harald

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

Re: [Patch, Fortran] libgfortran's ISO_Fortran_binding.c: Use GCC11 version for backward-only code [PR108056]

[Richard Biener]

On Tue, Dec 13, 2022 at 5:29 PM Tobias Burnus <tobias@codesourcery.com> wrote:
>
> This is a 12/13 regression as come changes to fix the GFC/CFI descriptor
> that went into GCC 12 fail with the (bogus) descriptor passed via by a
> GCC-11-compiled program.
>
> As later GCC 12 changes moved the descriptor to the front end, those
> functions are only in libgomp.so to cater for old program. Richard
> suggested in the PR that the best way is to move to the GCC 11 version,
> such that libgfortran.so won't regress.
>
> I now did so - except for three fixes (cf. changelog). See also
> PR: https://gcc.gnu.org/PR108056
>
> There is no testcase as it needs to be compiled by GCC <= 11 and then
> run with linking (dynamically) to a GCC 12 or 13 libgfortran.
>
> OK for mainline and GCC 12?

OK for the branch if approved for trunk.

Richard.

>   * * *
>
> Note: It is strongly recommended to use GCC 12 (or 13) with array-descriptor
> C interop as many issues were fixed. Like for the testcase in the PR; in GCC 11
> the type arriving in libgomp is BT_ASSUME ('type(*)'). But as the effective
> argument is passed as array descriptor through out, the 'float' (real(4)) type
> info is actually preservable (as GCC 12 cf. testcase of comment 0 and my comment
> in the PR for the C part of the testcase).(*)
>
> Tobias
>
> ((*) This is not possible if using a scalar 'type(*)' or a non-array-descriptor
> array in between. I think GCC 12 uses 'CFI_other' in the information-is-lost case.)
> -----------------
> 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