Re: [pph] Fix var order when streaming in. (issue4635074)

Re: [pph] Fix var order when streaming in. (issue4635074)

[gchare]

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#newcode1144
> 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


http://codereview.appspot.com/4635074/

Re: [pph] Fix var order when streaming in. (issue4635074)

[dnovillo]

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#newcode1144
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/

http://codereview.appspot.com/4635074/

Re: [pph] Fix var order when streaming in. (issue4635074)

[dnovillo]

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.

http://codereview.appspot.com/4635074/

[pph] Fix var order when streaming in. (issue4635074)

[Gabriel Charette]

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