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

  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
  

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
  

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
  

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
>
  

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
  

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
  

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
>