pr59016

pr59016

[Evangelos Drikos]

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


Hi,

The attached patch, type 0, has been discussed a little at:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59016

Yet, the final version submitted is slightly different from the ones discussed and tested in the above link.

Having read the GNU Coding Conventions, I created a patch using svn diff (“-up”) and I added a small test case.

Further, I've run the “check_GNU_style.sh” script to ensure that the source code meets the GNU style requirements. 

Also, I don’t have DejaGNU installed and thus I think that I cannot run all the tests. In other words, evaluation is up to the GNU team.

Finally, it might be obvious that the patch is for trunk (not gcc-4.9.2).

Regards,
Ev. Drikos 




[-- Attachment #2: gcc-5.0-pr59016.c_log --]
[-- Type: application/octet-stream, Size: 604 bytes --]

gcc/fortran/ChangeLog:

2015-04-06  Ev. Drikoss  <drikosev@otenet.gr>

	PR fortran/59016
	* decl.c (gfc_match_decl_type_spec): save old
	generic values in two additional arguments.
	(gfc_match_implicit): pass two more aguments.
	(gfc_match_data_decl): pass two more aguments;
	cleanup, if a declaration type spec is erroneous.
	(gfc_match_prefix): pass two more aguments.
	(match_procedure_interface): pass two more args.
	*match.h: add two args in function declaration.

gcc/testsuite/ChangeLog:

2015-04-06  Ev. Drikos  <drikosev@otenet.gr>

	PR fortran/59016
	* gfortran.dg/pr59016.f90:  New test.
	


[-- Attachment #3: gcc-5.0-pr59016.patch --]
[-- Type: application/octet-stream, Size: 5326 bytes --]

Index: gcc/fortran/decl.c
===================================================================
--- gcc/fortran/decl.c	(revision 221872)
+++ gcc/fortran/decl.c	(working copy)
@@ -2601,7 +2601,8 @@ done:
    statement correctly.  */
 
 match
-gfc_match_decl_type_spec (gfc_typespec *ts, int implicit_flag)
+gfc_match_decl_type_spec (gfc_typespec *ts, int implicit_flag,
+						  gfc_symbol** ifunlink,gfc_interface** old_generic)
 {
   char name[GFC_MAX_SYMBOL_LEN + 1];
   gfc_symbol *sym, *dt_sym;
@@ -2897,8 +2898,11 @@ gfc_match_decl_type_spec (gfc_typespec *
       intr->sym = dt_sym;
       intr->where = gfc_current_locus;
       intr->next = head;
+      *old_generic = sym->generic;
       sym->generic = intr;
       sym->attr.if_source = IFSRC_DECL;
+      *ifunlink=sym;
+
     }
 
   gfc_set_sym_referenced (dt_sym);
@@ -3167,7 +3171,8 @@ gfc_match_implicit (void)
       gfc_clear_new_implicit ();
 
       /* A basic type is mandatory here.  */
-      m = gfc_match_decl_type_spec (&ts, 1);
+      gfc_symbol *ifunlink=NULL; gfc_interface *old_generic=NULL;
+      m = gfc_match_decl_type_spec (&ts, 1, &ifunlink,&old_generic);
       if (m == MATCH_ERROR)
 	goto error;
       if (m == MATCH_NO)
@@ -4341,8 +4346,8 @@ gfc_match_data_decl (void)
   int elem;
 
   num_idents_on_line = 0;
-
-  m = gfc_match_decl_type_spec (&current_ts, 0);
+  gfc_symbol *ifunlink=NULL; gfc_interface *old_generic=NULL;
+  m = gfc_match_decl_type_spec (&current_ts, 0,&ifunlink,&old_generic);
   if (m != MATCH_YES)
     return m;
 
@@ -4427,6 +4432,40 @@ ok:
   gfc_free_data_all (gfc_current_ns);
 
 cleanup:
+  //<pr59016> in gfc_match_data_decl; cleanup the garbages
+  gfc_symbol *csym=NULL;
+  if ( (m==MATCH_ERROR)       //clean only if stmt not matched and
+  &&   (ifunlink!=NULL)     //the symbol was indeed linked in chain.
+  &&   (current_ts.u.derived &&
+		current_ts.u.derived->name))
+  {
+      const char *pname = current_ts.u.derived->name;
+      //In case the dt name is in title instead of lower case.
+      if ( current_ts.u.derived->name[0] !=
+		   TOLOWER (current_ts.u.derived->name[0]))
+	  {
+		  char iname[129]; iname[128]=0;
+		  for (int i=0; (i < 128);i++)
+		  {
+			  iname[i]=current_ts.u.derived->name[i];
+			  if (current_ts.u.derived->name[i]==0)
+				  break;
+		  }//for
+		  iname[0] = TOLOWER (iname[0]);
+		  pname    = iname ;
+	  }//if
+	  for (int i=0; i<4;i++) {  //try iface=0, 1, 2, and 3
+		  gfc_find_symbol (pname, NULL, i, &csym) ;
+		  if ( csym && csym->generic &&
+			 ( csym->generic->sym == current_ts.u.derived))
+		  {
+			  ifunlink->generic->next=csym->generic->next;	//remove from chain
+			  csym->generic = old_generic;					//restore old value
+			  break;
+		  }//if
+	  }//for
+  }//if
+  //</pr59016>
   gfc_free_array_spec (current_as);
   current_as = NULL;
   return m;
@@ -4455,9 +4494,10 @@ gfc_match_prefix (gfc_typespec *ts)
   do
     {
       found_prefix = false;
+      gfc_symbol *ifunlink=NULL; gfc_interface *old_generic=NULL;
 
       if (!seen_type && ts != NULL
-	  && gfc_match_decl_type_spec (ts, 0) == MATCH_YES
+	  && gfc_match_decl_type_spec (ts, 0,&ifunlink,&old_generic) == MATCH_YES
 	  && gfc_match_space () == MATCH_YES)
 	{
 
@@ -4888,7 +4928,8 @@ match_procedure_interface (gfc_symbol **
 
   /* Get the type spec. for the procedure interface.  */
   old_loc = gfc_current_locus;
-  m = gfc_match_decl_type_spec (&current_ts, 0);
+  gfc_symbol *ifunlink=NULL; gfc_interface *old_generic=NULL;
+  m = gfc_match_decl_type_spec (&current_ts, 0,&ifunlink,&old_generic);
   gfc_gobble_whitespace ();
   if (m == MATCH_YES || (m == MATCH_NO && gfc_peek_ascii_char () == ')'))
     goto got_ts;
Index: gcc/fortran/match.h
===================================================================
--- gcc/fortran/match.h	(revision 221872)
+++ gcc/fortran/match.h	(working copy)
@@ -193,7 +193,8 @@ match gfc_match_data (void);
 match gfc_match_null (gfc_expr **);
 match gfc_match_kind_spec (gfc_typespec *, bool);
 match gfc_match_old_kind_spec (gfc_typespec *);
-match gfc_match_decl_type_spec (gfc_typespec *, int);
+match gfc_match_decl_type_spec (gfc_typespec *,
+								int,gfc_symbol**,gfc_interface**);
 
 match gfc_match_end (gfc_statement *);
 match gfc_match_data_decl (void);
Index: gcc/testsuite/gfortran.dg/pr59016.f90
===================================================================
--- gcc/testsuite/gfortran.dg/pr59016.f90	(revision 0)
+++ gcc/testsuite/gfortran.dg/pr59016.f90	(working copy)
@@ -0,0 +1,23 @@
+! { dg-do compile }
+!
+! PR fortran/59016
+!
+! Check cleanup.
+!
+! Contributed by Evangelos Drikos.
+!
+
+MODULE atomic_kind_types
+  PUBLIC :: atomic_kind_type,&
+            dft_plus_u_type
+CONTAINS
+  SUBROUTINE get_atomic_kind_set(atomic_kind_set,maxatom,maxcgf,&
+       zetsoft_max,basis_set_id)
+       CALL stop_program(routineN,moduleN,__LINE__,&              ! { dg-error "Syntax error in argument list" }
+            "The pointer atomic_kind_set is not associated")
+  END SUBROUTINE get_atomic_kind_set
+  PURE FUNCTION is_hydrogen(atomic_kind) RESULT(res)              ! { dg-error "must be INTENT(IN) or VALUE" }
+    TYPE(atomic_kind_type), POINTER          :: atomic_kind       ! { dg-error "is being used before it is defined" }
+  END FUNCTION is_hydrogen
+END MODULE atomic_kind_types
+

Re: pr59016

[Mikael Morin]

Le 06/04/2015 01:04, Evangelos Drikos a écrit :
> 
> Hi,
> 
> The attached patch, type 0, has been discussed a little at:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59016
> 
> Yet, the final version submitted is slightly different from the ones discussed and tested in the above link.
> 
> Having read the GNU Coding Conventions, I created a patch using svn diff (“-up”) and I added a small test case.
> 
> Further, I've run the “check_GNU_style.sh” script to ensure that the source code meets the GNU style requirements. 
> 
> Also, I donÂ’t have DejaGNU installed and thus I think that I cannot run all the tests. In other words, evaluation is up to the GNU team.
> 
> Finally, it might be obvious that the patch is for trunk (not gcc-4.9.2).
> 
Hello,

first, have you signed the copyright assignment form?  See the legal
prerequisites section at https://gcc.gnu.org/contribute.html but I think
you have read that page already.

Regarding the patch, I don't understand why the existing symbol
restoration code doesn't work here (see
gfc_restore_last_undo_checkpoint, restore_old_symbol).  I have to
investigate more.  For now, I have made below a few comments about the
patch in its current state.

Most of the comments are purely syntactic: the code you insert should
have the same formatting as the rest of the code.

Mikael

PS: It's better if you manage to install DejaGNU, so that you can run
the testsuite yourself.  We are already short of manpower.


> Index: gcc/fortran/decl.c
> ===================================================================
> --- gcc/fortran/decl.c	(revision 221872)
> +++ gcc/fortran/decl.c	(working copy)
> @@ -4427,6 +4432,40 @@ ok:
>    gfc_free_data_all (gfc_current_ns);
>  
>  cleanup:
> +  //<pr59016> in gfc_match_data_decl; cleanup the garbages
No need to reference the PR, unless the code is so subtle that one has
to read the PR to understand it.
And don't produce code that subtle. ;-)

> +  gfc_symbol *csym=NULL;
> +  if ( (m==MATCH_ERROR)       //clean only if stmt not matched and
No extra parenthesis around the equality, single spaces around an
operator, no space inside parenthesis.

> +  &&   (ifunlink!=NULL)     //the symbol was indeed linked in chain.
Indent the operator the same as its left operand

> +  &&   (current_ts.u.derived &&
> +		current_ts.u.derived->name))
No extra parenthesis, operator at the beginning of the line, not at the end.

> +  {
indent the brace by two spaces wrt the preceding indentation.

> +      const char *pname = current_ts.u.derived->name;
> +      //In case the dt name is in title instead of lower case.
> +      if ( current_ts.u.derived->name[0] !=
> +		   TOLOWER (current_ts.u.derived->name[0]))
> +	  {
> +		  char iname[129]; iname[128]=0;
Indent the inner code two spaces more than the preceding indentation
(that of the brace).

> +		  for (int i=0; (i < 128);i++)
> +		  {
> +			  iname[i]=current_ts.u.derived->name[i];
> +			  if (current_ts.u.derived->name[i]==0)
> +				  break;
> +		  }//for
Comment useless.

> +		  iname[0] = TOLOWER (iname[0]);
> +		  pname    = iname ;
> +	  }//if
> +	  for (int i=0; i<4;i++) {  //try iface=0, 1, 2, and 3
This loop doesn't make sense; gfc_find_symbol's third argument shall be
either zero or non-zero, and it doesn't make sense to test both variants
in a row.

> +		  gfc_find_symbol (pname, NULL, i, &csym) ;
> +		  if ( csym && csym->generic &&
> +			 ( csym->generic->sym == current_ts.u.derived))
> +		  {
> +			  ifunlink->generic->next=csym->generic->next;	//remove from chain
> +			  csym->generic = old_generic;					//restore old value
> +			  break;
> +		  }//if
> +	  }//for
> +  }//if
> +  //</pr59016>
>    gfc_free_array_spec (current_as);
>    current_as = NULL;
>    return m;

Re: pr59016

[Mikael Morin]

Le 06/04/2015 20:26, Mikael Morin a écrit :
> Regarding the patch, I don't understand why the existing symbol
> restoration code doesn't work here (see
> gfc_restore_last_undo_checkpoint, restore_old_symbol).  I have to
> investigate more.

I think the problem is the usage of gfc_find_symbol in
gfc_match_decl_type_spec.
In opposition to the gfc_get_* family of functions, the gfc_find_*
functions don't version symbols, so that changes made to the symbol are
not thrown away when the statement is rejected.

Mikael

Re: pr59016

[Mikael Morin]

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

Le 07/04/2015 14:25, Mikael Morin a écrit :
> Le 06/04/2015 20:26, Mikael Morin a écrit :
>> Regarding the patch, I don't understand why the existing symbol
>> restoration code doesn't work here (see
>> gfc_restore_last_undo_checkpoint, restore_old_symbol).  I have to
>> investigate more.
> 
> I think the problem is the usage of gfc_find_symbol in
> gfc_match_decl_type_spec.
> In opposition to the gfc_get_* family of functions, the gfc_find_*
> functions don't version symbols, so that changes made to the symbol are
> not thrown away when the statement is rejected.
> 
So something like the following should be preferred over Evangelos' patch.
Except that the following ... ahem ... doesn't work.

Mikael


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pr59016_attempt.diff --]
[-- Type: text/x-patch; name="pr59016_attempt.diff", Size: 1906 bytes --]

Index: decl.c
===================================================================
--- decl.c	(révision 221654)
+++ decl.c	(copie de travail)
@@ -2840,7 +2840,7 @@ gfc_match_decl_type_spec (gfc_typespec *ts, int im
   if (ts->kind != -1)
     {
       gfc_get_ha_symbol (name, &sym);
-      if (sym->generic && gfc_find_symbol (dt_name, NULL, 0, &dt_sym))
+      if (sym->generic && gfc_get_symbol (dt_name, NULL, &dt_sym))
 	{
 	  gfc_error ("Type name %qs at %C is ambiguous", name);
 	  return MATCH_ERROR;
@@ -2850,10 +2850,11 @@ gfc_match_decl_type_spec (gfc_typespec *ts, int im
     }
   else if (ts->kind == -1)
     {
-      int iface = gfc_state_stack->previous->state != COMP_INTERFACE
-		    || gfc_current_ns->has_import_set;
-      gfc_find_symbol (name, NULL, iface, &sym);
-      if (sym && sym->generic && gfc_find_symbol (dt_name, NULL, 1, &dt_sym))
+      gfc_get_ha_symbol (name, &sym);
+      if (sym == NULL || sym->gfc_new)
+	return MATCH_NO;
+
+      if (sym && sym->generic && gfc_get_ha_symbol (dt_name, &dt_sym))
 	{
 	  gfc_error ("Type name %qs at %C is ambiguous", name);
 	  return MATCH_ERROR;
@@ -2862,8 +2863,6 @@ gfc_match_decl_type_spec (gfc_typespec *ts, int im
 	dt_sym = gfc_find_dt_in_generic (sym);
 
       ts->kind = 0;
-      if (sym == NULL)
-	return MATCH_NO;
     }
 
   if ((sym->attr.flavor != FL_UNKNOWN
@@ -2885,12 +2884,13 @@ gfc_match_decl_type_spec (gfc_typespec *ts, int im
       && !gfc_add_function (&sym->attr, sym->name, NULL))
     return MATCH_ERROR;
 
-  if (!dt_sym)
+  if (!dt_sym || dt_sym->gfc_new)
     {
       gfc_interface *intr, *head;
 
       /* Use upper case to save the actual derived-type symbol.  */
-      gfc_get_symbol (dt_name, NULL, &dt_sym);
+      if (!dt_sym)
+	gfc_get_symbol (dt_name, NULL, &dt_sym);
       dt_sym->name = gfc_get_string (sym->name);
       head = sym->generic;
       intr = gfc_get_interface ();

Re: pr59016

[Mikael Morin]

Le 08/04/2015 12:29, Mikael Morin a écrit :
> Except that the following ... ahem ... doesn't work.
> 
And it doesn't work because gfc_get_ha_symbol doesn't version
host-associated symbols.
So one has to call symbol.c's save_symbol_data by hand.  And then, we
can as well keep the original gfc_find_symbol calls.
Evangelos, do you want to propose a patch along those lines?

Mikael

[Patch, fortran] PR59016, second version

[Mikael Morin]

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

Hello,

I have finally taken over PR59016.
The bug was a pointer to a freed (derived type) symbol retained in the
generic one with the same name, because the symbols weren't versioned.

Evangelos proposed a patch that was freeing the memory by hand.  The
simpler one here, on the other hand, saves the symbol before any
modification and lets the symbol undo code do its job.
Thanks to Evangelos anyway for isolating the bug and proposing a fix.

This patch has been tested on x86_64-unknown-linux-gnu.
OK for trunk/4.9/4.8?

Mikael



[-- Attachment #2: pr59016_v3.CL --]
[-- Type: text/plain, Size: 573 bytes --]

2015-04-09  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/56674
	PR fortran/58813
	PR fortran/59016
	PR fortran/59024
	* symbol.c (save_symbol_data, gfc_save_symbol_data): Rename the
	former to the latter and make non-static.  Update callers.
	* gfortran.h (gfc_save_symbol_data): New prototype.
	* decl.c (gfc_match_decl_type_spec): Call 'gfc_save_symbol_data'
	before modifying symbols 'sym' and 'dt_sym'.

2015-04-09  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/56674
	PR fortran/58813
	PR fortran/59016
	PR fortran/59024
	* gfortran.dg/used_types_27.f90: New.
	


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: pr59016_v3.diff --]
[-- Type: text/x-patch; name="pr59016_v3.diff", Size: 2138 bytes --]

Index: decl.c
===================================================================
--- decl.c	(révision 221654)
+++ decl.c	(copie de travail)
@@ -2876,6 +2876,7 @@ gfc_match_decl_type_spec (gfc_typespec *ts, int im
       return MATCH_ERROR;
     }
 
+  gfc_save_symbol_data (sym);
   gfc_set_sym_referenced (sym);
   if (!sym->attr.generic
       && !gfc_add_generic (&sym->attr, sym->name, NULL))
@@ -2900,6 +2901,8 @@ gfc_match_decl_type_spec (gfc_typespec *ts, int im
       sym->generic = intr;
       sym->attr.if_source = IFSRC_DECL;
     }
+  else
+    gfc_save_symbol_data (dt_sym);
 
   gfc_set_sym_referenced (dt_sym);
 
Index: gfortran.h
===================================================================
--- gfortran.h	(révision 221657)
+++ gfortran.h	(copie de travail)
@@ -2819,6 +2819,7 @@ bool verify_bind_c_derived_type (gfc_symbol *);
 bool verify_com_block_vars_c_interop (gfc_common_head *);
 gfc_symtree *generate_isocbinding_symbol (const char *, iso_c_binding_symbol,
 					  const char *, gfc_symtree *, bool);
+void gfc_save_symbol_data (gfc_symbol *);
 int gfc_get_sym_tree (const char *, gfc_namespace *, gfc_symtree **, bool);
 int gfc_get_ha_symbol (const char *, gfc_symbol **);
 int gfc_get_ha_sym_tree (const char *, gfc_symtree **);
Index: symbol.c
===================================================================
--- symbol.c	(révision 221654)
+++ symbol.c	(copie de travail)
@@ -2758,8 +2758,8 @@ single_undo_checkpoint_p (void)
 
 /* Save symbol with the information necessary to back it out.  */
 
-static void
-save_symbol_data (gfc_symbol *sym)
+void
+gfc_save_symbol_data (gfc_symbol *sym)
 {
   gfc_symbol *s;
   unsigned i;
@@ -2860,7 +2860,7 @@ gfc_get_sym_tree (const char *name, gfc_namespace
       p->mark = 1;
 
       /* Copy in case this symbol is changed.  */
-      save_symbol_data (p);
+      gfc_save_symbol_data (p);
     }
 
   *result = st;
@@ -2899,7 +2899,7 @@ gfc_get_ha_sym_tree (const char *name, gfc_symtree
 
   if (st != NULL)
     {
-      save_symbol_data (st->n.sym);
+      gfc_save_symbol_data (st->n.sym);
       *result = st;
       return i;
     }



[-- Attachment #4: used_types_27.f90 --]
[-- Type: text/x-fortran, Size: 528 bytes --]

! { dg-do compile }
!
! PR fortran/56674
! PR fortran/58813
! PR fortran/59016
! PR fortran/59024
! The generic name 'atomic_kind_types' was keeping pointers to freed
! symbols, leading to random error-recovery ICEs.
!
! Original test case from Joost VandeVondele <Joost.VandeVondele@mat.ethz.ch>.

MODULE atomic_kind_types
  PUBLIC :: atomic_kind_type
CONTAINS
  INTEGER FUNCTION is_hydrogen(atomic_kind)
    TYPE(atomic_kind_type), pointer :: atomic_kind ! { dg-error "used before it is defined" }
  END FUNCTION
END MODULE

Re: [Patch, fortran] PR59016, second version

[Tobias Burnus]

Hello,

Mikael Morin wrote:
> I have finally taken over PR59016.
> The bug was a pointer to a freed (derived type) symbol retained in the
> generic one with the same name, because the symbols weren't versioned.
>
> Evangelos proposed a patch that was freeing the memory by hand.  The
> simpler one here, on the other hand, saves the symbol before any
> modification and lets the symbol undo code do its job.
> Thanks to Evangelos anyway for isolating the bug and proposing a fix.
>
> This patch has been tested on x86_64-unknown-linux-gnu.
> OK for trunk/4.9/4.8?

Thanks for the work on this, Evangelos and Mikael!
The patch looks good to me.

Cheers,

Tobias