Bug 87566 - ICE with class(*) and select
Summary: ICE with class(*) and select
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 8.2.0
: P4 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: ice-on-valid-code
Depends on:
Blocks:
 
Reported: 2018-10-09 14:31 UTC by Antony Lewis
Modified: 2019-03-06 20:45 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-10-09 00:00:00


Attachments
Full test case (156 bytes, text/plain)
2018-10-09 14:31 UTC, Antony Lewis
Details
Partial draft patch (435 bytes, patch)
2018-10-12 20:40 UTC, Tobias Burnus
Details | Diff
Fix for the PR (686 bytes, patch)
2018-10-13 13:34 UTC, Paul Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antony Lewis 2018-10-09 14:31:58 UTC
Created attachment 44818 [details]
Full test case

Segmentation fault ICE compiling with 6.4. 7.3 or 8.2.0.

    
    subroutine AddArray()
    type Object_array_pointer
        class(*), pointer :: p(:) => null()
    end type Object_array_pointer
    class(*), pointer :: Pt => null()

    select type (Pt)
    class is (object_array_pointer)
        select type (Point=> Pt%P)
        end select
    end select

    end subroutine AddArray
Comment 1 Dominique d'Humieres 2018-10-09 16:33:46 UTC
Confirmed from 4.8 up to trunk (9.0). An instrumented compiler gives

../../work/gcc/fortran/resolve.c:8883:21: runtime error: member access within null pointer of type 'struct gfc_component'
Comment 2 Tobias Burnus 2018-10-12 20:40:20 UTC
Created attachment 44831 [details]
Partial draft patch

If I apply the attached patch (no idea whether it really makes sense, CLASS is quite confusing!), it converts to gimple but then fails because of the following.

For some odd reasons the gimplfier does not like that f951 assigns a value to the digit 0 (last but one line of the "finally") ...
[Otherwise, the code looks fine to me.]

  struct __class__STAR_1_0p * point;
  struct __class__STAR_1_0p class.0;
     class.0._data = __tmp_class_object_array_pointer->_data->p._data;
     class.0._vptr = __tmp_class_object_array_pointer->_data->p._vptr;
...
     point = &class.0;
...
 finally
   {
     __tmp_class_object_array_pointer->_data->p._data = class.0._data;
     __tmp_class_object_array_pointer->_data->p._vptr = class.0._vptr;
     0 = (unsigned long) class.0._len;
     __tmp_class_object_array_pointer->_data->p._len = class.0._len;
Comment 3 Tobias Burnus 2018-10-12 21:07:51 UTC
@Paul: Some guidance is welcome!

(In reply to Tobias Burnus from comment #2)
> For some odd reasons the gimplfier does not like that f951 assigns a value
> to the digit 0 (last but one line of the "finally") ...

That's in trans-expr.c's gfc_conv_class_to_class():

      if (UNLIMITED_POLY (e))
        tmp = gfc_class_len_get (tmp);
      else if (e->ts.type == BT_CHARACTER)
          tmp = slen;
      else
        tmp = build_zero_cst (size_type_node);
      gfc_add_modify (&parmse->pre, ctree,
                      fold_convert (TREE_TYPE (ctree), tmp));

OK so far – now comes the finally part:

      if (!elemental && full_array && copyback)
          gfc_add_modify (&parmse->post, tmp,
                          fold_convert (TREE_TYPE (tmp), ctree));

And here we have assigned (if the "else" branch for tmp branch was taken) a value to the build_zero_cst()!


Actually, I wonder whether the _len = 0 is needed. The current code is:

                class.0._len = 0;
                class.0._data = __tmp_class_object_array_pointer->_data->p._data;
                class.0._vptr = __tmp_class_object_array_pointer->_data->p._vptr;
                class.0._len = __tmp_class_object_array_pointer->_data->p._len;

i.e. we set _len twice. And also in finally, we have it also twice:

     0 = (unsigned long) class.0._len;
     __tmp_class_object_array_pointer->_data->p._len = class.0._len;

As band-aid the following works (on top of the patch from attachment 44831 [details])

--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -1133,3 +1133,3 @@ gfc_conv_class_to_class (gfc_se *parmse, gfc_expr *e, gfc_typespec class_ts,
        references, where the dynamic type cannot change.  */
-      if (!elemental && full_array && copyback)
+      if (!elemental && full_array && copyback && !INTEGER_CST_P (tmp))
          gfc_add_modify (&parmse->post, tmp,
Comment 4 paul.richard.thomas@gmail.com 2018-10-13 06:45:16 UTC
Hi Tobias,

You are grappling with exactly the error that I am grappling with in
backporting my deferred character patches to 8-branch. The problem is
the following and it is specific to deferred character components:

1) The string length of these components is stored in a hidden
component, which gfc_conv_expr and gfc_conv_expr_descriptor
successfully return as the string length.
2) Unfortunately, we are forced in the field decl for these components
to make them zero length (NULL fails for reasons that I simply do not
understand). This means that the ts.u.backend_decl is also constant
zero and so must not be assigned to.
3) What I have been doing is to avoid assigning to the backend_decl by
applying a guard that it is not integer_zerop and instead, looking for
TREE_CODE (se->string_length) == COMPONENT_REF and assigning to that.

I have to run Jan to the eye hospital this morning for an injection
(the final one). When I get home, I will take a look at your partial
patch and try to identify the spot where this happens.

It's good to have you back. Thomas and I were getting rather lonely!

Regards

Paul

On Fri, 12 Oct 2018 at 22:07, burnus at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87566
>
> --- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
> @Paul: Some guidance is welcome!
>
> (In reply to Tobias Burnus from comment #2)
> > For some odd reasons the gimplfier does not like that f951 assigns a value
> > to the digit 0 (last but one line of the "finally") ...
>
> That's in trans-expr.c's gfc_conv_class_to_class():
>
>       if (UNLIMITED_POLY (e))
>         tmp = gfc_class_len_get (tmp);
>       else if (e->ts.type == BT_CHARACTER)
>           tmp = slen;
>       else
>         tmp = build_zero_cst (size_type_node);
>       gfc_add_modify (&parmse->pre, ctree,
>                       fold_convert (TREE_TYPE (ctree), tmp));
>
> OK so far – now comes the finally part:
>
>       if (!elemental && full_array && copyback)
>           gfc_add_modify (&parmse->post, tmp,
>                           fold_convert (TREE_TYPE (tmp), ctree));
>
> And here we have assigned (if the "else" branch for tmp branch was taken) a
> value to the build_zero_cst()!
>
>
> Actually, I wonder whether the _len = 0 is needed. The current code is:
>
>                 class.0._len = 0;
>                 class.0._data =
> __tmp_class_object_array_pointer->_data->p._data;
>                 class.0._vptr =
> __tmp_class_object_array_pointer->_data->p._vptr;
>                 class.0._len = __tmp_class_object_array_pointer->_data->p._len;
>
> i.e. we set _len twice. And also in finally, we have it also twice:
>
>      0 = (unsigned long) class.0._len;
>      __tmp_class_object_array_pointer->_data->p._len = class.0._len;
>
> As band-aid the following works (on top of the patch from attachment 44831 [details])
>
> --- a/gcc/fortran/trans-expr.c
> +++ b/gcc/fortran/trans-expr.c
> @@ -1133,3 +1133,3 @@ gfc_conv_class_to_class (gfc_se *parmse, gfc_expr *e,
> gfc_typespec class_ts,
>         references, where the dynamic type cannot change.  */
> -      if (!elemental && full_array && copyback)
> +      if (!elemental && full_array && copyback && !INTEGER_CST_P (tmp))
>           gfc_add_modify (&parmse->post, tmp,
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 5 paul.richard.thomas@gmail.com 2018-10-13 06:49:23 UTC
Sorry, forget that last. I got out on the wrong side of the bed I
think. I will take a proper look later.

Cheers

Paul

On Sat, 13 Oct 2018 at 07:45, paul.richard.thomas at gmail dot com
<gcc-bugzilla@gcc.gnu.org> wrote:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87566
>
> --- Comment #4 from paul.richard.thomas at gmail dot com <paul.richard.thomas at gmail dot com> ---
> Hi Tobias,
>
> You are grappling with exactly the error that I am grappling with in
> backporting my deferred character patches to 8-branch. The problem is
> the following and it is specific to deferred character components:
>
> 1) The string length of these components is stored in a hidden
> component, which gfc_conv_expr and gfc_conv_expr_descriptor
> successfully return as the string length.
> 2) Unfortunately, we are forced in the field decl for these components
> to make them zero length (NULL fails for reasons that I simply do not
> understand). This means that the ts.u.backend_decl is also constant
> zero and so must not be assigned to.
> 3) What I have been doing is to avoid assigning to the backend_decl by
> applying a guard that it is not integer_zerop and instead, looking for
> TREE_CODE (se->string_length) == COMPONENT_REF and assigning to that.
>
> I have to run Jan to the eye hospital this morning for an injection
> (the final one). When I get home, I will take a look at your partial
> patch and try to identify the spot where this happens.
>
> It's good to have you back. Thomas and I were getting rather lonely!
>
> Regards
>
> Paul
>
> On Fri, 12 Oct 2018 at 22:07, burnus at gcc dot gnu.org
> <gcc-bugzilla@gcc.gnu.org> wrote:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87566
> >
> > --- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
> > @Paul: Some guidance is welcome!
> >
> > (In reply to Tobias Burnus from comment #2)
> > > For some odd reasons the gimplfier does not like that f951 assigns a value
> > > to the digit 0 (last but one line of the "finally") ...
> >
> > That's in trans-expr.c's gfc_conv_class_to_class():
> >
> >       if (UNLIMITED_POLY (e))
> >         tmp = gfc_class_len_get (tmp);
> >       else if (e->ts.type == BT_CHARACTER)
> >           tmp = slen;
> >       else
> >         tmp = build_zero_cst (size_type_node);
> >       gfc_add_modify (&parmse->pre, ctree,
> >                       fold_convert (TREE_TYPE (ctree), tmp));
> >
> > OK so far – now comes the finally part:
> >
> >       if (!elemental && full_array && copyback)
> >           gfc_add_modify (&parmse->post, tmp,
> >                           fold_convert (TREE_TYPE (tmp), ctree));
> >
> > And here we have assigned (if the "else" branch for tmp branch was taken) a
> > value to the build_zero_cst()!
> >
> >
> > Actually, I wonder whether the _len = 0 is needed. The current code is:
> >
> >                 class.0._len = 0;
> >                 class.0._data =
> > __tmp_class_object_array_pointer->_data->p._data;
> >                 class.0._vptr =
> > __tmp_class_object_array_pointer->_data->p._vptr;
> >                 class.0._len = __tmp_class_object_array_pointer->_data->p._len;
> >
> > i.e. we set _len twice. And also in finally, we have it also twice:
> >
> >      0 = (unsigned long) class.0._len;
> >      __tmp_class_object_array_pointer->_data->p._len = class.0._len;
> >
> > As band-aid the following works (on top of the patch from attachment 44831 [details])
> >
> > --- a/gcc/fortran/trans-expr.c
> > +++ b/gcc/fortran/trans-expr.c
> > @@ -1133,3 +1133,3 @@ gfc_conv_class_to_class (gfc_se *parmse, gfc_expr *e,
> > gfc_typespec class_ts,
> >         references, where the dynamic type cannot change.  */
> > -      if (!elemental && full_array && copyback)
> > +      if (!elemental && full_array && copyback && !INTEGER_CST_P (tmp))
> >           gfc_add_modify (&parmse->post, tmp,
> >
> > --
> > You are receiving this mail because:
> > You are on the CC list for the bug.
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 6 Paul Thomas 2018-10-13 13:34:54 UTC
Created attachment 44835 [details]
Fix for the PR

Hi Tobias,

The problem that you found occurs in trans-expr.c (gfc_conv_class_to_class). Once found, the fix was trivial (See the attachment) and the testcase below compiles and executes correctly.

The call to gfc_conv_class_to_class is made at trans-stmt.c:1822. The argument 'copy_back' is set true. However, the copyback is made to the select type temporary, rather than to 'Pt'. Therefore, the assignment works but pointing to a new target does not. Setting 'copy_back' to false regtests OK but I suspect that it should break the associate construct for some cases.

That said, to my surprise, this causes an ICE:
    call AddArray
contains
    subroutine AddArray()
    type Object_array_pointer
        class(*), pointer :: p(:) => null()
    end type Object_array_pointer

    type (Object_array_pointer) :: obj
    character(3), target :: tgt1(2) = ['one','two']
    character(5), target :: tgt2(2) = ['three','four ']

    obj%p => tgt1
    associate (point => obj%p)
    end associate

    end subroutine AddArray

end

However, your patch in resolve.c caused a good number of regressions and so I put that right too.

Over to you!

Paul

    call AddArray
contains
    subroutine AddArray()
    type Object_array_pointer
        class(*), pointer :: p(:) => null()
    end type Object_array_pointer
    class(*), pointer :: Pt => null()
    character(3), target :: tgt1(2) = ['one','two']

    allocate (Pt, source = Object_array_pointer ())
    select type (Pt)
      type is (object_array_pointer)
        Pt%p => tgt1
    end select

    select type (Pt)
    class is (object_array_pointer)
        select type (Point=> Pt%P)
          type is (character(*))
            if (any (Point .ne. tgt1)) stop 1
            Point = ['abc','efg']
        end select
    end select

    select type (Pt)
    class is (object_array_pointer)
        select type (Point=> Pt%P)
          type is (character(*))
            if (any (Point .ne. ['abc','efg'])) stop 2
        end select
    end select

    end subroutine AddArray

end
Comment 7 Paul Thomas 2018-10-15 16:31:50 UTC
Author: pault
Date: Mon Oct 15 16:31:15 2018
New Revision: 265171

URL: https://gcc.gnu.org/viewcvs?rev=265171&root=gcc&view=rev
Log:
2018-10-15  Paul Thomas  <pault@gcc.gnu.org>
	    Tobias Burnus  <burnus@gcc.gnu.org>

	PR fortran/87566
	* resolve.c (resolve_assoc_var): Add missing array spec for
	class associate names.
	(resolve_select_type): Handle case where last typed component
	of the selector has a different type to the expression.
	* trans-expr.c (gfc_find_and_cut_at_last_class_ref): Replace
	call to gfc_expr_to_initialize with call to gfc_copy_expr.
	(gfc_conv_class_to_class): Guard assignment to 'len' field
	against case where zero constant is supplied.

2018-10-15  Paul Thomas  <pault@gcc.gnu.org>
	    Tobias Burnus  <burnus@gcc.gnu.org>

	PR fortran/87566
	* gfortran.dg/select_type_44.f90: New test.
	* gfortran.dg/associate_42.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/associate_42.f90
    trunk/gcc/testsuite/gfortran.dg/select_type_44.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/resolve.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog
Comment 8 Paul Thomas 2018-10-15 16:56:01 UTC
Fixed on trunk.

I am going back on my original intention to backport the recent patches to 8-branch. Or, rather, I will do them one at a time if at all. The trouble is that an omnibus patch doesn't work for all the new testcases because of some earlier patches applied to trunk.

I will keep this open in the hope that a backport will work but do not hold your breath.

Best regards

Paul
Comment 9 Paul Thomas 2018-10-19 14:04:25 UTC
If I don't take it, I will lose it!

Cheers

Paul
Comment 10 Antony Lewis 2019-01-23 12:01:09 UTC
In the latest 9.0 trunk I still see what looks like a similar ICE error (though I have not tried to isolate it again). See

https://travis-ci.org/cmbant/forutils/builds/483365115

when running test script in https://github.com/cmbant/forutils/tree/gcc_bug1 .
(code is reported invalid in gcc6, but SEG faults in 7,8,9).
Comment 11 Antony Lewis 2019-01-25 23:58:51 UTC
I posted remaining ICE in 9.0.0 20190119 as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89069
Comment 12 Neil Carlson 2019-02-23 21:16:10 UTC
The commit r265171 that fixed this issue also introduced a regression in 8.2 and 9, and certainly the 7 branch too if it was back-ported to it. See PR89174 for the example, which had worked correctly for 7, 8, and 9, but which now segfaults on at least 8 and 9. I suppose it's too late revert that patch ...
Comment 13 Paul Thomas 2019-02-24 10:50:39 UTC
(In reply to Neil Carlson from comment #12)
> The commit r265171 that fixed this issue also introduced a regression in 8.2
> and 9, and certainly the 7 branch too if it was back-ported to it. See
> PR89174 for the example, which had worked correctly for 7, 8, and 9, but
> which now segfaults on at least 8 and 9. I suppose it's too late revert that
> patch ...

Hi Neil,

As I said in comment #8, I decided that backporting was not going to work because there were too many antecedent patches.

I just checked through the ChangeLog for 8-branch and there is no sign of anybody else attempting the fix.

I'll take a look at 89174.

Thanks

Paul
Comment 14 Thomas Koenig 2019-03-06 20:45:14 UTC
(In reply to Paul Thomas from comment #13)

> As I said in comment #8, I decided that backporting was not going to work
> because there were too many antecedent patches.

So, let's close this one as FIXED.

We cannot backport all fixes to all branches, or else all
gfortran versions would be the same :-)