Patchwork PR fortran/51727: make module files reproducible, question on C++ in gcc

login
register
mail settings
Submitter Tobias Schlüter
Date Oct. 13, 2012, 1:26 p.m.
Message ID <50796BF5.4060100@physik.uni-muenchen.de>
Download mbox | patch
Permalink /patch/191298/
State New
Headers show

Comments

Tobias Schlüter - Oct. 13, 2012, 1:26 p.m.
Hi,

first a question also to non-gfortraners: if I want to use std::map, 
where do I "#include <map>"?  In system.h?

Now to the patch-specific part: in this PR, module files are produced 
with random changes because the order in which symbols are written can 
depend on the memory layout.  This patch fixes this by recording which 
symbols need to be written and then processing them in order.  The patch 
doesn't make the more involved effort of putting all symbols into the 
module in an easily predicted order, instead it only makes sure that the 
order remains fixed across the compiler invocations.  The reason why the 
former is difficult is that during the process of writing a symbol, it 
can turn out that other symbols will have to be written as well (say, 
because they appear in array specifications).  Since the module-writing 
code determines which symbols to output while actually writing the file, 
recording all the symbols that need to be written before writing to the 
file would mean a lot of surgery.

I'm putting forward two patches.  One uses a C++ map to very concisely 
build up and handle the ordered list of symbols.  This has three problems:
1) gfortran maintainers may not want C++isms (even though in this case
    it's very localized, and in my opinion very transparent), and
2) it can't be backported to old release branches which are still
    compiled as C.  Joost expressed interested in a backport.
3) I don't know where to #include <map> (see above)
Therefore I also propose a patch where I added the necessary ~50 lines 
of boilerplate code and added the necessary traversal function to use 
gfortran's GFC_BBT to maintain the ordered tree of symbols.

Both patches pass the testsuite and Joost confirms that they fix the 
problem with CP2K.  I also verified with a few examples that they both 
produce identical .mod files as they should.

Is the C++ patch, modified to do the #include correctly, ok for the 
trunk?  If not, the C-only patch?  Can I put the C-only patch on the 
release branches?  And which?

Cheers,
- Tobi
2012-10-13  Tobias Schlüter  <tobi@gcc.gnu.org>

	PR fortran/51727
	* module.c (sorted_pointer_info): New.
	(gfc_get_sorted_pointer_info): New.
	(free_sorted_pointer_info_tree): New.
	(compare_sorted_pointer_info): New.
	(find_symbols_to_write): New.
	(write_symbol1_recursion): New.
	(write_symbol1): Collect symbols that need writing, output in order.
	(write_generic): Traverse tree in order.
2012-10-10  Tobias Schlüter  <tobi@gcc.gnu.org>

	    PR fortran/51727
	    * module.c (pointer_info_to_map): New.
	    (write_symbol1): Write symbols in order of integer field.
	    (write_generic): Traverse tree in left-to-right order.

diff --git a/gcc/fortran/module.c b/gcc/fortran/module.c
index 5cfc335..6d37144 100644
--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -78,6 +78,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "cpp.h"
 #include "tree.h"
 
+#include <map>
+
 #define MODULE_EXTENSION ".mod"
 
 /* Don't put any single quote (') in MOD_VERSION, 
@@ -5150,6 +5152,23 @@ write_symbol0 (gfc_symtree *st)
 }
 
 
+/* Recursively collects the symbols that need to be written, stores
+   them into a map sorted on their integer.  */
+
+static void
+find_symbols_to_write(std::map<int, pointer_info*>& m, pointer_info *p)
+{
+  if (!p)
+    return;
+
+  if (p->type == P_SYMBOL && p->u.wsym.state == NEEDS_WRITE)
+    m[p->integer] = p;
+
+  find_symbols_to_write (m, p->left);
+  find_symbols_to_write (m, p->right);
+}
+
+
 /* Recursive traversal function to write the secondary set of symbols
    to the module file.  These are symbols that were not public yet are
    needed by the public symbols or another dependent symbol.  The act
@@ -5157,24 +5176,30 @@ write_symbol0 (gfc_symtree *st)
    traversal if we find a symbol to write.  We return nonzero if a
    symbol was written and pass that information upwards.  */
 
-static int
+static bool
 write_symbol1 (pointer_info *p)
 {
-  int result;
-
   if (!p)
     return 0;
 
-  result = write_symbol1 (p->left);
+  int result = 0;
+
+  /* Put symbols into a map, traversal is then sorted.  */
+  std::map <int, pointer_info *> mpointers;
+  find_symbols_to_write (mpointers, p);
 
-  if (!(p->type != P_SYMBOL || p->u.wsym.state != NEEDS_WRITE))
+  for (std::map <int, pointer_info *>::iterator it = mpointers.begin();
+       it != mpointers.end(); it++)
     {
-      p->u.wsym.state = WRITTEN;
-      write_symbol (p->integer, p->u.wsym.sym);
+      pointer_info *p1 = it->second;
+      gcc_assert (p1 && it->first == p1->integer);
+      gcc_assert (p1->type == P_SYMBOL && p1->u.wsym.state == NEEDS_WRITE);
+
+      p1->u.wsym.state = WRITTEN;
+      write_symbol (p1->integer, p1->u.wsym.sym);
       result = 1;
     }
 
-  result |= write_symbol1 (p->right);
   return result;
 }
 
@@ -5205,19 +5230,18 @@ write_generic (gfc_symtree *st)
     return;
 
   write_generic (st->left);
-  write_generic (st->right);
 
   sym = st->n.sym;
-  if (!sym || check_unique_name (st->name))
-    return;
-
-  if (sym->generic == NULL || !gfc_check_symbol_access (sym))
-    return;
+  if (sym && !check_unique_name (st->name)
+      && sym->generic && gfc_check_symbol_access (sym))
+    {
+      if (!sym->module)
+	sym->module = module_name;
 
-  if (sym->module == NULL)
-    sym->module = module_name;
+      mio_symbol_interface (&st->name, &sym->module, &sym->generic);
+    }
 
-  mio_symbol_interface (&st->name, &sym->module, &sym->generic);
+  write_generic (st->right);
 }
Tobias Schlüter - Oct. 13, 2012, 1:28 p.m.
ps I forgot to mention that I also changed write_generic to traverse the 
tree in defined order, this is the same in the C++ and the C-only patch.

Cheers,
- Tobi

On 2012-10-13 15:26, Tobias Schlüter wrote:
>
> Hi,
>
> first a question also to non-gfortraners: if I want to use std::map,
> where do I "#include <map>"?  In system.h?
>
> Now to the patch-specific part: in this PR, module files are produced
> with random changes because the order in which symbols are written can
> depend on the memory layout.  This patch fixes this by recording which
> symbols need to be written and then processing them in order.  The patch
> doesn't make the more involved effort of putting all symbols into the
> module in an easily predicted order, instead it only makes sure that the
> order remains fixed across the compiler invocations.  The reason why the
> former is difficult is that during the process of writing a symbol, it
> can turn out that other symbols will have to be written as well (say,
> because they appear in array specifications).  Since the module-writing
> code determines which symbols to output while actually writing the file,
> recording all the symbols that need to be written before writing to the
> file would mean a lot of surgery.
>
> I'm putting forward two patches.  One uses a C++ map to very concisely
> build up and handle the ordered list of symbols.  This has three problems:
> 1) gfortran maintainers may not want C++isms (even though in this case
>     it's very localized, and in my opinion very transparent), and
> 2) it can't be backported to old release branches which are still
>     compiled as C.  Joost expressed interested in a backport.
> 3) I don't know where to #include <map> (see above)
> Therefore I also propose a patch where I added the necessary ~50 lines
> of boilerplate code and added the necessary traversal function to use
> gfortran's GFC_BBT to maintain the ordered tree of symbols.
>
> Both patches pass the testsuite and Joost confirms that they fix the
> problem with CP2K.  I also verified with a few examples that they both
> produce identical .mod files as they should.
>
> Is the C++ patch, modified to do the #include correctly, ok for the
> trunk?  If not, the C-only patch?  Can I put the C-only patch on the
> release branches?  And which?
>
> Cheers,
> - Tobi
Diego Novillo - Oct. 13, 2012, 6 p.m.
On 2012-10-13 09:26 , Tobias Schlüter wrote:
>
> Hi,
>
> first a question also to non-gfortraners: if I want to use std::map,
> where do I "#include <map>"?  In system.h?

No.  Ada includes system.h in pure C code.  Why not include it exactly 
where you need it?


Diego.
Tobias Schlüter - Oct. 13, 2012, 6:04 p.m.
On 2012-10-13 20:00, Diego Novillo wrote:
> On 2012-10-13 09:26 , Tobias Schlüter wrote:
>> first a question also to non-gfortraners: if I want to use std::map,
>> where do I "#include <map>"?  In system.h?
>
> No.  Ada includes system.h in pure C code.  Why not include it exactly
> where you need it?

Ok, I was just wondering if there's a rule because a quick grep revealed 
no non-header source file that includes system headers.

Thanks,
- Tobi
Diego Novillo - Oct. 13, 2012, 6:21 p.m.
On 2012-10-13 14:04 , Tobias Schlüter wrote:
> On 2012-10-13 20:00, Diego Novillo wrote:
>> On 2012-10-13 09:26 , Tobias Schlüter wrote:
>>> first a question also to non-gfortraners: if I want to use std::map,
>>> where do I "#include <map>"?  In system.h?
>>
>> No.  Ada includes system.h in pure C code.  Why not include it exactly
>> where you need it?
>
> Ok, I was just wondering if there's a rule because a quick grep revealed
> no non-header source file that includes system headers.

Joseph or others may have reason to create a system.hxx file or some 
such, but in principle I don't see why we shouldn't include std library 
headers as we need them.

Joseph?


Diego.
Joseph S. Myers - Oct. 13, 2012, 10:34 p.m.
On Sat, 13 Oct 2012, Diego Novillo wrote:

> On 2012-10-13 14:04 , Tobias Schlüter wrote:
> > On 2012-10-13 20:00, Diego Novillo wrote:
> > > On 2012-10-13 09:26 , Tobias Schlüter wrote:
> > > > first a question also to non-gfortraners: if I want to use std::map,
> > > > where do I "#include <map>"?  In system.h?
> > > 
> > > No.  Ada includes system.h in pure C code.  Why not include it exactly
> > > where you need it?
> > 
> > Ok, I was just wondering if there's a rule because a quick grep revealed
> > no non-header source file that includes system headers.
> 
> Joseph or others may have reason to create a system.hxx file or some such, but
> in principle I don't see why we shouldn't include std library headers as we
> need them.
> 
> Joseph?

The poisoning of various standard library functions that should not be 
used directly in GCC can be problematic if system headers, using those 
functions, are included after that poisoning.  Thus in general it's better 
for system header includes to go in system.h, before the poisoning.

In addition, where there is some autoconf configuration about whether to 
include a header, it's generally preferred to keep down the number of 
places with such a conditional on host features (encapsulating whatever 
features the header provides in some way so that other code can use them 
unconditionally).
Janne Blomqvist - Oct. 14, 2012, 9:35 p.m.
On Sat, Oct 13, 2012 at 4:26 PM, Tobias Schlüter
<tobias.schlueter@physik.uni-muenchen.de> wrote:
>
> Hi,
>
> first a question also to non-gfortraners: if I want to use std::map, where
> do I "#include <map>"?  In system.h?
>
> Now to the patch-specific part: in this PR, module files are produced with
> random changes because the order in which symbols are written can depend on
> the memory layout.  This patch fixes this by recording which symbols need to
> be written and then processing them in order.  The patch doesn't make the
> more involved effort of putting all symbols into the module in an easily
> predicted order, instead it only makes sure that the order remains fixed
> across the compiler invocations.  The reason why the former is difficult is
> that during the process of writing a symbol, it can turn out that other
> symbols will have to be written as well (say, because they appear in array
> specifications).  Since the module-writing code determines which symbols to
> output while actually writing the file, recording all the symbols that need
> to be written before writing to the file would mean a lot of surgery.
>
> I'm putting forward two patches.  One uses a C++ map to very concisely build
> up and handle the ordered list of symbols.  This has three problems:
> 1) gfortran maintainers may not want C++isms (even though in this case
>    it's very localized, and in my opinion very transparent), and
> 2) it can't be backported to old release branches which are still
>    compiled as C.  Joost expressed interested in a backport.
> 3) I don't know where to #include <map> (see above)
> Therefore I also propose a patch where I added the necessary ~50 lines of
> boilerplate code and added the necessary traversal function to use
> gfortran's GFC_BBT to maintain the ordered tree of symbols.
>
> Both patches pass the testsuite and Joost confirms that they fix the problem
> with CP2K.  I also verified with a few examples that they both produce
> identical .mod files as they should.
>
> Is the C++ patch, modified to do the #include correctly, ok for the trunk?
> If not, the C-only patch?  Can I put the C-only patch on the release
> branches?  And which?

Hi,

I'm pleasantly surprised that you managed to fix this PR with so little code!

- Personally, I'd prefer the C++ version; The C++ standard library is
widely used and documented and using it in favour of rolling our own
is IMHO a good idea.

- I'd be vary wrt backporting, in my experience the module.c code is
somewhat fragile and easily causes regressions. In any case, AFAICS PR
51727 is not a regression.

- I think one could go a step further and get rid of the BBT stuff in
pointer_info, replacing it with two file-level maps

std::map<void*, pointer_info*> pmap; // Or could be std::unordered_map
if available
std::map<int, pointer_info*> imap;

So when writing a module, use pmap similar to how pointer_info BBT is
used now, and then use imap to get a consistent order per your patch.
When reading, lookup/create mostly via imap, creating a pmap entry
also when creating a new imap entry; this avoids having to do a
brute-force search when looking up via pointer when reading (see
find_pointer2()).

(This 3rd point is mostly an idea for further work, and is not meant
as a requirement for accepting the patch)

Ok for trunk, although wait for a few days in case there is a storm of
protest on the C vs. C++ issue from other gfortran maintainers.
Jakub Jelinek - Oct. 14, 2012, 9:44 p.m.
On Mon, Oct 15, 2012 at 12:35:27AM +0300, Janne Blomqvist wrote:
> On Sat, Oct 13, 2012 at 4:26 PM, Tobias Schlüter
> > I'm putting forward two patches.  One uses a C++ map to very concisely build
> > up and handle the ordered list of symbols.  This has three problems:
> > 1) gfortran maintainers may not want C++isms (even though in this case
> >    it's very localized, and in my opinion very transparent), and

Even if you prefer a C++isms, why don't you go for "hash-table.h"?
std::map at least with the default allocator will just crash the compiler
if malloc returns NULL (remember that we build with -fno-exceptions),
while when you use hash-table.h (or hashtab.h) you get proper OOM diagnostics.

	Jakub
Tobias Schlüter - Oct. 15, 2012, 12:41 p.m.
On 2012-10-14 23:44, Jakub Jelinek wrote:
> On Mon, Oct 15, 2012 at 12:35:27AM +0300, Janne Blomqvist wrote:
>> On Sat, Oct 13, 2012 at 4:26 PM, Tobias Schlüter
>>> I'm putting forward two patches.  One uses a C++ map to very concisely build
>>> up and handle the ordered list of symbols.  This has three problems:
>>> 1) gfortran maintainers may not want C++isms (even though in this case
>>>     it's very localized, and in my opinion very transparent), and
>
> Even if you prefer a C++isms, why don't you go for "hash-table.h"?
> std::map at least with the default allocator will just crash the compiler
> if malloc returns NULL (remember that we build with -fno-exceptions),
> while when you use hash-table.h (or hashtab.h) you get proper OOM diagnostics.

I wasn't aware of the OOM problem.  Couldn't gcc install a default 
memory handler that gives the correct diagnostics?  That certainly 
sounds like the most sensible solution, but I don't know if it's possible.

 From looking over hash-table.h, I dislike two feature about it, one 
aesthetical, one practical: 1) I need to use a callback function during 
the iteration, which is less transparent than the for() loop and 2) I 
can't read from the comments whether traversal is ordered (I don't think 
it is, but ordered traversal is the whole point of my patch).

Cheers,
- Tobi
Tobias Schlüter - Oct. 15, 2012, 12:53 p.m.
Hi Janne,

thanks for the review.

On 2012-10-14 23:35, Janne Blomqvist wrote:
> - Personally, I'd prefer the C++ version; The C++ standard library is
> widely used and documented and using it in favour of rolling our own
> is IMHO a good idea.
>
> - I'd be vary wrt backporting, in my experience the module.c code is
> somewhat fragile and easily causes regressions. In any case, AFAICS PR
> 51727 is not a regression.

Ok to both.  The bug is a real problem, and I don't think the patch is 
at all dangerous, but it's good to have a second opinion.

> - I think one could go a step further and get rid of the BBT stuff in
> pointer_info, replacing it with two file-level maps
>
> std::map<void*, pointer_info*> pmap; // Or could be std::unordered_map
> if available
> std::map<int, pointer_info*> imap;
>
> So when writing a module, use pmap similar to how pointer_info BBT is
> used now, and then use imap to get a consistent order per your patch.
> When reading, lookup/create mostly via imap, creating a pmap entry
> also when creating a new imap entry; this avoids having to do a
> brute-force search when looking up via pointer when reading (see
> find_pointer2()).
>
> (This 3rd point is mostly an idea for further work, and is not meant
> as a requirement for accepting the patch)

Of course the BBT is equivalent to a map (or hash-table) with different 
syntax, but I agree that it would be nice to enhance the code to do away 
with brute-force searching.

> Ok for trunk, although wait for a few days in case there is a storm of
> protest on the C vs. C++ issue from other gfortran maintainers.

Yes, of course, we don't want to end up in a situation where gfortran 
maintainers suddenly need to know C, Fortran and all of C++, so I want 
to be careful about this patch.  Besides that concern, I'll also wait 
until I get more input concerning the issue that Jakub raised.

Cheers,
- Tobi
Tobias Schlüter - Oct. 15, 2012, 5:47 p.m.
On 2012-10-14 23:44, Jakub Jelinek wrote:
> On Mon, Oct 15, 2012 at 12:35:27AM +0300, Janne Blomqvist wrote:
>> On Sat, Oct 13, 2012 at 4:26 PM, Tobias Schlüter
>>> I'm putting forward two patches.  One uses a C++ map to very concisely build
>>> up and handle the ordered list of symbols.  This has three problems:
>>> 1) gfortran maintainers may not want C++isms (even though in this case
>>>     it's very localized, and in my opinion very transparent), and
>
> Even if you prefer a C++isms, why don't you go for "hash-table.h"?
> std::map at least with the default allocator will just crash the compiler
> if malloc returns NULL (remember that we build with -fno-exceptions),
> while when you use hash-table.h (or hashtab.h) you get proper OOM diagnostics.

I don't know these parts of C++ very well, but maybe an easy fix, 
addressing this once and for all, would be doing the equivalent of 
"set_new_handler (gcc_unreachable)" (or maybe a wrapper around fatal 
("out of memory")?) at some point during gcc's initialization?  This 
should have the desired effect, shouldn't it?

Cheers,
- Tobi
Tobias Schlüter - Nov. 8, 2012, 7:02 p.m.
Hi all,

On 2012-10-14 23:35, Janne Blomqvist wrote:
> On Sat, Oct 13, 2012 at 4:26 PM, Tobias Schlüter
> <tobias.schlueter@physik.uni-muenchen.de> wrote:
> - Personally, I'd prefer the C++ version; The C++ standard library is
> widely used and documented and using it in favour of rolling our own
> is IMHO a good idea.

After receiving no reply for two weeks and two pings of my attempts at 
addressing Jakub's complaint about error handling in the STL (which, I 
might add, is explicitly allowed by the coding standard) went 
unanswered, I committed the C-only version after re-testing.

It's my first commit by means of git, please alert me of any problems.

Cheers,
- Tobi
Tobias Schlüter - Nov. 29, 2012, 10:40 a.m.
Dear gfortraners,

On 2012-10-14 23:35, Janne Blomqvist wrote:
> - I'd be vary wrt backporting, in my experience the module.c code is
> somewhat fragile and easily causes regressions. In any case, AFAICS PR
> 51727 is not a regression.

Can this be reconsidered, as the benefits for users seem to be fairly 
large?  According to bugzilla, the patch didn't cause any regressions 
after three weeks in the trunk.  Joost has used this patch in 
hand-patched 4.7 compilers for a few weeks more with no negative effects.

Cheers,
- Tobi

Patch

diff --git a/gcc/fortran/module.c b/gcc/fortran/module.c
index 5cfc335..4cfcae4 100644
--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -5150,32 +5150,122 @@  write_symbol0 (gfc_symtree *st)
 }
 
 
-/* Recursive traversal function to write the secondary set of symbols
-   to the module file.  These are symbols that were not public yet are
-   needed by the public symbols or another dependent symbol.  The act
-   of writing a symbol can modify the pointer_info tree, so we cease
-   traversal if we find a symbol to write.  We return nonzero if a
-   symbol was written and pass that information upwards.  */
+/* Type for the temporary tree used when writing secondary symbols.  */
+
+struct sorted_pointer_info
+{
+  BBT_HEADER (sorted_pointer_info);
+
+  pointer_info *p;
+};
+
+#define gfc_get_sorted_pointer_info() XCNEW (sorted_pointer_info)
+
+/* Recursively traverse the temporary tree, free its contents.  */
+
+static void
+free_sorted_pointer_info_tree (sorted_pointer_info *p)
+{
+  if (!p)
+    return;
+
+  free_sorted_pointer_info_tree (p->left);
+  free_sorted_pointer_info_tree (p->right);
+
+  free (p);
+}
+
+/* Comparison function for the temporary tree.  */
 
 static int
-write_symbol1 (pointer_info *p)
+compare_sorted_pointer_info (void *_spi1, void *_spi2)
 {
-  int result;
+  sorted_pointer_info *spi1, *spi2;
+  spi1 = (sorted_pointer_info *)_spi1;
+  spi2 = (sorted_pointer_info *)_spi2;
 
+  if (spi1->p->integer < spi2->p->integer)
+    return -1;
+  if (spi1->p->integer > spi2->p->integer)
+    return 1;
+  return 0;
+}
+
+
+/* Finds the symbols that need to be written and collects them in the
+   sorted_pi tree so that they can be traversed in an order
+   independent of memory addresses.  */
+
+static void
+find_symbols_to_write(sorted_pointer_info **tree, pointer_info *p)
+{
+  if (!p)
+    return;
+
+  if (p->type == P_SYMBOL && p->u.wsym.state == NEEDS_WRITE)
+    {
+      sorted_pointer_info *sp = gfc_get_sorted_pointer_info();
+      sp->p = p; 
+ 
+      gfc_insert_bbt (tree, sp, compare_sorted_pointer_info);
+   }
+
+  find_symbols_to_write (tree, p->left);
+  find_symbols_to_write (tree, p->right);
+}
+
+
+/* Recursive function that traverses the tree of symbols that need to be
+   written and writes them in order.  */
+
+static void
+write_symbol1_recursion (sorted_pointer_info *sp)
+{
+  if (!sp)
+    return;
+
+  write_symbol1_recursion (sp->left);
+
+  pointer_info *p1 = sp->p;
+  gcc_assert (p1->type == P_SYMBOL && p1->u.wsym.state == NEEDS_WRITE);
+
+  p1->u.wsym.state = WRITTEN;
+  write_symbol (p1->integer, p1->u.wsym.sym);
+ 
+  write_symbol1_recursion (sp->right);
+}
+
+
+/* Write the secondary set of symbols to the module file.  These are
+   symbols that were not public yet are needed by the public symbols
+   or another dependent symbol.  The act of writing a symbol can add
+   symbols to the pointer_info tree, so we return nonzero if a symbol
+   was written and pass that information upwards.  The caller will
+   then call this function again until nothing was written.  It uses
+   the utility functions and a temporary tree to ensure a reproducible
+   ordering of the symbol output and thus the module file.  */
+
+static int
+write_symbol1 (pointer_info *p)
+{
   if (!p)
     return 0;
 
-  result = write_symbol1 (p->left);
+  /* Put symbols that need to be written into a tree sorted on the
+     integer field.  */
 
-  if (!(p->type != P_SYMBOL || p->u.wsym.state != NEEDS_WRITE))
-    {
-      p->u.wsym.state = WRITTEN;
-      write_symbol (p->integer, p->u.wsym.sym);
-      result = 1;
-    }
+  sorted_pointer_info *spi_root = NULL;
+  find_symbols_to_write (&spi_root, p);
+
+  /* No symbols to write, return.  */
+  if (!spi_root)
+    return 0;
+
+  /* Otherwise, write and free the tree again.  */
+  write_symbol1_recursion (spi_root);
+  free_sorted_pointer_info_tree (spi_root);
 
-  result |= write_symbol1 (p->right);
-  return result;
+  return 1;
 }
 
 
@@ -5205,19 +5295,18 @@  write_generic (gfc_symtree *st)
     return;
 
   write_generic (st->left);
-  write_generic (st->right);
 
   sym = st->n.sym;
-  if (!sym || check_unique_name (st->name))
-    return;
-
-  if (sym->generic == NULL || !gfc_check_symbol_access (sym))
-    return;
+  if (sym && !check_unique_name (st->name)
+      && sym->generic && gfc_check_symbol_access (sym))
+    {
+      if (!sym->module)
+	sym->module = module_name;
 
-  if (sym->module == NULL)
-    sym->module = module_name;
+      mio_symbol_interface (&st->name, &sym->module, &sym->generic);
+    }
 
-  mio_symbol_interface (&st->name, &sym->module, &sym->generic);
+  write_generic (st->right);
 }