diff mbox series

Aw: Re: [Patch, Fortran] PR92898 - [9/10 Regression] ICE in gfc_check_is_contiguous, at fortran/check.c:7157

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

Commit Message

Harald Anlauf Dec. 11, 2019, 10:24 p.m. UTC 
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



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.

Comments

Steve Kargl Dec. 11, 2019, 10:35 p.m. UTC | #1 
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.
Tobias Burnus Dec. 12, 2019, 8:01 a.m. UTC | #2 
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.
>
Harald Anlauf Dec. 12, 2019, 8:57 p.m. UTC | #3 
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.
> >
>
diff mbox series

Patch

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