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