Message ID | trinity-034de6e1-6f50-4056-821a-6b820cfcfec0-1576103075877@3c-app-gmx-bap02 |
---|---|
State | New |
Headers | show |
Series | Aw: Re: [Patch, Fortran] PR92898 - [9/10 Regression] ICE in gfc_check_is_contiguous, at fortran/check.c:7157 | expand |
On Wed, Dec 11, 2019 at 11:24:35PM +0100, Harald Anlauf wrote: > > > Gesendet: Dienstag, 10. Dezember 2019 um 23:34 Uhr > > Von: "Thomas Koenig" <tkoenig@netcologne.de> > > An: "Harald Anlauf" <anlauf@gmx.de>, gfortran <fortran@gcc.gnu.org>, gcc-patches <gcc-patches@gcc.gnu.org> > > Betreff: Re: [Patch, Fortran] PR92898 - [9/10 Regression] ICE in gfc_check_is_contiguous, at fortran/check.c:7157 > > > > > > > Index: gcc/fortran/check.c > > > =================================================================== > > > --- gcc/fortran/check.c (Revision 279183) > > > +++ gcc/fortran/check.c (Arbeitskopie) > > > @@ -7154,7 +7154,9 @@ bool > > > gfc_check_is_contiguous (gfc_expr *array) > > > { > > > if (array->expr_type == EXPR_NULL > > > - && array->symtree->n.sym->attr.pointer == 1) > > > + && (!array->symtree || > > > + (array->symtree->n.sym && > > > + array->symtree->n.sym->attr.pointer == 1))) > > > > I have to admit I do not understand the original code here, nor > > do I quite understand your fix. > > > > Is there any circumstance where array->expr_type == EXPR_NULL, but > > is_contiguous is valid? What would go wrong if the other tests > > were removed? > > Actually I do not know what the additional check was supposed to do. > Removing it does not seem to do any harm. See below. > The orginal testcase has NULL(Z) where Z has the pointer attribute. See 16.9.144. NULL(Z) then has the pointer attribute. I did not consider NULL(), which is of course a valid reference.
Hi Harald, let's add a LGTM or OK to this – the patch is rather obvious and Steve explained how the now-removed check ended up in gfortran. Thanks for the patch! Tobias On 12/11/19 11:24 PM, Harald Anlauf wrote: > Hi Thomas, > >> Gesendet: Dienstag, 10. Dezember 2019 um 23:34 Uhr >> Von: "Thomas Koenig" <tkoenig@netcologne.de> >> An: "Harald Anlauf" <anlauf@gmx.de>, gfortran <fortran@gcc.gnu.org>, gcc-patches <gcc-patches@gcc.gnu.org> >> Betreff: Re: [Patch, Fortran] PR92898 - [9/10 Regression] ICE in gfc_check_is_contiguous, at fortran/check.c:7157 >> >> Hello Harald, >> >>> Index: gcc/fortran/check.c >>> =================================================================== >>> --- gcc/fortran/check.c (Revision 279183) >>> +++ gcc/fortran/check.c (Arbeitskopie) >>> @@ -7154,7 +7154,9 @@ bool >>> gfc_check_is_contiguous (gfc_expr *array) >>> { >>> if (array->expr_type == EXPR_NULL >>> - && array->symtree->n.sym->attr.pointer == 1) >>> + && (!array->symtree || >>> + (array->symtree->n.sym && >>> + array->symtree->n.sym->attr.pointer == 1))) >> I have to admit I do not understand the original code here, nor >> do I quite understand your fix. >> >> Is there any circumstance where array->expr_type == EXPR_NULL, but >> is_contiguous is valid? What would go wrong if the other tests >> were removed? > Actually I do not know what the additional check was supposed to do. > Removing it does not seem to do any harm. See below. > >>> Index: gcc/testsuite/gfortran.dg/pr91641.f90 >>> =================================================================== >>> --- gcc/testsuite/gfortran.dg/pr91641.f90 (Revision 279183) >>> +++ gcc/testsuite/gfortran.dg/pr91641.f90 (Arbeitskopie) >>> @@ -1,7 +1,9 @@ >>> ! { dg-do compile } >>> ! PR fortran/91641 >>> -! Code conyributed by Gerhard Steinmetz >>> +! PR fortran/92898 >>> +! Code contributed by Gerhard Steinmetz >>> program p >>> real, pointer :: z(:) >>> print *, is_contiguous (null(z)) ! { dg-error "shall be an associated" } >>> + print *, is_contiguous (null()) ! { dg-error "shall be an associated" } >>> end >> Sometimes, it is necessary to change test cases, when error messages >> change. If this is not the case, it is better to add new tests to >> new test cases - this makes regression hunting much easier. >> >> Regards >> >> Thomas > Agreed. Please find the modified patches below. OK for trunk / 9 ? > > Thanks, > Harald > > Index: gcc/fortran/check.c > =================================================================== > --- gcc/fortran/check.c (Revision 279254) > +++ gcc/fortran/check.c (Arbeitskopie) > @@ -7153,8 +7153,7 @@ gfc_check_ttynam_sub (gfc_expr *unit, gfc_expr *na > bool > gfc_check_is_contiguous (gfc_expr *array) > { > - if (array->expr_type == EXPR_NULL > - && array->symtree->n.sym->attr.pointer == 1) > + if (array->expr_type == EXPR_NULL) > { > gfc_error ("Actual argument at %L of %qs intrinsic shall be an " > "associated pointer", &array->where, gfc_current_intrinsic); > > > > Index: gcc/testsuite/gfortran.dg/pr92898.f90 > =================================================================== > --- gcc/testsuite/gfortran.dg/pr92898.f90 (nicht existent) > +++ gcc/testsuite/gfortran.dg/pr92898.f90 (Arbeitskopie) > @@ -0,0 +1,6 @@ > +! { dg-do compile } > +! PR fortran/92898 > +! Code contributed by Gerhard Steinmetz > +program p > + print *, is_contiguous (null()) ! { dg-error "shall be an associated" } > +end > > > 2019-12-11 Harald Anlauf <anlauf@gmx.de> > > PR fortran/92898 > * check.c (gfc_check_is_contiguous): Simplify check to handle > arbitrary NULL() argument. > > 2019-12-11 Harald Anlauf <anlauf@gmx.de> > > PR fortran/92898 > * gfortran.dg/pr92898.f90: New test. >
Hi Tobias, > Gesendet: Donnerstag, 12. Dezember 2019 um 09:01 Uhr > Von: "Tobias Burnus" <tobias@codesourcery.com> > An: "Harald Anlauf" <anlauf@gmx.de>, "Thomas Koenig" <tkoenig@netcologne.de> > Cc: gfortran <fortran@gcc.gnu.org>, gcc-patches <gcc-patches@gcc.gnu.org> > Betreff: Re: Aw: Re: [Patch, Fortran] PR92898 - [9/10 Regression] ICE in gfc_check_is_contiguous, at fortran/check.c:7157 > > Hi Harald, > > let's add a LGTM or OK to this – the patch is rather obvious and Steve > explained how the now-removed check ended up in gfortran. thanks for the clarification, and thanks for the quick review. > Thanks for the patch! Committed as svn rev.279314 (trunk) and 279315 (9-branch). Harald > Tobias > > On 12/11/19 11:24 PM, Harald Anlauf wrote: > > Hi Thomas, > > > >> Gesendet: Dienstag, 10. Dezember 2019 um 23:34 Uhr > >> Von: "Thomas Koenig" <tkoenig@netcologne.de> > >> An: "Harald Anlauf" <anlauf@gmx.de>, gfortran <fortran@gcc.gnu.org>, gcc-patches <gcc-patches@gcc.gnu.org> > >> Betreff: Re: [Patch, Fortran] PR92898 - [9/10 Regression] ICE in gfc_check_is_contiguous, at fortran/check.c:7157 > >> > >> Hello Harald, > >> > >>> Index: gcc/fortran/check.c > >>> =================================================================== > >>> --- gcc/fortran/check.c (Revision 279183) > >>> +++ gcc/fortran/check.c (Arbeitskopie) > >>> @@ -7154,7 +7154,9 @@ bool > >>> gfc_check_is_contiguous (gfc_expr *array) > >>> { > >>> if (array->expr_type == EXPR_NULL > >>> - && array->symtree->n.sym->attr.pointer == 1) > >>> + && (!array->symtree || > >>> + (array->symtree->n.sym && > >>> + array->symtree->n.sym->attr.pointer == 1))) > >> I have to admit I do not understand the original code here, nor > >> do I quite understand your fix. > >> > >> Is there any circumstance where array->expr_type == EXPR_NULL, but > >> is_contiguous is valid? What would go wrong if the other tests > >> were removed? > > Actually I do not know what the additional check was supposed to do. > > Removing it does not seem to do any harm. See below. > > > >>> Index: gcc/testsuite/gfortran.dg/pr91641.f90 > >>> =================================================================== > >>> --- gcc/testsuite/gfortran.dg/pr91641.f90 (Revision 279183) > >>> +++ gcc/testsuite/gfortran.dg/pr91641.f90 (Arbeitskopie) > >>> @@ -1,7 +1,9 @@ > >>> ! { dg-do compile } > >>> ! PR fortran/91641 > >>> -! Code conyributed by Gerhard Steinmetz > >>> +! PR fortran/92898 > >>> +! Code contributed by Gerhard Steinmetz > >>> program p > >>> real, pointer :: z(:) > >>> print *, is_contiguous (null(z)) ! { dg-error "shall be an associated" } > >>> + print *, is_contiguous (null()) ! { dg-error "shall be an associated" } > >>> end > >> Sometimes, it is necessary to change test cases, when error messages > >> change. If this is not the case, it is better to add new tests to > >> new test cases - this makes regression hunting much easier. > >> > >> Regards > >> > >> Thomas > > Agreed. Please find the modified patches below. OK for trunk / 9 ? > > > > Thanks, > > Harald > > > > Index: gcc/fortran/check.c > > =================================================================== > > --- gcc/fortran/check.c (Revision 279254) > > +++ gcc/fortran/check.c (Arbeitskopie) > > @@ -7153,8 +7153,7 @@ gfc_check_ttynam_sub (gfc_expr *unit, gfc_expr *na > > bool > > gfc_check_is_contiguous (gfc_expr *array) > > { > > - if (array->expr_type == EXPR_NULL > > - && array->symtree->n.sym->attr.pointer == 1) > > + if (array->expr_type == EXPR_NULL) > > { > > gfc_error ("Actual argument at %L of %qs intrinsic shall be an " > > "associated pointer", &array->where, gfc_current_intrinsic); > > > > > > > > Index: gcc/testsuite/gfortran.dg/pr92898.f90 > > =================================================================== > > --- gcc/testsuite/gfortran.dg/pr92898.f90 (nicht existent) > > +++ gcc/testsuite/gfortran.dg/pr92898.f90 (Arbeitskopie) > > @@ -0,0 +1,6 @@ > > +! { dg-do compile } > > +! PR fortran/92898 > > +! Code contributed by Gerhard Steinmetz > > +program p > > + print *, is_contiguous (null()) ! { dg-error "shall be an associated" } > > +end > > > > > > 2019-12-11 Harald Anlauf <anlauf@gmx.de> > > > > PR fortran/92898 > > * check.c (gfc_check_is_contiguous): Simplify check to handle > > arbitrary NULL() argument. > > > > 2019-12-11 Harald Anlauf <anlauf@gmx.de> > > > > PR fortran/92898 > > * gfortran.dg/pr92898.f90: New test. > > >
Index: gcc/fortran/check.c =================================================================== --- gcc/fortran/check.c (Revision 279254) +++ gcc/fortran/check.c (Arbeitskopie) @@ -7153,8 +7153,7 @@ gfc_check_ttynam_sub (gfc_expr *unit, gfc_expr *na bool gfc_check_is_contiguous (gfc_expr *array) { - if (array->expr_type == EXPR_NULL - && array->symtree->n.sym->attr.pointer == 1) + if (array->expr_type == EXPR_NULL) { gfc_error ("Actual argument at %L of %qs intrinsic shall be an " "associated pointer", &array->where, gfc_current_intrinsic); Index: gcc/testsuite/gfortran.dg/pr92898.f90 =================================================================== --- gcc/testsuite/gfortran.dg/pr92898.f90 (nicht existent) +++ gcc/testsuite/gfortran.dg/pr92898.f90 (Arbeitskopie) @@ -0,0 +1,6 @@ +! { dg-do compile } +! PR fortran/92898 +! Code contributed by Gerhard Steinmetz +program p + print *, is_contiguous (null()) ! { dg-error "shall be an associated" } +end