Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
Checks
Commit Message
Hi all,
The attached patch fixes a bug that causes valid OpenMP declare variant
directive and functions to be rejected with the following error (see
testcase):
c_ptr.f90:41:37:
41 | !$omp declare variant(foo_variant) &
| 1
Error: variant ‘foo_variant’ and base ‘foo’ at (1) have incompatible
types: Type mismatch in argument 'c_bv' (INTEGER(8)/TYPE(c_ptr))
The fix consists in special-casing this situation in gfc_compare_types().
OK for mainline?
Thanks,
Comments
Hi PA, hello all,
First, I hesitate to review/approve a patch I am involved in; Thus, I would like
if someone could have a second look.
Regarding the patch itself:
On 20.10.23 16:02, Paul-Antoine Arraswrote:
> Hi all,
>
> The attached patch fixes a bug that causes valid OpenMP declare
> variant directive
> and functions to be rejected with the following error (see testcase):
>
> [...]
> Error: variant ‘foo_variant’ and base ‘foo’ at (1) have incompatible
> types: Type mismatch in argument 'c_bv' (INTEGER(8)/TYPE(c_ptr))
>
> The fix consists in special-casing this situation in gfc_compare_types().
> OK for mainline?
...
> Subject: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and
> TYPE(c_ptr)
>
> In the context of an OpenMP declare variant directive, arguments of type C_PTR
> are sometimes recognised as C_PTR in the base function and as INTEGER(8) in the
> variant - or the other way around, depending on the parsing order.
> This patch prevents such situation from turning into a compile error.
>
> 2023-10-20 Paul-Antoine Arras<pa@codesourcery.com>
> Tobias Burnus<tobias@codesourcery.com>
>
> gcc/fortran/ChangeLog:
>
> * interface.cc (gfc_compare_types): Return true in this situation.
That's a bad description. It makes sense when reading the commit log but if you
only read gcc/fortran/ChangeLog, 'this situation' is a dangling reference.
> gcc/fortran/ChangeLog.omp | 5 ++
> gcc/testsuite/ChangeLog.omp | 4 ++
On mainline, the ChangeLog not ChangeLog.omp is used. This changelog is automatically
filled by the data in the commit log. Thus, no need to include it in the patch.
(Besides: It keeps getting outdated by any other commit to that file.)
As you have a commit, running the following, possibly with the commit hash as
argument (unless it is the last commit) will show that the nightly script will use:
./contrib/gcc-changelog/git_check_commit.py -v -p
It is additionally a good check whether you got the syntax right. (This is run
as pre-commit hook.)
* * *
Regarding the patch, I think it will work, but I wonder whether we can do
better - esp. regarding c_ptr vs. c_funptr.
I started by looking why the current code fails:
> index e9843e9549c..8bd35fd6d22 100644
> --- a/gcc/fortran/interface.cc
> +++ b/gcc/fortran/interface.cc
> @@ -705,12 +705,17 @@ gfc_compare_types (gfc_typespec *ts1, gfc_typespec *ts2)
> -
> - if (((ts1->type == BT_INTEGER && ts2->type == BT_DERIVED)
> - || (ts1->type == BT_DERIVED && ts2->type == BT_INTEGER))
> - && ts1->u.derived && ts2->u.derived
> - && ts1->u.derived == ts2->u.derived)
This does not work because the pointers to the derived type are different:
(gdb) p *ts1
$10 = {type = BT_INTEGER, kind = 8, u = {derived = 0x30c66b0, cl = 0x30c66b0, pad = 51144368}, interface = 0x0, is_c_interop = 1, is_iso_c = 0, f90_type = BT_VOID, deferred = false,
interop_kind = 0x0}
(gdb) p *ts2
$11 = {type = BT_DERIVED, kind = 0, u = {derived = 0x30c2930, cl = 0x30c2930, pad = 51128624}, interface = 0x0, is_c_interop = 0, is_iso_c = 0, f90_type = BT_UNKNOWN,
deferred = false, interop_kind = 0x0}
The reason seems to be that they are freshly created
in different namespaces. Consequently, attr.use_assoc = 1
and the namespace is different, i.e.
(gdb) p ts1->u.derived->ns->proc_name->name
$18 = 0x7ffff6ff4138 "foo"
(gdb) p ts2->u.derived->ns->proc_name->name
$19 = 0x7ffff6ffc260 "foo_variant"
* * *
Having said this, I think we can combine the current
and the modified version, i.e.
> + if ((ts1->type == BT_INTEGER && ts2->type == BT_DERIVED
> + && ts1->f90_type == BT_VOID
> + && ts2->u.derived->ts.is_iso_c
> + && ts2->u.derived->ts.u.derived->ts.f90_type == BT_VOID)
> + || (ts2->type == BT_INTEGER && ts1->type == BT_DERIVED
> + && ts2->f90_type == BT_VOID
> + && ts1->u.derived->ts.is_iso_c
> + && ts1->u.derived->ts.u.derived->ts.f90_type == BT_VOID))
See attached patch for a combined version, which checks now
whether from_intmod == INTMOD_ISO_C_BINDING and then compares
the names (to distinguish c_ptr and c_funptr). Those are unaffected
by 'use' renames, hence, we should be fine.
While in this example, the name pointers are identical, I fear that
won't hold in some more complex indirect use via module-use. Thus,
strcmp is used.
(gdb) p ts1->u.derived->name
$13 = 0x7ffff6ff4100 "c_ptr"
(gdb) p ts2->u.derived->name
$14 = 0x7ffff6ff4100 "c_ptr"
* * *
Additionally, I think it would be good to have a testcase which checks for
c_funptr vs. c_ptr
mismatch.
Just changing c_ptr to c_funptr in the testcase (+ commenting the c_f_pointer)
prints:
Error: variant ‘foo_variant’ and base ‘foo’ at (1) have incompatible types: Type mismatch in argument 'c_bv' (INTEGER(8)/TYPE(c_funptr))
I think that would be a good invalid testcase besides the valid one.
But with a tweak to get better messages to give:
Error: variant ‘foo_variant’ and base ‘foo’ at (1) have incompatible types: Type mismatch in argument 'c_bv' (TYPE(c_ptr)/TYPE(c_funptr))
cf. misc.cc in the attached proposal for the *.cc files, only.
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,
Please see the updated patch attached incorporating your input and
details below.
On 24/10/2023 18:12, you wrote:
> On 20.10.23 16:02, Paul-Antoine Arras wrote:
>> gcc/fortran/ChangeLog:
>>
>> * interface.cc (gfc_compare_types): Return true in this situation.
>
> That's a bad description. It makes sense when reading the commit log but
> if you
> only read gcc/fortran/ChangeLog, 'this situation' is a dangling reference.
Updated Changelog with a more helpful description.
>> gcc/fortran/ChangeLog.omp | 5 ++
>> gcc/testsuite/ChangeLog.omp | 4 ++
>
> On mainline, the ChangeLog not ChangeLog.omp is used. This changelog is
> automatically
> filled by the data in the commit log. Thus, no need to include it in the
> patch.
Removed ChangeLog.omp from the patch.
> See attached patch for a combined version, which checks now
> whether from_intmod == INTMOD_ISO_C_BINDING and then compares
> the names (to distinguish c_ptr and c_funptr). Those are unaffected
> by 'use' renames, hence, we should be fine.
Added the proposed diff for interface.cc and misc.cc to the patch.
> Additionally, I think it would be good to have a testcase which checks for
> c_funptr vs. c_ptr
> mismatch.
Added new testcase c_ptr_tests_21.f90 to check that incompatibilities
between c_funptr vs. c_ptr are properly reported.
Is this latest revision ready to commit?
Thanks,
Hi Paul-Antoine,
On 26.10.23 13:24, Paul-Antoine Arras wrote:
> Please see the updated patch attached incorporating your input and
> details below.
...
> Is this latest revision ready to commit?
LGTM.
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!
On 2023-10-26T13:24:04+0200, Paul-Antoine Arras <pa@codesourcery.com> wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
> @@ -0,0 +1,57 @@
> +! { dg-do compile }
> +! { dg-additional-options "-fopenmp" }
> +[...]
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
> @@ -0,0 +1,57 @@
> +! { dg-do compile }
> +! { dg-additional-options "-fopenmp" }
> +[...]
OpenMP is not universally supported across different GCC configurations,
so this will FAIL for some. Therefore, please either guard with
effective target:
@item fopenmp
Target supports OpenMP via @option{-fopenmp}.
..., or move into 'gcc/testsuite/gfortran.dg/gomp/' (may then remove
explicit 'dg-additional-options "-fopenmp"').
I don't know which variant makes more sense, here.
Grüße
Thomas
-----------------
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 Thomas,
On 26/10/2023 18:16, you wrote:
> Hi!
>
> On 2023-10-26T13:24:04+0200, Paul-Antoine Arras <pa@codesourcery.com> wrote:
>> --- /dev/null
>> +++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
>> @@ -0,0 +1,57 @@
>> +! { dg-do compile }
>> +! { dg-additional-options "-fopenmp" }
>> +[...]
>
>> --- /dev/null
>> +++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
>> @@ -0,0 +1,57 @@
>> +! { dg-do compile }
>> +! { dg-additional-options "-fopenmp" }
>> +[...]
>
> OpenMP is not universally supported across different GCC configurations,
> so this will FAIL for some. Therefore, please either guard with
> effective target:
>
> @item fopenmp
> Target supports OpenMP via @option{-fopenmp}.
>
Would the following be enough?
> diff --git gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90 gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
> index 7dd510400f3..131603d3819 100644
> --- gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
> +++ gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
> @@ -1,4 +1,5 @@
> ! { dg-do compile }
> +! { dg-require-effective-target fopenmp }
> ! { dg-additional-options "-fopenmp" }
> !
> ! This failed to compile the declare variant directive due to the C_PTR
> diff --git gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90 gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
> index 05ccb771eee..060d29d0275 100644
> --- gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
> +++ gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
> @@ -1,4 +1,5 @@
> ! { dg-do compile }
> +! { dg-require-effective-target fopenmp }
> ! { dg-additional-options "-fopenmp" }
> !
> ! Ensure that C_PTR and C_FUNPTR are reported as incompatible types in variant
Thanks,
Hi PA!
On 2023-10-26T18:28:07+0200, Paul-Antoine Arras <pa@codesourcery.com> wrote:
> On 26/10/2023 18:16, you wrote:
>> On 2023-10-26T13:24:04+0200, Paul-Antoine Arras <pa@codesourcery.com> wrote:
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
>>> @@ -0,0 +1,57 @@
>>> +! { dg-do compile }
>>> +! { dg-additional-options "-fopenmp" }
>>> +[...]
>>
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
>>> @@ -0,0 +1,57 @@
>>> +! { dg-do compile }
>>> +! { dg-additional-options "-fopenmp" }
>>> +[...]
>>
>> OpenMP is not universally supported across different GCC configurations,
>> so this will FAIL for some. Therefore, please either guard with
>> effective target:
>>
>> @item fopenmp
>> Target supports OpenMP via @option{-fopenmp}.
>>
>
> Would the following be enough?
>
>> diff --git gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90 gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
>> index 7dd510400f3..131603d3819 100644
>> --- gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
>> +++ gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
>> @@ -1,4 +1,5 @@
>> ! { dg-do compile }
>> +! { dg-require-effective-target fopenmp }
>> ! { dg-additional-options "-fopenmp" }
>> !
>> ! This failed to compile the declare variant directive due to the C_PTR
>> diff --git gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90 gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
>> index 05ccb771eee..060d29d0275 100644
>> --- gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
>> +++ gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
>> @@ -1,4 +1,5 @@
>> ! { dg-do compile }
>> +! { dg-require-effective-target fopenmp }
>> ! { dg-additional-options "-fopenmp" }
>> !
>> ! Ensure that C_PTR and C_FUNPTR are reported as incompatible types in variant
Yes, that looks good to me -- you may push "as obvious".
Grüße
Thomas
-----------------
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
Missed that, sorry. Please move the test to gfortran.dg/gomp/ — those tests are automatically compiled with -fopenmp, hence, no need for dg-(additional-)options.
Not applicable here, but tests that use omp.h or "use omp_lib" or the runtime have to be under libgomp/testsuite - or, for compile checks (esp. for is-invalid diagnostic), the required decls have to be copied into the test file.
Tobias
From 8e5fa4828678a1388e75795de2a1f253d9f0ec95 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras <pa@codesourcery.com>
Date: Fri, 20 Oct 2023 12:42:49 +0200
Subject: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and
TYPE(c_ptr)
In the context of an OpenMP declare variant directive, arguments of type C_PTR
are sometimes recognised as C_PTR in the base function and as INTEGER(8) in the
variant - or the other way around, depending on the parsing order.
This patch prevents such situation from turning into a compile error.
2023-10-20 Paul-Antoine Arras <pa@codesourcery.com>
Tobias Burnus <tobias@codesourcery.com>
gcc/fortran/ChangeLog:
* interface.cc (gfc_compare_types): Return true in this situation.
gcc/testsuite/ChangeLog:
* gfortran.dg/c_ptr_tests_20.f90: New test.
---
gcc/fortran/ChangeLog.omp | 5 ++
gcc/fortran/interface.cc | 17 +++---
gcc/testsuite/ChangeLog.omp | 4 ++
gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90 | 56 ++++++++++++++++++++
4 files changed, 76 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
@@ -1,3 +1,8 @@
+2023-10-20 Paul-Antoine Arras <pa@codesourcery.com>
+ Tobias Burnus <tobias@codesourcery.com>
+
+ * interface.cc (gfc_compare_types): Return true in this situation.
+
2023-09-19 Tobias Burnus <tobias@codesourcery.com>
Backported from master:
@@ -705,12 +705,17 @@ gfc_compare_types (gfc_typespec *ts1, gfc_typespec *ts2)
/* Special case for our C interop types. FIXME: There should be a
better way of doing this. When ISO C binding is cleared up,
- this can probably be removed. See PR 57048. */
-
- if (((ts1->type == BT_INTEGER && ts2->type == BT_DERIVED)
- || (ts1->type == BT_DERIVED && ts2->type == BT_INTEGER))
- && ts1->u.derived && ts2->u.derived
- && ts1->u.derived == ts2->u.derived)
+ this can probably be removed. See PR 57048.
+ Note that this does not distinguish between c_ptr and c_funptr. */
+
+ if ((ts1->type == BT_INTEGER && ts2->type == BT_DERIVED
+ && ts1->f90_type == BT_VOID
+ && ts2->u.derived->ts.is_iso_c
+ && ts2->u.derived->ts.u.derived->ts.f90_type == BT_VOID)
+ || (ts2->type == BT_INTEGER && ts1->type == BT_DERIVED
+ && ts2->f90_type == BT_VOID
+ && ts1->u.derived->ts.is_iso_c
+ && ts1->u.derived->ts.u.derived->ts.f90_type == BT_VOID))
return true;
/* The _data component is not always present, therefore check for its
@@ -1,3 +1,7 @@
+2023-10-20 Paul-Antoine Arras <pa@codesourcery.com>
+
+ * gfortran.dg/c_ptr_tests_20.f90: New test.
+
2023-09-20 Tobias Burnus <tobias@codesourcery.com>
Backported from master:
new file mode 100644
@@ -0,0 +1,56 @@
+! { dg-do compile }
+!
+! This failed to compile the declare variant directive due to the C_PTR
+! arguments to foo being recognised as INTEGER(8)
+
+program adjust_args
+ use iso_c_binding, only: c_loc
+ implicit none
+
+ integer, parameter :: N = 1024
+ real, allocatable, target :: av(:), bv(:), cv(:)
+
+ call foo(c_loc(bv), c_loc(av), N)
+
+ !$omp target data map(to: av(:N)) map(from: cv(:N))
+ !$omp parallel
+ call foo(c_loc(cv), c_loc(av), N)
+ !$omp end parallel
+ !$omp end target data
+
+contains
+ subroutine foo_variant(c_d_bv, c_d_av, n)
+ use iso_c_binding, only: c_ptr, c_f_pointer
+ type(c_ptr), intent(in) :: c_d_bv, c_d_av
+ integer, intent(in) :: n
+ real, pointer :: f_d_bv(:)
+ real, pointer :: f_d_av(:)
+ integer :: i
+
+ call c_f_pointer(c_d_bv, f_d_bv, [n])
+ call c_f_pointer(c_d_av, f_d_av, [n])
+ !$omp target teams loop is_device_ptr(f_d_bv, f_d_av)
+ do i = 1, n
+ f_d_bv(i) = f_d_av(i) * i
+ end do
+ end subroutine
+
+
+ subroutine foo(c_bv, c_av, n)
+ use iso_c_binding, only: c_ptr, c_f_pointer
+ type(c_ptr), intent(in) :: c_bv, c_av
+ integer, intent(in) :: n
+ real, pointer :: f_bv(:)
+ real, pointer :: f_av(:)
+ integer :: i
+ !$omp declare variant(foo_variant) &
+ !$omp match(construct={parallel})
+
+ call c_f_pointer(c_bv, f_bv, [n])
+ call c_f_pointer(c_av, f_av, [n])
+ !$omp parallel loop
+ do i = 1, n
+ f_bv(i) = f_av(i) * i
+ end do
+ end subroutine
+end program
--
2.42.0