Re: [Patch, Fortran] PR58787 ICE (error recovery) in check_proc_interface

2018-10-12 Thread Paul Richard Thomas
Hi Tobias,

This is OK.

Thanks for the patch and the extra careful regtesting :-)

Paul

On Fri, 12 Oct 2018 at 00:17, Tobias Burnus  wrote:
>
> Next patch, next try …
>
> The problem is the following: We have a use-associated symbol (a
> function) – which is once referenced (by use association).
>
> We now declare a same-name function in the 'contains' section.
>
> gfortran now parses the function name, fetches the symbol and reports
> that one duplicates the function (gfc_error_now). However, it continues
> afterwards and creates a sym_tree in the gfc_current_ns – and duly
> increases the sym->refs to 2.
>
> A bit later, the function gets rejected. (Parsing of the formal
> arguments fails when setting a attribute of a use-associated symbol, but
> I think this doesn't matter. reject_statement() is called, which rolls
> back the symbol, but again, I think that doesn't matter – it just makes
> some other solutions a tad more difficult.)
>
> Now, the contains namespace is cleaned up: As the function name is in
> the namespace's sym_tree, gfc_release_symbol() is invoked.
>
> The symbol itself is not freed (as --refs > 0), but sym->formal_ns is.
> (There are some safety nets, like sym->rev == 2 and sym->ns !=
> sym->formal_ns, but they don't help.)
>
> Later, the symbol of the parent's namespace is resolved – and this
> includes resolving the formal arguments. – And accessing freed memory
> might crash gfortran.
>
> As solution, I return early and with an error from get_proc_name(). I
> think that's cleaner than waiting longer – and the early return avoids
> creating the symbol at the first place. (Otherwise, modifications have
> to go beyond rolling back of the symbols; doing refs++ probably helps,
> but is rather intransparent.)
>
>
> Pro of the early return: Avoids accessing already freed memory, might
> generate shorter error output as some later fails (like for not being
> able to modify attributes of a use-associated symbol) are gone.
>
> Contra: As one returns before entering a procedure, all following lines
> are in the outer scope (usually 'contains') and there probably
> unexpected. Hence, one might get a bunch of follow-up errors which do
> not help.
>
>
> Build on x86-64 and even more carefully regtested.
>
> OK?
>
> Tobias
>


-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein


[Patch, Fortran] PR58787 ICE (error recovery) in check_proc_interface

2018-10-11 Thread Tobias Burnus

Next patch, next try …

The problem is the following: We have a use-associated symbol (a 
function) – which is once referenced (by use association).


We now declare a same-name function in the 'contains' section.

gfortran now parses the function name, fetches the symbol and reports 
that one duplicates the function (gfc_error_now). However, it continues 
afterwards and creates a sym_tree in the gfc_current_ns – and duly 
increases the sym->refs to 2.


A bit later, the function gets rejected. (Parsing of the formal 
arguments fails when setting a attribute of a use-associated symbol, but 
I think this doesn't matter. reject_statement() is called, which rolls 
back the symbol, but again, I think that doesn't matter – it just makes 
some other solutions a tad more difficult.)


Now, the contains namespace is cleaned up: As the function name is in 
the namespace's sym_tree, gfc_release_symbol() is invoked.


The symbol itself is not freed (as --refs > 0), but sym->formal_ns is. 
(There are some safety nets, like sym->rev == 2 and sym->ns != 
sym->formal_ns, but they don't help.)


Later, the symbol of the parent's namespace is resolved – and this 
includes resolving the formal arguments. – And accessing freed memory 
might crash gfortran.


As solution, I return early and with an error from get_proc_name(). I 
think that's cleaner than waiting longer – and the early return avoids 
creating the symbol at the first place. (Otherwise, modifications have 
to go beyond rolling back of the symbols; doing refs++ probably helps, 
but is rather intransparent.)



Pro of the early return: Avoids accessing already freed memory, might 
generate shorter error output as some later fails (like for not being 
able to modify attributes of a use-associated symbol) are gone.


Contra: As one returns before entering a procedure, all following lines 
are in the outer scope (usually 'contains') and there probably 
unexpected. Hence, one might get a bunch of follow-up errors which do 
not help.



Build on x86-64 and even more carefully regtested.

OK?

Tobias

2018-10-12  Tobias Burnus 

	PR fortran/58787
	* decl.c (get_proc_name): Return with error before
	creating sym_tree.

	PR fortran/58787
	* gfortran.dg/goacc/pr77765.f90: Modify dg-error.
	* gfortran.dg/interface_42.f90: Ditto.
	* gfortran.dg/internal_references_1.f90: Ditto.
	* gfortran.dg/invalid_procedure_name.f90: Ditto.
	* gfortran.dg/pr65453.f90: Ditto.
	* gfortran.dg/pr77414.f90: Ditto.
	* gfortran.dg/pr78741.f90: Ditto.
	* gfortran.dg/same_name_2.f90: Ditto.

diff --git a/gcc/fortran/decl.c b/gcc/fortran/decl.c
index 7f79811d152..87c736fb2db 100644
--- a/gcc/fortran/decl.c
+++ b/gcc/fortran/decl.c
@@ -1231,28 +1231,39 @@ get_proc_name (const char *name, gfc_symbol **result, bool module_fcn_entry)
 	  && sym->attr.proc != 0
 	  && (sym->attr.subroutine || sym->attr.function || sym->attr.entry)
 	  && sym->attr.if_source != IFSRC_UNKNOWN)
-	gfc_error_now ("Procedure %qs at %C is already defined at %L",
-		   name, >declared_at);
-
+	{
+	  gfc_error_now ("Procedure %qs at %C is already defined at %L",
+			 name, >declared_at);
+	  return true;
+	}
   if (sym->attr.flavor != 0
 	  && sym->attr.entry && sym->attr.if_source != IFSRC_UNKNOWN)
-	gfc_error_now ("Procedure %qs at %C is already defined at %L",
-		   name, >declared_at);
+	{
+	  gfc_error_now ("Procedure %qs at %C is already defined at %L",
+			 name, >declared_at);
+	  return true;
+	}
 
   if (sym->attr.external && sym->attr.procedure
 	  && gfc_current_state () == COMP_CONTAINS)
-	gfc_error_now ("Contained procedure %qs at %C clashes with "
-			"procedure defined at %L",
-		   name, >declared_at);
+	{
+	  gfc_error_now ("Contained procedure %qs at %C clashes with "
+			 "procedure defined at %L",
+			 name, >declared_at);
+	  return true;
+	}
 
   /* Trap a procedure with a name the same as interface in the
 	 encompassing scope.  */
   if (sym->attr.generic != 0
 	  && (sym->attr.subroutine || sym->attr.function)
 	  && !sym->attr.mod_proc)
-	gfc_error_now ("Name %qs at %C is already defined"
-		   " as a generic interface at %L",
-		   name, >declared_at);
+	{
+	  gfc_error_now ("Name %qs at %C is already defined"
+			 " as a generic interface at %L",
+			 name, >declared_at);
+	  return true;
+	}
 
   /* Trap declarations of attributes in encompassing scope.  The
 	 signature for this is that ts.kind is set.  Legitimate
@@ -1263,8 +1274,11 @@ get_proc_name (const char *name, gfc_symbol **result, bool module_fcn_entry)
 	  && gfc_current_ns->parent != NULL
 	  && sym->attr.access == 0
 	  && !module_fcn_entry)
-	gfc_error_now ("Procedure %qs at %C has an explicit interface "
+	{
+	  gfc_error_now ("Procedure %qs at %C has an explicit interface "
 		   "from a previous declaration",  name);
+	  return true;
+	}
 }
 
   /* C1246 (R1225) MODULE shall appear only in the function-stmt or
@@ -1276,17 +1290,23 @@ get_proc_name (const char *name, gfc_symbol