Re: [PATCH,FORTRAN 00/29] Move towards stringpool, part 1

[Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>]

On Fri, 7 Sep 2018 at 10:07, Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:
>
> On Wed, 5 Sep 2018 at 20:57, Janne Blomqvist <blomqvist.janne@gmail.com> wrote:
> >
> > On Wed, Sep 5, 2018 at 5:58 PM Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>
> >> Bootstrapped and regtested on x86_64-foo-linux.
> >>
> >> I'd appreciate if someone could double check for regressions on other
> >> setups. Git branch:
> >> https://gcc.gnu.org/git/?p=gcc.git;a=log;h=refs/heads/aldot/fortran-fe-stringpool
> >>
> >> Ok for trunk?
> >
> >
> > Hi,
> >
> > this is quite an impressive patch set. I have looked through all the patches, and on the surface they all look ok.
>
> Thanks alot for your appreciation!
> >
> > Unfortunately I don't have any exotic target to test on either, so I think you just have to commit it and check for regression reports. Though I don't see this set doing anything which would work differently on other targets, but you never know..
> >
> > I'd say wait a few days in case anybody else wants to comment on it, then commit it to trunk.
>
> Upon further testing i encountered a regression in module writing,
> manifesting itself in a failure to compile ieee_8.f90 (and only this).

> Sorry for that, I'll have another look during the weekend.

so in free_pi_tree we should not free true_name nor module:

@@ -239,12 +239,6 @@ free_pi_tree (pointer_info *p)
   free_pi_tree (p->left);
   free_pi_tree (p->right);

-  if (iomode == IO_INPUT)
-    {
-      XDELETEVEC (p->u.rsym.true_name);
-      XDELETEVEC (p->u.rsym.module);
-    }
-
   free (p);
 }

This fixes the module writing but leaks, obviously.
Now the reason why i initially did not use mio_pool_string for both
rsym.module and rsym.true_name was that mio_pool_string sets the name
to NULL if the string is empty.
Currently these are read by read_string() [which we should get rid of
entirely, the 2 mentioned fields are the last two who use
read_string()] which does not nullify the empty string but returns
just the pointer. For e.g. ieee_8.f90 using mio_pool_string gives us a
NULL module which leads to gfc_use_module -> load_needed ->
gfc_find_symbol -> gfc_find_sym_tree -> gfc_find_symtree which tries
to c = strcmp (name, st->name); where name is NULL.

The main culprits seem to be class finalization wrapper variables so
i'm adding modules to those now.
Which leaves me with regressions like allocate_with_source_14.f03.
"Fixing" these by falling back to gfc_current_ns->proc_name->name in
load_needed for !ns->proc_name if the rsym->module is NULL seems to
work.

Now there are a number of issues with names of fixups. Like the 9 in e.g.:

$ zcat /tmp/n/m.mod | egrep -v "^(\(\)|\(\(\)|$)"
GFORTRAN module version '15' created from generic_27.f90
(('testif' 'm' 2 3))
(4 'm' 'm' '' 1 ((MODULE UNKNOWN-INTENT UNKNOWN-PROC UNKNOWN UNKNOWN 0 0)
3 'test1' 'm' '' 1 ((PROCEDURE UNKNOWN-INTENT MODULE-PROC DECL UNKNOWN 0
0 FUNCTION) () (REAL 4 0 0 0 REAL ()) 5 0 (6) () 3 () () () 0 0)
2 'test2' 'm' '' 1 ((PROCEDURE UNKNOWN-INTENT MODULE-PROC DECL UNKNOWN 0
0 FUNCTION ARRAY_OUTER_DEPENDENCY) () (REAL 4 0 0 0 REAL ()) 7 0 (8) ()
2 () () () 0 0)
6 'obj' '' '' 5 ((VARIABLE UNKNOWN-INTENT UNKNOWN-PROC UNKNOWN UNKNOWN 0
0 DUMMY) () (REAL 4 0 0 0 REAL ()) 0 0 () () 0 () () () 0 0)
8 'pr' '' '' 7 ((PROCEDURE UNKNOWN-INTENT DUMMY-PROC UNKNOWN UNKNOWN 0 0
EXTERNAL DUMMY FUNCTION PROCEDURE ARRAY_OUTER_DEPENDENCY) () (REAL 4 9 0
0 REAL ()) 0 0 () () 8 () () () 0 0)
9 '' '' '' 7 ((PROCEDURE UNKNOWN-INTENT UNKNOWN-PROC UNKNOWN UNKNOWN 0 0
FUNCTION) () (REAL 4 0 0 0 REAL ()) 0 0 () () 0 () () () 0 0)
)
('m' 0 4 'test1' 0 3 'test2' 0 2)

which is a bit of a complication since we need them to verify proper
interface types and attributes etc, etc.
generic_27.f90 would then warn in check_proc_interface() that
"Interface %qs at %L must be explicit".
To bypass this warning i suggest to flag these as artificial like so:
@@ -6679,10 +6683,12 @@ match_procedure_decl (void)
            return MATCH_ERROR;
          sym->ts.interface = gfc_new_symbol ("", gfc_current_ns);
          sym->ts.interface->ts = current_ts;
          sym->ts.interface->attr.flavor = FL_PROCEDURE;
          sym->ts.interface->attr.function = 1;
+         /* Suppress warnings about explicit interface */
+         sym->ts.interface->attr.artificial = 1;
          sym->attr.function = 1;
          sym->attr.if_source = IFSRC_UNKNOWN;
        }

       if (gfc_match (" =>") == MATCH_YES)
@@ -6818,10 +6824,12 @@ match_ppc_decl (void)
          c->ts.interface = gfc_new_symbol ("", gfc_current_ns);
          c->ts.interface->result = c->ts.interface;
          c->ts.interface->ts = ts;
          c->ts.interface->attr.flavor = FL_PROCEDURE;
          c->ts.interface->attr.function = 1;
+         /* Suppress warnings about explicit interface */
+         c->ts.interface->attr.artificial = 1;
          c->attr.function = 1;
          c->attr.if_source = IFSRC_UNKNOWN;
        }

       if (gfc_match (" =>") == MATCH_YES)

and then not exclude ""-names but attr.artificial for the "must be
explicit" warning. This works fine.

Another spot where we encounter trouble with NULL module in the sym is
generic_1.f90 where we would be unable to distinguish interface sub
arguments during true_name lookup.
We find x in true names and consequently use this one sym for both the
REAL as well as the INTEGER variable which of course doesn't work when
resolving.
As it turns out we get away with punting true_name lookup if the module is NULL.

The next hintch are unlimited polymorphic component class containers
as in select_type_36.f03 when used in module context.
gfc_find_gsymbol() around the "upe" really wants a module that is
non-NULL which we luckily have at hand. This just extends the
proc_name-hack.

@@ -4061,6 +4061,10 @@ gfc_match_decl_type_spec (gfc_typespec *ts, int
implicit_flag)
              upe->refs++;
              upe->ts.type = BT_VOID;
              upe->attr.unlimited_polymorphic = 1;
+             /* Make sure gfc_find_gsymbol sees a (non-NULL) name to
+              * search for by plugging in some module name.  */
+             if (gfc_current_ns->proc_name != NULL)
+               upe->module = gfc_current_ns->proc_name->name;
              /* This is essential to force the construction of
                 unlimited polymorphic component class containers.  */
              upe->attr.zero_comp = 1;

The area of true_name and pi_root handling is a bit unpleasant to work
with, i must admit. But then i do not volunteer to rip it all out ;)
I think we will be able to remove some of these proc_name-hacks as
soon as we switch the symbol finding to pointer comparison, at least.

I'm cleaning up the above for a final test and will send it as
alternative, extended approach intended to replace the
"[PATCH,FORTRAN 25/29] Use stringpool on loading module symbols" (
https://gcc.gnu.org/ml/fortran/2018-09/msg00039.html )

patch, fwiw.

thanks,