[pph] Fix var order when streaming in.

[Gabriel Charette, 2011-06-28 00:27:04]

The names and namespaces chains are built by adding each new element to the
front of the list. When streaming it in we traverse the list of names and re-add
them to the current chains; thus reversing the order in which they were defined
in the header file.

Since this is a singly linked-list we cannot start from the tail; thus we
reverse the chain in place and then traverse it, now adding the bindings in the
same order they were found in the header file.

I introduced a new failing test to test this. The test showed the reverse
behaviour prior to the patch.
The test still fails however, there is another inversion problem between the
global variables and the .LFBO, .LCFI0, ...
This patch only fixes the inversion of the global variables declarations in the
assembly, not the second issue this is exposing.
This second issue is potentially already exposed by another test?? Do we need
this new test?

This fixes all of the assembly mismatches in c1limits-externalid.cc however!

Tested with bootstrap build and pph regression testing.

2011-06-27  Gabriel Charette  <gchare@google.com>

	* pph-streamer-in.c (pph_add_bindings_to_namespace): Reverse names and
	namespaces chains.

	* g++.dg/pph/c1limits-externalid.cc: Remove pph asm xdiff.
	* g++.dg/pph/c1varorder.cc: New.
	* g++.dg/pph/c1varorder.h: New.
	* g++.dg/pph/pph.map: Add c1varorder.h

diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index 93b5685..cfa2ce5 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -1141,6 +1141,11 @@ pph_add_bindings_to_namespace (struct cp_binding_level
*bl, tree ns)
 {
   tree t, chain;
 
+  /* The chains are built backwards (ref: add_decl_to_level@name-lookup.c),
+     reverse them before putting them back in.  */
+  bl->names = nreverse (bl->names);
+  bl->namespaces = nreverse (bl->namespaces);
+
   for (t = bl->names; t; t = chain)
     {
       /* Pushing a decl into a scope clobbers its DECL_CHAIN.
diff --git a/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc
b/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc
index 8d2da40..8b5039c 100644
--- a/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc
+++ b/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc
@@ -1,2 +1 @@
-// pph asm xdiff
 #include "c1limits-externalid.h"
diff --git a/gcc/testsuite/g++.dg/pph/c1varorder.cc
b/gcc/testsuite/g++.dg/pph/c1varorder.cc
new file mode 100644
index 0000000..2db8209
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pph/c1varorder.cc
@@ -0,0 +1,7 @@
+#include "c1varorder.h"
+// pph asm xdiff
+
+int foo(void)
+{
+  return var1 - var2;
+}
diff --git a/gcc/testsuite/g++.dg/pph/c1varorder.h
b/gcc/testsuite/g++.dg/pph/c1varorder.h
new file mode 100644
index 0000000..a87c80c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pph/c1varorder.h
@@ -0,0 +1,7 @@
+#ifndef __C1VARORDER_H
+#define __C1VARORDER_H
+
+int var1;
+int var2;
+
+#endif
diff --git a/gcc/testsuite/g++.dg/pph/pph.map b/gcc/testsuite/g++.dg/pph/pph.map
index 2735af8..2ed5bf2 100644
--- a/gcc/testsuite/g++.dg/pph/pph.map
+++ b/gcc/testsuite/g++.dg/pph/pph.map
@@ -25,6 +25,7 @@ c1simple2.h	c1simple2.pph
 c1struct.h	c1struct.pph
 c1typerefs.h	c1typerefs.pph
 c1variables.h	c1variables.pph
+c1varorder.h	c1varorder.pph
 c2builtin1.h	c2builtin1.pph
 c2builtin2.h	c2builtin2.pph
 c2builtin3.h	c2builtin3.pph

--
This patch is available for review at http://codereview.appspot.com/4635074

[Diego Novillo, 2011-06-28 11:27:41]

On 2011/06/28 00:27:04, Gabriel Charette wrote:
> The names and namespaces chains are built by adding each new element to the
> front of the list. When streaming it in we traverse the list of names and
re-add
> them to the current chains; thus reversing the order in which they were
defined
> in the header file.
> 
> Since this is a singly linked-list we cannot start from the tail; thus we
> reverse the chain in place and then traverse it, now adding the bindings in
the
> same order they were found in the header file.
> 
> I introduced a new failing test to test this. The test showed the reverse
> behaviour prior to the patch.
> The test still fails however, there is another inversion problem between the
> global variables and the .LFBO, .LCFI0, ...
> This patch only fixes the inversion of the global variables declarations in
the
> assembly, not the second issue this is exposing.
> This second issue is potentially already exposed by another test?? Do we need
> this new test?

It can't hurt.

> This fixes all of the assembly mismatches in c1limits-externalid.cc however!

Nice!

> 2011-06-27  Gabriel Charette  <mailto:gchare@google.com>
> 
> 	* pph-streamer-in.c (pph_add_bindings_to_namespace): Reverse names and
> 	namespaces chains.
> 
> 	* g++.dg/pph/c1limits-externalid.cc: Remove pph asm xdiff.
> 	* g++.dg/pph/c1varorder.cc: New.
> 	* g++.dg/pph/c1varorder.h: New.
> 	* g++.dg/pph/pph.map: Add c1varorder.h

OK with a minor comment nit.

[Diego Novillo, 2011-06-28 11:27:56]

http://codereview.appspot.com/4635074/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4635074/diff/1/gcc/cp/pph-streamer-in.c#newcode...
gcc/cp/pph-streamer-in.c:1144: /* The chains are built backwards (ref:
add_decl_to_level@name-lookup.c),
> 1143 
> 1144   /* The chains are built backwards (ref:
add_decl_to_level@name-lookup.c),

s/add_decl_to_level@name-lookup.c/add_decl_to_level/

[Gabriel Charette, 2011-06-28 17:42:17]

On 2011/06/28 11:27:56, Diego Novillo wrote:
> http://codereview.appspot.com/4635074/diff/1/gcc/cp/pph-streamer-in.c
> File gcc/cp/pph-streamer-in.c (right):
> 
>
http://codereview.appspot.com/4635074/diff/1/gcc/cp/pph-streamer-in.c#newcode...
> gcc/cp/pph-streamer-in.c:1144: /* The chains are built backwards (ref:
> mailto:add_decl_to_level@name-lookup.c),
> > 1143 
> > 1144   /* The chains are built backwards (ref:
> mailto:add_decl_to_level@name-lookup.c),
> 
> s/add_decl_to_level@name-lookup.c/add_decl_to_level/

Done.

Commited as r175592.

Gab