diff mbox

[fortran] PR fortran/42769 typebound call resolved incorrectly.

Message ID 50E88B8C.3000605@sfr.fr
State New
Headers show

Commit Message

Mikael Morin Jan. 5, 2013, 8:22 p.m. UTC 
Hello,

here is a fix for PR42769, where a type bound call was resolved 
statically into a normal procedure call, but using the local procedure 
(with the same name) instead of the original (and correct) one.
It turned out to not be OOP specific, and be a pure module loading 
problem in the way we associate a symbol number in the module file with 
the corresponding symbol pointer once module is loaded.

The first module.c hunk fixes comment #14 in the PR. Here, p == NULL, 
which means that the user didn't ask to load the symbol (USE,ONLY:...), 
so we can just continue to the next one.  However, as an optimization, 
we try to find the symbol in the current name space and associate the 
pointer info (mapping module's integer <-> symbol pointer) with it if 
found to avoid reloading it from the module file if needed.
The problem was there was no check that the symbol is really the same, 
not another symbol with the same name.  Fixed by checking symbol's name 
and module name too.  The original code was only reusing the symtree; I 
took the opportunity to reuse the symbol as well, and the test suite 
didn't complain :-).

The second module.c hunk fixes comment #24 in the PR.  Here, p != NULL: 
the symbol will have to be loaded.  Before creating a new symbol and a 
new symtree and inserting them in the current name space, we check 
whether a symtree of the same name exists in the current name space. If 
it exists and is not ambiguous, it is either the same one as the one 
from the module, or both are generic so we can reuse the existing symbol 
in both cases.  However, if the symtree in the name space is ambiguous 
we shall not reuse it as it is different than the one to be loaded. 
Thus fixed by not reusing it in that case, and letting a unique non 
ambiguous name being automatically generated later.

The resolve.c hunk fixes comment #34 in the PR.  This one is not a 
module problem, but let's fix all cases in one go.  Resolve_call reloads 
the call's procedure symbol to take into account contained procedure 
which were not known at parse time.  However, it uses the symbol's name 
(original name) for lookup instead of the symtree's (local name).  The 
resolve.c hunk changes that.

The tests are adapted from comment 14, 24, 34 from pr42769, and the 
original test cases from pr45836 and pr45900.
Regression tested on x86_64-unknown-linux-gnu. OK for trunk?
4.7?
4.6?

Mikael
2013-01-05  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/42769
	PR fortran/45836
	PR fortran/45900
	* module.c (read_module): Don't reuse local symtree if the associated
	symbol isn't exactly the one wanted.  Don't reuse local symtree if it is
	ambiguous.
	* resolve.c (resolve_call): Use symtree's name instead of symbol's to
	lookup the symtree.

2013-01-05  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/42769
	PR fortran/45836
	PR fortran/45900
	* gfortran.dg/use_23.f90: New test.
	* gfortran.dg/use_24.f90: New test.
	* gfortran.dg/use_25.f90: New test.
	* gfortran.dg/use_26.f90: New test.
	* gfortran.dg/use_27.f90: New test.

Comments

Janus Weil Jan. 6, 2013, 11:45 a.m. UTC | #1 
Hi Mikael,

> here is a fix for PR42769, where a type bound call was resolved statically
> into a normal procedure call, but using the local procedure (with the same
> name) instead of the original (and correct) one.
> It turned out to not be OOP specific, and be a pure module loading problem
> in the way we associate a symbol number in the module file with the
> corresponding symbol pointer once module is loaded.
>
> The first module.c hunk fixes comment #14 in the PR. Here, p == NULL, which
> means that the user didn't ask to load the symbol (USE,ONLY:...), so we can
> just continue to the next one.  However, as an optimization, we try to find
> the symbol in the current name space and associate the pointer info (mapping
> module's integer <-> symbol pointer) with it if found to avoid reloading it
> from the module file if needed.
> The problem was there was no check that the symbol is really the same, not
> another symbol with the same name.  Fixed by checking symbol's name and
> module name too.  The original code was only reusing the symtree; I took the
> opportunity to reuse the symbol as well, and the test suite didn't complain
> :-).
>
> The second module.c hunk fixes comment #24 in the PR.  Here, p != NULL: the
> symbol will have to be loaded.  Before creating a new symbol and a new
> symtree and inserting them in the current name space, we check whether a
> symtree of the same name exists in the current name space. If it exists and
> is not ambiguous, it is either the same one as the one from the module, or
> both are generic so we can reuse the existing symbol in both cases.
> However, if the symtree in the name space is ambiguous we shall not reuse it
> as it is different than the one to be loaded. Thus fixed by not reusing it
> in that case, and letting a unique non ambiguous name being automatically
> generated later.
>
> The resolve.c hunk fixes comment #34 in the PR.  This one is not a module
> problem, but let's fix all cases in one go.  Resolve_call reloads the call's
> procedure symbol to take into account contained procedure which were not
> known at parse time.  However, it uses the symbol's name (original name) for
> lookup instead of the symtree's (local name).  The resolve.c hunk changes
> that.
>
> The tests are adapted from comment 14, 24, 34 from pr42769, and the original
> test cases from pr45836 and pr45900.
> Regression tested on x86_64-unknown-linux-gnu. OK for trunk?

thanks for taking care of this rather long-standing bug. I was
convinced that it is a module-loading problem, but couldn't really
nail it down.

I think your patch is indeed ok for trunk.


> 4.7?
> 4.6?

Since it's not a regression, it does not necessarily need to be
backported. However, the sort of wrong-code behavior seen in comment
#24 is severe enough that it might justify backporting. I leave that
up to you.

Cheers,
Janus
Thomas Koenig Jan. 6, 2013, 12:06 p.m. UTC | #2 
Am 06.01.2013 12:45, schrieb Janus Weil:
> Since it's not a regression, it does not necessarily need to be
> backported. However, the sort of wrong-code behavior seen in comment
> #24 is severe enough that it might justify backporting. I leave that
> up to you.

FWITW, I think it should be backported to 4.7 at least.

	Thomas
Mikael Morin Jan. 6, 2013, 3:59 p.m. UTC | #3 
Le 06/01/2013 13:06, Thomas Koenig a écrit :
> Am 06.01.2013 12:45, schrieb Janus Weil:
>> I think your patch is indeed ok for trunk.
Thanks, applied as revision 194949.

>>> 4.7?
>>> 4.6?
>> Since it's not a regression, it does not necessarily need to be
>> backported. However, the sort of wrong-code behavior seen in comment
>> #24 is severe enough that it might justify backporting. I leave that
>> up to you.
>
> FWITW, I think it should be backported to 4.7 at least.
>
I will backport to both.

Mikael
diff mbox

Patch

diff --git a/module.c b/module.c
index cde5739..e63e510 100644
--- a/module.c
+++ b/module.c
@@ -4656,8 +4656,14 @@  read_module (void)
 	  if (p == NULL)
 	    {
 	      st = gfc_find_symtree (gfc_current_ns->sym_root, name);
-	      if (st != NULL)
-		info->u.rsym.symtree = st;
+	      if (st != NULL
+		  && strcmp (st->n.sym->name, info->u.rsym.true_name) == 0
+		  && st->n.sym->module != NULL
+		  && strcmp (st->n.sym->module, info->u.rsym.module) == 0)
+		{
+		  info->u.rsym.symtree = st;
+		  info->u.rsym.sym = st->n.sym;
+		}
 	      continue;
 	    }
 
@@ -4678,7 +4684,8 @@  read_module (void)
 	      /* Check for ambiguous symbols.  */
 	      if (check_for_ambiguous (st->n.sym, info))
 		st->ambiguous = 1;
-	      info->u.rsym.symtree = st;
+	      else
+		info->u.rsym.symtree = st;
 	    }
 	  else
 	    {
diff --git a/resolve.c b/resolve.c
index d4d5eb9..17ee716 100644
--- a/resolve.c
+++ b/resolve.c
@@ -3778,7 +3778,7 @@  resolve_call (gfc_code *c)
   if (csym && gfc_current_ns->parent && csym->ns != gfc_current_ns)
     {
       gfc_symtree *st;
-      gfc_find_sym_tree (csym->name, gfc_current_ns, 1, &st);
+      gfc_find_sym_tree (c->symtree->name, gfc_current_ns, 1, &st);
       sym = st ? st->n.sym : NULL;
       if (sym && csym != sym
 	      && sym->ns == gfc_current_ns