diff mbox series

[fortran] Fix PR 57048

Message ID 6f50fa14-64b4-3e6d-d051-e06bcdfd7f82@tkoenig.net
State New
Headers show
Series [fortran] Fix PR 57048 | expand

Commit Message

Thomas König Jan. 28, 2019, 7:49 p.m. UTC 
Hello world,

let the regression hunt continue!

the attached patch fixes a long-time regression where a c_funptr from a
module could not be found.

The solution is a bit of a hack, but so is our whole implementation of
the C interop stuff.

Regression-tested. OK for trunk?

Regards

	Thomas

2019-01-28  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/57048
	* interface.c (gfc_compare_types): If a derived type and an
	integer both have a derived type, and they are identical,
	this is a C binding type and compares equal.

2019-01-28  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/57048
	* gfortran.dg/c_funptr_1.f90: New file.
	* gfortran.dg/c_funptr_1_mod.f90: New file.

Comments

Steve Kargl Jan. 29, 2019, 7:18 p.m. UTC | #1 
On Mon, Jan 28, 2019 at 08:49:52PM +0100, Thomas König wrote:
> 
> the attached patch fixes a long-time regression where a c_funptr from a
> module could not be found.
> 
> The solution is a bit of a hack, but so is our whole implementation of
> the C interop stuff.
> 
> Regression-tested. OK for trunk?
> 
> Regards
> 
> 	Thomas
> 
> 2019-01-28  Thomas Koenig  <tkoenig@gcc.gnu.org>
> 
> 	PR fortran/57048
> 	* interface.c (gfc_compare_types): If a derived type and an
> 	integer both have a derived type, and they are identical,
> 	this is a C binding type and compares equal.

I don't understand this sentence.  How can an INTEGER have a
derived type?

> 
> 2019-01-28  Thomas Koenig  <tkoenig@gcc.gnu.org>
> 
> 	PR fortran/57048
> 	* gfortran.dg/c_funptr_1.f90: New file.
> 	* gfortran.dg/c_funptr_1_mod.f90: New file.

> Index: interface.c
> ===================================================================
> --- interface.c	(Revision 268104)
> +++ interface.c	(Arbeitskopie)
> @@ -692,6 +692,15 @@ gfc_compare_types (gfc_typespec *ts1, gfc_typespec
>    if (ts1->type == BT_VOID || ts2->type == BT_VOID)
>      return true;
>  
> +  /* Special case for our C interop types.  There should be a better
> +     way of doing this...  */
> +
> +  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)
> +    return true;

If the above is a test for ISO C binding, shouldn't the test
also check (ts1->is_c_interop || ts1->is_iso_c) and similar
for ts2?
Thomas König Jan. 29, 2019, 9:12 p.m. UTC | #2 
Hi Steve,

>> 	PR fortran/57048
>> 	* interface.c (gfc_compare_types): If a derived type and an
>> 	integer both have a derived type, and they are identical,
>> 	this is a C binding type and compares equal.
> 
> I don't understand this sentence.  How can an INTEGER have a
> derived type?

ISO C handling is a bit of a mess. Of course.
The original tree has

   symtree: 'C_funptr'    || symbol: 'c_funptr'
     type spec : (DERIVED c_funptr C_INTEROP ISO_C)
     attributes: (DERIVED  BIND(C) USE-ASSOC(__iso_c_binding))
     components:
     (c_address (INTEGER 8 C_INTEROP) () PRIVATE)

and we somehow later lose the derived type stuff and use a naked
integer.  The problem appears to be that the module is written out
when that conversion has not yet happened.

So, the C_funptr (capitalized because it is a derived type) is
written to the module file, and later compared to c_funptr upon
module reading. Hilarity ensues, and we cannot use a C function
pointer from a module, the topic of the PR.

>>
>> 2019-01-28  Thomas Koenig  <tkoenig@gcc.gnu.org>
>>
>> 	PR fortran/57048
>> 	* gfortran.dg/c_funptr_1.f90: New file.
>> 	* gfortran.dg/c_funptr_1_mod.f90: New file.
> 
>> Index: interface.c
>> ===================================================================
>> --- interface.c	(Revision 268104)
>> +++ interface.c	(Arbeitskopie)
>> @@ -692,6 +692,15 @@ gfc_compare_types (gfc_typespec *ts1, gfc_typespec
>>     if (ts1->type == BT_VOID || ts2->type == BT_VOID)
>>       return true;
>>   
>> +  /* Special case for our C interop types.  There should be a better
>> +     way of doing this...  */
>> +
>> +  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)
>> +    return true;
> 
> If the above is a test for ISO C binding, shouldn't the test
> also check (ts1->is_c_interop || ts1->is_iso_c) and similar
> for ts2?

Unfortunately, these are both set to zero.

Yep, this is a rather messy fix to a messy situation. I think the
C interop stuff could do with a thorough redesign, but that's
not going to happen for gcc 9.

So, I think this is about the best we can do - at least it gets the
bug fixed.  If you want me to put in a FIXME, I'll gladly do this.

Regards

	Thomas
Steve Kargl Jan. 29, 2019, 9:38 p.m. UTC | #3 
On Tue, Jan 29, 2019 at 10:12:49PM +0100, Thomas König wrote:
> Hi Steve,
> 
> >> 	PR fortran/57048
> >> 	* interface.c (gfc_compare_types): If a derived type and an
> >> 	integer both have a derived type, and they are identical,
> >> 	this is a C binding type and compares equal.
> > 
> > I don't understand this sentence.  How can an INTEGER have a
> > derived type?
> 
> ISO C handling is a bit of a mess. Of course.
> The original tree has
> 
>    symtree: 'C_funptr'    || symbol: 'c_funptr'
>      type spec : (DERIVED c_funptr C_INTEROP ISO_C)
>      attributes: (DERIVED  BIND(C) USE-ASSOC(__iso_c_binding))
>      components:
>      (c_address (INTEGER 8 C_INTEROP) () PRIVATE)
> 
> and we somehow later lose the derived type stuff and use a naked
> integer.  The problem appears to be that the module is written out
> when that conversion has not yet happened.
> 
> So, the C_funptr (capitalized because it is a derived type) is
> written to the module file, and later compared to c_funptr upon
> module reading. Hilarity ensues, and we cannot use a C function
> pointer from a module, the topic of the PR.

Ugh.  Thanks for the explanation.

> >> 2019-01-28  Thomas Koenig  <tkoenig@gcc.gnu.org>
> >>
> >> 	PR fortran/57048
> >> 	* gfortran.dg/c_funptr_1.f90: New file.
> >> 	* gfortran.dg/c_funptr_1_mod.f90: New file.
> > 
> 
> Yep, this is a rather messy fix to a messy situation. I think the
> C interop stuff could do with a thorough redesign, but that's
> not going to happen for gcc 9.
> 
> So, I think this is about the best we can do - at least it gets the
> bug fixed.  If you want me to put in a FIXME, I'll gladly do this.
> 

Perhaps, add the PR number to the comment in code so
history of why this is a special case can be found.

I'll leave up to you on adding a "FIXME: If ISO C Binding support is
ever rewritten, this should be revisted" or some such phrasing.

Thanks for the patch, and OK to commit.
Thomas König Jan. 29, 2019, 10:41 p.m. UTC | #4 
Hi Steve,

> Thanks for the patch, and OK to commit.

Committed to trunk (r268372).  Thanks!

I will backport to the affected branches later in the week.

Regards

	Thomas
diff mbox series

Patch

Index: interface.c
===================================================================
--- interface.c	(Revision 268104)
+++ interface.c	(Arbeitskopie)
@@ -692,6 +692,15 @@  gfc_compare_types (gfc_typespec *ts1, gfc_typespec
   if (ts1->type == BT_VOID || ts2->type == BT_VOID)
     return true;
 
+  /* Special case for our C interop types.  There should be a better
+     way of doing this...  */
+
+  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)
+    return true;
+
   /* The _data component is not always present, therefore check for its
      presence before assuming, that its derived->attr is available.
      When the _data component is not present, then nevertheless the