Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
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
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
Hi PA!
On 2023-10-26T18:28:07+0200, Paul-Antoine Arras wrote:
> On 26/10/2023 18:16, you wrote:
>> On 2023-10-26T13:24:04+0200, Paul-Antoine Arras
>> 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
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
Hi Thomas,
On 26/10/2023 18:16, you wrote:
Hi!
On 2023-10-26T13:24:04+0200, Paul-Antoine Arras 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,
--
PA
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
Hi!
On 2023-10-26T13:24:04+0200, Paul-Antoine Arras 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
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
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
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
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,
--
PA
From 691d1050ce39c27231dc610b799bf180871820b8 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras
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
Tobias Burnus
gcc/fortran/ChangeLog:
* interface.cc (gfc_compare_types): Return true if one type is C_PTR
and the other is a compatible INTEGER(8).
* misc.cc (gfc_typename): Handle the case where an INTEGER(8) actually
holds a TYPE(C_PTR).
gcc/testsuite/ChangeLog:
* gfortran.dg/c_ptr_tests_20.f90: New test, checking that INTEGER(8)
and TYPE(C_PTR) are recognised as compatible.
* gfortran.dg/c_ptr_tests_21.f90: New test, exercising the error
detection for C_FUNPTR.
---
gcc/fortran/interface.cc | 16 --
gcc/fortran/misc.cc | 7 ++-
gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90 | 57
gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90 | 57
4 files changed, 132 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
create mode 100644 gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
diff --git a/gcc/fortran/interface.cc b/gcc/fortran/interface.cc
index e9843e9549c..ed1613b16fb 100644
--- a/gcc/fortran/interface.cc
+++ b/gcc/fortran/interface.cc
@@ -707,10 +707,18 @@ gfc_compare_types (gfc_typespec *ts1, gfc_typespec *ts2)
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)
+ if ((ts1->type == BT_INTEGER
+ && ts2->type == BT_DERIVED
+ && ts1->f90_type == BT_VOID
+ && ts2->u.derived->from_intmod == INTMOD_ISO_C_BINDING
+ && ts1->u.derived
+ && strcmp (ts1->u.derived->name, ts2->u.derived->name) == 0)
+ || (ts2->type == BT_INTEGER
+ && ts1->type == BT_DERIVED
+ && ts2->f90_type == BT_VOID
+ && ts1->u.derived->from_intmod == INTMOD_ISO_C_BINDING
+ && ts2->u.derived
+ && strcmp (ts1->u.derived->name, ts2->u.derived->name) == 0))
return true;
/* The _data component is not always present, therefore check for its
diff --git a/gcc/fortran/misc.cc b/gcc/fortran/misc.cc
index bae6d292dc5..edffba07013 100644
--- a/gcc/fortran/misc.cc
+++ b/gcc/fortran/misc.cc
@@ -138,7 +138,12 @@ gfc_typename (gfc_typespec *ts, bool for_hash)
switch (ts->type)
{
case BT_INTEGER:
- sprintf (buffer, "INTEGER(%d)", ts->kind);
+ if (ts->f90_type == BT_VOID
+ && ts->u.derived
+ && ts->u.derived->from_intmod == INTMOD_ISO_C_BINDING)
+ sprintf (buffer, "TYPE(%s)", ts->u.derived->name);
+ else
+ sprintf (buffer, "INTEGER(%d)", ts->kind);
break;
case BT_REAL:
sprintf (buffer, "REAL(%d)", ts->kind);
diff -
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
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
Tobias Burnus
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 = 0x76ff4138 "foo"
(gdb) p ts2->u.derived->ns->proc_name->name
$19 = 0x76ffc260 "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 = 0x76ff4100 "c_ptr"
(gdb) p ts2->u.derived->name
$14 = 0x76ff4100 "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 b
[PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
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,
--
PA
From 8e5fa4828678a1388e75795de2a1f253d9f0ec95 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras
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
Tobias Burnus
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
diff --git a/gcc/fortran/ChangeLog.omp b/gcc/fortran/ChangeLog.omp
index 62a33475ee5..299223ceaa7 100644
--- a/gcc/fortran/ChangeLog.omp
+++ b/gcc/fortran/ChangeLog.omp
@@ -1,3 +1,8 @@
+2023-10-20 Paul-Antoine Arras
+ Tobias Burnus
+
+ * interface.cc (gfc_compare_types): Return true in this situation.
+
2023-09-19 Tobias Burnus
Backported from master:
diff --git a/gcc/fortran/interface.cc b/gcc/fortran/interface.cc
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)
/* 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
diff --git a/gcc/testsuite/ChangeLog.omp b/gcc/testsuite/ChangeLog.omp
index e9f4d1c63e6..1fc9b0606dc 100644
--- a/gcc/testsuite/ChangeLog.omp
+++ b/gcc/testsuite/ChangeLog.omp
@@ -1,3 +1,7 @@
+2023-10-20 Paul-Antoine Arras
+
+ * gfortran.dg/c_ptr_tests_20.f90: New test.
+
2023-09-20 Tobias Burnus
Backported from master:
diff --git a/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90 b/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
new file mode 100644
index 000..777181cece0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
@@ -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
