[fortran] PR90786 - [7/8/9/10 Regression] ICE on procedure pointer assignment to function with class pointer result

Message ID CAGkQGiK_34vgMt2K24fJ2LS3YuBX5D+BxejDeTcVtaGtUeuMqw@mail.gmail.com
State New
Headers show
Series
  • [fortran] PR90786 - [7/8/9/10 Regression] ICE on procedure pointer assignment to function with class pointer result
Related show

Commit Message

Paul Richard Thomas June 8, 2019, 3:56 p.m.
Committed as obvious in revision 272084.

The problem was that the lhs symbol itself was not being checked as a
proc_pointer - just the expression component.

I will get on with backporting tomorrow.

Cheers

Paul

2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/90786
    * trans-expr.c (pointer_assignment_is_proc_pointer) Remove as
    it is very simple and only called from one place.
    (gfc_trans_pointer_assignment): Rename non_proc_pointer_assign
    as non_proc_ptr_assign. Assign to it directly, rather than call
    to above, deleted function and use gfc_expr_attr instead of
    only checking the reference chain.

2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/90786
    * gfortran.dg/proc_ptr_51.f90 : New test.

Comments

Andrew Benson June 8, 2019, 4:25 p.m. | #1
Thanks Paul for the quick fix!

On Saturday, June 8, 2019 4:56:46 PM PDT Paul Richard Thomas wrote:
> Committed as obvious in revision 272084.

> 

> The problem was that the lhs symbol itself was not being checked as a

> proc_pointer - just the expression component.

> 

> I will get on with backporting tomorrow.

> 

> Cheers

> 

> Paul

> 

> 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> 

>     PR fortran/90786

>     * trans-expr.c (pointer_assignment_is_proc_pointer) Remove as

>     it is very simple and only called from one place.

>     (gfc_trans_pointer_assignment): Rename non_proc_pointer_assign

>     as non_proc_ptr_assign. Assign to it directly, rather than call

>     to above, deleted function and use gfc_expr_attr instead of

>     only checking the reference chain.

> 

> 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> 

>     PR fortran/90786

>     * gfortran.dg/proc_ptr_51.f90 : New test.



-- 

* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/galacticusdev/galacticus
Christophe Lyon June 10, 2019, 9:07 a.m. | #2
On Sat, 8 Jun 2019 at 18:25, Andrew Benson <abenson@carnegiescience.edu> wrote:
>

> Thanks Paul for the quick fix!

>

> On Saturday, June 8, 2019 4:56:46 PM PDT Paul Richard Thomas wrote:

> > Committed as obvious in revision 272084.

> >

> > The problem was that the lhs symbol itself was not being checked as a

> > proc_pointer - just the expression component.

> >

> > I will get on with backporting tomorrow.

> >

> > Cheers

> >

> > Paul

> >

> > 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> >

> >     PR fortran/90786

> >     * trans-expr.c (pointer_assignment_is_proc_pointer) Remove as

> >     it is very simple and only called from one place.

> >     (gfc_trans_pointer_assignment): Rename non_proc_pointer_assign

> >     as non_proc_ptr_assign. Assign to it directly, rather than call

> >     to above, deleted function and use gfc_expr_attr instead of

> >     only checking the reference chain.

> >

> > 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> >

> >     PR fortran/90786

> >     * gfortran.dg/proc_ptr_51.f90 : New test.

>

>


Hi,

I've noticed that this new test fails on arm/aarch64:
FAIL:gfortran.dg/proc_ptr_51.f90   -O2  execution test
FAIL:gfortran.dg/proc_ptr_51.f90   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions  execution
test
FAIL:gfortran.dg/proc_ptr_51.f90   -O3 -g  execution test

the logs say:
Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

Backtrace for this error:
#0  0xffffa938f66b in ???
#1  0x0 in ???

Christophe

> --

>

> * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

>

> * Galacticus: https://bitbucket.org/galacticusdev/galacticus

>
Paul Richard Thomas June 10, 2019, 11:05 a.m. | #3
Hi Christophe,

I'll have a think about this tonight. Is valgrind or some similar
available for arm/aarch64?

Many thanks for flagging it up.

Paul

On Mon, 10 Jun 2019 at 10:07, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>

> On Sat, 8 Jun 2019 at 18:25, Andrew Benson <abenson@carnegiescience.edu> wrote:

> >

> > Thanks Paul for the quick fix!

> >

> > On Saturday, June 8, 2019 4:56:46 PM PDT Paul Richard Thomas wrote:

> > > Committed as obvious in revision 272084.

> > >

> > > The problem was that the lhs symbol itself was not being checked as a

> > > proc_pointer - just the expression component.

> > >

> > > I will get on with backporting tomorrow.

> > >

> > > Cheers

> > >

> > > Paul

> > >

> > > 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> > >

> > >     PR fortran/90786

> > >     * trans-expr.c (pointer_assignment_is_proc_pointer) Remove as

> > >     it is very simple and only called from one place.

> > >     (gfc_trans_pointer_assignment): Rename non_proc_pointer_assign

> > >     as non_proc_ptr_assign. Assign to it directly, rather than call

> > >     to above, deleted function and use gfc_expr_attr instead of

> > >     only checking the reference chain.

> > >

> > > 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> > >

> > >     PR fortran/90786

> > >     * gfortran.dg/proc_ptr_51.f90 : New test.

> >

> >

>

> Hi,

>

> I've noticed that this new test fails on arm/aarch64:

> FAIL:gfortran.dg/proc_ptr_51.f90   -O2  execution test

> FAIL:gfortran.dg/proc_ptr_51.f90   -O3 -fomit-frame-pointer

> -funroll-loops -fpeel-loops -ftracer -finline-functions  execution

> test

> FAIL:gfortran.dg/proc_ptr_51.f90   -O3 -g  execution test

>

> the logs say:

> Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

>

> Backtrace for this error:

> #0  0xffffa938f66b in ???

> #1  0x0 in ???

>

> Christophe

>

> > --

> >

> > * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

> >

> > * Galacticus: https://bitbucket.org/galacticusdev/galacticus

> >




-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein
Christophe Lyon June 10, 2019, 1:37 p.m. | #4
On Mon, 10 Jun 2019 at 13:05, Paul Richard Thomas
<paul.richard.thomas@gmail.com> wrote:
>

> Hi Christophe,

>

> I'll have a think about this tonight. Is valgrind or some similar

> available for arm/aarch64?


Yes, valgrind is available. I don't know if it's installed on the
machines in the GCC computer farm though.

Christophe


>

> Many thanks for flagging it up.

>

> Paul

>

> On Mon, 10 Jun 2019 at 10:07, Christophe Lyon

> <christophe.lyon@linaro.org> wrote:

> >

> > On Sat, 8 Jun 2019 at 18:25, Andrew Benson <abenson@carnegiescience.edu> wrote:

> > >

> > > Thanks Paul for the quick fix!

> > >

> > > On Saturday, June 8, 2019 4:56:46 PM PDT Paul Richard Thomas wrote:

> > > > Committed as obvious in revision 272084.

> > > >

> > > > The problem was that the lhs symbol itself was not being checked as a

> > > > proc_pointer - just the expression component.

> > > >

> > > > I will get on with backporting tomorrow.

> > > >

> > > > Cheers

> > > >

> > > > Paul

> > > >

> > > > 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> > > >

> > > >     PR fortran/90786

> > > >     * trans-expr.c (pointer_assignment_is_proc_pointer) Remove as

> > > >     it is very simple and only called from one place.

> > > >     (gfc_trans_pointer_assignment): Rename non_proc_pointer_assign

> > > >     as non_proc_ptr_assign. Assign to it directly, rather than call

> > > >     to above, deleted function and use gfc_expr_attr instead of

> > > >     only checking the reference chain.

> > > >

> > > > 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> > > >

> > > >     PR fortran/90786

> > > >     * gfortran.dg/proc_ptr_51.f90 : New test.

> > >

> > >

> >

> > Hi,

> >

> > I've noticed that this new test fails on arm/aarch64:

> > FAIL:gfortran.dg/proc_ptr_51.f90   -O2  execution test

> > FAIL:gfortran.dg/proc_ptr_51.f90   -O3 -fomit-frame-pointer

> > -funroll-loops -fpeel-loops -ftracer -finline-functions  execution

> > test

> > FAIL:gfortran.dg/proc_ptr_51.f90   -O3 -g  execution test

> >

> > the logs say:

> > Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

> >

> > Backtrace for this error:

> > #0  0xffffa938f66b in ???

> > #1  0x0 in ???

> >

> > Christophe

> >

> > > --

> > >

> > > * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

> > >

> > > * Galacticus: https://bitbucket.org/galacticusdev/galacticus

> > >

>

>

>

> --

> "If you can't explain it simply, you don't understand it well enough"

> - Albert Einstein
Paul Richard Thomas June 10, 2019, 4:53 p.m. | #5
Hi Christophe,

I cannot see anything wrong with the optimized code and valgrind gives
a clean bill of health on x86_64.

We need help of somebody with access to an arm/aarch64 device.

Cheers

Paul

On Mon, 10 Jun 2019 at 14:37, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>

> On Mon, 10 Jun 2019 at 13:05, Paul Richard Thomas

> <paul.richard.thomas@gmail.com> wrote:

> >

> > Hi Christophe,

> >

> > I'll have a think about this tonight. Is valgrind or some similar

> > available for arm/aarch64?

>

> Yes, valgrind is available. I don't know if it's installed on the

> machines in the GCC computer farm though.

>

> Christophe

>

>

> >

> > Many thanks for flagging it up.

> >

> > Paul

> >

> > On Mon, 10 Jun 2019 at 10:07, Christophe Lyon

> > <christophe.lyon@linaro.org> wrote:

> > >

> > > On Sat, 8 Jun 2019 at 18:25, Andrew Benson <abenson@carnegiescience.edu> wrote:

> > > >

> > > > Thanks Paul for the quick fix!

> > > >

> > > > On Saturday, June 8, 2019 4:56:46 PM PDT Paul Richard Thomas wrote:

> > > > > Committed as obvious in revision 272084.

> > > > >

> > > > > The problem was that the lhs symbol itself was not being checked as a

> > > > > proc_pointer - just the expression component.

> > > > >

> > > > > I will get on with backporting tomorrow.

> > > > >

> > > > > Cheers

> > > > >

> > > > > Paul

> > > > >

> > > > > 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> > > > >

> > > > >     PR fortran/90786

> > > > >     * trans-expr.c (pointer_assignment_is_proc_pointer) Remove as

> > > > >     it is very simple and only called from one place.

> > > > >     (gfc_trans_pointer_assignment): Rename non_proc_pointer_assign

> > > > >     as non_proc_ptr_assign. Assign to it directly, rather than call

> > > > >     to above, deleted function and use gfc_expr_attr instead of

> > > > >     only checking the reference chain.

> > > > >

> > > > > 2019-06-08  Paul Thomas  <pault@gcc.gnu.org>

> > > > >

> > > > >     PR fortran/90786

> > > > >     * gfortran.dg/proc_ptr_51.f90 : New test.

> > > >

> > > >

> > >

> > > Hi,

> > >

> > > I've noticed that this new test fails on arm/aarch64:

> > > FAIL:gfortran.dg/proc_ptr_51.f90   -O2  execution test

> > > FAIL:gfortran.dg/proc_ptr_51.f90   -O3 -fomit-frame-pointer

> > > -funroll-loops -fpeel-loops -ftracer -finline-functions  execution

> > > test

> > > FAIL:gfortran.dg/proc_ptr_51.f90   -O3 -g  execution test

> > >

> > > the logs say:

> > > Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

> > >

> > > Backtrace for this error:

> > > #0  0xffffa938f66b in ???

> > > #1  0x0 in ???

> > >

> > > Christophe

> > >

> > > > --

> > > >

> > > > * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

> > > >

> > > > * Galacticus: https://bitbucket.org/galacticusdev/galacticus

> > > >

> >

> >

> >

> > --

> > "If you can't explain it simply, you don't understand it well enough"

> > - Albert Einstein




-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein
Thomas Koenig June 10, 2019, 5:04 p.m. | #6
Hi Paul,

> I cannot see anything wrong with the optimized code and valgrind gives

> a clean bill of health on x86_64.

> 

> We need help of somebody with access to an arm/aarch64 device.


I'm currently running a bootstrap on an aarch64 machine.  These
are not known to be the fastest of machines, but it should
be done sometime today.

Regards

	Thomas

Patch

Index: gcc/fortran/trans-expr.c
===================================================================
*** gcc/fortran/trans-expr.c	(revision 272076)
--- gcc/fortran/trans-expr.c	(working copy)
*************** class_array_fcn:
*** 4881,4887 ****
      parmse->expr = gfc_build_addr_expr (NULL_TREE, parmse->expr);

    /* Basically make this into
!
       if (present)
         {
  	 if (contiguous)
--- 4881,4887 ----
      parmse->expr = gfc_build_addr_expr (NULL_TREE, parmse->expr);

    /* Basically make this into
!
       if (present)
         {
  	 if (contiguous)
*************** trans_caf_token_assign (gfc_se *lse, gfc
*** 8979,9001 ****
      }
  }

- /* Indentify class valued proc_pointer assignments.  */
-
- static bool
- pointer_assignment_is_proc_pointer (gfc_expr * expr1, gfc_expr * expr2)
- {
-   gfc_ref * ref;
-
-   ref = expr1->ref;
-   while (ref && ref->next)
-      ref = ref->next;
-
-   return ref && ref->type == REF_COMPONENT
-       && ref->u.c.component->attr.proc_pointer
-       && expr2->expr_type == EXPR_VARIABLE
-       && expr2->symtree->n.sym->attr.flavor == FL_PROCEDURE;
- }
-

  /* Do everything that is needed for a CLASS function expr2.  */

--- 8979,8984 ----
*************** gfc_trans_pointer_assignment (gfc_expr *
*** 9048,9054 ****
    tree desc;
    tree tmp;
    tree expr1_vptr = NULL_TREE;
!   bool scalar, non_proc_pointer_assign;
    gfc_ss *ss;

    gfc_start_block (&block);
--- 9031,9037 ----
    tree desc;
    tree tmp;
    tree expr1_vptr = NULL_TREE;
!   bool scalar, non_proc_ptr_assign;
    gfc_ss *ss;

    gfc_start_block (&block);
*************** gfc_trans_pointer_assignment (gfc_expr *
*** 9056,9062 ****
    gfc_init_se (&lse, NULL);

    /* Usually testing whether this is not a proc pointer assignment.  */
!   non_proc_pointer_assign = !pointer_assignment_is_proc_pointer (expr1, expr2);

    /* Check whether the expression is a scalar or not; we cannot use
       expr1->rank as it can be nonzero for proc pointers.  */
--- 9039,9047 ----
    gfc_init_se (&lse, NULL);

    /* Usually testing whether this is not a proc pointer assignment.  */
!   non_proc_ptr_assign = !(gfc_expr_attr (expr1).proc_pointer
! 			&& expr2->expr_type == EXPR_VARIABLE
! 			&& expr2->symtree->n.sym->attr.flavor == FL_PROCEDURE);

    /* Check whether the expression is a scalar or not; we cannot use
       expr1->rank as it can be nonzero for proc pointers.  */
*************** gfc_trans_pointer_assignment (gfc_expr *
*** 9066,9072 ****
      gfc_free_ss_chain (ss);

    if (expr1->ts.type == BT_DERIVED && expr2->ts.type == BT_CLASS
!       && expr2->expr_type != EXPR_FUNCTION && non_proc_pointer_assign)
      {
        gfc_add_data_component (expr2);
        /* The following is required as gfc_add_data_component doesn't
--- 9051,9057 ----
      gfc_free_ss_chain (ss);

    if (expr1->ts.type == BT_DERIVED && expr2->ts.type == BT_CLASS
!       && expr2->expr_type != EXPR_FUNCTION && non_proc_ptr_assign)
      {
        gfc_add_data_component (expr2);
        /* The following is required as gfc_add_data_component doesn't
*************** gfc_trans_pointer_assignment (gfc_expr *
*** 9086,9092 ****
        else
  	gfc_conv_expr (&rse, expr2);

!       if (non_proc_pointer_assign && expr1->ts.type == BT_CLASS)
  	{
  	  trans_class_vptr_len_assignment (&block, expr1, expr2, &rse, NULL,
  					   NULL);
--- 9071,9077 ----
        else
  	gfc_conv_expr (&rse, expr2);

!       if (non_proc_ptr_assign && expr1->ts.type == BT_CLASS)
  	{
  	  trans_class_vptr_len_assignment (&block, expr1, expr2, &rse, NULL,
  					   NULL);
Index: gcc/testsuite/gfortran.dg/proc_ptr_51.f90
===================================================================
*** gcc/testsuite/gfortran.dg/proc_ptr_51.f90	(nonexistent)
--- gcc/testsuite/gfortran.dg/proc_ptr_51.f90	(working copy)
***************
*** 0 ****
--- 1,38 ----
+ ! { dg-do run }
+ !
+ ! Test the fix for PR90786.
+ !
+ ! Contributed by Andrew benson  <abensonca@gmail.com>
+ !
+ module f
+ procedure(c), pointer :: c_
+
+  type :: s
+    integer :: i = 42
+  end type s
+  class(s), pointer :: res, tgt
+
+ contains
+
+  function c()
+    implicit none
+    class(s), pointer ::  c
+    c => tgt
+    return
+  end function c
+
+  subroutine fs()
+    implicit none
+    c_ => c  ! This used to ICE
+    return
+  end subroutine fs
+
+ end module f
+
+   use f
+   allocate (tgt, source = s(99))
+   call fs()
+   res => c_()
+   if (res%i .ne. 99) stop 1
+   deallocate (tgt)
+ end