[pph] Add alternate addresses to register in the cache

[Diego Novillo, 2011-07-11 21:42:37]

This patch adapts an idea from Gab that allow us to register alternate
addresses in the cache.  The problem here is making sure that symbols
read from a PPH file reference the right bindings.

If a symbol is in the global namespace when compiling a header file,
its bindings will point to NAMESPACE_LEVEL(global_namespace)->bindings,
but that global_namespace is the global_namespace instantiated for the
header file.  When reading that PPH image from a translation unit, we
need to refer to the bindings of the *current* global_namespace.

In general we solve this by inserting the pointer in the streamer
cache.  For instance, to avoid instantiating a second global_namespace
decl, the initialization code of both the writer and the reader store
global_namespace into the streaming cache.  This way, all the
references to global_namespace point to the current global_namespace
as known by the writer and the reader.

However, we cannot use the same trick on the bindings for
global_namespace.  If we simply inserted it into the cache then
writing out NAMESPACE_LEVEL(global_namespace)->bindings would simply
write a reference to the current one and on the reader side, it would
simply restore a pointer to the current translation unit's bindings.
Without ever actually writing or reading anything (since it was
satisified from the cache).

Therefore, we want a mechanism that allows the reader to: (a) read all
the symbols in the global bindings, and (b) references to the
global binding made by the symbols should point to the global bindings
of the current translation unit (instead of the one in the PPH image).

That's where ALLOC_AND_REGISTER_ALTERNATE comes in.  When called, it
allocates the data structure but registers another pointer in the
cache.  We use this trick when calling pph_in_binding_level from the
toplevel:

+  new_bindings = pph_in_binding_level (stream, scope_chain->bindings);

This way, when pph_in_binding_level tries to allocate the binding
structure read from STREAM, it registers scope_chain->bindings in the
cache.  This way, references to the original file's global binding are
automatically redirected to the current translation unit's global
bindings.

Gab, I modified your original implementation to move all the logic to
the place where we need to make this decision.  This way, it is easier
to tell which functions need this alternate registration, instead of
relying on some status flag squirreled away in the STREAM data
structure.


Tested on x86_64.  Applied to branch.

2011-07-11   Diego Novillo  <dnovillo@google.com>
 	     Gabriel Charette  <gchare@google.com>

	* pph-streamer-in.c (ALLOC_AND_REGISTER_ALTERNATE): Define.
	(pph_in_binding_level): Add argument TO_REGISTER.  Call
	ALLOC_AND_REGISTER_ALTERNATE if set.
	Update all users.
	(pph_register_decls_in_symtab): Call varpool_finalize_decl
	on all file-local symbols.
	(pph_in_scope_chain): Call pph_in_binding_level with
	scope_chain->bindings as the alternate pointer to
	register in the streaming cache.

diff --git a/gcc/cp/ChangeLog.pph b/gcc/cp/ChangeLog.pph
index 1011902..f18c2f4 100644
--- a/gcc/cp/ChangeLog.pph
+++ b/gcc/cp/ChangeLog.pph
@@ -1,3 +1,16 @@
+2011-07-11   Diego Novillo  <dnovillo@google.com>
+	     Gabriel Charette  <gchare@google.com>
+
+	* pph-streamer-in.c (ALLOC_AND_REGISTER_ALTERNATE): Define.
+	(pph_in_binding_level): Add argument TO_REGISTER.  Call
+	ALLOC_AND_REGISTER_ALTERNATE if set.
+	Update all users.
+	(pph_register_decls_in_symtab): Call varpool_finalize_decl
+	on all file-local symbols.
+	(pph_in_scope_chain): Call pph_in_binding_level with
+	scope_chain->bindings as the alternate pointer to
+	register in the streaming cache.
+
 2011-07-07   Diego Novillo  <dnovillo@google.com>
 
 	* pph-streamer-in.c (pph_register_decls_in_symtab): Rename
diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index 571ebf5..903cd94 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -42,6 +42,18 @@ along with GCC; see the file COPYING3.  If not see
       pph_register_shared_data (STREAM, DATA, IX);		\
     } while (0)
 
+/* Same as ALLOC_AND_REGISTER, but instead of registering DATA into the
+   cache at slot IX, it registers ALT_DATA.  Used to support mapping
+   pointers to global data in the original STREAM that need to point
+   to a different instance when aggregating individual PPH files into
+   the current translation unit (see pph_in_binding_level for an
+   example).  */
+#define ALLOC_AND_REGISTER_ALTERNATE(STREAM, IX, DATA, ALLOC_EXPR, ALT_DATA)\
+    do {								\
+      (DATA) = (ALLOC_EXPR);						\
+      pph_register_shared_data (STREAM, ALT_DATA, IX);			\
+    } while (0)
+
 /* Callback for unpacking value fields in ASTs.  BP is the bitpack 
    we are unpacking from.  EXPR is the tree to unpack.  */
 
@@ -482,7 +494,8 @@ pph_in_qual_use_vec (pph_stream *stream)
 
 
 /* Forward declaration to break cyclic dependencies.  */
-static struct cp_binding_level *pph_in_binding_level (pph_stream *);
+static struct cp_binding_level *pph_in_binding_level (pph_stream *,
+						    struct cp_binding_level *);
 
 /* Helper for pph_in_cxx_binding.  Read and return a cxx_binding
    instance from STREAM.  */
@@ -505,7 +518,7 @@ pph_in_cxx_binding_1 (pph_stream *stream)
   value = pph_in_tree (stream);
   type = pph_in_tree (stream);
   ALLOC_AND_REGISTER (stream, ix, cb, cxx_binding_make (value, type));
-  cb->scope = pph_in_binding_level (stream);
+  cb->scope = pph_in_binding_level (stream, NULL);
   bp = pph_in_bitpack (stream);
   cb->value_is_inherited = bp_unpack_value (&bp, 1);
   cb->is_local = bp_unpack_value (&bp, 1);
@@ -581,10 +594,26 @@ pph_in_label_binding (pph_stream *stream)
 }
 
 
-/* Read and return an instance of cp_binding_level from STREAM.  */
+/* Read and return an instance of cp_binding_level from STREAM.
+   TO_REGISTER is used when the caller wants to read a binding level,
+   but register a different binding level in the streaming cache.
+   This is needed when reading the binding for global_namespace.
+
+   When the file was created global_namespace was localized to that
+   particular file.  However, when instantiating a PPH file into
+   memory, we are now using the global_namespace for the current
+   translation unit.  Therefore, every reference to the
+   global_namespace and its bindings should be pointing to the
+   CURRENT global namespace, not the one in STREAM.
+
+   Therefore, if TO_REGISTER is set, and we read a binding level from
+   STREAM for the first time, instead of registering the newly
+   instantiated binding level into the cache, we register the binding
+   level given in TO_REGISTER.  This way, subsequent references to the
+   global binding level will be done to the one set in TO_REGISTER.  */
 
 static struct cp_binding_level *
-pph_in_binding_level (pph_stream *stream)
+pph_in_binding_level (pph_stream *stream, struct cp_binding_level *to_register)
 {
   unsigned i, num, ix;
   struct cp_binding_level *bl;
@@ -597,7 +626,14 @@ pph_in_binding_level (pph_stream *stream)
   else if (marker == PPH_RECORD_SHARED)
     return (struct cp_binding_level *) pph_in_shared_data (stream, ix);
 
-  ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level ());
+  /* If TO_REGISTER is set, register that binding level instead of the newly
+     allocated binding level into slot IX.  */
+  if (to_register == NULL)
+    ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level ());
+  else
+    ALLOC_AND_REGISTER_ALTERNATE (stream, ix, bl,
+				  ggc_alloc_cleared_cp_binding_level (),
+				  to_register);
 
   bl->names = pph_in_chain (stream);
   bl->namespaces = pph_in_chain (stream);
@@ -627,7 +663,7 @@ pph_in_binding_level (pph_stream *stream)
 
   bl->blocks = pph_in_chain (stream);
   bl->this_entity = pph_in_tree (stream);
-  bl->level_chain = pph_in_binding_level (stream);
+  bl->level_chain = pph_in_binding_level (stream, NULL);
   bl->dead_vars_from_for = pph_in_tree_vec (stream);
   bl->statement_list = pph_in_chain (stream);
   bl->binding_depth = pph_in_uint (stream);
@@ -713,7 +749,7 @@ pph_in_language_function (pph_stream *stream)
 
   /* FIXME pph.  We are not reading lf->x_named_labels.  */
 
-  lf->bindings = pph_in_binding_level (stream);
+  lf->bindings = pph_in_binding_level (stream, NULL);
   lf->x_local_names = pph_in_tree_vec (stream);
 
   /* FIXME pph.  We are not reading lf->extern_decl_map.  */
@@ -854,7 +890,7 @@ pph_in_struct_function (pph_stream *stream, tree decl)
 static void
 pph_in_ld_ns (pph_stream *stream, struct lang_decl_ns *ldns)
 {
-  ldns->level = pph_in_binding_level (stream);
+  ldns->level = pph_in_binding_level (stream, NULL);
 }
 
 
@@ -1140,10 +1176,10 @@ pph_in_lang_type (pph_stream *stream)
 
 
 /* Register all the symbols in binding level BL in the callgraph symbol
-   table.  NS is the namespace where all the symbols in BL live.  */
+   table.  */
 
 static void
-pph_register_decls_in_symtab (struct cp_binding_level *bl, tree ns)
+pph_register_decls_in_symtab (struct cp_binding_level *bl)
 {
   tree t;
 
@@ -1153,18 +1189,16 @@ pph_register_decls_in_symtab (struct cp_binding_level
*bl, tree ns)
   bl->namespaces = nreverse (bl->namespaces);
 
   for (t = bl->names; t; t = DECL_CHAIN (t))
-    if (DECL_NAME (t) && IDENTIFIER_NAMESPACE_BINDINGS (DECL_NAME (t)))
-      {
-	cxx_binding *b = IDENTIFIER_NAMESPACE_BINDINGS (DECL_NAME (t));
-	b->scope = NAMESPACE_LEVEL (ns);
-
-	if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL (t))
-	  varpool_finalize_decl (t);
-      }
+    {
+      /* Add file-local symbols to the varpool.  */
+      if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL (t))
+	varpool_finalize_decl (t);
+    }
 
+  /* Recurse into the namespaces contained in BL.  */
   for (t = bl->namespaces; t; t = DECL_CHAIN (t))
     if (NAMESPACE_LEVEL (t))
-      pph_register_decls_in_symtab (NAMESPACE_LEVEL (t), t);
+      pph_register_decls_in_symtab (NAMESPACE_LEVEL (t));
 }
 
 
@@ -1173,18 +1207,21 @@ pph_register_decls_in_symtab (struct cp_binding_level
*bl, tree ns)
 static void
 pph_in_scope_chain (pph_stream *stream)
 {
-  struct saved_scope *file_scope_chain;
   unsigned i;
   tree decl;
   cp_class_binding *cb;
   cp_label_binding *lb;
   struct cp_binding_level *cur_bindings, *new_bindings;
 
-  file_scope_chain = ggc_alloc_cleared_saved_scope ();
-  file_scope_chain->bindings = new_bindings = pph_in_binding_level (stream);
+  /* When reading the symbols in STREAM's global binding level, make
+     sure that references to the global binding level point to
+     scope_chain->bindings.  Otherwise, identifiers read from STREAM
+     will have the wrong bindings and will fail name lookups.  */
   cur_bindings = scope_chain->bindings;
+  new_bindings = pph_in_binding_level (stream, scope_chain->bindings);
 
-  pph_register_decls_in_symtab (new_bindings, global_namespace);
+  /* Register all the symbols in STREAM with the call graph.  */
+  pph_register_decls_in_symtab (new_bindings);
 
   /* Merge the bindings from STREAM into saved_scope->bindings.  */
   chainon (cur_bindings->names, new_bindings->names);


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

[Gabriel Charette, 2011-07-12 17:56:55]

I like this implementation!

Only one thing, if we ACTUALLY want "to_register" NULL instead of the read
value we can't as in your current implementation NULL means don't do the
alternate registration.

Could that be a problem or do we expect the structures we pass to this to
always be non-null? (i.e. for scope_chain->bindings this is definitely ok,
but what about other potential uses?), if we keep it this way we should at
least make it clear in the function's comment that NULL means don't do
alternate registration I think.

Gab

On Mon, Jul 11, 2011 at 2:42 PM, Diego Novillo <dnovillo@google.com> wrote:

>
> This patch adapts an idea from Gab that allow us to register alternate
> addresses in the cache.  The problem here is making sure that symbols
> read from a PPH file reference the right bindings.
>
> If a symbol is in the global namespace when compiling a header file,
> its bindings will point to NAMESPACE_LEVEL(global_namespace)->bindings,
> but that global_namespace is the global_namespace instantiated for the
> header file.  When reading that PPH image from a translation unit, we
> need to refer to the bindings of the *current* global_namespace.
>
> In general we solve this by inserting the pointer in the streamer
> cache.  For instance, to avoid instantiating a second global_namespace
> decl, the initialization code of both the writer and the reader store
> global_namespace into the streaming cache.  This way, all the
> references to global_namespace point to the current global_namespace
> as known by the writer and the reader.
>
> However, we cannot use the same trick on the bindings for
> global_namespace.  If we simply inserted it into the cache then
> writing out NAMESPACE_LEVEL(global_namespace)->bindings would simply
> write a reference to the current one and on the reader side, it would
> simply restore a pointer to the current translation unit's bindings.
> Without ever actually writing or reading anything (since it was
> satisified from the cache).
>
> Therefore, we want a mechanism that allows the reader to: (a) read all
> the symbols in the global bindings, and (b) references to the
> global binding made by the symbols should point to the global bindings
> of the current translation unit (instead of the one in the PPH image).
>
> That's where ALLOC_AND_REGISTER_ALTERNATE comes in.  When called, it
> allocates the data structure but registers another pointer in the
> cache.  We use this trick when calling pph_in_binding_level from the
> toplevel:
>
> +  new_bindings = pph_in_binding_level (stream, scope_chain->bindings);
>
> This way, when pph_in_binding_level tries to allocate the binding
> structure read from STREAM, it registers scope_chain->bindings in the
> cache.  This way, references to the original file's global binding are
> automatically redirected to the current translation unit's global
> bindings.
>
> Gab, I modified your original implementation to move all the logic to
> the place where we need to make this decision.  This way, it is easier
> to tell which functions need this alternate registration, instead of
> relying on some status flag squirreled away in the STREAM data
> structure.
>
>
> Tested on x86_64.  Applied to branch.
>
> 2011-07-11   Diego Novillo  <dnovillo@google.com>
>             Gabriel Charette  <gchare@google.com>
>
>        * pph-streamer-in.c (ALLOC_AND_REGISTER_ALTERNATE): Define.
>        (pph_in_binding_level): Add argument TO_REGISTER.  Call
>        ALLOC_AND_REGISTER_ALTERNATE if set.
>        Update all users.
>        (pph_register_decls_in_symtab): Call varpool_finalize_decl
>        on all file-local symbols.
>        (pph_in_scope_chain): Call pph_in_binding_level with
>        scope_chain->bindings as the alternate pointer to
>        register in the streaming cache.
>
> diff --git a/gcc/cp/ChangeLog.pph b/gcc/cp/ChangeLog.pph
> index 1011902..f18c2f4 100644
> --- a/gcc/cp/ChangeLog.pph
> +++ b/gcc/cp/ChangeLog.pph
> @@ -1,3 +1,16 @@
> +2011-07-11   Diego Novillo  <dnovillo@google.com>
> +            Gabriel Charette  <gchare@google.com>
> +
> +       * pph-streamer-in.c (ALLOC_AND_REGISTER_ALTERNATE): Define.
> +       (pph_in_binding_level): Add argument TO_REGISTER.  Call
> +       ALLOC_AND_REGISTER_ALTERNATE if set.
> +       Update all users.
> +       (pph_register_decls_in_symtab): Call varpool_finalize_decl
> +       on all file-local symbols.
> +       (pph_in_scope_chain): Call pph_in_binding_level with
> +       scope_chain->bindings as the alternate pointer to
> +       register in the streaming cache.
> +
>  2011-07-07   Diego Novillo  <dnovillo@google.com>
>
>        * pph-streamer-in.c (pph_register_decls_in_symtab): Rename
> diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
> index 571ebf5..903cd94 100644
> --- a/gcc/cp/pph-streamer-in.c
> +++ b/gcc/cp/pph-streamer-in.c
> @@ -42,6 +42,18 @@ along with GCC; see the file COPYING3.  If not see
>       pph_register_shared_data (STREAM, DATA, IX);             \
>     } while (0)
>
> +/* Same as ALLOC_AND_REGISTER, but instead of registering DATA into the
> +   cache at slot IX, it registers ALT_DATA.  Used to support mapping
> +   pointers to global data in the original STREAM that need to point
> +   to a different instance when aggregating individual PPH files into
> +   the current translation unit (see pph_in_binding_level for an
> +   example).  */
> +#define ALLOC_AND_REGISTER_ALTERNATE(STREAM, IX, DATA, ALLOC_EXPR,
> ALT_DATA)\
> +    do {                                                               \
> +      (DATA) = (ALLOC_EXPR);                                           \
> +      pph_register_shared_data (STREAM, ALT_DATA, IX);                 \
> +    } while (0)
> +
>  /* Callback for unpacking value fields in ASTs.  BP is the bitpack
>    we are unpacking from.  EXPR is the tree to unpack.  */
>
> @@ -482,7 +494,8 @@ pph_in_qual_use_vec (pph_stream *stream)
>
>
>  /* Forward declaration to break cyclic dependencies.  */
> -static struct cp_binding_level *pph_in_binding_level (pph_stream *);
> +static struct cp_binding_level *pph_in_binding_level (pph_stream *,
> +                                                   struct cp_binding_level
> *);
>
>  /* Helper for pph_in_cxx_binding.  Read and return a cxx_binding
>    instance from STREAM.  */
> @@ -505,7 +518,7 @@ pph_in_cxx_binding_1 (pph_stream *stream)
>   value = pph_in_tree (stream);
>   type = pph_in_tree (stream);
>   ALLOC_AND_REGISTER (stream, ix, cb, cxx_binding_make (value, type));
> -  cb->scope = pph_in_binding_level (stream);
> +  cb->scope = pph_in_binding_level (stream, NULL);
>   bp = pph_in_bitpack (stream);
>   cb->value_is_inherited = bp_unpack_value (&bp, 1);
>   cb->is_local = bp_unpack_value (&bp, 1);
> @@ -581,10 +594,26 @@ pph_in_label_binding (pph_stream *stream)
>  }
>
>
> -/* Read and return an instance of cp_binding_level from STREAM.  */
> +/* Read and return an instance of cp_binding_level from STREAM.
> +   TO_REGISTER is used when the caller wants to read a binding level,
> +   but register a different binding level in the streaming cache.
> +   This is needed when reading the binding for global_namespace.
> +
> +   When the file was created global_namespace was localized to that
> +   particular file.  However, when instantiating a PPH file into
> +   memory, we are now using the global_namespace for the current
> +   translation unit.  Therefore, every reference to the
> +   global_namespace and its bindings should be pointing to the
> +   CURRENT global namespace, not the one in STREAM.
> +
> +   Therefore, if TO_REGISTER is set, and we read a binding level from
> +   STREAM for the first time, instead of registering the newly
> +   instantiated binding level into the cache, we register the binding
> +   level given in TO_REGISTER.  This way, subsequent references to the
> +   global binding level will be done to the one set in TO_REGISTER.  */
>
>  static struct cp_binding_level *
> -pph_in_binding_level (pph_stream *stream)
> +pph_in_binding_level (pph_stream *stream, struct cp_binding_level
> *to_register)
>  {
>   unsigned i, num, ix;
>   struct cp_binding_level *bl;
> @@ -597,7 +626,14 @@ pph_in_binding_level (pph_stream *stream)
>   else if (marker == PPH_RECORD_SHARED)
>     return (struct cp_binding_level *) pph_in_shared_data (stream, ix);
>
> -  ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level
> ());
> +  /* If TO_REGISTER is set, register that binding level instead of the
> newly
> +     allocated binding level into slot IX.  */
> +  if (to_register == NULL)
> +    ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level
> ());
> +  else
> +    ALLOC_AND_REGISTER_ALTERNATE (stream, ix, bl,
> +                                 ggc_alloc_cleared_cp_binding_level (),
> +                                 to_register);
>
>   bl->names = pph_in_chain (stream);
>   bl->namespaces = pph_in_chain (stream);
> @@ -627,7 +663,7 @@ pph_in_binding_level (pph_stream *stream)
>
>   bl->blocks = pph_in_chain (stream);
>   bl->this_entity = pph_in_tree (stream);
> -  bl->level_chain = pph_in_binding_level (stream);
> +  bl->level_chain = pph_in_binding_level (stream, NULL);
>   bl->dead_vars_from_for = pph_in_tree_vec (stream);
>   bl->statement_list = pph_in_chain (stream);
>   bl->binding_depth = pph_in_uint (stream);
> @@ -713,7 +749,7 @@ pph_in_language_function (pph_stream *stream)
>
>   /* FIXME pph.  We are not reading lf->x_named_labels.  */
>
> -  lf->bindings = pph_in_binding_level (stream);
> +  lf->bindings = pph_in_binding_level (stream, NULL);
>   lf->x_local_names = pph_in_tree_vec (stream);
>
>   /* FIXME pph.  We are not reading lf->extern_decl_map.  */
> @@ -854,7 +890,7 @@ pph_in_struct_function (pph_stream *stream, tree decl)
>  static void
>  pph_in_ld_ns (pph_stream *stream, struct lang_decl_ns *ldns)
>  {
> -  ldns->level = pph_in_binding_level (stream);
> +  ldns->level = pph_in_binding_level (stream, NULL);
>  }
>
>
> @@ -1140,10 +1176,10 @@ pph_in_lang_type (pph_stream *stream)
>
>
>  /* Register all the symbols in binding level BL in the callgraph symbol
> -   table.  NS is the namespace where all the symbols in BL live.  */
> +   table.  */
>
>  static void
> -pph_register_decls_in_symtab (struct cp_binding_level *bl, tree ns)
> +pph_register_decls_in_symtab (struct cp_binding_level *bl)
>  {
>   tree t;
>
> @@ -1153,18 +1189,16 @@ pph_register_decls_in_symtab (struct
> cp_binding_level *bl, tree ns)
>   bl->namespaces = nreverse (bl->namespaces);
>
>   for (t = bl->names; t; t = DECL_CHAIN (t))
> -    if (DECL_NAME (t) && IDENTIFIER_NAMESPACE_BINDINGS (DECL_NAME (t)))
> -      {
> -       cxx_binding *b = IDENTIFIER_NAMESPACE_BINDINGS (DECL_NAME (t));
> -       b->scope = NAMESPACE_LEVEL (ns);
> -
> -       if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL
> (t))
> -         varpool_finalize_decl (t);
> -      }
> +    {
> +      /* Add file-local symbols to the varpool.  */
> +      if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL
> (t))
> +       varpool_finalize_decl (t);
> +    }
>
> +  /* Recurse into the namespaces contained in BL.  */
>   for (t = bl->namespaces; t; t = DECL_CHAIN (t))
>     if (NAMESPACE_LEVEL (t))
> -      pph_register_decls_in_symtab (NAMESPACE_LEVEL (t), t);
> +      pph_register_decls_in_symtab (NAMESPACE_LEVEL (t));
>  }
>
>
> @@ -1173,18 +1207,21 @@ pph_register_decls_in_symtab (struct
> cp_binding_level *bl, tree ns)
>  static void
>  pph_in_scope_chain (pph_stream *stream)
>  {
> -  struct saved_scope *file_scope_chain;
>   unsigned i;
>   tree decl;
>   cp_class_binding *cb;
>   cp_label_binding *lb;
>   struct cp_binding_level *cur_bindings, *new_bindings;
>
> -  file_scope_chain = ggc_alloc_cleared_saved_scope ();
> -  file_scope_chain->bindings = new_bindings = pph_in_binding_level
> (stream);
> +  /* When reading the symbols in STREAM's global binding level, make
> +     sure that references to the global binding level point to
> +     scope_chain->bindings.  Otherwise, identifiers read from STREAM
> +     will have the wrong bindings and will fail name lookups.  */
>   cur_bindings = scope_chain->bindings;
> +  new_bindings = pph_in_binding_level (stream, scope_chain->bindings);
>
> -  pph_register_decls_in_symtab (new_bindings, global_namespace);
> +  /* Register all the symbols in STREAM with the call graph.  */
> +  pph_register_decls_in_symtab (new_bindings);
>
>   /* Merge the bindings from STREAM into saved_scope->bindings.  */
>   chainon (cur_bindings->names, new_bindings->names);
>
>
> --
> This patch is available for review at
> http://codereview.appspot.com/4685054
>

[Gabriel Charette, 2011-07-12 18:03:37]

Re-adding gcc-patches (forgot to send plain text last time...sigh!)

On Tue, Jul 12, 2011 at 10:56 AM, Gabriel Charette <gchare@google.com> wrote:
> I like this implementation!
> Only one thing, if we ACTUALLY want "to_register" NULL instead of the read
> value we can't as in your current implementation NULL means don't do the
> alternate registration.
> Could that be a problem or do we expect the structures we pass to this to
> always be non-null? (i.e. for scope_chain->bindings this is definitely ok,
> but what about other potential uses?), if we keep it this way we should at
> least make it clear in the function's comment that NULL means don't do
> alternate registration I think.
> Gab
>
> On Mon, Jul 11, 2011 at 2:42 PM, Diego Novillo <dnovillo@google.com> wrote:
>>
>> This patch adapts an idea from Gab that allow us to register alternate
>> addresses in the cache.  The problem here is making sure that symbols
>> read from a PPH file reference the right bindings.
>>
>> If a symbol is in the global namespace when compiling a header file,
>> its bindings will point to NAMESPACE_LEVEL(global_namespace)->bindings,
>> but that global_namespace is the global_namespace instantiated for the
>> header file.  When reading that PPH image from a translation unit, we
>> need to refer to the bindings of the *current* global_namespace.
>>
>> In general we solve this by inserting the pointer in the streamer
>> cache.  For instance, to avoid instantiating a second global_namespace
>> decl, the initialization code of both the writer and the reader store
>> global_namespace into the streaming cache.  This way, all the
>> references to global_namespace point to the current global_namespace
>> as known by the writer and the reader.
>>
>> However, we cannot use the same trick on the bindings for
>> global_namespace.  If we simply inserted it into the cache then
>> writing out NAMESPACE_LEVEL(global_namespace)->bindings would simply
>> write a reference to the current one and on the reader side, it would
>> simply restore a pointer to the current translation unit's bindings.
>> Without ever actually writing or reading anything (since it was
>> satisified from the cache).
>>
>> Therefore, we want a mechanism that allows the reader to: (a) read all
>> the symbols in the global bindings, and (b) references to the
>> global binding made by the symbols should point to the global bindings
>> of the current translation unit (instead of the one in the PPH image).
>>
>> That's where ALLOC_AND_REGISTER_ALTERNATE comes in.  When called, it
>> allocates the data structure but registers another pointer in the
>> cache.  We use this trick when calling pph_in_binding_level from the
>> toplevel:
>>
>> +  new_bindings = pph_in_binding_level (stream, scope_chain->bindings);
>>
>> This way, when pph_in_binding_level tries to allocate the binding
>> structure read from STREAM, it registers scope_chain->bindings in the
>> cache.  This way, references to the original file's global binding are
>> automatically redirected to the current translation unit's global
>> bindings.
>>
>> Gab, I modified your original implementation to move all the logic to
>> the place where we need to make this decision.  This way, it is easier
>> to tell which functions need this alternate registration, instead of
>> relying on some status flag squirreled away in the STREAM data
>> structure.
>>
>>
>> Tested on x86_64.  Applied to branch.
>>
>> 2011-07-11   Diego Novillo  <dnovillo@google.com>
>>             Gabriel Charette  <gchare@google.com>
>>
>>        * pph-streamer-in.c (ALLOC_AND_REGISTER_ALTERNATE): Define.
>>        (pph_in_binding_level): Add argument TO_REGISTER.  Call
>>        ALLOC_AND_REGISTER_ALTERNATE if set.
>>        Update all users.
>>        (pph_register_decls_in_symtab): Call varpool_finalize_decl
>>        on all file-local symbols.
>>        (pph_in_scope_chain): Call pph_in_binding_level with
>>        scope_chain->bindings as the alternate pointer to
>>        register in the streaming cache.
>>
>> diff --git a/gcc/cp/ChangeLog.pph b/gcc/cp/ChangeLog.pph
>> index 1011902..f18c2f4 100644
>> --- a/gcc/cp/ChangeLog.pph
>> +++ b/gcc/cp/ChangeLog.pph
>> @@ -1,3 +1,16 @@
>> +2011-07-11   Diego Novillo  <dnovillo@google.com>
>> +            Gabriel Charette  <gchare@google.com>
>> +
>> +       * pph-streamer-in.c (ALLOC_AND_REGISTER_ALTERNATE): Define.
>> +       (pph_in_binding_level): Add argument TO_REGISTER.  Call
>> +       ALLOC_AND_REGISTER_ALTERNATE if set.
>> +       Update all users.
>> +       (pph_register_decls_in_symtab): Call varpool_finalize_decl
>> +       on all file-local symbols.
>> +       (pph_in_scope_chain): Call pph_in_binding_level with
>> +       scope_chain->bindings as the alternate pointer to
>> +       register in the streaming cache.
>> +
>>  2011-07-07   Diego Novillo  <dnovillo@google.com>
>>
>>        * pph-streamer-in.c (pph_register_decls_in_symtab): Rename
>> diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
>> index 571ebf5..903cd94 100644
>> --- a/gcc/cp/pph-streamer-in.c
>> +++ b/gcc/cp/pph-streamer-in.c
>> @@ -42,6 +42,18 @@ along with GCC; see the file COPYING3.  If not see
>>       pph_register_shared_data (STREAM, DATA, IX);             \
>>     } while (0)
>>
>> +/* Same as ALLOC_AND_REGISTER, but instead of registering DATA into the
>> +   cache at slot IX, it registers ALT_DATA.  Used to support mapping
>> +   pointers to global data in the original STREAM that need to point
>> +   to a different instance when aggregating individual PPH files into
>> +   the current translation unit (see pph_in_binding_level for an
>> +   example).  */
>> +#define ALLOC_AND_REGISTER_ALTERNATE(STREAM, IX, DATA, ALLOC_EXPR,
>> ALT_DATA)\
>> +    do {                                                               \
>> +      (DATA) = (ALLOC_EXPR);                                           \
>> +      pph_register_shared_data (STREAM, ALT_DATA, IX);                 \
>> +    } while (0)
>> +
>>  /* Callback for unpacking value fields in ASTs.  BP is the bitpack
>>    we are unpacking from.  EXPR is the tree to unpack.  */
>>
>> @@ -482,7 +494,8 @@ pph_in_qual_use_vec (pph_stream *stream)
>>
>>
>>  /* Forward declaration to break cyclic dependencies.  */
>> -static struct cp_binding_level *pph_in_binding_level (pph_stream *);
>> +static struct cp_binding_level *pph_in_binding_level (pph_stream *,
>> +                                                   struct
>> cp_binding_level *);
>>
>>  /* Helper for pph_in_cxx_binding.  Read and return a cxx_binding
>>    instance from STREAM.  */
>> @@ -505,7 +518,7 @@ pph_in_cxx_binding_1 (pph_stream *stream)
>>   value = pph_in_tree (stream);
>>   type = pph_in_tree (stream);
>>   ALLOC_AND_REGISTER (stream, ix, cb, cxx_binding_make (value, type));
>> -  cb->scope = pph_in_binding_level (stream);
>> +  cb->scope = pph_in_binding_level (stream, NULL);
>>   bp = pph_in_bitpack (stream);
>>   cb->value_is_inherited = bp_unpack_value (&bp, 1);
>>   cb->is_local = bp_unpack_value (&bp, 1);
>> @@ -581,10 +594,26 @@ pph_in_label_binding (pph_stream *stream)
>>  }
>>
>>
>> -/* Read and return an instance of cp_binding_level from STREAM.  */
>> +/* Read and return an instance of cp_binding_level from STREAM.
>> +   TO_REGISTER is used when the caller wants to read a binding level,
>> +   but register a different binding level in the streaming cache.
>> +   This is needed when reading the binding for global_namespace.
>> +
>> +   When the file was created global_namespace was localized to that
>> +   particular file.  However, when instantiating a PPH file into
>> +   memory, we are now using the global_namespace for the current
>> +   translation unit.  Therefore, every reference to the
>> +   global_namespace and its bindings should be pointing to the
>> +   CURRENT global namespace, not the one in STREAM.
>> +
>> +   Therefore, if TO_REGISTER is set, and we read a binding level from
>> +   STREAM for the first time, instead of registering the newly
>> +   instantiated binding level into the cache, we register the binding
>> +   level given in TO_REGISTER.  This way, subsequent references to the
>> +   global binding level will be done to the one set in TO_REGISTER.  */
>>
>>  static struct cp_binding_level *
>> -pph_in_binding_level (pph_stream *stream)
>> +pph_in_binding_level (pph_stream *stream, struct cp_binding_level
>> *to_register)
>>  {
>>   unsigned i, num, ix;
>>   struct cp_binding_level *bl;
>> @@ -597,7 +626,14 @@ pph_in_binding_level (pph_stream *stream)
>>   else if (marker == PPH_RECORD_SHARED)
>>     return (struct cp_binding_level *) pph_in_shared_data (stream, ix);
>>
>> -  ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level
>> ());
>> +  /* If TO_REGISTER is set, register that binding level instead of the
>> newly
>> +     allocated binding level into slot IX.  */
>> +  if (to_register == NULL)
>> +    ALLOC_AND_REGISTER (stream, ix, bl,
>> ggc_alloc_cleared_cp_binding_level ());
>> +  else
>> +    ALLOC_AND_REGISTER_ALTERNATE (stream, ix, bl,
>> +                                 ggc_alloc_cleared_cp_binding_level (),
>> +                                 to_register);
>>
>>   bl->names = pph_in_chain (stream);
>>   bl->namespaces = pph_in_chain (stream);
>> @@ -627,7 +663,7 @@ pph_in_binding_level (pph_stream *stream)
>>
>>   bl->blocks = pph_in_chain (stream);
>>   bl->this_entity = pph_in_tree (stream);
>> -  bl->level_chain = pph_in_binding_level (stream);
>> +  bl->level_chain = pph_in_binding_level (stream, NULL);
>>   bl->dead_vars_from_for = pph_in_tree_vec (stream);
>>   bl->statement_list = pph_in_chain (stream);
>>   bl->binding_depth = pph_in_uint (stream);
>> @@ -713,7 +749,7 @@ pph_in_language_function (pph_stream *stream)
>>
>>   /* FIXME pph.  We are not reading lf->x_named_labels.  */
>>
>> -  lf->bindings = pph_in_binding_level (stream);
>> +  lf->bindings = pph_in_binding_level (stream, NULL);
>>   lf->x_local_names = pph_in_tree_vec (stream);
>>
>>   /* FIXME pph.  We are not reading lf->extern_decl_map.  */
>> @@ -854,7 +890,7 @@ pph_in_struct_function (pph_stream *stream, tree decl)
>>  static void
>>  pph_in_ld_ns (pph_stream *stream, struct lang_decl_ns *ldns)
>>  {
>> -  ldns->level = pph_in_binding_level (stream);
>> +  ldns->level = pph_in_binding_level (stream, NULL);
>>  }
>>
>>
>> @@ -1140,10 +1176,10 @@ pph_in_lang_type (pph_stream *stream)
>>
>>
>>  /* Register all the symbols in binding level BL in the callgraph symbol
>> -   table.  NS is the namespace where all the symbols in BL live.  */
>> +   table.  */
>>
>>  static void
>> -pph_register_decls_in_symtab (struct cp_binding_level *bl, tree ns)
>> +pph_register_decls_in_symtab (struct cp_binding_level *bl)
>>  {
>>   tree t;
>>
>> @@ -1153,18 +1189,16 @@ pph_register_decls_in_symtab (struct
>> cp_binding_level *bl, tree ns)
>>   bl->namespaces = nreverse (bl->namespaces);
>>
>>   for (t = bl->names; t; t = DECL_CHAIN (t))
>> -    if (DECL_NAME (t) && IDENTIFIER_NAMESPACE_BINDINGS (DECL_NAME (t)))
>> -      {
>> -       cxx_binding *b = IDENTIFIER_NAMESPACE_BINDINGS (DECL_NAME (t));
>> -       b->scope = NAMESPACE_LEVEL (ns);
>> -
>> -       if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL
>> (t))
>> -         varpool_finalize_decl (t);
>> -      }
>> +    {
>> +      /* Add file-local symbols to the varpool.  */
>> +      if (TREE_CODE (t) == VAR_DECL && TREE_STATIC (t) && !DECL_EXTERNAL
>> (t))
>> +       varpool_finalize_decl (t);
>> +    }
>>
>> +  /* Recurse into the namespaces contained in BL.  */
>>   for (t = bl->namespaces; t; t = DECL_CHAIN (t))
>>     if (NAMESPACE_LEVEL (t))
>> -      pph_register_decls_in_symtab (NAMESPACE_LEVEL (t), t);
>> +      pph_register_decls_in_symtab (NAMESPACE_LEVEL (t));
>>  }
>>
>>
>> @@ -1173,18 +1207,21 @@ pph_register_decls_in_symtab (struct
>> cp_binding_level *bl, tree ns)
>>  static void
>>  pph_in_scope_chain (pph_stream *stream)
>>  {
>> -  struct saved_scope *file_scope_chain;
>>   unsigned i;
>>   tree decl;
>>   cp_class_binding *cb;
>>   cp_label_binding *lb;
>>   struct cp_binding_level *cur_bindings, *new_bindings;
>>
>> -  file_scope_chain = ggc_alloc_cleared_saved_scope ();
>> -  file_scope_chain->bindings = new_bindings = pph_in_binding_level
>> (stream);
>> +  /* When reading the symbols in STREAM's global binding level, make
>> +     sure that references to the global binding level point to
>> +     scope_chain->bindings.  Otherwise, identifiers read from STREAM
>> +     will have the wrong bindings and will fail name lookups.  */
>>   cur_bindings = scope_chain->bindings;
>> +  new_bindings = pph_in_binding_level (stream, scope_chain->bindings);
>>
>> -  pph_register_decls_in_symtab (new_bindings, global_namespace);
>> +  /* Register all the symbols in STREAM with the call graph.  */
>> +  pph_register_decls_in_symtab (new_bindings);
>>
>>   /* Merge the bindings from STREAM into saved_scope->bindings.  */
>>   chainon (cur_bindings->names, new_bindings->names);
>>
>>
>> --
>> This patch is available for review at
>> http://codereview.appspot.com/4685054
>
>

[Diego Novillo, 2011-07-12 18:14:48]

On Tue, Jul 12, 2011 at 13:56, Gabriel Charette <gchare@google.com> wrote:
> I like this implementation!
> Only one thing, if we ACTUALLY want "to_register" NULL instead of the read
> value we can't as in your current implementation NULL means don't do the
> alternate registration.

I don't think that's a problem.  Note too that the original
implementation also treated NULL to mean "don't do alternate
registration".


Diego.

[Gabriel Charette, 2011-07-12 18:23:06]

Right, I remember my original implementation had the same behaviour,
but I'm pretty sure I had a comment mentioning that in the function
usage comment. I'm just saying it should be mentioned what passing
NULL means (especially since we do it all over the place).

On Tue, Jul 12, 2011 at 11:14 AM, Diego Novillo <dnovillo@google.com> wrote:
> On Tue, Jul 12, 2011 at 13:56, Gabriel Charette <gchare@google.com> wrote:
>> I like this implementation!
>> Only one thing, if we ACTUALLY want "to_register" NULL instead of the read
>> value we can't as in your current implementation NULL means don't do the
>> alternate registration.
>
> I don't think that's a problem.  Note too that the original
> implementation also treated NULL to mean "don't do alternate
> registration".
>
>
> Diego.
>

[Diego Novillo, 2011-07-12 18:30:17]

On Tue, Jul 12, 2011 at 14:23, Gabriel Charette <gchare@google.com> wrote:
> Right, I remember my original implementation had the same behaviour,
> but I'm pretty sure I had a comment mentioning that in the function
> usage comment. I'm just saying it should be mentioned what passing
> NULL means (especially since we do it all over the place).

Oh, absolutely.


Diego.