[pph] Support for references to symbols in other PPH images (issue4873051)

This patch finishes the support for external references to symbols in
other PPH files.

This is used when one PPH image includes another.  The symbols in
the included image should not be emitted from the parent, and references
to them should go to the correct location in the child's pickle cache.

The main change is the addition of two kinds of references.  We used
to mark references to already pickled nodes with PPH_RECORD_SHARED.
We now distinguish internal (PPH_RECORD_IREF) and external
(PPH_RECORD_XREF) references.

In the first variant, only one index is written as reference: the slot
into the pickle cache.  In the second variant, two indices are
written: an index into the include table specifying which child image
contains the data, and an index into the child's pickle cache.

When writing PPH images, we also need to make sure that we do not
write any symbols in the file bindings that belong to included images.
Otherwise, we will emit the same symbol twice.  This is implemented by
a new filter PPHF_NO_XREFS, which is used when writing the current
image's global bindings.

Tested on x86_64.  Committed to branch.


Diego.


cp/ChangeLog.pph
	* name-lookup.c (pph_out_binding_table): Call
 	pph_out_record_marker instead of pph_out_uchar.
 	(pph_in_binding_table): Call pph_in_record_marker instead of
 	pph_in_uchar.
 	* pph-streamer-in.c (pph_read_images): Declare.
 	(pph_is_reference_marker): New.  Update all previous users of
 	PPH_RECORD_SHARED.
 	(pph_in_start_record): Add argument INCLUDE_IX.  Update all
 	users.
 	Handle PPH_RECORD_XREF markers.
 	(pph_in_includes): After reading INCLUDE_NAME, call
 	pph_add_include.
 	(pph_read_file_contents): Move debugging output from ...
 	(pph_read_file): ... here.
 	Change it to return the opened stream.
 	Add STREAM to pph_read_images.
 	(pph_reader_finish): New.
 	(pph_read_file_1): Rename from pph_read_file_contents.  Update
 	all users.
 	* pph-streamer-out.c (pph_out_start_record): Re-write to
 	handle PPH_RECORD_IREF and PPH_RECORD_XREF.
 	(pph_write_file_contents): Embed ...
 	(pph_write_file): ... here.
 	(pph_add_include): Add new argument INCLUDE.  Update all
 	users.
 	(pph_writer_finish): Do nothing if pph_out_stream is NULL.
 	(pph_tree_matches): New.
 	(pph_out_tree_vec_filtered): New.
 	(pph_out_binding_level): Add new argument FILTER.  Update all users.
 	* pph-streamer.c (pph_stream_close_1): Embed ...
 	(pph_stream_close): ... here.
 	Do not traverse the list of includes.
 	(pph_cache_lookup): Factor out of ...
 	(pph_cache_add): ... here.
 	(pph_cache_lookup_in_includes): New.
 	(pph_cache_get): Add new argument INCLUDE_IX.  Update all users.
 	* pph-streamer.h (enum pph_record_marker): Add PPH_RECORD_IREF
 	and PPH_RECORD_XREF.  Remove PPH_RECORD_SHARED.  Update all users.
 	(struct pph_stream): Remove field open_p and nested_p.  Update
 	all users.
 	(enum chain_filter): Remove.  Change values to #define constants.
 	Update all users.
 	(PPHF_NO_XREFS): Define.
 	(pph_out_record_marker): New.
 	(pph_in_record_marker): New.
 	* pph.c (pph_finish): Always call pph_writer_finish.
 	Call pph_reader_finish.

testsuite/ChangeLog.pph
	* g++.dg/pph/x1tmplclass2.cc: Update expected ICE.
	* g++.dg/pph/x4tmplclass2.cc: Update expected ICE.
	* g++.dg/pph/z4tmplclass2.cc: Update expected ICE.

I really like the way this is implemented! In particular having our
own cache is so great!

A few comments inline below.

Cheers,
Gab

On Mon, Aug 15, 2011 at 8:05 PM, Diego Novillo <dnovillo@google.com> wrote:
> This patch finishes the support for external references to symbols in
> other PPH files.
>
> This is used when one PPH image includes another.  The symbols in
> the included image should not be emitted from the parent, and references
> to them should go to the correct location in the child's pickle cache.
>
> The main change is the addition of two kinds of references.  We used
> to mark references to already pickled nodes with PPH_RECORD_SHARED.
> We now distinguish internal (PPH_RECORD_IREF) and external
> (PPH_RECORD_XREF) references.
>
> In the first variant, only one index is written as reference: the slot
> into the pickle cache.  In the second variant, two indices are
> written: an index into the include table specifying which child image
> contains the data, and an index into the child's pickle cache.
>
> When writing PPH images, we also need to make sure that we do not
> write any symbols in the file bindings that belong to included images.
> Otherwise, we will emit the same symbol twice.  This is implemented by
> a new filter PPHF_NO_XREFS, which is used when writing the current
> image's global bindings.
>
> Tested on x86_64.  Committed to branch.
>
>
> Diego.
>
>
> cp/ChangeLog.pph
>        * name-lookup.c (pph_out_binding_table): Call
>        pph_out_record_marker instead of pph_out_uchar.
>        (pph_in_binding_table): Call pph_in_record_marker instead of
>        pph_in_uchar.
>        * pph-streamer-in.c (pph_read_images): Declare.
>        (pph_is_reference_marker): New.  Update all previous users of
>        PPH_RECORD_SHARED.
>        (pph_in_start_record): Add argument INCLUDE_IX.  Update all
>        users.
>        Handle PPH_RECORD_XREF markers.
>        (pph_in_includes): After reading INCLUDE_NAME, call
>        pph_add_include.
>        (pph_read_file_contents): Move debugging output from ...
>        (pph_read_file): ... here.
>        Change it to return the opened stream.
>        Add STREAM to pph_read_images.
>        (pph_reader_finish): New.
>        (pph_read_file_1): Rename from pph_read_file_contents.  Update
>        all users.
>        * pph-streamer-out.c (pph_out_start_record): Re-write to
>        handle PPH_RECORD_IREF and PPH_RECORD_XREF.
>        (pph_write_file_contents): Embed ...
>        (pph_write_file): ... here.
>        (pph_add_include): Add new argument INCLUDE.  Update all
>        users.
>        (pph_writer_finish): Do nothing if pph_out_stream is NULL.
>        (pph_tree_matches): New.
>        (pph_out_tree_vec_filtered): New.
>        (pph_out_binding_level): Add new argument FILTER.  Update all users.
>        * pph-streamer.c (pph_stream_close_1): Embed ...
>        (pph_stream_close): ... here.
>        Do not traverse the list of includes.
>        (pph_cache_lookup): Factor out of ...
>        (pph_cache_add): ... here.
>        (pph_cache_lookup_in_includes): New.
>        (pph_cache_get): Add new argument INCLUDE_IX.  Update all users.
>        * pph-streamer.h (enum pph_record_marker): Add PPH_RECORD_IREF
>        and PPH_RECORD_XREF.  Remove PPH_RECORD_SHARED.  Update all users.
>        (struct pph_stream): Remove field open_p and nested_p.  Update
>        all users.
>        (enum chain_filter): Remove.  Change values to #define constants.
>        Update all users.
>        (PPHF_NO_XREFS): Define.
>        (pph_out_record_marker): New.
>        (pph_in_record_marker): New.
>        * pph.c (pph_finish): Always call pph_writer_finish.
>        Call pph_reader_finish.
>
> testsuite/ChangeLog.pph
>        * g++.dg/pph/x1tmplclass2.cc: Update expected ICE.
>        * g++.dg/pph/x4tmplclass2.cc: Update expected ICE.
>        * g++.dg/pph/z4tmplclass2.cc: Update expected ICE.
>
> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
> index 50c6b66..7798f24 100644
> --- a/gcc/cp/name-lookup.c
> +++ b/gcc/cp/name-lookup.c
> @@ -6002,12 +6002,12 @@ pph_out_binding_table (pph_stream *stream, binding_table bt)
>     {
>       if (bt->chain[i])
>        {
> -         pph_out_uchar (stream, PPH_RECORD_START);
> +         pph_out_record_marker (stream, PPH_RECORD_START);
>          pph_out_tree_or_ref (stream, bt->chain[i]->name);
>          pph_out_tree_or_ref (stream, bt->chain[i]->type);
>        }
>       else
> -       pph_out_uchar (stream, PPH_RECORD_END);
> +       pph_out_record_marker (stream, PPH_RECORD_END);
>     }
>   pph_out_uint (stream, bt->entry_count);
>  }
> @@ -6025,13 +6025,15 @@ pph_in_binding_table (pph_stream *stream)
>   bt = binding_table_new (chain_count);
>   for (i = 0; i < chain_count; i++)
>     {
> -      unsigned char record_marker = pph_in_uchar (stream);
> -      if (record_marker == PPH_RECORD_START)
> +      enum pph_record_marker marker = pph_in_record_marker (stream);
> +      if (marker == PPH_RECORD_START)
>        {
>          tree name = pph_in_tree (stream);
>          tree type = pph_in_tree (stream);
>          binding_table_insert (bt, name, type);
>        }
> +      else
> +       gcc_assert (marker == PPH_RECORD_END);
>     }
>   bt->entry_count = pph_in_uint (stream);
>
> diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
> index e644113..f87e6a5 100644
> --- a/gcc/cp/pph-streamer-in.c
> +++ b/gcc/cp/pph-streamer-in.c
> @@ -33,6 +33,13 @@ along with GCC; see the file COPYING3.  If not see
>  #include "cppbuiltin.h"
>  #include "toplev.h"
>
> +/* List of PPH images read during parsing.  Images opened during #include
> +   processing and opened from pph_in_includes cannot be closed
> +   immediately after reading, because the pickle cache contained in
> +   them may be referenced from other images.  We delay closing all of
> +   them until the end of parsing (when pph_reader_finish is called).  */
> +static VEC(pph_stream_ptr,heap) *pph_read_images = NULL;
> +
>  typedef char *char_p;
>  DEF_VEC_P(char_p);
>  DEF_VEC_ALLOC_P(char_p,heap);
> @@ -135,43 +142,57 @@ pph_init_read (pph_stream *stream)
>  }
>
>
> -/* Read and return a record marker from STREAM.  When a PPH_RECORD_START
> +/* Return true if MARKER is either PPH_RECORD_IREF or PPH_RECORD_XREF.  */
> +
> +static inline bool
> +pph_is_reference_marker (enum pph_record_marker marker)
> +{
> +  return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF;
> +}
> +
> +
> +/* Read and return a record header from STREAM.  When a PPH_RECORD_START
>    marker is read, the next word read is an index into the streamer
>    cache where the rematerialized data structure should be stored.
>    When the writer stored this data structure for the first time, it
> -   added it to its own streamer cache at slot number *CACHE_IX.
> +   added it to its own streamer cache at slot number *CACHE_IX_P.
>
>    This way, if the same data structure was written a second time to
>    the stream, instead of writing the whole structure again, only the
> -   index *CACHE_IX is written as a PPH_RECORD_SHARED record.
> +   index *CACHE_IX_P is written as a PPH_RECORD_IREF record.
>
> -   Therefore, when reading a PPH_RECORD_START marker, *CACHE_IX will
> +   Therefore, when reading a PPH_RECORD_START marker, *CACHE_IX_P will
>    contain the slot number where the materialized data should be
> -   cached at.  When reading a PPH_RECORD_SHARED marker, *CACHE_IX will
> +   cached at.  When reading a PPH_RECORD_IREF marker, *CACHE_IX_P will
>    contain the slot number the reader can find the previously
> -   materialized structure.  */
> +   materialized structure.
> +
> +   If the record starts with PPH_RECORD_XREF, this means that the data
> +   we are about to read is located in the pickle cache of one of
> +   STREAM's included images.  In this case, the record consists of two
> +   indices: the first one (*INCLUDE_IX_P) indicates which included
> +   image contains the data (it is an index into STREAM->INCLUDES), the
> +   second one indicates which slot in that image's pickle cache we can
> +   find the data.  */
>
>  static inline enum pph_record_marker
> -pph_in_start_record (pph_stream *stream, unsigned *cache_ix)
> +pph_in_start_record (pph_stream *stream, unsigned *include_ix_p,
> +                    unsigned *cache_ix_p)
>  {
> -  enum pph_record_marker marker;
> +  enum pph_record_marker marker = pph_in_record_marker (stream);
>
> -  marker = (enum pph_record_marker) pph_in_uchar (stream);
> +  *include_ix_p = (unsigned) -1;
> +  *cache_ix_p = (unsigned) -1;

Setting *cache_ix to (unsigned) -1 used to be a "hack" (with a comment
explaining it which was removed just below), to avoid a warning about
it being unset in a branch, but that branch was only PPH_RECORD_END,
in which case it didn't matter.

Now that we actually check in pph_cache_get that include_ix ==
(unsigned) -1, I'm not so sure this is good... while (unsigned) -1 is
a very large unsigned int, it's not impossible to have an actual cache
entry with that number...

> -  /* For PPH_RECORD_START and PPH_RECORD_SHARED markers, read the
> +  /* For PPH_RECORD_START and PPH_RECORD_IREF markers, read the
>      streamer cache slot where we should store or find the
>      rematerialized data structure (see description above).  */
> -  if (marker == PPH_RECORD_START || marker == PPH_RECORD_SHARED)
> -    *cache_ix = pph_in_uint (stream);
> -  else
> +  if (marker == PPH_RECORD_START || marker == PPH_RECORD_IREF)
> +    *cache_ix_p = pph_in_uint (stream);
> +  else if (marker == PPH_RECORD_XREF)
>     {
> -      gcc_assert (marker == PPH_RECORD_END);
> -
> -      /* Initialize CACHE_IX to an invalid index. Even though this
> -        is never used in practice, the compiler will throw an error
> -        if the optimizer inlines this function in a given build as
> -        it will complain that " 'ix' may be used uninitialized".  */

...this is the comment I'm referring to above.

> -      *cache_ix = -1;
> +      *include_ix_p = pph_in_uint (stream);
> +      *cache_ix_p = pph_in_uint (stream);
>     }
>
>   return marker;
> @@ -457,13 +478,13 @@ pph_in_cxx_binding_1 (pph_stream *stream)
>   cxx_binding *cb;
>   tree value, type;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned ix, include_ix;

Sometimes you call the local variable "include_ix"...

>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &include_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (cxx_binding *) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (cxx_binding *) pph_cache_get (stream, include_ix, ix);
>
>   value = pph_in_tree (stream);
>   type = pph_in_tree (stream);
> @@ -505,13 +526,13 @@ pph_in_class_binding (pph_stream *stream)
>  {
>   cp_class_binding *cb;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;

... and sometimes image_ix: consistency would be nice, although not necessary...

>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (cp_class_binding *) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (cp_class_binding *) pph_cache_get (stream, image_ix, ix);
>
>   ALLOC_AND_REGISTER (stream, ix, cb, ggc_alloc_cleared_cp_class_binding ());
>   cb->base = pph_in_cxx_binding (stream);
> @@ -528,13 +549,13 @@ pph_in_label_binding (pph_stream *stream)
>  {
>   cp_label_binding *lb;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (cp_label_binding *) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (cp_label_binding *) pph_cache_get (stream, image_ix, ix);
>
>   ALLOC_AND_REGISTER (stream, ix, lb, ggc_alloc_cleared_cp_label_binding ());
>   lb->label = pph_in_tree (stream);
> @@ -565,16 +586,16 @@ pph_in_label_binding (pph_stream *stream)
>  static cp_binding_level *
>  pph_in_binding_level (pph_stream *stream, cp_binding_level *to_register)
>  {
> -  unsigned i, num, ix;
> +  unsigned i, num, image_ix, ix;
>   cp_binding_level *bl;
>   struct bitpack_d bp;
>   enum pph_record_marker marker;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (cp_binding_level *) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (cp_binding_level *) pph_cache_get (stream, image_ix, ix);
>
>   /* If TO_REGISTER is set, register that binding level instead of the newly
>      allocated binding level into slot IX.  */
> @@ -644,13 +665,13 @@ pph_in_c_language_function (pph_stream *stream)
>  {
>   struct c_language_function *clf;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (struct c_language_function *) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (struct c_language_function *) pph_cache_get (stream, image_ix, ix);
>
>   ALLOC_AND_REGISTER (stream, ix, clf,
>                      ggc_alloc_cleared_c_language_function ());
> @@ -669,13 +690,13 @@ pph_in_language_function (pph_stream *stream)
>   struct bitpack_d bp;
>   struct language_function *lf;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (struct language_function *) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (struct language_function *) pph_cache_get (stream, image_ix, ix);
>
>   ALLOC_AND_REGISTER (stream, ix, lf, ggc_alloc_cleared_language_function ());
>   memcpy (&lf->base, pph_in_c_language_function (stream),
> @@ -759,17 +780,17 @@ static struct function *
>  pph_in_struct_function (pph_stream *stream)
>  {
>   size_t count, i;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>   enum pph_record_marker marker;
>   struct function *fn;
>   tree decl;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
>
>   /* Since struct function is embedded in every decl, fn cannot be shared.  */
> -  gcc_assert (marker != PPH_RECORD_SHARED);
> +  gcc_assert (!pph_is_reference_marker (marker));
>
>   decl = pph_in_tree (stream);
>   allocate_struct_function (decl, false);
> @@ -864,15 +885,15 @@ pph_in_lang_specific (pph_stream *stream, tree decl)
>   struct lang_decl *ld;
>   struct lang_decl_base *ldb;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return;
> -  else if (marker == PPH_RECORD_SHARED)
> +  else if (pph_is_reference_marker (marker))
>     {
>       DECL_LANG_SPECIFIC (decl) =
> -       (struct lang_decl *) pph_cache_get (stream, ix);
> +       (struct lang_decl *) pph_cache_get (stream, image_ix, ix);
>       return;
>     }
>
> @@ -960,13 +981,13 @@ pph_in_sorted_fields_type (pph_stream *stream)
>   unsigned i, num_fields;
>   struct sorted_fields_type *v;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (struct sorted_fields_type *) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (struct sorted_fields_type *) pph_cache_get (stream, image_ix, ix);
>
>   num_fields = pph_in_uint (stream);
>   ALLOC_AND_REGISTER (stream, ix, v, sorted_fields_type_new (num_fields));
> @@ -984,7 +1005,7 @@ pph_in_lang_type_class (pph_stream *stream, struct lang_type_class *ltc)
>  {
>   struct bitpack_d bp;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>
>   ltc->align = pph_in_uchar (stream);
>
> @@ -1039,14 +1060,14 @@ pph_in_lang_type_class (pph_stream *stream, struct lang_type_class *ltc)
>   ltc->typeinfo_var = pph_in_tree (stream);
>   ltc->vbases = pph_in_tree_vec (stream);
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_START)
>     {
>       ltc->nested_udts = pph_in_binding_table (stream);
>       pph_cache_insert_at (stream, ltc->nested_udts, ix);
>     }
> -  else if (marker == PPH_RECORD_SHARED)
> -    ltc->nested_udts = (binding_table) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    ltc->nested_udts = (binding_table) pph_cache_get (stream, image_ix, ix);
>
>   ltc->as_base = pph_in_tree (stream);
>   ltc->pure_virtuals = pph_in_tree_vec (stream);
> @@ -1079,13 +1100,13 @@ pph_in_lang_type (pph_stream *stream)
>  {
>   struct lang_type *lt;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (struct lang_type *) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (struct lang_type *) pph_cache_get (stream, image_ix, ix);
>
>   ALLOC_AND_REGISTER (stream, ix, lt,
>                      ggc_alloc_cleared_lang_type (sizeof (struct lang_type)));
> @@ -1346,10 +1367,8 @@ pph_in_includes (pph_stream *stream)
>   for (i = 0; i < num; i++)
>     {
>       const char *include_name = pph_in_string (stream);
> -      /* FIXME pph.  Disabled for now.  Need to re-work caching so
> -        external symbol references are properly saved and restored.  */
> -      if (0)
> -       pph_read_file (include_name);
> +      pph_stream *include = pph_read_file (include_name);
> +      pph_add_include (stream, include);
>     }
>  }
>
> @@ -1454,10 +1473,10 @@ pph_in_and_merge_line_table (pph_stream *stream, struct line_maps *linetab)
>  }
>
>
> -/* Read contents of PPH file in STREAM.  */
> +/* Helper for pph_read_file.  Read contents of PPH file in STREAM.  */
>
>  static void
> -pph_read_file_contents (pph_stream *stream)
> +pph_read_file_1 (pph_stream *stream)
>  {
>   bool verified;
>   cpp_ident_use *bad_use;
> @@ -1467,6 +1486,9 @@ pph_read_file_contents (pph_stream *stream)
>   unsigned i;
>   VEC(tree,gc) *file_unemitted_tinfo_decls;
>
> +  if (flag_pph_debug >= 1)
> +    fprintf (pph_logfile, "PPH: Reading %s\n", stream->name);
> +
>   /* Read all the images included by STREAM.  */
>   pph_in_includes (stream);
>
> @@ -1483,7 +1505,7 @@ 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 in the pph's line table and merge it in the current line table.  */
> +  /* Read in STREAM's line table and merge it in the current line table.  */
>   pph_in_and_merge_line_table (stream, line_table);
>
>   /* Read the bindings from STREAM and merge them with the current bindings.  */
> @@ -1514,28 +1536,27 @@ pph_read_file_contents (pph_stream *stream)
>      STREAM will need to be read again the next time we want to read
>      the image we are now generating.  */
>   if (pph_out_file)
> -    pph_add_include (stream);
> +    pph_add_include (NULL, stream);
>  }
>
>
> -/* Read PPH file FILENAME.  */
> +/* Read PPH file FILENAME.  Return the in-memory pph_stream instance.  */
>
> -void
> +pph_stream *
>  pph_read_file (const char *filename)
>  {
>   pph_stream *stream;
>
> -  if (flag_pph_debug >= 1)
> -    fprintf (pph_logfile, "PPH: Reading %s\n", filename);
> -
>   stream = pph_stream_open (filename, "rb");
>   if (stream)
>     {
> -      pph_read_file_contents (stream);
> -      pph_stream_close (stream);
> +      pph_read_file_1 (stream);
> +      VEC_safe_push (pph_stream_ptr, heap, pph_read_images, stream);
>     }
>   else
>     error ("Cannot open PPH file for reading: %s: %m", filename);
> +
> +  return stream;
>  }
>
>
> @@ -1981,13 +2002,13 @@ pph_read_tree (struct lto_input_block *ib ATTRIBUTE_UNUSED,
>   tree expr;
>   bool fully_read_p;
>   enum pph_record_marker marker;
> -  unsigned ix;
> +  unsigned image_ix, ix;
>
> -  marker = pph_in_start_record (stream, &ix);
> +  marker = pph_in_start_record (stream, &image_ix, &ix);
>   if (marker == PPH_RECORD_END)
>     return NULL;
> -  else if (marker == PPH_RECORD_SHARED)
> -    return (tree) pph_cache_get (stream, ix);
> +  else if (pph_is_reference_marker (marker))
> +    return (tree) pph_cache_get (stream, image_ix, ix);
>
>   /* We did not find the tree in the pickle cache, allocate the tree by
>      reading the header fields (different tree nodes need to be
> @@ -1998,3 +2019,17 @@ pph_read_tree (struct lto_input_block *ib ATTRIBUTE_UNUSED,
>
>   return expr;
>  }
> +
> +
> +/* Finalize the PPH reader.  */
> +
> +void
> +pph_reader_finish (void)
> +{
> +  unsigned i;
> +  pph_stream *image;
> +
> +  /* Close any images read during parsing.  */
> +  FOR_EACH_VEC_ELT (pph_stream_ptr, pph_read_images, i, image)
> +    pph_stream_close (image);
> +}
> diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
> index 2e5543a..b7a965c 100644
> --- a/gcc/cp/pph-streamer-out.c
> +++ b/gcc/cp/pph-streamer-out.c
> @@ -180,32 +180,43 @@ pph_flush_buffers (pph_stream *stream)
>  static inline bool
>  pph_out_start_record (pph_stream *stream, void *data)
>  {
> -  if (data)
> +  unsigned ix, include_ix;
> +
> +  /* Represent NULL pointers with a single PPH_RECORD_END.  */
> +  if (data == NULL)
> +    {
> +      pph_out_record_marker (stream, PPH_RECORD_END);
> +      return false;
> +    }
> +
> +  /* See if we have data in STREAM's cache.  If so, write an internal
> +     reference to it and inform the caller that it should not write a
> +     physical representation for DATA.  */
> +  if (pph_cache_lookup (stream, data, &ix))
>     {
> -      bool existed_p;
> -      unsigned ix;
> -      enum pph_record_marker marker;
> -
> -      /* If the memory at DATA has already been streamed out, make
> -        sure that we don't write it more than once.  Otherwise,
> -        the reader will instantiate two different pointers for
> -        the same object.
> -
> -        Write the index into the cache where DATA has been stored.
> -        This way, the reader will know at which slot to
> -        re-materialize DATA the first time and where to access it on
> -        subsequent reads.  */
> -      existed_p = pph_cache_add (stream, data, &ix);
> -      marker = (existed_p) ? PPH_RECORD_SHARED : PPH_RECORD_START;
> -      pph_out_uchar (stream, marker);
> +      pph_out_record_marker (stream, PPH_RECORD_IREF);
>       pph_out_uint (stream, ix);
> -      return marker == PPH_RECORD_START;
> +      return false;
>     }
> -  else
> +
> +  /* DATA is not in STREAM's cache.  See if it is in any of STREAM's
> +     included images.  If it is, write an external reference to it
> +     and inform the caller that it should not write a physical
> +     representation for DATA.  */
> +  if (pph_cache_lookup_in_includes (stream, data, &include_ix, &ix))
>     {
> -      pph_out_uchar (stream, PPH_RECORD_END);
> +      pph_out_record_marker (stream, PPH_RECORD_XREF);
> +      pph_out_uint (stream, include_ix);
> +      pph_out_uint (stream, ix);
>       return false;
>     }
> +
> +  /* DATA is in none of the pickle caches, add it to STREAM's pickle
> +     cache and tell the caller that it should pickle DATA out.  */
> +  pph_cache_add (stream, data, &ix);
> +  pph_out_record_marker (stream, PPH_RECORD_START);
> +  pph_out_uint (stream, ix);
> +  return true;
>  }
>
>
> @@ -449,7 +460,25 @@ pph_out_ld_min (pph_stream *stream, struct lang_decl_min *ldm)
>  }
>
>
> -/* Write all the trees in gc VEC V to STREAM.  */
> +/* Return true if T matches FILTER for STREAM.  */
> +
> +static inline bool
> +pph_tree_matches (pph_stream *stream, tree t, unsigned filter)
> +{
> +  if ((filter & PPHF_NO_BUILTINS)
> +      && DECL_P (t)
> +      && DECL_IS_BUILTIN (t))
> +    return false;
> +
> +  if ((filter & PPHF_NO_XREFS)
> +      && pph_cache_lookup_in_includes (stream, t, NULL, NULL))
> +    return false;
> +
> +  return true;
> +}
> +
> +
> +/* Write all the trees in VEC V to STREAM.  */
>
>  static void
>  pph_out_tree_vec (pph_stream *stream, VEC(tree,gc) *v)
> @@ -463,6 +492,34 @@ pph_out_tree_vec (pph_stream *stream, VEC(tree,gc) *v)
>  }
>
>
> +/* Write all the trees matching FILTER in VEC V to STREAM.  */
> +
> +static void
> +pph_out_tree_vec_filtered (pph_stream *stream, VEC(tree,gc) *v, unsigned filter)
> +{
> +  unsigned i;
> +  tree t;
> +  VEC(tree, heap) *to_write = NULL;
> +
> +  /* Special case.  If the caller wants no filtering, it is much
> +     faster to just call pph_out_tree_vec.  */
> +  if (filter == PPHF_NONE)
> +    {
> +      pph_out_tree_vec (stream, v);
> +      return;
> +    }
> +
> +  /* Collect all the nodes that match the filter.  */
> +  FOR_EACH_VEC_ELT (tree, v, i, t)
> +    if (pph_tree_matches (stream, t, filter))
> +      VEC_safe_push (tree, heap, to_write, t);
> +
> +  /* Write them.  */
> +  pph_out_tree_vec (stream, (VEC(tree,gc) *)to_write);
> +  VEC_free (tree, heap, to_write);
> +}
> +
> +
>  /* Write all the qualified_typedef_usage_t in VEC V to STREAM.  */
>
>  static void
> @@ -482,7 +539,7 @@ pph_out_qual_use_vec (pph_stream *stream, VEC(qualified_typedef_usage_t,gc) *v)
>
>
>  /* Forward declaration to break cyclic dependencies.  */
> -static void pph_out_binding_level (pph_stream *, cp_binding_level *);
> +static void pph_out_binding_level (pph_stream *, cp_binding_level *, unsigned);
>
>
>  /* Helper for pph_out_cxx_binding.  STREAM and CB are as in
> @@ -498,7 +555,7 @@ pph_out_cxx_binding_1 (pph_stream *stream, cxx_binding *cb)
>
>   pph_out_tree_or_ref (stream, cb->value);
>   pph_out_tree_or_ref (stream, cb->type);
> -  pph_out_binding_level (stream, cb->scope);
> +  pph_out_binding_level (stream, cb->scope, PPHF_NONE);
>   bp = bitpack_create (stream->encoder.w.ob->main_stream);
>   bp_pack_value (&bp, cb->value_is_inherited, 1);
>   bp_pack_value (&bp, cb->is_local, 1);
> @@ -573,61 +630,51 @@ pph_out_chained_tree (pph_stream *stream, tree t)
>    nodes that do not match FILTER.  */
>
>  static void
> -pph_out_chain_filtered (pph_stream *stream, tree first,
> -                       enum chain_filter filter)
> +pph_out_chain_filtered (pph_stream *stream, tree first, unsigned filter)
>  {
> -  unsigned count;
>   tree t;
> +  VEC(tree, heap) *to_write = NULL;
>
>   /* Special case.  If the caller wants no filtering, it is much
>      faster to just call pph_out_chain directly.  */
> -  if (filter == NONE)
> +  if (filter == PPHF_NONE)
>     {
>       pph_out_chain (stream, first);
>       return;
>     }
>
> -  /* Count all the nodes that match the filter.  */
> -  for (t = first, count = 0; t; t = TREE_CHAIN (t))
> -    {
> -      if (filter == NO_BUILTINS && DECL_P (t) && DECL_IS_BUILTIN (t))
> -       continue;
> -      count++;
> -    }
> -  pph_out_uint (stream, count);
> -
> -  /* Output all the nodes that match the filter.  */
> +  /* Collect all the nodes that match the filter.  */
>   for (t = first; t; t = TREE_CHAIN (t))
> -    {
> -      /* Apply filters to T.  */
> -      if (filter == NO_BUILTINS && DECL_P (t) && DECL_IS_BUILTIN (t))
> -       continue;
> +    if (pph_tree_matches (stream, t, filter))
> +      VEC_safe_push (tree, heap, to_write, t);
>
> -      pph_out_chained_tree (stream, t);
> -    }
> +  /* Write them.  */
> +  pph_out_tree_vec (stream, (VEC(tree,gc) *)to_write);
> +  VEC_free (tree, heap, to_write);
>  }
>
>
> -/* Write all the fields of cp_binding_level instance BL to STREAM.  */
> +/* Helper for pph_out_binding_level.  Write all the fields of BL to
> +   STREAM, without checking whether BL was in the streamer cache or not.
> +   Do not emit any nodes in BL that do not match FILTER.  */
>
>  static void
> -pph_out_binding_level (pph_stream *stream, cp_binding_level *bl)
> +pph_out_binding_level_1 (pph_stream *stream, cp_binding_level *bl,
> +                        unsigned filter)
>  {
>   unsigned i;
>   cp_class_binding *cs;
>   cp_label_binding *sl;
>   struct bitpack_d bp;
>
> -  if (!pph_out_start_record (stream, bl))
> -    return;
> -
> -  pph_out_chain_filtered (stream, bl->names, NO_BUILTINS);
> -  pph_out_chain_filtered (stream, bl->namespaces, NO_BUILTINS);
> +  pph_out_chain_filtered (stream, bl->names, PPHF_NO_BUILTINS | filter);
> +  pph_out_chain_filtered (stream, bl->namespaces, PPHF_NO_BUILTINS | filter);
>
> -  pph_out_tree_vec (stream, bl->static_decls);
> +  pph_out_tree_vec_filtered (stream, bl->static_decls, filter);
>
> -  pph_out_chain_filtered (stream, bl->usings, NO_BUILTINS);
> -  pph_out_chain_filtered (stream, bl->using_directives, NO_BUILTINS);
> +  pph_out_chain_filtered (stream, bl->usings, PPHF_NO_BUILTINS | filter);
> +  pph_out_chain_filtered (stream, bl->using_directives,
> +                         PPHF_NO_BUILTINS | filter);
>
>   pph_out_uint (stream, VEC_length (cp_class_binding, bl->class_shadowed));
>   FOR_EACH_VEC_ELT (cp_class_binding, bl->class_shadowed, i, cs)
> @@ -641,7 +688,7 @@ pph_out_binding_level (pph_stream *stream, cp_binding_level *bl)
>
>   pph_out_chain (stream, bl->blocks);
>   pph_out_tree_or_ref (stream, bl->this_entity);
> -  pph_out_binding_level (stream, bl->level_chain);
> +  pph_out_binding_level (stream, bl->level_chain, filter);
>   pph_out_tree_vec (stream, bl->dead_vars_from_for);
>   pph_out_chain (stream, bl->statement_list);
>   pph_out_uint (stream, bl->binding_depth);
> @@ -655,6 +702,20 @@ pph_out_binding_level (pph_stream *stream, cp_binding_level *bl)
>  }
>
>
> +/* Write all the fields of cp_binding_level instance BL to STREAM.  Do not
> +   emit any nodes in BL that do not match FILTER.  */
> +
> +static void
> +pph_out_binding_level (pph_stream *stream, cp_binding_level *bl,
> +                      unsigned filter)
> +{
> +  if (!pph_out_start_record (stream, bl))
> +    return;
> +
> +  pph_out_binding_level_1 (stream, bl, filter);
> +}
> +
> +
>  /* Write out the tree_common fields from T to STREAM.  */
>
>  static void
> @@ -708,7 +769,7 @@ pph_out_language_function (pph_stream *stream, struct language_function *lf)
>
>   /* FIXME pph.  We are not writing lf->x_named_labels.  */
>
> -  pph_out_binding_level (stream, lf->bindings);
> +  pph_out_binding_level (stream, lf->bindings, PPHF_NONE);
>   pph_out_tree_vec (stream, lf->x_local_names);
>
>   /* FIXME pph.  We are not writing lf->extern_decl_map.  */
> @@ -847,7 +908,7 @@ pph_out_struct_function (pph_stream *stream, struct function *fn)
>  static void
>  pph_out_ld_ns (pph_stream *stream, struct lang_decl_ns *ldns)
>  {
> -  pph_out_binding_level (stream, ldns->level);
> +  pph_out_binding_level (stream, ldns->level, PPHF_NONE);
>  }
>
>
> @@ -1058,36 +1119,46 @@ pph_out_lang_type (pph_stream *stream, tree type)
>  }
>
>
> -/* 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.  */
> +/* Write the global bindings in scope_chain to STREAM.  */
>
>  static void
> -pph_out_scope_chain (pph_stream *stream, struct saved_scope *ss)
> +pph_out_scope_chain (pph_stream *stream)
>  {
>   /* 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));
> +  gcc_assert (scope_chain->old_namespace == global_namespace
> +             && !(scope_chain->class_name
> +                  || scope_chain->class_type
> +                  || scope_chain->access_specifier
> +                  || scope_chain->function_decl
> +                  || scope_chain->template_parms
> +                  || scope_chain->x_saved_tree
> +                  || scope_chain->class_bindings
> +                  || scope_chain->prev
> +                  || scope_chain->unevaluated_operand
> +                  || scope_chain->inhibit_evaluation_warnings
> +                  || scope_chain->x_processing_template_decl
> +                  || scope_chain->x_processing_specialization
> +                  || scope_chain->x_processing_explicit_instantiation
> +                  || scope_chain->need_pop_function_context
> +                  || scope_chain->x_stmt_tree.x_cur_stmt_list
> +                  || scope_chain->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);
> +     be NULL or be some temporary disposable state.
> +
> +     Note that we explicitly force the pickling of
> +     scope_chain->bindings.  If we had previously read another PPH
> +     image, scope_chain->bindings will be in the other image's pickle
> +     cache.  This would cause pph_out_binding_level to emit a cache
> +     reference to it, instead of writing its fields.  */
> +  {
> +    unsigned ix;
> +    pph_cache_add (stream, scope_chain->bindings, &ix);
> +    pph_out_record_marker (stream, PPH_RECORD_START);
> +    pph_out_uint (stream, ix);
> +    pph_out_binding_level_1 (stream, scope_chain->bindings, PPHF_NO_XREFS);
> +  }
>  }
>
>
> @@ -1278,24 +1349,30 @@ pph_out_line_table (pph_stream *stream, struct line_maps *linetab)
>   pph_out_uint (stream, linetab->max_column_hint);
>  }
>
> -/* Write PPH output symbols and IDENTS_USED to STREAM as an object.  */
> +/* Write all the contents of STREAM.  */
>
>  static void
> -pph_write_file_contents (pph_stream *stream, cpp_idents_used *idents_used)
> -{
> +pph_write_file (pph_stream *stream)
> +{
> +  cpp_idents_used idents_used;
> +
> +  if (flag_pph_debug >= 1)
> +    fprintf (pph_logfile, "PPH: Writing %s\n", pph_out_file);
> +
>   /* Emit the list of PPH files included by STREAM.  These files will
>      be read and instantiated before any of the content in STREAM.  */
>   pph_out_includes (stream);
>
>   /* Emit all the identifiers and pre-processor symbols in the global
>      namespace.  */
> -  pph_out_identifiers (stream, idents_used);
> +  idents_used = cpp_lt_capture (parse_in);
> +  pph_out_identifiers (stream, &idents_used);
>
>   /* Emit the line table entries.  */
>   pph_out_line_table (stream, line_table);
>
>   /* Emit the bindings for the global namespace.  */
> -  pph_out_scope_chain (stream, scope_chain);
> +  pph_out_scope_chain (stream);
>   if (flag_pph_dump_tree)
>     pph_dump_namespace (pph_logfile, global_namespace);
>
> @@ -1314,21 +1391,6 @@ pph_write_file_contents (pph_stream *stream, cpp_idents_used *idents_used)
>  }
>
>
> -/* Write all the collected parse trees to STREAM.  */
> -
> -static void
> -pph_write_file (pph_stream *stream)
> -{
> -  cpp_idents_used idents_used;
> -
> -  if (flag_pph_debug >= 1)
> -    fprintf (pph_logfile, "PPH: Writing %s\n", pph_out_file);
> -
> -  idents_used = cpp_lt_capture (parse_in);
> -  pph_write_file_contents (stream, &idents_used);
> -}
> -
> -
>  /* Emit the fields of FUNCTION_DECL FNDECL to STREAM.  */
>
>  static void
> @@ -1794,14 +1856,16 @@ pph_add_decl_to_symtab (tree decl)
>  }
>
>
> -/* Add STREAM to the list of files included by pph_out_stream.  */
> +/* Add INCLUDE to the list of files included by STREAM.  If STREAM is
> +   NULL, INCLUDE is added to the list of includes for pph_out_stream
> +   (the image that we are currently generating).  */
>
>  void
> -pph_add_include (pph_stream *stream)
> +pph_add_include (pph_stream *stream, pph_stream *include)
>  {
> -  pph_stream *out_stream = pph_out_stream;
> -  VEC_safe_push (pph_stream_ptr, heap, out_stream->includes, stream);
> -  stream->nested_p = true;
> +  if (stream == NULL)
> +    stream = pph_out_stream;
> +  VEC_safe_push (pph_stream_ptr, heap, stream->includes, include);
>  }
>
>
> @@ -1823,14 +1887,17 @@ pph_writer_init (void)
>  void
>  pph_writer_finish (void)
>  {
> -  pph_stream *out_stream = pph_out_stream;
> -  const char *offending_file = cpp_main_missing_guard (parse_in);
> +  const char *offending_file;
> +
> +  if (pph_out_stream == NULL)
> +    return;
>
> +  offending_file = cpp_main_missing_guard (parse_in);
>   if (offending_file == NULL)
> -    pph_write_file (out_stream);
> +    pph_write_file (pph_out_stream);
>   else
>     error ("header lacks guard for PPH: %s", offending_file);
>
> -  pph_stream_close (out_stream);
> +  pph_stream_close (pph_out_stream);
>   pph_out_stream = NULL;
>  }
> diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
> index afabeb5..1ac5bf4 100644
> --- a/gcc/cp/pph-streamer.c
> +++ b/gcc/cp/pph-streamer.c
> @@ -114,7 +114,6 @@ pph_stream_open (const char *name, const char *mode)
>   stream->name = xstrdup (name);
>   stream->write_p = (strchr (mode, 'w') != NULL);
>   stream->cache.m = pointer_map_create ();
> -  stream->open_p = true;
>   stream->symtab.m = pointer_set_create ();
>   if (stream->write_p)
>     pph_init_write (stream);
> @@ -128,15 +127,11 @@ pph_stream_open (const char *name, const char *mode)
>
>
>
> -/* Helper for pph_stream_close.  Do not check whether STREAM is a
> -   nested header.  */
> +/* Close PPH stream STREAM.  */
>
> -static void
> -pph_stream_close_1 (pph_stream *stream)
> +void
> +pph_stream_close (pph_stream *stream)
>  {
> -  unsigned i;
> -  pph_stream *include;
> -
>   if (flag_pph_debug >= 1)
>     fprintf (pph_logfile, "PPH: Closing %s\n", stream->name);
>
> @@ -147,10 +142,6 @@ pph_stream_close_1 (pph_stream *stream)
>
>   fclose (stream->file);
>
> -  /* Close all the streams we opened for included PPH images.  */
> -  FOR_EACH_VEC_ELT (pph_stream_ptr, stream->includes, i, include)
> -    pph_stream_close_1 (include);
> -
>   /* Deallocate all memory used.  */
>   stream->file = NULL;
>   VEC_free (void_p, heap, stream->cache.v);
> @@ -180,26 +171,6 @@ pph_stream_close_1 (pph_stream *stream)
>  }
>
>
> -/* Close PPH stream STREAM.  */
> -
> -void
> -pph_stream_close (pph_stream *stream)
> -{
> -  /* If STREAM is nested into another PPH file, then we cannot
> -     close it just yet.  The parent PPH file will need to access
> -     STREAM's symbol table (to avoid writing the same symbol
> -     more than once).  In this case, STREAM will be closed by the
> -     parent file.  */
> -  if (stream->nested_p)
> -    {
> -      gcc_assert (!stream->write_p);
> -      return;
> -    }
> -
> -  pph_stream_close_1 (stream);
> -}
> -
> -
>  /* Data types supported by the PPH tracer.  */
>  enum pph_trace_type
>  {
> @@ -420,13 +391,12 @@ pph_cache_insert_at (pph_stream *stream, void *data, unsigned ix)
>  }
>
>
> -/* Add pointer DATA to the pickle cache in STREAM.  If IX_P is not
> -   NULL, on exit *IX_P will contain the slot number where DATA is
> -   stored.  Return true if DATA already existed in the cache, false
> -   otherwise.  */
> +/* Return true if DATA exists in STREAM's pickle cache.  If IX_P is not
> +   NULL, store the cache slot where DATA resides in *IX_P (or (unsigned)-1
> +   if DATA is not found).  */
>
>  bool
> -pph_cache_add (pph_stream *stream, void *data, unsigned *ix_p)
> +pph_cache_lookup (pph_stream *stream, void *data, unsigned *ix_p)
>  {
>   void **map_slot;
>   unsigned ix;
> @@ -436,13 +406,12 @@ pph_cache_add (pph_stream *stream, void *data, unsigned *ix_p)
>   if (map_slot == NULL)
>     {
>       existed_p = false;
> -      ix = VEC_length (void_p, stream->cache.v);
> -      pph_cache_insert_at (stream, data, ix);
> +      ix = (unsigned) -1;
>     }
>   else
>     {
> -      unsigned HOST_WIDE_INT slot_ix = (unsigned HOST_WIDE_INT) *map_slot;
> -      gcc_assert (slot_ix == (unsigned) slot_ix);
> +      intptr_t slot_ix = (intptr_t) *map_slot;
> +      gcc_assert (slot_ix == (intptr_t)(unsigned) slot_ix);
>       ix = (unsigned) slot_ix;
>       existed_p = true;
>     }
> @@ -454,13 +423,98 @@ pph_cache_add (pph_stream *stream, void *data, unsigned *ix_p)
>  }
>
>
> -/* Return the pointer at slot IX in STREAM's pickle cache.  */
> +/* Return true if DATA is in the pickle cache of one of STREAM's
> +   included images.
> +
> +   If DATA is found:
> +      - the index for INCLUDE_P into IMAGE->INCLUDES is returned in
> +       *INCLUDE_IX_P (if INCLUDE_IX_P is not NULL),
> +      - the cache slot index for DATA into *INCLUDE_P's pickle cache
> +       is returned in *IX_P (if IX_P is not NULL), and,
> +      - the function returns true.
> +
> +   If DATA is not found:
> +      - *INCLUDE_IX_P is set to -1 (if INCLUDE_IX_P is not NULL),
> +      - *IX_P is set to -1 (if IX_P is not NULL), and,
> +      - the function returns false.  */
> +
> +bool
> +pph_cache_lookup_in_includes (pph_stream *stream, void *data,
> +                             unsigned *include_ix_p, unsigned *ix_p)
> +{
> +  unsigned include_ix, ix;
> +  pph_stream *include;
> +  bool found_it;
> +
> +  found_it = false;
> +  FOR_EACH_VEC_ELT (pph_stream_ptr, stream->includes, include_ix, include)
> +    if (pph_cache_lookup (include, data, &ix))
> +      {
> +       found_it = true;
> +       break;
> +      }
> +
> +  if (!found_it)
> +    {
> +      include_ix = ix = (unsigned) -1;
> +      ix = (unsigned) -1;
> +    }
> +
> +  if (include_ix_p)
> +    *include_ix_p = include_ix;
> +
> +  if (ix_p)
> +    *ix_p = ix;

Instead of wasting time on an if to check that include_ix_p or ix_p
are not null, couldn't we simply put a gcc_assert that they aren't
NULL at the beginning of the function? I can't think of a situation
where we want to call this with NULL pointers, i.e. without retrieving
the values we are asking for... unless sometimes it's called only
caring about the boolean returned value?

> +
> +  return found_it;
> +}
> +
> +
> +/* Add pointer DATA to the pickle cache in STREAM.  If IX_P is not
> +   NULL, on exit *IX_P will contain the slot number where DATA is
> +   stored.  Return true if DATA already existed in the cache, false
> +   otherwise.  */
> +
> +bool
> +pph_cache_add (pph_stream *stream, void *data, unsigned *ix_p)
> +{
> +  unsigned ix;
> +  bool existed_p;
> +
> +  if (pph_cache_lookup (stream, data, &ix))
> +    existed_p = true;
> +  else
> +    {
> +      existed_p = false;
> +      ix = VEC_length (void_p, stream->cache.v);
> +      pph_cache_insert_at (stream, data, ix);
> +    }
> +
> +  if (ix_p)
> +    *ix_p = ix;
> +
> +  return existed_p;
> +}
> +
> +
> +/* Return the pointer at slot IX in STREAM's pickle cache.  If INCLUDE_IX
> +   is not -1U, then instead of looking up in STREAM's pickle cache,
> +   the pointer is looked up in the pickle cache for
> +   STREAM->INCLUDES[INCLUDE_IX].  */
>
>  void *
> -pph_cache_get (pph_stream *stream, unsigned ix)
> +pph_cache_get (pph_stream *stream, unsigned include_ix, unsigned ix)
>  {
> -  void *data = VEC_index (void_p, stream->cache.v, ix);
> -  gcc_assert (data);
> +  void *data;
> +  pph_stream *image;
> +
> +  /* Determine which image's pickle cache to use.  */
> +  if (include_ix == (unsigned) -1)

As mentioned above, I feel this is dangerous..

> +    image = stream;
> +  else
> +    image = VEC_index (pph_stream_ptr, stream->includes, include_ix);
>
> +  data = VEC_index (void_p, image->cache.v, ix);
> +  gcc_assert (data);
>   return data;
>  }
> diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h
> index d9fe53b..19578fd 100644
> --- a/gcc/cp/pph-streamer.h
> +++ b/gcc/cp/pph-streamer.h
> @@ -28,9 +28,26 @@ along with GCC; see the file COPYING3.  If not see
>
>  /* Record markers.  */
>  enum pph_record_marker {
> -  PPH_RECORD_START = 0xfd,
> +  /* This record contains the physical representation of the memory data.  */
> +  PPH_RECORD_START = 0x23,
> +
> +  /* End of record marker.  If a record starts with PPH_RECORD_END, the
> +     reader should return a NULL pointer.  */
>   PPH_RECORD_END,
> -  PPH_RECORD_SHARED
> +
> +  /* Internal reference.  This marker indicates that this data has
> +     been written before and it resides in the pickle cache for the
> +     current image.  Following this marker, the reader will find the
> +     cache slot where the data has been stored.  */
> +  PPH_RECORD_IREF,
> +
> +  /* External reference.  This marker indicates that this data has
> +     been written before and it resides in the pickle cache for
> +     another image.  Following this marker, the reader will find two
> +     indices: (1) the index into the include table where the other
> +     image lives, and (2) the cache slot into that image's pickle
> +     cache where the data resides.  */
> +  PPH_RECORD_XREF
>  };
>
>  /* Symbol table markers.  These are all the symbols that need to be
> @@ -155,15 +172,6 @@ typedef struct pph_stream {
>   /* Nonzero if the stream was opened for writing.  */
>   unsigned int write_p : 1;
>
> -  /* Nonzero if the file associated with this stream is open.
> -     After we read a PPH image, we deallocate all the memory used
> -     during streaming, but we keep the stream around to access its
> -     symbol table.  */
> -  unsigned int open_p : 1;
> -
> -  /* Nonzero if this PPH file is included from another PPH file.  */
> -  unsigned int nested_p : 1;
> -
>   /* Symbol table.  This is collected as the compiler instantiates
>     symbols and functions.  Once we finish parsing the header file,
>     this array is written out to the PPH image.  This way, the reader
> @@ -177,8 +185,10 @@ typedef struct pph_stream {
>   VEC(pph_stream_ptr,heap) *includes;
>  } pph_stream;
>
> -/* Filter values for pph_out_chain_filtered.  */
> -enum chain_filter { NONE, NO_BUILTINS };
> +/* Filter values to avoid emitting certain objects to a PPH file.  */
> +#define PPHF_NONE              0
> +#define PPHF_NO_BUILTINS       (1 << 0)
> +#define PPHF_NO_XREFS          (1 << 1)
>
>  /* In pph-streamer.c.  */
>  pph_stream *pph_stream_open (const char *, const char *);
> @@ -192,15 +202,18 @@ void pph_trace_location (pph_stream *, location_t);
>  void pph_trace_chain (pph_stream *, tree);
>  void pph_trace_bitpack (pph_stream *, struct bitpack_d *);
>  void pph_cache_insert_at (pph_stream *, void *, unsigned);
> +bool pph_cache_lookup (pph_stream *, void *, unsigned *);
> +bool pph_cache_lookup_in_includes (pph_stream *, void *, unsigned *,
> +                                  unsigned *);
>  bool pph_cache_add (pph_stream *, void *, unsigned *);
> -void *pph_cache_get (pph_stream *, unsigned);
> +void *pph_cache_get (pph_stream *, unsigned, unsigned);
>
>  /* In pph-streamer-out.c.  */
>  void pph_flush_buffers (pph_stream *);
>  void pph_init_write (pph_stream *);
>  void pph_write_tree (struct output_block *, tree, bool);
>  void pph_add_decl_to_symtab (tree);
> -void pph_add_include (pph_stream *);
> +void pph_add_include (pph_stream *, pph_stream *);
>  void pph_writer_init (void);
>  void pph_writer_finish (void);
>  void pph_write_location (struct output_block *, location_t);
> @@ -214,7 +227,8 @@ struct binding_table_s *pph_in_binding_table (pph_stream *);
>  void pph_init_read (pph_stream *);
>  tree pph_read_tree (struct lto_input_block *, struct data_in *);
>  location_t pph_read_location (struct lto_input_block *, struct data_in *);
> -void pph_read_file (const char *);
> +pph_stream *pph_read_file (const char *);
> +void pph_reader_finish (void);
>
>  /* In pt.c.  */
>  extern void pph_out_pending_templates_list (pph_stream *stream);
> @@ -487,4 +501,24 @@ pph_in_bitpack (pph_stream *stream)
>   return bp;
>  }
>
> +/* Write record MARKER to STREAM.  */
> +static inline void
> +pph_out_record_marker (pph_stream *stream, enum pph_record_marker marker)
> +{
> +  gcc_assert (marker == (enum pph_record_marker)(unsigned char) marker);
> +  pph_out_uchar (stream, marker);
> +}
> +
> +/* Read and return a record marker from STREAM.  */
> +static inline enum pph_record_marker
> +pph_in_record_marker (pph_stream *stream)
> +{
> +  enum pph_record_marker m = (enum pph_record_marker) pph_in_uchar (stream);
> +  gcc_assert (m == PPH_RECORD_START
> +             || m == PPH_RECORD_END
> +             || m == PPH_RECORD_IREF
> +             || m == PPH_RECORD_XREF);
> +  return m;
> +}
> +
>  #endif  /* GCC_CP_PPH_STREAMER_H  */
> diff --git a/gcc/cp/pph.c b/gcc/cp/pph.c
> index 9b92f88..91dc622 100644
> --- a/gcc/cp/pph.c
> +++ b/gcc/cp/pph.c
> @@ -171,9 +171,13 @@ pph_init (void)
>  void
>  pph_finish (void)
>  {
> -  if (pph_out_file != NULL)
> +  /* Finalize the writer.  */
>     pph_writer_finish ();
>
> +  /* Finalize the reader.  */
> +  pph_reader_finish ();
> +
> +  /* Close log files.  */
>   if (flag_pph_debug >= 1)
>     fprintf (pph_logfile, "PPH: Finishing.\n");
>
> diff --git a/gcc/testsuite/ChangeLog.pph b/gcc/testsuite/ChangeLog.pph
> index 7ece8ce..b2b87f0 100644
> --- a/gcc/testsuite/ChangeLog.pph
> +++ b/gcc/testsuite/ChangeLog.pph
> @@ -1,3 +1,9 @@
> +2011-08-15   Diego Novillo  <dnovillo@google.com>
> +
> +       * g++.dg/pph/x1tmplclass2.cc: Update expected ICE.
> +       * g++.dg/pph/x4tmplclass2.cc: Update expected ICE.
> +       * g++.dg/pph/z4tmplclass2.cc: Update expected ICE.
> +
>  2011-08-10   Diego Novillo  <dnovillo@google.com>
>
>        * g++.dg/pph/x1tmplclass2.cc: Update expected ICE message.
> diff --git a/gcc/testsuite/g++.dg/pph/x1tmplclass2.cc b/gcc/testsuite/g++.dg/pph/x1tmplclass2.cc
> index 2c7a8f3..f04335d 100644
> --- a/gcc/testsuite/g++.dg/pph/x1tmplclass2.cc
> +++ b/gcc/testsuite/g++.dg/pph/x1tmplclass2.cc
> @@ -1,5 +1,5 @@
>  // { dg-xfail-if "BOGUS" { "*-*-*" } { "-fpph-map=pph.map" } }
> -// { dg-bogus "x1tmplclass2.cc:1:0: internal compiler error: in pph_in_start_record, at cp/pph-streamer-in.c" "" { xfail *-*-* } 0 }
> +// { dg-bogus "x1tmplclass2.cc:1:0: internal compiler error: in pph_in_record_marker, at cp/pph-streamer.h" "" { xfail *-*-* } 0 }
>
>  #include "x0tmplclass23.h"
>  #include "a0tmplclass2_u.h"
> diff --git a/gcc/testsuite/g++.dg/pph/x4tmplclass2.cc b/gcc/testsuite/g++.dg/pph/x4tmplclass2.cc
> index 7c7e6a5..585d4c0 100644
> --- a/gcc/testsuite/g++.dg/pph/x4tmplclass2.cc
> +++ b/gcc/testsuite/g++.dg/pph/x4tmplclass2.cc
> @@ -1,5 +1,5 @@
>  // { dg-xfail-if "BOGUS" { "*-*-*" } { "-fpph-map=pph.map" } }
> -// { dg-bogus "x4tmplclass2.cc:1:0: internal compiler error: in pph_in_start_record, at cp/pph-streamer-in.c" "" { xfail *-*-* } 0 }
> +// { dg-bogus "x4tmplclass2.cc:1:0: internal compiler error: in pph_in_record_marker, at cp/pph-streamer.h" "" { xfail *-*-* } 0 }
>
>  #include "x0tmplclass21.h"
>  #include "x0tmplclass22.h"
> diff --git a/gcc/testsuite/g++.dg/pph/z4tmplclass2.cc b/gcc/testsuite/g++.dg/pph/z4tmplclass2.cc
> index b932f5e..0243829 100644
> --- a/gcc/testsuite/g++.dg/pph/z4tmplclass2.cc
> +++ b/gcc/testsuite/g++.dg/pph/z4tmplclass2.cc
> @@ -1,5 +1,5 @@
>  // { dg-xfail-if "BOGUS" { "*-*-*" } { "-fpph-map=pph.map" } }
> -// { dg-bogus "z4tmplclass2.cc:1:0: internal compiler error: in pph_in_start_record, at cp/pph-streamer-in.c" "" { xfail *-*-* } 0 }
> +// { dg-bogus "z4tmplclass2.cc:1:0: internal compiler error: in pph_in_record_marker, at cp/pph-streamer.h" "" { xfail *-*-* } 0 }
>
>  #include "x0tmplclass23.h"
>  #include "x0tmplclass24.h"
> --
> 1.7.3.1
>
>
> --
> This patch is available for review at http://codereview.appspot.com/4873051
>

On 08/16/2011 02:04 PM, Gabriel Charette wrote:

> Setting *cache_ix to (unsigned) -1 used to be a "hack" (with a comment
> explaining it which was removed just below), to avoid a warning about
> it being unset in a branch, but that branch was only PPH_RECORD_END,
> in which case it didn't matter.

Yeah, I remember, but I set them to -1 now as an actual return value now.

> Now that we actually check in pph_cache_get that include_ix ==
> (unsigned) -1, I'm not so sure this is good... while (unsigned) -1 is
> a very large unsigned int, it's not impossible to have an actual cache
> entry with that number...

Oh, no.  If we ever need 4B entries in the cache, we will die much 
sooner than this.

>> @@ -457,13 +478,13 @@ pph_in_cxx_binding_1 (pph_stream *stream)
>>    cxx_binding *cb;
>>    tree value, type;
>>    enum pph_record_marker marker;
>> -  unsigned ix;
>> +  unsigned ix, include_ix;
>
> Sometimes you call the local variable "include_ix"...
>
>> @@ -505,13 +526,13 @@ pph_in_class_binding (pph_stream *stream)
>>   {
>>    cp_class_binding *cb;
>>    enum pph_record_marker marker;
>> -  unsigned ix;
>> +  unsigned image_ix, ix;
>
> ... and sometimes image_ix: consistency would be nice, although not necessary...

Thanks, I'll fix.  I'm horribly inconsistent with naming schemes (blame 
my evil twin).


Diego.