diff mbox

[fortran] PR60898 premature release of entry symbols

Message ID 54E0DC05.4000100@sfr.fr
State New
Headers show

Commit Message

Mikael Morin Feb. 15, 2015, 5:48 p.m. UTC
Hello,

I propose a fix for PR60898, where a symbol is freed despite remaining
reachable in the symbol tree.
The problem comes from this code in resolve_symbol:
> 
>     /* If we find that a flavorless symbol is an interface in one of the
>        parent namespaces, find its symtree in this namespace, free the
>        symbol and set the symtree to point to the interface symbol.  */
>       for (ns = gfc_current_ns->parent; ns; ns = ns->parent)
> 	{
> 	  symtree = gfc_find_symtree (ns->sym_root, sym->name);
> 	  if (symtree && [...])
> 	    {
> 	      this_symtree = gfc_find_symtree (gfc_current_ns->sym_root,
> 					       sym->name);
> 	      gfc_release_symbol (sym);
> 	      symtree->n.sym->refs++;
> 	      this_symtree->n.sym = symtree->n.sym;
> 	      return;
> 	    }
> 	}
> 

Here, the target of an element of the current namespace's name tree is
changed to point to the outer symbol.  And the current symbol is freed,
without checking that it really was what was in the name tree before.

In the testcase https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60898#c7 ,
the problematic symbol is an entry, which is available in the name tree
only through a mangled name (created by gfc_get_unique_symtree in
get_proc_name), so gfc_find_symtree won't find it by name lookup.
In this case, what gfc_find_symtree finds is a symbol that is already
the outer interface symbol, so reassigning this_symtree.n.sym would be a
no-op.

The patch proposed checks that sym == this_symtree->n.sym, so that the
symbol reassignment is only made in that case.  Otherwise, the regular
symbol resolution happens normally.

This patch is a stripped down version of what I posted before in the PR,
which contained a symbol.c part which was increasing the reference count
locally in do_traverse_symtree, to delay symbol release after all of
them have been processed.  That part was useless because if a symbol had
to be processed more than once (meaning it was available under different
names), it will have the corresponding reference count set so that it
won't be freed too early in any case.
Worse, that part was interacting badly with the hack used to break
circular references in gfc_release_symbol, so it was better left out.

Anyway, this is regression tested[*] on x86_64-unknown-linux-gnu. OK for
trunk/4.9/4.8 ?

Mikael

[*] I have a few failing testcases (also without the patch), namely the
following; does this ring a bell ?
FAIL: gfortran.dg/erf_3.F90
FAIL: gfortran.dg/fmt_g0_7.f08
FAIL: gfortran.dg/fmt_en.f90
FAIL: gfortran.dg/nan_7.f90
FAIL: gfortran.dg/quad_2.f90
FAIL: gfortran.dg/quad_3.f90
FAIL: gfortran.dg/round_4.f90
2015-02-15  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/60898
	* resolve.c (resolve_symbol): Check that the symbol found from
	name lookup really is the current symbol being resolved.

2015-02-15  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/60898
	* gfortran.dg/entry_20.f90: New.

Comments

Jerry DeLisle Feb. 15, 2015, 6 p.m. UTC | #1
On 02/15/2015 09:48 AM, Mikael Morin wrote:

> [*] I have a few failing testcases (also without the patch), namely the
> following; does this ring a bell ?
> FAIL: gfortran.dg/erf_3.F90
> FAIL: gfortran.dg/fmt_g0_7.f08
> FAIL: gfortran.dg/fmt_en.f90
> FAIL: gfortran.dg/nan_7.f90
> FAIL: gfortran.dg/quad_2.f90
> FAIL: gfortran.dg/quad_3.f90
> FAIL: gfortran.dg/round_4.f90
>

fmt_g0_7.f08 is a new test that should be passing on x86-64 unless you have not 
updated scanner.c.  Are these fails on x86-64? I do not see them here on mine.

Jerry
Mikael Morin Feb. 16, 2015, 11:22 a.m. UTC | #2
Le 15/02/2015 19:00, Jerry DeLisle a écrit :
> On 02/15/2015 09:48 AM, Mikael Morin wrote:
> 
>> [*] I have a few failing testcases (also without the patch), namely the
>> following; does this ring a bell ?
>> FAIL: gfortran.dg/erf_3.F90
>> FAIL: gfortran.dg/fmt_g0_7.f08
>> FAIL: gfortran.dg/fmt_en.f90
>> FAIL: gfortran.dg/nan_7.f90
>> FAIL: gfortran.dg/quad_2.f90
>> FAIL: gfortran.dg/quad_3.f90
>> FAIL: gfortran.dg/round_4.f90
>>
> 
> fmt_g0_7.f08 is a new test that should be passing on x86-64 unless you
> have not updated scanner.c.  Are these fails on x86-64? I do not see
> them here on mine.
> 
On x86_64, yes.  I cleared my build tree, bootstrapped again, and the
failures are gone. :-)

Mikael
Paul Richard Thomas March 1, 2015, 9:03 p.m. UTC | #3
Dear Mikael,

That looks to be OK for 4.8, 4.9 and 5.0.

Strange testcase, though... :-)

Thanks for the patch

Paul

On 15 February 2015 at 18:48, Mikael Morin <mikael.morin@sfr.fr> wrote:
> Hello,
>
> I propose a fix for PR60898, where a symbol is freed despite remaining
> reachable in the symbol tree.
> The problem comes from this code in resolve_symbol:
>>
>>     /* If we find that a flavorless symbol is an interface in one of the
>>        parent namespaces, find its symtree in this namespace, free the
>>        symbol and set the symtree to point to the interface symbol.  */
>>       for (ns = gfc_current_ns->parent; ns; ns = ns->parent)
>>       {
>>         symtree = gfc_find_symtree (ns->sym_root, sym->name);
>>         if (symtree && [...])
>>           {
>>             this_symtree = gfc_find_symtree (gfc_current_ns->sym_root,
>>                                              sym->name);
>>             gfc_release_symbol (sym);
>>             symtree->n.sym->refs++;
>>             this_symtree->n.sym = symtree->n.sym;
>>             return;
>>           }
>>       }
>>
>
> Here, the target of an element of the current namespace's name tree is
> changed to point to the outer symbol.  And the current symbol is freed,
> without checking that it really was what was in the name tree before.
>
> In the testcase https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60898#c7 ,
> the problematic symbol is an entry, which is available in the name tree
> only through a mangled name (created by gfc_get_unique_symtree in
> get_proc_name), so gfc_find_symtree won't find it by name lookup.
> In this case, what gfc_find_symtree finds is a symbol that is already
> the outer interface symbol, so reassigning this_symtree.n.sym would be a
> no-op.
>
> The patch proposed checks that sym == this_symtree->n.sym, so that the
> symbol reassignment is only made in that case.  Otherwise, the regular
> symbol resolution happens normally.
>
> This patch is a stripped down version of what I posted before in the PR,
> which contained a symbol.c part which was increasing the reference count
> locally in do_traverse_symtree, to delay symbol release after all of
> them have been processed.  That part was useless because if a symbol had
> to be processed more than once (meaning it was available under different
> names), it will have the corresponding reference count set so that it
> won't be freed too early in any case.
> Worse, that part was interacting badly with the hack used to break
> circular references in gfc_release_symbol, so it was better left out.
>
> Anyway, this is regression tested[*] on x86_64-unknown-linux-gnu. OK for
> trunk/4.9/4.8 ?
>
> Mikael
>
> [*] I have a few failing testcases (also without the patch), namely the
> following; does this ring a bell ?
> FAIL: gfortran.dg/erf_3.F90
> FAIL: gfortran.dg/fmt_g0_7.f08
> FAIL: gfortran.dg/fmt_en.f90
> FAIL: gfortran.dg/nan_7.f90
> FAIL: gfortran.dg/quad_2.f90
> FAIL: gfortran.dg/quad_3.f90
> FAIL: gfortran.dg/round_4.f90
diff mbox

Patch

Index: resolve.c
===================================================================
--- resolve.c	(révision 220514)
+++ resolve.c	(copie de travail)
@@ -13125,10 +13125,13 @@  resolve_symbol (gfc_symbol *sym)
 	    {
 	      this_symtree = gfc_find_symtree (gfc_current_ns->sym_root,
 					       sym->name);
-	      gfc_release_symbol (sym);
-	      symtree->n.sym->refs++;
-	      this_symtree->n.sym = symtree->n.sym;
-	      return;
+	      if (this_symtree->n.sym == sym)
+		{
+		  symtree->n.sym->refs++;
+		  gfc_release_symbol (sym);
+		  this_symtree->n.sym = symtree->n.sym;
+		  return;
+		}
 	    }
 	}