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

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

[Tobias Schlüter]

[-- Attachment #1: Type: text/plain, Size: 2067 bytes --]


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

[-- Attachment #2: pr51727_strictlyC.diff.txt --]
[-- Type: text/plain, Size: 5250 bytes --]

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.

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);
 }
 
 

[-- Attachment #3: pr51727.diff.txt --]
[-- Type: text/plain, Size: 3194 bytes --]

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);
 }
 
 

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

[Tobias Schlüter]

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

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

[Diego Novillo]

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.

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

[Tobias Schlüter]

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

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

[Diego Novillo]

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.

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

[Joseph S. Myers]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

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).

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Janne Blomqvist]

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.


-- 
Janne Blomqvist

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

[Jakub Jelinek]

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

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

[Tobias Schlüter]

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

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

[Tobias Schlüter]

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

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

[Tobias Schlüter]

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

[PATCH] Install error handler for out-of-memory when using STL containers Re: PR fortran/51727: make module files reproducible, question on C++ in gcc

[Tobias Schlüter]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]


Hi,

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.

The attached patch adds out-of-memory diagnostics for code using STL 
containers by using set_new_handler.  Since the intended allocation size 
is not available to a new_handler, I had to forego a more detailed error 
message such as the one from xmalloc_failed().  fatal_error() and 
abort() don't give a meaningful location when the new_handler is called, 
so I chose to put together the error message manually as is done in 
xmalloc_failed().  I would have found it more appealing to have operator 
new call xmalloc() unless a custom allocator is given, but I don't think 
there's a standard way of doing this.

Built and tested on the C and Fortran testsuites.  Ok for trunk?

Cheers,
- Tobi


[-- Attachment #2: new_handler.diff.txt --]
[-- Type: text/plain, Size: 1320 bytes --]

2012-10-15  Tobias Schlüter  <tobi@gcc.gnu.org>

	* toplev.c: Add '#include <new>'.
	(cxx_out_of_memory): New function.
	(general_init): Install cxx_out_of_memory as handler for
	out-of-memory condition.

diff --git a/gcc/toplev.c b/gcc/toplev.c
index 2c9329f..2e6248a 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -89,6 +89,8 @@ along with GCC; see the file COPYING3.  If not see
 				   declarations for e.g. AIX 4.x.  */
 #endif
 
+#include <new>
+
 static void general_init (const char *);
 static void do_compile (void);
 static void process_options (void);
@@ -1061,6 +1063,21 @@ open_auxiliary_file (const char *ext)
   return file;
 }
 
+
+/* Error handler for use with C++ memory allocation.  Will be
+   installed via std::set_new_handler().  */
+
+static void
+cxx_out_of_memory()
+{
+  fprintf (stderr,
+	   "\n%s%sout of memory\n",
+	   progname, *progname ? ": " : "");
+
+  xexit (1);
+}
+
+
 /* Initialization of the front end environment, before command line
    options are parsed.  Signal handlers, internationalization etc.
    ARGV0 is main's argv[0].  */
@@ -1074,6 +1091,8 @@ general_init (const char *argv0)
     --p;
   progname = p;
 
+  std::set_new_handler (cxx_out_of_memory);
+
   xmalloc_set_program_name (progname);
 
   hex_init ();

Ping: [PATCH] Install error handler for out-of-memory when using STL containers

[Tobias Schlüter]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]


Ping.  This issue stands in the way of a very simple solution of PR 
fortran/51727.  I've re-attached the patch for your convenience.

On 15 Oct 2012 at 22:51:05 +0200 Tobias Schlüter wrote:
> The attached patch adds out-of-memory diagnostics for code using STL
> containers by using set_new_handler. Since the intended allocation
> size is not available to a new_handler, I had to forego a more
> detailed error message such as the one from xmalloc_failed().
> fatal_error() and abort() don't give a meaningful location when the
> new_handler is called, so I chose to put together the error message
> manually as is done in xmalloc_failed(). I would have found it more
> appealing to have operator new call xmalloc() unless a custom
> allocator is given, but I don't think there's a standard way of doing
> this.
>
> Built and tested on the C and Fortran testsuites. Ok for trunk?

Best regards,
- Tobi

2012-10-15  Tobias Schlüter  <tobi@gcc.gnu.org>

	* toplev.c: Add '#include <new>'.
	(cxx_out_of_memory): New function.
	(general_init): Install cxx_out_of_memory as handler for
	out-of-memory condition.

[-- Attachment #2: new_handler.diff.txt --]
[-- Type: text/plain, Size: 1320 bytes --]

2012-10-15  Tobias Schlüter  <tobi@gcc.gnu.org>

	* toplev.c: Add '#include <new>'.
	(cxx_out_of_memory): New function.
	(general_init): Install cxx_out_of_memory as handler for
	out-of-memory condition.

diff --git a/gcc/toplev.c b/gcc/toplev.c
index 2c9329f..2e6248a 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -89,6 +89,8 @@ along with GCC; see the file COPYING3.  If not see
 				   declarations for e.g. AIX 4.x.  */
 #endif
 
+#include <new>
+
 static void general_init (const char *);
 static void do_compile (void);
 static void process_options (void);
@@ -1061,6 +1063,21 @@ open_auxiliary_file (const char *ext)
   return file;
 }
 
+
+/* Error handler for use with C++ memory allocation.  Will be
+   installed via std::set_new_handler().  */
+
+static void
+cxx_out_of_memory()
+{
+  fprintf (stderr,
+	   "\n%s%sout of memory\n",
+	   progname, *progname ? ": " : "");
+
+  xexit (1);
+}
+
+
 /* Initialization of the front end environment, before command line
    options are parsed.  Signal handlers, internationalization etc.
    ARGV0 is main's argv[0].  */
@@ -1074,6 +1091,8 @@ general_init (const char *argv0)
     --p;
   progname = p;
 
+  std::set_new_handler (cxx_out_of_memory);
+
   xmalloc_set_program_name (progname);
 
   hex_init ();

Re: Ping: [PATCH] Install error handler for out-of-memory when using STL containers

[Paul Richard Thomas]

Hi Tobi,

It looks straightforward to me but I am not convinced that a fortran
maintainer should be giving the green light on this :-)

Cheers

Paul

On 28 October 2012 16:28, Tobias Schlüter
<tobias.schlueter@physik.uni-muenchen.de> wrote:
>
> Ping.  This issue stands in the way of a very simple solution of PR
> fortran/51727.  I've re-attached the patch for your convenience.
>
> On 15 Oct 2012 at 22:51:05 +0200 Tobias Schlüter wrote:
>>
>> The attached patch adds out-of-memory diagnostics for code using STL
>> containers by using set_new_handler. Since the intended allocation
>> size is not available to a new_handler, I had to forego a more
>> detailed error message such as the one from xmalloc_failed().
>> fatal_error() and abort() don't give a meaningful location when the
>> new_handler is called, so I chose to put together the error message
>> manually as is done in xmalloc_failed(). I would have found it more
>> appealing to have operator new call xmalloc() unless a custom
>> allocator is given, but I don't think there's a standard way of doing
>> this.
>>
>> Built and tested on the C and Fortran testsuites. Ok for trunk?
>
>
> Best regards,
> - Tobi
>
> 2012-10-15  Tobias Schlüter  <tobi@gcc.gnu.org>
>
>         * toplev.c: Add '#include <new>'.
>         (cxx_out_of_memory): New function.
>         (general_init): Install cxx_out_of_memory as handler for
>         out-of-memory condition.



-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

Re: Ping: [PATCH] Install error handler for out-of-memory when using STL containers

[Mike Stump]

On Oct 29, 2012, at 2:02 AM, Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
> It looks straightforward to me but I am not convinced that a fortran
> maintainer should be giving the green light on this :-)

Well, seems reasonable to me as well…  I'd let Diego or Jason approve it.  :-)

Ping**2: [PATCH] Install error handler for out-of-memory when using STL containers

[Tobias Schlüter]

Hi,

any comments appreciated, also if you think it's unnecessary.  This 
issue came up when I submitted a patch for PR fortran/51727 that uses 
std::map.  If this isn't approved within the next few days, I'll fix PR 
fortran/51727 in a way that doesn't use C++ and thus needs more 
boilerplate code (see 
<http://gcc.gnu.org/ml/gcc-patches/2012-10/msg01284.html> for context.)

I'm CCing Ian as the last person who approved a patch to toplev.c and 
also Jakub as a global write maintainer and as the person who brought up 
this subject.

Link to patch: <http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02515.html>

On 2012-10-28 16:28, Tobias Schlüter wrote:
>
> Ping.  This issue stands in the way of a very simple solution of PR
> fortran/51727.  I've re-attached the patch for your convenience.
>
> On 15 Oct 2012 at 22:51:05 +0200 Tobias Schlüter wrote:
>> The attached patch adds out-of-memory diagnostics for code using STL
>> containers by using set_new_handler. Since the intended allocation
>> size is not available to a new_handler, I had to forego a more
>> detailed error message such as the one from xmalloc_failed().
>> fatal_error() and abort() don't give a meaningful location when the
>> new_handler is called, so I chose to put together the error message
>> manually as is done in xmalloc_failed(). I would have found it more
>> appealing to have operator new call xmalloc() unless a custom
>> allocator is given, but I don't think there's a standard way of doing
>> this.
>>
>> Built and tested on the C and Fortran testsuites. Ok for trunk?
>
> Best regards,
> - Tobi
>
> 2012-10-15  Tobias Schlüter  <tobi@gcc.gnu.org>
>
>      * toplev.c: Add '#include <new>'.
>      (cxx_out_of_memory): New function.
>      (general_init): Install cxx_out_of_memory as handler for
>      out-of-memory condition.

Cheers,
- Tobi

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

[Tobias Schlüter]

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

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

[Tobias Schlüter]

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