Issue 4661045: [pph] Stream scope_chain->bindings instead of global namespace

Gabriel Charette

We were streaming out the whole global namespace tree and only using its bindings on ...

13 years, 9 months ago (2011-06-22 19:53:39 UTC) #1

We were streaming out the whole global namespace tree and only using its
bindings on the way in. These bindings (defined by NAMESPACE_LEVEL
(global_namespace))
are the same as scope_chain->bindings.

Stream those bindings only instead.

This also allows us to append the global_namespace itself to the lto cache
early so that anything pointing to global namespace in the underlying
structure of the bindings (and anything else potentially?!) will point to the
actual global namespace when streamed back in (as opposed to the stale version
we were streaming in).

Can someone confirm that we really didn't need to stream anything but the
bindings from the global_namespace?

This patch also changes the behaviour of the dumper (used for debug only) in
that it dumps the global namespace on read AFTER the bindings were merged with
the current global namespace; as opposed to the prior behaviour which was
to dump the namespace READ in.
This is actually a good thing, because I just realized some of the bindings
are read, but not merged correctly (working on that next), and this exposes
it.

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

	* gcc/cp/pph-streamer-in.c (pph_add_names_to_namespace): Replaced by
	pph_add_bindings_to_namespace.
	(pph_add_bindings_to_namespace): New.
	(pph_in_scope_chain): New.
	(pph_read_file_contents): Remove unused variable file_ns.
	(pph_read_file_contents): Call pph_in_scope_chain.
	* gcc/cp/pph-streamer-out.c (pph_out_scope_chain): New.
	(pph_write_file_contents): Call pph_out_scope_chain.
	* gcc/cp/pph-streamer.c (pph_preload_common_nodes):
	Call lto_streamer_cache_append.

diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index e71f744..2e3545a 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -976,15 +976,14 @@ pph_in_lang_type (pph_stream *stream)
 }
 
 
-/* Add all the new names declared in NEW_NS to NS.  */
+/* Add all bindings declared in BL to NS.  */
 
 static void
-pph_add_names_to_namespace (tree ns, tree new_ns)
+pph_add_bindings_to_namespace (struct cp_binding_level *bl, tree ns)
 {
   tree t, chain;
-  struct cp_binding_level *level = NAMESPACE_LEVEL (new_ns);
 
-  for (t = level->names; t; t = chain)
+  for (t = bl->names; t; t = chain)
     {
       /* Pushing a decl into a scope clobbers its DECL_CHAIN.
 	 Preserve it.  */
@@ -992,18 +991,35 @@ pph_add_names_to_namespace (tree ns, tree new_ns)
       pushdecl_into_namespace (t, ns);
     }
 
-  for (t = level->namespaces; t; t = chain)
+  for (t = bl->namespaces; t; t = chain)
     {
       /* Pushing a decl into a scope clobbers its DECL_CHAIN.
 	 Preserve it.  */
       /* FIXME pph: we should first check to see if it isn't already there.  */
       chain = DECL_CHAIN (t);
       pushdecl_into_namespace (t, ns);
-      pph_add_names_to_namespace (t, t);
+      /* FIXME pph: this carried over from pph_add_names_to_namespace,
+	 it only makes sense to use this when merging names in an existing
+	 namespace.
+      pph_add_bindings_to_namespace (NAMESPACE_LEVEL (t), t);  */
     }
 }
 
 
+/* Merge scope_chain bindings from the stream into SS. */
+
+static void
+pph_in_scope_chain (pph_stream *stream)
+{
+  struct cp_binding_level *pph_bindings;
+
+  pph_bindings = pph_in_binding_level (stream);
+
+  /* Merge the bindings obtained from STREAM in the global namespace.  */
+  pph_add_bindings_to_namespace (pph_bindings, global_namespace);
+}
+
+
 /* Wrap a macro DEFINITION for printing in an error.  */
 
 static char *
@@ -1128,7 +1144,6 @@ pph_read_file_contents (pph_stream *stream)
   cpp_ident_use *bad_use;
   const char *cur_def;
   cpp_idents_used idents_used;
-  tree file_ns;
 
   pth_load_identifiers (&idents_used, stream);
 
@@ -1141,12 +1156,12 @@ pph_read_file_contents (pph_stream *stream)
   /* Re-instantiate all the pre-processor symbols defined by STREAM.  */
   cpp_lt_replay (parse_in, &idents_used);
 
-  /* Read global_namespace from STREAM and add all the names defined
-     there to the current global_namespace.  */
-  file_ns = pph_in_tree (stream);
+  /* Read the bindings from STREAM and merge them with the current bindings. 
*/
+  pph_in_scope_chain (stream);
+
   if (flag_pph_dump_tree)
-    pph_dump_namespace (pph_logfile, file_ns);
-  pph_add_names_to_namespace (global_namespace, file_ns);
+    pph_dump_namespace (pph_logfile, global_namespace);
+
   keyed_classes = pph_in_tree (stream);
   unemitted_tinfo_decls = pph_in_tree_vec (stream);
   /* FIXME pph: This call replaces the tinfo, we should merge instead.
diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index 3187338..691cfdd 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -935,6 +935,33 @@ pph_out_lang_type (pph_stream *stream, tree type, bool
ref_p)
 }
 
 
+/* Write saved_scope information stored in SS, does NOT output all fields,
+   meant to be used for the global variable "scope_chain" only.
+   Output bindings as references if REF_P is true.  */
+
+static void
+pph_out_scope_chain (pph_stream *stream, struct saved_scope *ss, bool ref_p)
+{
+  /* old_namespace should be global_namespace and all entries listed below
+     should be NULL or 0; otherwise the header parsed was incomplete.  */
+  gcc_assert ( ss->old_namespace == global_namespace
+      && !(ss->class_name || ss->class_type || ss->access_specifier
+	   || ss->function_decl || ss->template_parms || ss->x_saved_tree
+	   || ss->class_bindings || ss->prev || ss->unevaluated_operand
+	   || ss->inhibit_evaluation_warnings
+	   || ss->x_processing_template_decl
+	   || ss->x_processing_specialization
+	   || ss->x_processing_explicit_instantiation
+	   || ss->need_pop_function_context
+	   || ss->x_stmt_tree.x_cur_stmt_list
+	   || ss->x_stmt_tree.stmts_are_full_exprs_p));
+
+  /* We only need to write out the bindings, everything else should
+     be NULL or be some temporary disposable state.  */
+  pph_out_binding_level (stream, ss->bindings, ref_p);
+}
+
+
 /* Save the IDENTIFIERS to the STREAM.  */
 
 static void
@@ -991,9 +1018,9 @@ static void
 pph_write_file_contents (pph_stream *stream, cpp_idents_used *idents_used)
 { 
   pth_save_identifiers (idents_used, stream);
+  pph_out_scope_chain (stream, scope_chain, false);
   if (flag_pph_dump_tree)
     pph_dump_namespace (pph_logfile, global_namespace);
-  pph_out_tree (stream, global_namespace, false);
   pph_out_tree (stream, keyed_classes, false);
   pph_out_tree_vec (stream, unemitted_tinfo_decls, false);
 }
diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
index 7c9b862..1e0a8c4 100644
--- a/gcc/cp/pph-streamer.c
+++ b/gcc/cp/pph-streamer.c
@@ -31,6 +31,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-pass.h"
 #include "version.h"
 #include "cppbuiltin.h"
+#include "name-lookup.h"
 
 /* Return true if the given tree T is streamable.  */
 
@@ -78,6 +79,8 @@ pph_preload_common_nodes (struct lto_streamer_cache_d *cache)
   for (i = 0; i < CTI_MAX; i++)
     if (c_global_trees[i])
       lto_streamer_cache_append (cache, c_global_trees[i]);
+
+  lto_streamer_cache_append (cache, global_namespace);
 }
 
 

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

Sign in to reply to this message.

Diego Novillo

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-in.c File gcc/cp/pph-streamer-in.c (right): http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-in.c#newcode1003 gcc/cp/pph-streamer-in.c:1003: namespace. 1001 /* FIXME pph: this carried over from ...

13 years, 9 months ago (2011-06-22 20:51:58 UTC) #2

Gabriel Charette

So it looks like my mail using the upload script didn't make it out... let ...

13 years, 9 months ago (2011-06-22 22:25:06 UTC) #3

So it looks like my mail using the upload script didn't make it out... let me
retype it...

On 2011/06/22 20:51:58, Diego Novillo wrote:
> http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-in.c
> File gcc/cp/pph-streamer-in.c (right):
> 
>
http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-in.c#newcode...
> gcc/cp/pph-streamer-in.c:1003: namespace.
>  1001       /* FIXME pph: this carried over from pph_add_names_to_namespace,
>  1002 »        it only makes sense to use this when merging names in an
existing
>  1003 »        namespace.
> 
> pph_add_names_to_namespace does not exist anymore.  I don't understand this
> comment.

What I meant is that pph_add_bindings_to_namespace is essentially just a
modified version
of pph_add_names_to_namespace which used to do this recursive call (which was
useless as it
only makes sense to add the bindings from the "streamed in" namespace if we
found a corresponding
existing namespace, adding the bindings streamed in to itself makes no sense (as
they're already there)... and commenting out that line in the old code wouldn't
change anything in the test results, proving my point.)

I fixed the comment: removing it and elaborating on the FIXME above it
mentioning that
we need to look if the namespace already exists.

> 
> http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c
> File gcc/cp/pph-streamer-out.c (right):
> 
>
http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c#newcod...
> gcc/cp/pph-streamer-out.c:947: gcc_assert ( ss->old_namespace ==
> global_namespace
>   945   /* old_namespace should be global_namespace and all entries listed
below
>   946      should be NULL or 0; otherwise the header parsed was incomplete. 
*/
>   947   gcc_assert ( ss->old_namespace == global_namespace
> 
> No space after '('.

FIXED.

> 
>
http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c#newcod...
> gcc/cp/pph-streamer-out.c:949: || ss->function_decl || ss->template_parms ||
> ss->x_saved_tree
>   948       && !(ss->class_name || ss->class_type || ss->access_specifier
>   949            || ss->function_decl || ss->template_parms ||
ss->x_saved_tree
> 
> Align '&&' vertically with the open brace.

FIXED.

> 
> http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer.c
> File gcc/cp/pph-streamer.c (right):
> 
> http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer.c#newcode34
> gcc/cp/pph-streamer.c:34: #include "name-lookup.h"
>   33 #include "cppbuiltin.h"
> + 34 #include "name-lookup.h"
> 
> You also need to add cp/name-lookup.h to the list of dependencies for cp/pph.o
> in cp/Make-lang.in.

I removed the include, I originally included it because that's where
global_namespace is defined, but it compiles without it (which I guess is fine
if we don't need to stick to "include what you use").

Sign in to reply to this message.

Diego Novillo

On Wed, Jun 22, 2011 at 18:25, <gchare@google.com> wrote: > I fixed the comment: removing ...

13 years, 9 months ago (2011-06-23 14:38:12 UTC) #4

Diego Novillo

I've made a couple of minor edits to comments and formatting and committed to the ...

13 years, 9 months ago (2011-06-23 17:21:36 UTC) #5

I've made a couple of minor edits to comments and formatting and
committed to the branch (final patch below).


Diego.

commit 2f0fa40cd3e0c9debb4efae6c65530a7c6d3fb0f
Author: dnovillo <dnovillo@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Jun 23 17:18:27 2011 +0000

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

        * pph-streamer-in.c (pph_add_names_to_namespace): Replaced by
        pph_add_bindings_to_namespace.
        (pph_add_bindings_to_namespace): New.
        (pph_in_scope_chain): New.
        (pph_read_file_contents): Remove unused variable file_ns.
        (pph_read_file_contents): Call pph_in_scope_chain.
        * pph-streamer-out.c (pph_out_scope_chain): New.
        (pph_write_file_contents): Call pph_out_scope_chain.
        * pph-streamer.c (pph_preload_common_nodes):
        Call lto_streamer_cache_append.

    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/pph@175346
138bc75d-0d04-0410-961f-82ee72b054a4

diff --git a/gcc/cp/ChangeLog.pph b/gcc/cp/ChangeLog.pph
index 8cdc575..24e1584 100644
--- a/gcc/cp/ChangeLog.pph
+++ b/gcc/cp/ChangeLog.pph
@@ -1,5 +1,18 @@
 2011-06-22  Gabriel Charette  <gchare@google.com>

+       * pph-streamer-in.c (pph_add_names_to_namespace): Replaced by
+       pph_add_bindings_to_namespace.
+       (pph_add_bindings_to_namespace): New.
+       (pph_in_scope_chain): New.
+       (pph_read_file_contents): Remove unused variable file_ns.
+       (pph_read_file_contents): Call pph_in_scope_chain.
+       * pph-streamer-out.c (pph_out_scope_chain): New.
+       (pph_write_file_contents): Call pph_out_scope_chain.
+       * pph-streamer.c (pph_preload_common_nodes):
+       Call lto_streamer_cache_append.
+
+2011-06-22  Gabriel Charette  <gchare@google.com>
+
        * pph-streamer-out.c (pph_out_lang_specific): Removed extra space.
        (pph_write_tree): Removed extra space.

diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index 0e8c6bf..7d501ef 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -977,15 +977,14 @@ pph_in_lang_type (pph_stream *stream)
 }


-/* Add all the new names declared in NEW_NS to NS.  */
+/* Add all bindings declared in BL to NS.  */


 static void
-pph_add_names_to_namespace (tree ns, tree new_ns)
+pph_add_bindings_to_namespace (struct cp_binding_level *bl, tree ns)
 {
   tree t, chain;
-  struct cp_binding_level *level = NAMESPACE_LEVEL (new_ns);

-  for (t = level->names; t; t = chain)
+  for (t = bl->names; t; t = chain)
     {
       /* Pushing a decl into a scope clobbers its DECL_CHAIN.
         Preserve it.  */
@@ -993,18 +992,34 @@ pph_add_names_to_namespace (tree ns, tree new_ns)
       pushdecl_into_namespace (t, ns);
     }

-  for (t = level->namespaces; t; t = chain)
+  for (t = bl->namespaces; t; t = chain)
     {
       /* Pushing a decl into a scope clobbers its DECL_CHAIN.
         Preserve it.  */
-      /* FIXME pph: we should first check to see if it isn't already there.  */
       chain = DECL_CHAIN (t);
+
+      /* FIXME pph: we should first check to see if it isn't already there.
+        If it is, we should use this function recursively to merge
+        the bindings in T in the corresponding namespace.  */
       pushdecl_into_namespace (t, ns);
-      pph_add_names_to_namespace (t, t);
     }
 }


+/* Merge scope_chain bindings from STREAM into global_namespace. */
+
+static void
+pph_in_scope_chain (pph_stream *stream)
+{
+  struct cp_binding_level *pph_bindings;
+
+  pph_bindings = pph_in_binding_level (stream);
+
+  /* Merge the bindings obtained from STREAM in the global namespace.  */
+  pph_add_bindings_to_namespace (pph_bindings, global_namespace);
+}
+
+
 /* Wrap a macro DEFINITION for printing in an error.  */

 static char *
@@ -1129,7 +1144,6 @@ pph_read_file_contents (pph_stream *stream)
   cpp_ident_use *bad_use;
   const char *cur_def;
   cpp_idents_used idents_used;
-  tree file_ns;

   pth_load_identifiers (&idents_used, stream);

@@ -1142,16 +1156,16 @@ pph_read_file_contents (pph_stream *stream)
   /* Re-instantiate all the pre-processor symbols defined by STREAM.  */
   cpp_lt_replay (parse_in, &idents_used);

-  /* Read global_namespace from STREAM and add all the names defined
-     there to the current global_namespace.  */
-  file_ns = pph_in_tree (stream);
+  /* Read the bindings from STREAM and merge them with the current
bindings.  */
+  pph_in_scope_chain (stream);
+
   if (flag_pph_dump_tree)
-    pph_dump_namespace (pph_logfile, file_ns);
-  pph_add_names_to_namespace (global_namespace, file_ns);

+    pph_dump_namespace (pph_logfile, global_namespace);
+
   keyed_classes = pph_in_tree (stream);
-  unemitted_tinfo_decls = pph_in_tree_vec (stream);
   /* FIXME pph: This call replaces the tinfo, we should merge instead.
-     See pph_in_tree_VEC.  */
+     See pph_in_tree_vec.  */
+  unemitted_tinfo_decls = pph_in_tree_vec (stream);
 }


diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index f219cef..dd26571 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -912,6 +912,35 @@ pph_out_lang_type (pph_stream *stream, tree type,
bool ref_p)
 }


+/* Write saved_scope information stored in SS into STREAM.
+   This does NOT output all fields, it is meant to be used for the
+   global variable "scope_chain" only.  Output bindings as references
+   if REF_P is true.  */
+
+static void
+pph_out_scope_chain (pph_stream *stream, struct saved_scope *ss, bool ref_p)
+{
+  /* old_namespace should be global_namespace and all entries listed below
+     should be NULL or 0; otherwise the header parsed was incomplete.  */
+  gcc_assert (ss->old_namespace == global_namespace
+             && !(ss->class_name || ss->class_type || ss->access_specifier
+                  || ss->function_decl || ss->template_parms
+                  || ss->x_saved_tree || ss->class_bindings || ss->prev
+                  || ss->unevaluated_operand
+                  || ss->inhibit_evaluation_warnings
+                  || ss->x_processing_template_decl
+                  || ss->x_processing_specialization
+                  || ss->x_processing_explicit_instantiation
+                  || ss->need_pop_function_context
+                  || ss->x_stmt_tree.x_cur_stmt_list
+                  || ss->x_stmt_tree.stmts_are_full_exprs_p));
+
+  /* We only need to write out the bindings, everything else should
+     be NULL or be some temporary disposable state.  */
+  pph_out_binding_level (stream, ss->bindings, ref_p);
+}
+
+
 /* Save the IDENTIFIERS to the STREAM.  */

 static void
@@ -968,9 +997,9 @@ static void
 pph_write_file_contents (pph_stream *stream, cpp_idents_used *idents_used)
 {
   pth_save_identifiers (idents_used, stream);
+  pph_out_scope_chain (stream, scope_chain, false);
   if (flag_pph_dump_tree)
     pph_dump_namespace (pph_logfile, global_namespace);
-  pph_out_tree (stream, global_namespace, false);
   pph_out_tree (stream, keyed_classes, false);
   pph_out_tree_vec (stream, unemitted_tinfo_decls, false);
 }
diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
index 7c9b862..e919baf 100644
--- a/gcc/cp/pph-streamer.c
+++ b/gcc/cp/pph-streamer.c
@@ -78,6 +78,8 @@ pph_preload_common_nodes (struct lto_streamer_cache_d *cache)
   for (i = 0; i < CTI_MAX; i++)
     if (c_global_trees[i])
       lto_streamer_cache_append (cache, c_global_trees[i]);

+
+  lto_streamer_cache_append (cache, global_namespace);
 }

Sign in to reply to this message.

Diego Novillo

On Thu, Jun 23, 2011 at 13:21, Diego Novillo <dnovillo@google.com> wrote: > I've made a ...

13 years, 9 months ago (2011-06-23 17:23:52 UTC) #6

Gabriel Charette

13 years, 9 months ago (2011-06-23 18:38:36 UTC) #7

Yes I did fill the form, included you as an approver, haven't heard
back from it yet.

Gab

On Thu, Jun 23, 2011 at 10:23 AM, Diego Novillo <dnovillo@google.com> wrote:
>
> On Thu, Jun 23, 2011 at 13:21, Diego Novillo <dnovillo@google.com> wrote:
> > I've made a couple of minor edits to comments and formatting and
> > committed to the branch (final patch below).
>
> Incidentally, did you fill-in the svn write access form?  You've
> produced enough good patches already.  Time for you to be able to
> commit your own.
>
>
> Diego.

Sign in to reply to this message.

Issue 4661045: [pph] Stream scope_chain->bindings instead of global namespace (Closed)

Patch Set 1 #

Patch Set 2 : [pph] Stream scope_chain->bindings instead of global namespace #

Messages