[Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Janus Weil]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

Hi all,

here is another rather simple patch for an ice-on-invalid problem. The
patch consists of three hunks, out of which the last two actually fix
two ICEs that occurred on the different test cases: The second one
removes an assert which seems unnecessary to me (since there are
anyway other cases where tb becomes NULL), and the third checks
whether the symtree exists already before creating a new one.

The first hunk is just cosmetics / code simplification, using
gfc_get_tbp_symtree instead of the (equivalent) combination of
gfc_find_symtree and gfc_new_symtree.

[Btw, speaking of gfc_get_tbp_symtree: Can anyone tell me by chance
why it is necessary to nullify 'result->n.tb' on a newly-created
symtree?]

The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?

Cheers,
Janus



2016-11-11  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/77501
    * decl.c (gfc_match_decl_type_spec): Use gfc_get_tbp_symtree,
    fix indentation.
    (gfc_match_generic): Remove an unnecessary assert.
    Use gfc_get_tbp_symtree to avoid ICE.

2016-11-11  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/77501
    * gfortran.dg/typebound_generic_16.f90: New test.

[-- Attachment #2: pr77501_v2.diff --]
[-- Type: text/plain, Size: 1410 bytes --]

Index: gcc/fortran/decl.c
===================================================================
--- gcc/fortran/decl.c	(Revision 242066)
+++ gcc/fortran/decl.c	(Arbeitskopie)
@@ -3198,13 +3198,11 @@ gfc_match_decl_type_spec (gfc_typespec *ts, int im
 	      upe->attr.zero_comp = 1;
 	      if (!gfc_add_flavor (&upe->attr, FL_DERIVED, NULL,
 				   &gfc_current_locus))
-	  return MATCH_ERROR;
-	}
+	      return MATCH_ERROR;
+	    }
 	  else
 	    {
-	      st = gfc_find_symtree (gfc_current_ns->sym_root, "STAR");
-	      if (st == NULL)
-		st = gfc_new_symtree (&gfc_current_ns->sym_root, "STAR");
+	      st = gfc_get_tbp_symtree (&gfc_current_ns->sym_root, "STAR");
 	      st->n.sym = upe;
 	      upe->refs++;
 	    }
@@ -9731,14 +9729,7 @@ gfc_match_generic (void)
 	gfc_symtree* st;
 
 	st = gfc_find_symtree (is_op ? ns->tb_uop_root : ns->tb_sym_root, name);
-	if (st)
-	  {
-	    tb = st->n.tb;
-	    gcc_assert (tb);
-	  }
-	else
-	  tb = NULL;
-
+	tb = st ? st->n.tb : NULL;
 	break;
       }
 
@@ -9783,10 +9774,8 @@ gfc_match_generic (void)
 	case INTERFACE_USER_OP:
 	  {
 	    const bool is_op = (op_type == INTERFACE_USER_OP);
-	    gfc_symtree* st;
-
-	    st = gfc_new_symtree (is_op ? &ns->tb_uop_root : &ns->tb_sym_root,
-				  name);
+	    gfc_symtree* st = gfc_get_tbp_symtree (is_op ? &ns->tb_uop_root :
+						   &ns->tb_sym_root, name);
 	    gcc_assert (st);
 	    st->n.tb = tb;
 

[-- Attachment #3: typebound_generic_16.f90 --]
[-- Type: text/x-fortran, Size: 534 bytes --]

! { dg-do compile }
!
! PR 77501: [F03] ICE in gfc_match_generic, at fortran/decl.c:9429
!
! Contributed by Gerhard Steinmetz <gerhard.steinmetz.fortran@t-online.de>

module m1
  type t
  contains
    generic :: f => g  ! { dg-error "must target a specific binding" }
    generic :: g => h  ! { dg-error "Undefined specific binding" }
  end type
end

module m2
  type t
  contains
    generic :: f => g  ! { dg-error "must target a specific binding" }
    generic :: g => f  ! { dg-error "Undefined specific binding" }
  end type
end

Re: [Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Janus Weil]

2016-11-11 14:38 GMT+01:00 Janus Weil <janus@gcc.gnu.org>:
> [Btw, speaking of gfc_get_tbp_symtree: Can anyone tell me by chance
> why it is necessary to nullify 'result->n.tb' on a newly-created
> symtree?]

Removing the corresponding line does not do any harm to the testsuite,
as I just verified:

Index: gcc/fortran/class.c
===================================================================
--- gcc/fortran/class.c    (Revision 242066)
+++ gcc/fortran/class.c    (Arbeitskopie)
@@ -2970,7 +2970,6 @@ gfc_get_tbp_symtree (gfc_symtree **root, const cha
     {
       result = gfc_new_symtree (root, name);
       gcc_assert (result);
-      result->n.tb = NULL;
     }

   return result;


After all, n.tb should anyway be NULL in a symtree we just created,
right? So is it ok to remove that?

Cheers,
Janus

Re: [Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Steve Kargl]

On Fri, Nov 11, 2016 at 02:38:25PM +0100, Janus Weil wrote:
> 
> 2016-11-11  Janus Weil  <janus@gcc.gnu.org>
> 
>     PR fortran/77501
>     * decl.c (gfc_match_decl_type_spec): Use gfc_get_tbp_symtree,
>     fix indentation.
>     (gfc_match_generic): Remove an unnecessary assert.
>     Use gfc_get_tbp_symtree to avoid ICE.
> 
> 2016-11-11  Janus Weil  <janus@gcc.gnu.org>
> 
>     PR fortran/77501
>     * gfortran.dg/typebound_generic_16.f90: New test.

OK.

-- 
Steve

Re: [Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Steve Kargl]

On Fri, Nov 11, 2016 at 03:05:06PM +0100, Janus Weil wrote:
> 2016-11-11 14:38 GMT+01:00 Janus Weil <janus@gcc.gnu.org>:
> > [Btw, speaking of gfc_get_tbp_symtree: Can anyone tell me by chance
> > why it is necessary to nullify 'result->n.tb' on a newly-created
> > symtree?]
> 
> Removing the corresponding line does not do any harm to the testsuite,
> as I just verified:
> 
> Index: gcc/fortran/class.c
> ===================================================================
> --- gcc/fortran/class.c    (Revision 242066)
> +++ gcc/fortran/class.c    (Arbeitskopie)
> @@ -2970,7 +2970,6 @@ gfc_get_tbp_symtree (gfc_symtree **root, const cha
>      {
>        result = gfc_new_symtree (root, name);
>        gcc_assert (result);
> -      result->n.tb = NULL;
>      }
> 
>    return result;
> 

I think the assert can be removed as well.  gfc_new_symtree
is defined by XCNEW, which is defined in terms of xcalloc,
which is defined in libiberty/xmalloc.c in terms of calloc.
calloc zeros allocated memory.  xcalloc also checks for a
valid allocation, so gcc_assert is redundant.


-- 
Steve

Re: [Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Janus Weil]

2016-11-11 19:21 GMT+01:00 Steve Kargl <sgk@troutmask.apl.washington.edu>:
> On Fri, Nov 11, 2016 at 02:38:25PM +0100, Janus Weil wrote:
>>
>> 2016-11-11  Janus Weil  <janus@gcc.gnu.org>
>>
>>     PR fortran/77501
>>     * decl.c (gfc_match_decl_type_spec): Use gfc_get_tbp_symtree,
>>     fix indentation.
>>     (gfc_match_generic): Remove an unnecessary assert.
>>     Use gfc_get_tbp_symtree to avoid ICE.
>>
>> 2016-11-11  Janus Weil  <janus@gcc.gnu.org>
>>
>>     PR fortran/77501
>>     * gfortran.dg/typebound_generic_16.f90: New test.
>
> OK.

Thanks Steve, committed as r242335 (together with the additional
changes to gfc_get_tbp_symtree).

Cheers,
Janus

Re: [Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Mikael Morin]

Le 11/11/2016 à 19:30, Steve Kargl a écrit :
> On Fri, Nov 11, 2016 at 03:05:06PM +0100, Janus Weil wrote:
>> 2016-11-11 14:38 GMT+01:00 Janus Weil <janus@gcc.gnu.org>:
>>> [Btw, speaking of gfc_get_tbp_symtree: Can anyone tell me by chance
>>> why it is necessary to nullify 'result->n.tb' on a newly-created
>>> symtree?]
>>
>> Removing the corresponding line does not do any harm to the testsuite,
>> as I just verified:
>>
>> Index: gcc/fortran/class.c
>> ===================================================================
>> --- gcc/fortran/class.c    (Revision 242066)
>> +++ gcc/fortran/class.c    (Arbeitskopie)
>> @@ -2970,7 +2970,6 @@ gfc_get_tbp_symtree (gfc_symtree **root, const cha
>>      {
>>        result = gfc_new_symtree (root, name);
>>        gcc_assert (result);
>> -      result->n.tb = NULL;
>>      }
>>
>>    return result;
>>
>
> I think the assert can be removed as well.  gfc_new_symtree
> is defined by XCNEW, which is defined in terms of xcalloc,
> which is defined in libiberty/xmalloc.c in terms of calloc.
> calloc zeros allocated memory.  xcalloc also checks for a
> valid allocation, so gcc_assert is redundant.
>
>
And you can remove «tbp» from the function name, as there is nothing 
related to typebound procedures any more.

Re: [Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Janus Weil]

2016-11-12 20:15 GMT+01:00 Mikael Morin <morin-mikael@orange.fr>:
>>>> [Btw, speaking of gfc_get_tbp_symtree: Can anyone tell me by chance
>>>> why it is necessary to nullify 'result->n.tb' on a newly-created
>>>> symtree?]
>>>
>>>
>>> Removing the corresponding line does not do any harm to the testsuite,
>>> as I just verified:
>>>
>>> Index: gcc/fortran/class.c
>>> ===================================================================
>>> --- gcc/fortran/class.c    (Revision 242066)
>>> +++ gcc/fortran/class.c    (Arbeitskopie)
>>> @@ -2970,7 +2970,6 @@ gfc_get_tbp_symtree (gfc_symtree **root, const cha
>>>      {
>>>        result = gfc_new_symtree (root, name);
>>>        gcc_assert (result);
>>> -      result->n.tb = NULL;
>>>      }
>>>
>>>    return result;
>>>
>>
>> I think the assert can be removed as well.  gfc_new_symtree
>> is defined by XCNEW, which is defined in terms of xcalloc,
>> which is defined in libiberty/xmalloc.c in terms of calloc.
>> calloc zeros allocated memory.  xcalloc also checks for a
>> valid allocation, so gcc_assert is redundant.
>>
>>
> And you can remove «tbp» from the function name, as there is nothing related
> to typebound procedures any more.

True. Probably one should also move it to symbol.c.

However, one can wonder whether renaming to gfc_get_symtree would not
make the name too similar to gfc_get_sym_tree, which also lives in
symbol.c ...?

Cheers,
Janus

Re: [Patch, Fortran, F03] PR 77501: ICE in gfc_match_generic, at fortran/decl.c:9429

[Janus Weil]

2016-11-12 21:21 GMT+01:00 Janus Weil <janus@gcc.gnu.org>:
>>>> Index: gcc/fortran/class.c
>>>> ===================================================================
>>>> --- gcc/fortran/class.c    (Revision 242066)
>>>> +++ gcc/fortran/class.c    (Arbeitskopie)
>>>> @@ -2970,7 +2970,6 @@ gfc_get_tbp_symtree (gfc_symtree **root, const cha
>>>>      {
>>>>        result = gfc_new_symtree (root, name);
>>>>        gcc_assert (result);
>>>> -      result->n.tb = NULL;
>>>>      }
>>>>
>>>>    return result;
>>>>
>>>
>>> I think the assert can be removed as well.  gfc_new_symtree
>>> is defined by XCNEW, which is defined in terms of xcalloc,
>>> which is defined in libiberty/xmalloc.c in terms of calloc.
>>> calloc zeros allocated memory.  xcalloc also checks for a
>>> valid allocation, so gcc_assert is redundant.
>>>
>>>
>> And you can remove «tbp» from the function name, as there is nothing related
>> to typebound procedures any more.
>
> True. Probably one should also move it to symbol.c.
>
> However, one can wonder whether renaming to gfc_get_symtree would not
> make the name too similar to gfc_get_sym_tree, which also lives in
> symbol.c ...?

I have just opened PR 78377 to track this:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78377

Cheers,
Janus