[pph] Add initial support for including nested pph images

[Diego Novillo, 2011-08-04 15:41:52]

This patch adds initial support for PPH images that include other PPH
images.  Currently, we support this but it is inefficient.  When a
parent image includes a child image, the parent embeds all the
contents of the child in its image.  This is a waste of space.

There is one other missing piece for this to work, however.  We need
to change the way references work.  Currently, they are simply an
offset into the streamer cache, which is created for every file we
open.  When we have images including other images, the references to
symbols in another file's symbol table should be 2-dimensional.

I am currently adding that support, but this will be more intrusive,
so I'm flushing this out first.

The main changes:

- Add a symbol table in every stream (field symtab in pph_stream).
  This supports efficient searches using a pointer set and has the
  symbols arranged in declaration order.

- Initialize PPH support before the parser starts.  This is needed to
  allow the parser to register symbols into the PPH symbol table for
  the image that we are currently generating.

- New stream flags to determine whether the stream is currently open,
  nested images and a table of included images.

There is a pretty big potential change in the order in which files are
included.  Suppose we have file parent.h:

---- parent.h ------
int X;
class A { ... };
#include "child.pph"
TypeFromChild L;
--------------------

When parent and child are converted to pph images, the image child.pph
will be read *before* any of the contents of parent.pph.  We already
require child.pph to not depend on any content from the outside, but
child.pph could alter macros defined and used by parent.h before the
original inclusion.  The plan for now is to disallow this.

Tested on x86_64.


Diego.

cp/ChangeLog.pph

	* parser.c (cp_lexer_new_main): Move call to pph_init ...
	(c_parse_file): ... here.
	Call pph_finish only if pph_enabled_p returns true.
	* pph-streamer-in.c (pph_in_includes): New.
	(pph_read_file_contents): Call it.
	(pph_read_file): Call pph_add_include.
	* pph-streamer-out.c (current_pph_file): Remove.  Update all users.
	(decls_to_register_t) Likewise.
	(decls_to_register): Likewise.
	(pph_out_stream): Declare.
	(pph_out_symtab_marker): Assert that the MARKER fits in an
	unsigned char.
	(pph_out_includes): New.
	(pph_write_file): Make static.  Assume that STREAM is opened and
	closed by the caller.
	(pph_add_include): New.
	(pph_writer_init): Open pph_out_file into pph_out_stream.
	(pph_writer_finish): Close pph_out_stream.  Call pph_write_file
	if no errors were found.
	(pph_add_decl_to_symtab): Rename from pph_add_decl_to_register.
	Update all users.
	* pph-streamer.c (pph_stream_open): Tidy.
	Initialize stream->symtab.
	(pph_stream_close_1): Factor out of ...
	(pph_stream_close): ... here.
	Only call pph_stream_close_1 if STREAM is not nested in another
	PPH file.
	* pph-streamer.h (struct pph_symtab): Declare.
	(pph_stream_ptr): Declare.
	(struct pph_stream): Add fields open_p, nested_p, symtab
	and includes.
	(pph_add_decl_to_symtab): Declare.
	(pph_add_include): Declare.
	(pph_writer_init): Declare.
	(pph_writer_finish): Declare.
	* pph.c (pph_init): Call pph_writer_init if pph_out_file is set.
	(pph_finish): Call pph_writer_finish if pph_out_file is set.
	* pph.h (pph_enabled_p): New.

testsuite/ChangeLog.pph:

	* g++.dg/pph/c1pr44948-1a.cc: Use the same flags as the header file.
	Mark fixed.

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 5896688..3b312d3 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -5879,10 +5879,9 @@ cp_rest_of_decl_compilation (tree decl, int top_level,
int at_end)
 {
   rest_of_decl_compilation (decl, top_level, at_end);
 
-  /* If we are generating a PPH image, add DECL to the list of
-     declarations that need to be registered when this image is read.  */
+  /* If we are generating a PPH image, add DECL to its symbol table.  */
   if (pph_out_file)
-    pph_add_decl_to_register (decl);
+    pph_add_decl_to_symtab (decl);
 }
 
 
@@ -12815,10 +12814,9 @@ start_preparsed_function (tree decl1, tree attrs, int
flags)
 
   store_parm_decls (current_function_parms);
 
-  /* If we are generating a PPH image, add DECL1 to the list of
-     declarations that need to be registered when restoring the image.  */
+  /* If we are generating a PPH image, add DECL1 to its symbol table.  */
   if (pph_out_file)
-    pph_add_decl_to_register (decl1);
+    pph_add_decl_to_symtab (decl1);
 }
 
 
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index c48807d..e069441 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -604,20 +604,19 @@ cp_lexer_new_main (void)
   cp_lexer *lexer;
   cp_token token;
 
-  if (pph_out_file != NULL || query_have_pph_map ())
+  if (pph_enabled_p ())
     {
       /* FIXME pph.  PPH is incompatible with PCH, so do not read ahead to
 	 the first token looking for a PCH pragma.  This convoluted
 	 initialization could be simplified if PCH was implemented in
-	 terms of the incremental compiler.  */
+	 terms of PPH.  */
       lexer = cp_lexer_alloc ();
-      pph_init ();
       cp_lexer_get_tokens (lexer);
     }
   else
     {
       /* FIXME pph.  Get rid of this duplicate and call cp_lexer_get_tokens
-	 all the time.  This is needed only because for PCH support we
+	 all the time.  This is needed only because, for PCH support, we
 	 are forced to read one token in advance before starting the
 	 main lexer loop.  */
 
@@ -25662,12 +25661,17 @@ c_parse_file (void)
     }
   already_called = true;
 
+  if (pph_enabled_p ())
+    pph_init ();
+
   the_parser = cp_parser_new ();
   push_deferring_access_checks (flag_access_control
 				? dk_no_deferred : dk_no_check);
   cp_parser_translation_unit (the_parser);
   the_parser = NULL;
-  pph_finish ();
+
+  if (pph_enabled_p ())
+    pph_finish ();
 }
 
 #include "gt-cp-parser.h"
diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index da006a1..eda93b8 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -1412,6 +1412,25 @@ pph_in_symtab (pph_stream *stream)
 }
 
 
+/* Read all the images included by STREAM.  */
+
+static void
+pph_in_includes (pph_stream *stream)
+{
+  unsigned i, num;
+
+  num = pph_in_uint (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);
+    }
+}
+
+
 /* Read contents of PPH file in STREAM.  */
 
 static void
@@ -1425,6 +1444,11 @@ pph_read_file_contents (pph_stream *stream)
   unsigned i;
   VEC(tree,gc) *file_unemitted_tinfo_decls;
 
+  /* Read all the images included by STREAM.  */
+  pph_in_includes (stream);
+
+  /* Read all the identifiers and pre-processor symbols in the global
+     namespace.  */
   pph_in_identifiers (stream, &idents_used);
 
   /* FIXME pph: This validation is weak.  */
@@ -1459,6 +1483,12 @@ pph_read_file_contents (pph_stream *stream)
 
   /* Read and process the symbol table.  */
   pph_in_symtab (stream);
+
+  /* If we are generating an image, the PPH contents we just read from
+     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);
 }
 
 
@@ -1477,9 +1507,6 @@ pph_read_file (const char *filename)
     {
       pph_read_file_contents (stream);
       pph_stream_close (stream);
-
-      if (flag_pph_debug >= 1)
-        fprintf (pph_logfile, "PPH: Closing %s\n", filename);
     }
   else
     error ("Cannot open PPH file for reading: %s: %m", filename);
diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index 0e42131..cbd2446 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -33,26 +33,11 @@ along with GCC; see the file COPYING3.  If not see
 #include "cppbuiltin.h"
 #include "cgraph.h"
 
-/* FIXME pph.  This holds the FILE handle for the current PPH file
-   that we are writing.  It is necessary because the LTO callbacks do
-   not allow passing a FILE handle to them.  */
-static FILE *current_pph_file = NULL;
-
-/* List of declarations to register with the middle end.  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 will be able to instantiate these
-   symbols in the same order that they were instantiated originally.  */
-typedef struct decls_to_register_t {
-  /* Table of all the declarations to register in declaration order.  */
-  VEC(tree,heap) *v;
-
-  /* Set of declarations to register used to avoid adding duplicate
-     entries to the table.  */
-  struct pointer_set_t *m;
-} decls_to_register_t;
-
-static decls_to_register_t decls_to_register = { NULL, NULL };
+/* PPH stream that we are currently generating.  FIXME pph, this
+   global is needed because we call back from various parts of the
+   compiler that do not know about PPH (e.g., some LTO callbacks,
+   cp_rest_of_decl_compilation).  */
+static pph_stream *pph_out_stream = NULL;
 
 /* Callback for packing value fields in ASTs.  BP is the bitpack 
    we are packing into.  EXPR is the tree to pack.  */
@@ -119,13 +104,13 @@ pph_begin_section (const char *name ATTRIBUTE_UNUSED)
 
 
 /* Callback for lang_hooks.lto.append_data.  Write LEN bytes from DATA
-   into current_pph_file.  BLOCK is currently unused.  */
+   into pph_out_stream.  BLOCK is currently unused.  */
 
 static void
 pph_out (const void *data, size_t len, void *block ATTRIBUTE_UNUSED)
 {
   if (data)
-    fwrite (data, len, 1, current_pph_file);
+    fwrite (data, len, 1, pph_out_stream->file);
 }
 
 
@@ -194,9 +179,6 @@ pph_out_body (pph_stream *stream)
 void
 pph_flush_buffers (pph_stream *stream)
 {
-  gcc_assert (current_pph_file == NULL);
-  current_pph_file = stream->file;
-
   /* Redirect the LTO basic I/O langhooks.  */
   lang_hooks.lto.begin_section = pph_begin_section;
   lang_hooks.lto.append_data = pph_out;
@@ -207,7 +189,6 @@ pph_flush_buffers (pph_stream *stream)
   pph_out_header (stream);
   pph_out_body (stream);
   lto_end_section ();
-  current_pph_file = NULL;
 }
 
 
@@ -1186,7 +1167,7 @@ pph_out_identifiers (pph_stream *stream, cpp_idents_used
*identifiers)
 static inline void
 pph_out_symtab_marker (pph_stream *stream, enum pph_symtab_marker marker)
 {
-  pph_out_uchar (stream, marker);
+  pph_out_uchar (stream, (unsigned char) marker);
 }
 
 
@@ -1204,8 +1185,8 @@ pph_out_symtab (pph_stream *stream)
   tree decl;
   unsigned i;
 
-  pph_out_uint (stream, VEC_length (tree, decls_to_register.v));
-  FOR_EACH_VEC_ELT (tree, decls_to_register.v, i, decl)
+  pph_out_uint (stream, VEC_length (tree, stream->symtab.v));
+  FOR_EACH_VEC_ELT (tree, stream->symtab.v, i, decl)
     if (TREE_CODE (decl) == FUNCTION_DECL && DECL_STRUCT_FUNCTION (decl))
       {
 	if (DECL_SAVED_TREE (decl))
@@ -1219,13 +1200,20 @@ pph_out_symtab (pph_stream *stream)
 	pph_out_symtab_marker (stream, PPH_SYMTAB_DECL);
 	pph_out_tree (stream, decl);
       }
+}
 
-  if (decls_to_register.m)
-    {
-      VEC_free (tree, heap, decls_to_register.v);
-      pointer_set_destroy (decls_to_register.m);
-      decls_to_register.m = NULL;
-    }
+
+/* Emit the list of all the PPH files included by STREAM.  */
+
+static void
+pph_out_includes (pph_stream *stream)
+{
+  unsigned i;
+  pph_stream *include;
+
+  pph_out_uint (stream, VEC_length (pph_stream_ptr, stream->includes));
+  FOR_EACH_VEC_ELT (pph_stream_ptr, stream->includes, i, include)
+    pph_out_string (stream, include->name);
 }
 
 
@@ -1234,7 +1222,12 @@ pph_out_symtab (pph_stream *stream)
 static void
 pph_write_file_contents (pph_stream *stream, cpp_idents_used *idents_used)
 { 
-  /* Emit all the identifiers and symbols in the global namespace.  */
+  /* 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);
 
   /* Emit the bindings for the global namespace.  */
@@ -1257,25 +1250,18 @@ pph_write_file_contents (pph_stream *stream,
cpp_idents_used *idents_used)
 }
 
 
-/* Write PPH output file.  */
+/* Write all the collected parse trees to STREAM.  */
 
-void
-pph_write_file (void)
+static void
+pph_write_file (pph_stream *stream)
 {
-  pph_stream *stream;
   cpp_idents_used idents_used;
 
   if (flag_pph_debug >= 1)
     fprintf (pph_logfile, "PPH: Writing %s\n", pph_out_file);
 
-  stream = pph_stream_open (pph_out_file, "wb");
-  if (!stream)
-    fatal_error ("Cannot open PPH file for writing: %s: %m", pph_out_file);
-
   idents_used = cpp_lt_capture (parse_in);
   pph_write_file_contents (stream, &idents_used);
-
-  pph_stream_close (stream);
 }
 
 
@@ -1634,18 +1620,58 @@ pph_write_tree (struct output_block *ob, tree expr,
 }
 
 
-/* Add DECL to the list of symbols that need to be registered with the
-   middle end when reading current_pph_stream.  */
+/* Add DECL to the symbol table for pph_out_stream.  */
 
 void
-pph_add_decl_to_register (tree decl)
+pph_add_decl_to_symtab (tree decl)
 {
-  if (decl)
-    {
-      if (decls_to_register.m == NULL)
-	decls_to_register.m = pointer_set_create ();
+  pph_stream *stream = pph_out_stream;
 
-      if (!pointer_set_insert (decls_to_register.m, decl))
-	VEC_safe_push (tree, heap, decls_to_register.v, decl);
-    }
+  if (decl == NULL || stream == NULL)
+    return;
+
+  if (!pointer_set_insert (stream->symtab.m, decl))
+    VEC_safe_push (tree, heap, stream->symtab.v, decl);
+}
+
+
+/* Add STREAM to the list of files included by pph_out_stream.  */
+
+void
+pph_add_include (pph_stream *stream)
+{
+  pph_stream *out_stream = pph_out_stream;
+  VEC_safe_push (pph_stream_ptr, heap, out_stream->includes, stream);
+  stream->nested_p = true;
+}
+
+
+/* Initialize the PPH writer.  */
+
+void
+pph_writer_init (void)
+{
+  gcc_assert (pph_out_stream == NULL);
+
+  pph_out_stream = pph_stream_open (pph_out_file, "wb");
+  if (pph_out_stream == NULL)
+    fatal_error ("Cannot open PPH file for writing: %s: %m", pph_out_file);
+}
+
+
+/* Finalize the PPH writer.  */
+
+void
+pph_writer_finish (void)
+{
+  pph_stream *out_stream = pph_out_stream;
+  const char *offending_file = cpp_main_missing_guard (parse_in);
+
+  if (offending_file == NULL)
+    pph_write_file (out_stream);
+  else
+    error ("header lacks guard for PPH: %s", offending_file);
+
+  pph_stream_close (out_stream);
+  pph_out_stream = NULL;
 }
diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
index 08b8a46..a7a59a9 100644
--- a/gcc/cp/pph-streamer.c
+++ b/gcc/cp/pph-streamer.c
@@ -127,37 +127,56 @@ pph_stream_open (const char *name, const char *mode)
 
   stream = NULL;
   f = fopen (name, mode);
-  if (f)
-    {
-      pph_hooks_init ();
-      stream = XCNEW (pph_stream);
-      stream->file = f;
-      stream->name = xstrdup (name);
-      stream->write_p = (strchr (mode, 'w') != NULL);
-      if (stream->write_p)
-	pph_init_write (stream);
-      else
-	pph_init_read (stream);
-      stream->cache.v = NULL;
-      stream->cache.m = pointer_map_create ();
-    }
+  if (!f)
+    return NULL;
+
+  pph_hooks_init ();
+  stream = XCNEW (pph_stream);
+  stream->file = f;
+  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);
+  else
+    pph_init_read (stream);
 
   return stream;
 }
 
 
-/* Close PPH stream STREAM.  Write all the ASTs to disk and deallocate
-   all memory used by it.  */
 
-void
-pph_stream_close (pph_stream *stream)
+/* Helper for pph_stream_close.  Do not check whether STREAM is a
+   nested header.  */
+
+static void
+pph_stream_close_1 (pph_stream *stream)
 {
+  unsigned i;
+  pph_stream *include;
+
+  if (flag_pph_debug >= 1)
+    fprintf (pph_logfile, "PPH: Closing %s\n", stream->name);
+
+  /* If we were writing to STREAM, flush all the memory buffers.  This
+     does the actual writing of all the pickled data structures.  */
   if (stream->write_p)
     pph_flush_buffers (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);
   pointer_map_destroy (stream->cache.m);
+  VEC_free (tree, heap, stream->symtab.v);
+  pointer_set_destroy (stream->symtab.m);
 
   if (stream->write_p)
     {
@@ -181,6 +200,26 @@ pph_stream_close (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
 {
diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h
index d0c7f85..0f29c31 100644
--- a/gcc/cp/pph-streamer.h
+++ b/gcc/cp/pph-streamer.h
@@ -92,6 +92,22 @@ typedef struct pph_pickle_cache {
   struct pointer_map_t *m;
 } pph_pickle_cache;
 
+/* Symbol table for a PPH stream.  */
+typedef struct pph_symtab
+{
+  /* Table of all the declarations to register in declaration order.  */
+  VEC(tree,heap) *v;
+
+  /* Set of declarations to register used to avoid adding duplicate
+     entries to the table.  */
+  struct pointer_set_t *m;
+} pph_symtab;
+
+/* Vector of pph_stream pointers.  */
+struct pph_stream;
+typedef struct pph_stream *pph_stream_ptr;
+DEF_VEC_P(pph_stream_ptr);
+DEF_VEC_ALLOC_P(pph_stream_ptr,heap);
 
 /* Data structures used to encode and decode trees.  */
 
@@ -128,6 +144,27 @@ 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
+    will be able to instantiate these symbols in the same order that
+    they were instantiated originally.  */
+  pph_symtab symtab;
+
+  /* List of PPH files included by the PPH file that we are currently
+     generating.  Note that this list only contains PPH files, not
+     regular text headers.  Those are embedded in this stream.  */
+  VEC(pph_stream_ptr,heap) *includes;
 } pph_stream;
 
 /* Filter values for pph_out_chain_filtered.  */
@@ -154,8 +191,10 @@ void pph_init_write (pph_stream *);
 void pph_write_tree (struct output_block *, tree, bool);
 void pph_pack_value_fields (struct bitpack_d *, tree);
 void pph_out_tree_header (struct output_block *, tree);
-void pph_write_file (void);
-void pph_add_decl_to_register (tree);
+void pph_add_decl_to_symtab (tree);
+void pph_add_include (pph_stream *);
+void pph_writer_init (void);
+void pph_writer_finish (void);
 
 /* In name-lookup.c.  */
 struct binding_table_s;
@@ -348,7 +387,7 @@ pph_in_bytes (pph_stream *stream, void *p, size_t n)
     pph_trace_bytes (stream, p, n);
 }
 
-/* Read and return a string of up to MAX characters from STREAM.  */
+/* Read and return a string from STREAM.  */
 
 static inline const char *
 pph_in_string (pph_stream *stream)
diff --git a/gcc/cp/pph.c b/gcc/cp/pph.c
index 4e1a593..4bb3842 100644
--- a/gcc/cp/pph.c
+++ b/gcc/cp/pph.c
@@ -152,16 +152,17 @@ pph_init (void)
   if (flag_pph_debug >= 1)
     fprintf (pph_logfile, "PPH: Initializing.\n");
 
+  /* Set up the libcpp handler for #include.  */
   cb = cpp_get_callbacks (parse_in);
   cb->include = pph_include_handler;
-  /* FIXME pph: Use file change instead.
-  state->file_change_prev = cb->file_change;
-  cb->file_change = pph_file_change;
-  */
 
   table = cpp_lt_exchange (parse_in,
                            cpp_lt_create (cpp_lt_order, flag_pph_debug));
   gcc_assert (table == NULL);
+
+  /* If we are generating a PPH file, initialize the writer.  */
+  if (pph_out_file != NULL)
+    pph_writer_init ();
 }
 
 
@@ -171,13 +172,7 @@ void
 pph_finish (void)
 {
   if (pph_out_file != NULL)
-    {
-      const char *offending_file = cpp_main_missing_guard (parse_in);
-      if (offending_file == NULL)
-        pph_write_file ();
-      else
-        error ("header lacks guard for PPH: %s", offending_file);
-    }
+    pph_writer_finish ();
 
   if (flag_pph_debug >= 1)
     fprintf (pph_logfile, "PPH: Finishing.\n");
diff --git a/gcc/cp/pph.h b/gcc/cp/pph.h
index dc41ff6..b312685 100644
--- a/gcc/cp/pph.h
+++ b/gcc/cp/pph.h
@@ -43,10 +43,17 @@ typedef struct cp_token_hunk *cp_token_hunk_ptr;
 DEF_VEC_P (cp_token_hunk_ptr);
 DEF_VEC_ALLOC_P (cp_token_hunk_ptr,gc);
 
+/* Return true if PPH has been enabled.  */
+static inline bool
+pph_enabled_p (void)
+{
+  return pph_out_file != NULL || query_have_pph_map ();
+}
+
 /* Global state.  FIXME pph, get rid of these.  */
 
 /* Log file where PPH analysis is written to.  Controlled by
-   -fpph_logfile.  If this flag is not given, stdout is used.  */
+   -fpph-logfile.  If this flag is not given, stdout is used.  */
 extern FILE *pph_logfile;
 
 /* In pph.c  */
diff --git a/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
b/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
index c13ee87..d64c9ae 100644
--- a/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
+++ b/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
@@ -1,3 +1,2 @@
-// { dg-xfail-if "INFINITE" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "internal compiler error: in lto_streamer_cache_get, at
lto-streamer.c" "" { xfail *-*-* } 0 }
+/* { dg-options "-O -Wno-psabi -mtune=generic" } */
 #include "c0pr44948-1a.h"

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

[Gabriel Charette, 2011-08-04 21:47:41]

So if I understand correctly, the reference system doesn't work yet, you just
put out the design in this patch and you'll fill up functionality in an upcoming
patch so that's it's clearer what you're adding?

Cheers,
Gab

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

http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode...
gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream);
does this mean the included pph has the include information for all of the
headers in it's transitive closure? i.e. we don't need to load child information
of a child? if so, isn't this bad from a dependency point of view?

http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c
File gcc/cp/pph.c (right):

http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c#newcode164
gcc/cp/pph.c:164: if (pph_out_file != NULL)
shouldn't you use your new pph_enabled_p function here?

http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr449...
File gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc (right):

http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr449...
gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc:1: /* { dg-options "-O -Wno-psabi
-mtune=generic" } */
Is this related to the rest of the changes in this patch? I don't see it
mentioned in your patch comments

[Diego Novillo, 2011-08-05 16:42:45]

On Thu, Aug 4, 2011 at 17:47,  <gchare@google.com> wrote:
> So if I understand correctly, the reference system doesn't work yet, you
> just put out the design in this patch and you'll fill up functionality
> in an upcoming patch so that's it's clearer what you're adding?

Right, the 'if (0)' in pph_in_includes() disables support for it.

> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c
> File gcc/cp/pph-streamer-in.c (right):
>
>
http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode...
> gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream);
> does this mean the included pph has the include information for all of
> the headers in it's transitive closure? i.e. we don't need to load child
> information of a child? if so, isn't this bad from a dependency point of
> view?

Hmm?  Each header knows what other images it includes, so before
reading its own content it reads the contents from the others.  The
dependencies don't change.  The parent will not have the physical
representation of the ASTs from the children, just references to them.

> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c
> File gcc/cp/pph.c (right):
>
> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c#newcode164
> gcc/cp/pph.c:164: if (pph_out_file != NULL)
> shouldn't you use your new pph_enabled_p function here?

No.  I need to know whether we are *creating* a PPH image.  Perhaps I
should add a different predicate.

>
http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr449...
> File gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc (right):
>
>
http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr449...
> gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc:1: /* { dg-options "-O
> -Wno-psabi -mtune=generic" } */
> Is this related to the rest of the changes in this patch? I don't see it
> mentioned in your patch comments

Gah.  I was going to commit this separately but forgot.  Sorry about that.


Diego.

[Gabriel Charette, 2011-08-05 18:10:33]

On Fri, Aug 5, 2011 at 9:42 AM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, Aug 4, 2011 at 17:47,  <gchare@google.com> wrote:
>> So if I understand correctly, the reference system doesn't work yet, you
>> just put out the design in this patch and you'll fill up functionality
>> in an upcoming patch so that's it's clearer what you're adding?
>
> Right, the 'if (0)' in pph_in_includes() disables support for it.
>
>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c
>> File gcc/cp/pph-streamer-in.c (right):
>>
>>
http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode...
>> gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream);
>> does this mean the included pph has the include information for all of
>> the headers in it's transitive closure? i.e. we don't need to load child
>> information of a child? if so, isn't this bad from a dependency point of
>> view?
>
> Hmm?  Each header knows what other images it includes, so before
> reading its own content it reads the contents from the others.  The
> dependencies don't change.  The parent will not have the physical
> representation of the ASTs from the children, just references to them.
>

What I mean is if you have the following include structure A.c -> B.h
-> C.h -> D.h

Would B.h hold references to both C.h and D.h or only to C which in
turn knows about D?

From what I understood it seemed like all of the children in the
transitive closure of B will be referenced in B and that B will not
need to look at references in C when we read it (which is great from
an I/O standpoint, but maybe not from a dependency point of view? just
trying to understand the design)


>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c
>> File gcc/cp/pph.c (right):
>>
>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c#newcode164
>> gcc/cp/pph.c:164: if (pph_out_file != NULL)
>> shouldn't you use your new pph_enabled_p function here?
>
> No.  I need to know whether we are *creating* a PPH image.  Perhaps I
> should add a different predicate.
>

Ah ok I see, ya perhaps another inline function could clarify..

>>
http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr449...
>> File gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc (right):
>>
>>
http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr449...
>> gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc:1: /* { dg-options "-O
>> -Wno-psabi -mtune=generic" } */
>> Is this related to the rest of the changes in this patch? I don't see it
>> mentioned in your patch comments
>
> Gah.  I was going to commit this separately but forgot.  Sorry about that.
>
K, why does this fix the test? seems weird that we had an infinite
loop before and changing the flags gets rid of it?

Thanks,
Gab

[Diego Novillo, 2011-08-05 18:36:54]

On Fri, Aug 5, 2011 at 14:10, Gabriel Charette <gchare@google.com> wrote:
> On Fri, Aug 5, 2011 at 9:42 AM, Diego Novillo <dnovillo@google.com> wrote:
>> On Thu, Aug 4, 2011 at 17:47,  <gchare@google.com> wrote:
>>> So if I understand correctly, the reference system doesn't work yet, you
>>> just put out the design in this patch and you'll fill up functionality
>>> in an upcoming patch so that's it's clearer what you're adding?
>>
>> Right, the 'if (0)' in pph_in_includes() disables support for it.
>>
>>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c
>>> File gcc/cp/pph-streamer-in.c (right):
>>>
>>>
http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode...
>>> gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream);
>>> does this mean the included pph has the include information for all of
>>> the headers in it's transitive closure? i.e. we don't need to load child
>>> information of a child? if so, isn't this bad from a dependency point of
>>> view?
>>
>> Hmm?  Each header knows what other images it includes, so before
>> reading its own content it reads the contents from the others.  The
>> dependencies don't change.  The parent will not have the physical
>> representation of the ASTs from the children, just references to them.
>>
>
> What I mean is if you have the following include structure A.c -> B.h
> -> C.h -> D.h
>
> Would B.h hold references to both C.h and D.h or only to C which in
> turn knows about D?

Ah, no.  B.pph only knows about C.pph.  When we load C.pph, it then
proceeds to load D.pph.
So, we shouldn't be loading D.pph twice when including B.pph.  I think
this now doesn't work and we will try to load D twice (which is a
bug).


>> Gah.  I was going to commit this separately but forgot.  Sorry about that.
>>
> K, why does this fix the test? seems weird that we had an infinite
> loop before and changing the flags gets rid of it?

It wasn't an infinite loop but an ICE in the tree caches.  The
different options were producing different ASTs, so the tree cache had
different trees on write-out and read-in.


Diego.

[Gabriel Charette, 2011-08-10 00:52:30]

>>> Gah.  I was going to commit this separately but forgot.  Sorry about that.
>>>
>> K, why does this fix the test? seems weird that we had an infinite
>> loop before and changing the flags gets rid of it?
>
> It wasn't an infinite loop but an ICE in the tree caches.  The
> different options were producing different ASTs, so the tree cache had
> different trees on write-out and read-in.
>

Ah ok I see, this is actually the test I was aiming at fixing next, nice.

I remember we were discussing it was the build system's job to ensure
flags matched. In this case however it's much worse then the asm diff
we were getting before in the c1builtin-integral test when using
different flags, which was fine as the assembly was still valid and
was just adapting to the behavior wanted by the user.

An ICE in lto_streamer_cache_get is not very intuitive to point out
the problem to the user in this case imo....

I'm thinking we should at least warn when some flags like this one
differ and when that difference has an impact on the internal
representation and can produce ICEs.

Gab

[Diego Novillo, 2011-08-10 15:36:31]

On Tue, Aug 9, 2011 at 20:52, Gabriel Charette <gchare@google.com> wrote:

> An ICE in lto_streamer_cache_get is not very intuitive to point out
> the problem to the user in this case imo....

Right.

> I'm thinking we should at least warn when some flags like this one
> differ and when that difference has an impact on the internal
> representation and can produce ICEs.

Yes, we can use the header of the PPH file to add anything we need to
do compatibility tests: original flags, version numbers, etc.  When we
open the file we can do these checks to avoid reading files
incompatible with the current compilation context.


Diego.