[pph] Make libcpp symbol validation a warning

[Diego Novillo, 2011-10-11 20:26:45]

Currently, the consistency check done on pre-processor symbols is
triggering on symbols that are not really problematic (e.g., symbols
used for double-include guards).

The problem is that in the testsuite, we are refusing to process PPH
images that fail that test, which means we don't get to test other
issues.  To avoid this, I changed the error() call to warning().  Seemed
innocent enough, but there were more problems behind that one:

1- We do not really try to avoid reading PPH images more than once.
   This problem is different than the usual double-inclusion guard.
   For instance, suppose a file foo.pph includes 1.pph, 2.pph and
   3.pph.  When generating foo.pph, we read all 3 files just once and
   double-include guards do not need to trigger.  However, if we are
   later building a TU with:
   	#include 2.pph
   	#include foo.pph
   we first read 2.pph and when reading foo.pph, we try to read 2.pph
   again, because it is mentioned in foo.pph's line map table.

   I added a guard in pph_stream_open() so it doesn't try to open the
   same file more than once, but that meant adjusting some of the
   assertions while reading the line table.  We should not expect to
   find foo.pph's line map table exactly like the one we wrote.
   
2- We cannot keep a global list of included files to consult external
   caches.  In the example above, if foo.pph needs to resolve an
   external reference to 2.pph, it needs to index into the 2nd slot in
   its include vector.  However, the file including foo.pph needs to
   index into the 1st slot in its include vector (since 2.pph is
   included first).  This meant moving the includes field inside
   struct pph_stream.

3- When reading a type, we should not try to access the method vector
   for it, unless the type is a class.

There's some more consequences of this, but the patch was starting to
become too big, so I'm submitting it now.  This fixes a couple of
files and changes the expected error on another two.  I'll be fixing
them separately.

Tested on x86_64.  Committed to branch.


Diego.


	* pph-streamer-in.c (pph_reading_includes): Remove.  Update all users.
	(pph_in_include): Call pph_add_include with the newly
	materialized stream.
	(pph_in_line_table_and_includes): Document differences between
	non-PPH compiles and PPH compiles wrt line table behaviour.
	Modify assertions accordingly.
	(pph_read_tree_header): Tidy.
	(report_validation_error): Change error() call to warning().
	(pph_image_already_read): Remove.  Update all users.
	(pph_read_file_1): If STREAM->IN_MEMORY_P is set, return.
	Call pph_mark_strea_read.
	(pph_add_read_image): Remove.
	(pph_read_file): Change return type to pph_stream *.  Update all
	users.
	(pph_reader_finish): Remove.
	* pph-streamer-out.c (pph_writer_init): Tidy.
	(pph_add_include): Remove.
	(pph_get_marker_for): Always consult the pre-loaded cache first.
	(pph_writer_add_include): New.
	* pph-streamer.c (pph_read_images): Make static.
	(pph_init_preloaded_cache): Make static.
	(pph_streamer_init): New.
	(pph_streamer_finish): New.
	(pph_find_stream_for): New.
	(pph_mark_stream_read): New.
	(pph_stream_open): Call pph_find_stream_for.  If the stream
	already exists, return it.
	(pph_add_include): Move from pph-streamer-in.c.  Add new
	argument STREAM.
	(pph_cache_lookup_in_includes): Add new argument STREAM.
	Update all users.
	* pph-streamer.h (pph_read_images): Remove extern declaration.
	Move field INCLUDES out of union W.  Update all users.
	Add field IN_MEMORY_P.
	(pph_streamer_init): Declare.
	(pph_streamer_finish): Declare.
	(pph_mark_stream_read): Declare.
	(pph_add_include): Declare.
	(pph_writer_add_include): Declare.
	* pph.c (pph_include_handler): Call pph_writer_add_include.
	(pph_init): Call pph_streamer_init.
	(pph_finish): Call pph_streamer_finish.

testsuite/ChangeLog.pph

	* g++.dg/pph/d1symnotinc.cc: Change expected error.
	* g++.dg/pph/x7dynarray6.cc: Likewise.
	* g++.dg/pph/x7dynarray7.cc: Likewise.
	* g++.dg/pph/x5dynarray7.h: Mark fixed.
	* g++.dg/pph/x6dynarray6.h: Mark fixed.

diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index fbd78d0..ffa1433 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -47,12 +47,6 @@ DEF_VEC_ALLOC_P(char_p,heap);
   memory will remain allocated until the end of compilation.  */
 static VEC(char_p,heap) *string_tables = NULL;
 
-/* Increment when we are in the process of reading includes as we do not want
-   to add those to the parent pph stream's list of includes to be written out.
-   Decrement when done. We cannot use a simple true/false flag as read includes
-   will call pph_in_includes as well.  */
-static int pph_reading_includes = 0;
-
 /* Wrapper for memory allocation calls that should have their results
    registered in the PPH streamer cache.  DATA is the pointer returned
    by the memory allocation call in ALLOC_EXPR.  IX is the cache slot 
@@ -289,6 +283,7 @@ pph_in_include (pph_stream *stream)
 {
   int old_loc_offset;
   const char *include_name;
+  pph_stream *include_stream;
   source_location prev_start_loc = pph_in_source_location (stream);
 
   /* Simulate highest_location to be as it would be at this point in a non-pph
@@ -301,7 +296,11 @@ pph_in_include (pph_stream *stream)
   old_loc_offset = pph_loc_offset;
 
   include_name = pph_in_string (stream);
-  pph_read_file (include_name);
+  include_stream = pph_read_file (include_name);
+
+  /* Add INCLUDE_STREAM, and the images included by it, to the list
+     of included images for STREAM.  */
+  pph_add_include (stream, include_stream);
 
   pph_loc_offset = old_loc_offset;
 }
@@ -328,8 +327,6 @@ pph_in_line_table_and_includes (pph_stream *stream)
   int entries_offset = line_table->used - PPH_NUM_IGNORED_LINE_TABLE_ENTRIES;
   enum pph_linetable_marker next_lt_marker = pph_in_linetable_marker (stream);
 
-  pph_reading_includes++;
-
   for (first = true; next_lt_marker != PPH_LINETABLE_END;
        next_lt_marker = pph_in_linetable_marker (stream))
     {
@@ -373,19 +370,33 @@ pph_in_line_table_and_includes (pph_stream *stream)
 	  else
 	    lm->included_from += entries_offset;
 
-	  gcc_assert (lm->included_from < (int) line_table->used);
-
 	  lm->start_location += pph_loc_offset;
 
 	  line_table->used++;
 	}
     }
 
-  pph_reading_includes--;
+  /* We used to expect exactly the same number of entries, but files
+     included from this PPH file may sometimes not be needed.  For
+     example,
+
+	#include "2.pph"
+	#include "foo.pph"
+	  +-->	#include "1.pph"
+		#include "2.pph"
+		#include "3.pph"
+
+     When foo.pph was originally created, the line table was built
+     with inclusions of 1.pph, 2.pph and 3.pph.  But when compiling
+     the main translation unit, we include 2.pph before foo.pph, so
+     the inclusion of 2.pph from foo.pph does nothing.  Leaving the
+     line table in a different shape than the original compilation.
 
+     Instead of insisting on getting EXPECTED_IN entries, we expect at
+     most EXPECTED_IN entries.  */
   {
     unsigned int expected_in = pph_in_uint (stream);
-    gcc_assert (line_table->used - used_before == expected_in);
+    gcc_assert (line_table->used - used_before <= expected_in);
   }
 
   line_table->highest_location = pph_loc_offset + pph_in_uint (stream);
@@ -1638,7 +1649,7 @@ pph_in_tcc_type (pph_stream *stream, tree type)
      order than the original compile, the pointer values will be
      different.  This will cause name lookups to fail, unless we
      resort the vector.  */
-  if (TYPE_LANG_SPECIFIC (type) && CLASSTYPE_METHOD_VEC (type))
+  if (CLASS_TYPE_P (type) && CLASSTYPE_METHOD_VEC (type))
     finish_struct_methods (type);
 }
 
@@ -1917,26 +1928,24 @@ pph_unpack_value_fields (struct bitpack_d *bp, tree
expr)
 static tree
 pph_read_tree_header (pph_stream *stream, enum LTO_tags tag)
 {
-  /* Find data.  */
   struct lto_input_block *ib = stream->encoder.r.ib;
   struct data_in *data_in = stream->encoder.r.data_in;
-
-      struct bitpack_d bp;
+  struct bitpack_d bp;
+  tree expr;
 
   /* Allocate the tree.  */
-  tree expr = streamer_alloc_tree (ib, data_in, tag);
+  expr = streamer_alloc_tree (ib, data_in, tag);
 
   /* Read the language-independent bitfields for EXPR.  */
   bp = streamer_read_tree_bitfields (ib, expr);
 
-      /* Unpack all language-dependent bitfields.  */
+  /* Unpack all language-dependent bitfields.  */
   pph_unpack_value_fields (&bp, expr);
 
-
-
   return expr;
 }
 
+
 /* Read a tree from the STREAM.  It ENCLOSING_NAMESPACE is not null,
    the tree may be unified with an existing tree in that namespace.  */
 
@@ -2214,8 +2223,8 @@ report_validation_error (const char *filename,
   char* quote_found = wrap_macro_def (found);
   char* quote_before = wrap_macro_def (before);
   char* quote_after = wrap_macro_def (after);
-  error ("PPH file %s fails macro validation, "
-         "%s is %s and should be %s or %s\n",
+  warning (0, "PPH file %s fails macro validation, "
+           "%s is %s and should be %s or %s\n",
          filename, ident, quote_found, quote_before, quote_after);
   free (quote_found);
   free (quote_before);
@@ -2355,23 +2364,6 @@ pph_in_scope_chain (pph_stream *stream)
 }
 
 
-/* If FILENAME has already been read, return the stream associated with it.  */
-
-static pph_stream *
-pph_image_already_read (const char *filename)
-{
-  pph_stream *include;
-  unsigned i;
-
-  /* FIXME pph, implement a hash map to avoid this linear search.  */
-  FOR_EACH_VEC_ELT (pph_stream_ptr, pph_read_images, i, include)
-    if (strcmp (include->name, filename) == 0)
-      return include;
-
-  return NULL;
-}
-
-
 /* Helper for pph_read_file.  Read contents of PPH file in STREAM.  */
 
 static void
@@ -2386,6 +2378,11 @@ pph_read_file_1 (pph_stream *stream)
   VEC(tree,gc) *file_unemitted_tinfo_decls;
   source_location cpp_token_replay_loc;
 
+  /* If we have read STREAM before, we do not need to re-read the rest
+     of its body.  We only needed to read its line table.  */
+  if (stream->in_memory_p)
+    return;
+
   if (flag_pph_tracer >= 1)
     fprintf (pph_logfile, "PPH: Reading %s\n", stream->name);
 
@@ -2394,11 +2391,6 @@ pph_read_file_1 (pph_stream *stream)
      read.  */
   cpp_token_replay_loc = pph_in_line_table_and_includes (stream);
 
-  /* If we have read STREAM before, we do not need to re-read the rest
-     of its body.  We only needed to read its line table.  */
-  if (pph_image_already_read (stream->name))
-    return;
-
   /* Read all the identifiers and pre-processor symbols in the global
      namespace.  */
   pph_in_identifiers (stream, &idents_used);
@@ -2440,54 +2432,22 @@ pph_read_file_1 (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_writer_enabled_p () && !pph_reading_includes)
-    pph_add_include (stream);
-}
-
-
-/* Add STREAM to the list of read images.  */
-
-static void
-pph_add_read_image (pph_stream *stream)
-{
-  VEC_safe_push (pph_stream_ptr, heap, pph_read_images, stream);
+  /* Mark this file as read.  If other images need to access its contents,
+     we will not need to actually read it again.  */
+  pph_mark_stream_read (stream);
 }
 
 
 /* Read PPH file FILENAME.  Return the in-memory pph_stream instance.  */
 
-void
+pph_stream *
 pph_read_file (const char *filename)
 {
-  pph_stream *stream;
-
-  stream = pph_stream_open (filename, "rb");
-  if (stream)
-    pph_read_file_1 (stream);
-  else
-    error ("Cannot open PPH file for reading: %s: %m", filename);
-
-  pph_add_read_image (stream);
-}
-
-
-/******************************************************* stream finalization */
-
-
-/* Finalize the PPH reader.  */
-
-void
-pph_reader_finish (void)
-{
-  unsigned i;
-  pph_stream *image;
+  pph_stream *stream = pph_stream_open (filename, "rb");
+  if (stream == NULL)
+    fatal_error ("cannot open PPH file %s for reading: %m", filename);
 
-  /* Close any images read during parsing.  */
-  FOR_EACH_VEC_ELT (pph_stream_ptr, pph_read_images, i, image)
-    pph_stream_close (image);
+  pph_read_file_1 (stream);
 
-  VEC_free (pph_stream_ptr, heap, pph_read_images);
+  return stream;
 }
diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index 3740993..6a8db20 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -66,10 +66,9 @@ 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);
+    fatal_error ("Cannot open PPH file %s for writing: %m", pph_out_file);
 }
 
 
@@ -254,16 +253,6 @@ pph_filename_eq_ignoring_path (const char *header_path,
const char *pph_path)
 }
 
 
-/* Add INCLUDE to the list of files included by pph_out_stream.  */
-
-void
-pph_add_include (pph_stream *include)
-{
-  VEC_safe_push (pph_stream_ptr, heap, pph_out_stream->encoder.w.includes,
-		 include);
-}
-
-
 /* Return the *NEXT_INCLUDE_IX'th pph_stream in STREAM's list of includes.
    Returns NULL if we have read all includes.  Increments *NEXT_INCLUDE_IX
    when sucessful.  */
@@ -271,9 +260,8 @@ pph_add_include (pph_stream *include)
 static inline pph_stream *
 pph_get_next_include (pph_stream *stream, unsigned int *next_incl_ix)
 {
-  if (*next_incl_ix < VEC_length (pph_stream_ptr, stream->encoder.w.includes))
-    return VEC_index (pph_stream_ptr, stream->encoder.w.includes,
-	              (*next_incl_ix)++);
+  if (*next_incl_ix < VEC_length (pph_stream_ptr, stream->includes))
+    return VEC_index (pph_stream_ptr, stream->includes, (*next_incl_ix)++);
   else
     return NULL;
 }
@@ -356,10 +344,9 @@ pph_out_line_table_and_includes (pph_stream *stream)
   /* Output the number of entries written to validate on input.  */
   pph_out_uint (stream, line_table->used - PPH_NUM_IGNORED_LINE_TABLE_ENTRIES);
 
-  /* Every pph header included should have been seen and skipped in the
+  /* Every PPH header included should have been seen and skipped in the
      line_table streaming above.  */
-  gcc_assert (next_incl_ix == VEC_length (pph_stream_ptr,
-					  stream->encoder.w.includes));
+  gcc_assert (next_incl_ix == VEC_length (pph_stream_ptr, stream->includes));
 
   pph_out_source_location (stream, line_table->highest_location);
   pph_out_source_location (stream, line_table->highest_line);
@@ -379,8 +366,11 @@ pph_out_line_table_and_includes (pph_stream *stream)
    *IX_P.  If DATA is in the cache of an image included by STREAM,
    return the image's index in *INCLUDE_IX_P.
 
-   In all other cases, *IX_P and *INCLUDE_IX_P will be set to -1 (if
-   given).  */
+   Caches are consulted in order of preference: preloaded, internal
+   and external.
+
+   If DATA is not found anywhere, return PPH_RECORD_START and, if
+   given, set *IX_P and *INCLUDE_IX_P to -1.  */
 
 static enum pph_record_marker
 pph_get_marker_for (pph_stream *stream, void *data, unsigned *include_ix_p,
@@ -396,20 +386,20 @@ pph_get_marker_for (pph_stream *stream, void *data,
unsigned *include_ix_p,
   if (data == NULL)
     return PPH_RECORD_END;
 
+  /* If DATA is a pre-loaded tree node, return a pre-loaded reference
+     marker.  */
+  if (pph_cache_lookup (NULL, data, ix_p, tag))
+    return PPH_RECORD_PREF;
+
   /* If DATA is in STREAM's cache, return an internal reference marker.  */
   if (pph_cache_lookup (&stream->cache, data, ix_p, tag))
     return PPH_RECORD_IREF;
 
   /* If DATA is in the cache of an included image, return an external
      reference marker.  */
-  if (pph_cache_lookup_in_includes (data, include_ix_p, ix_p, tag))
+  if (pph_cache_lookup_in_includes (stream, data, include_ix_p, ix_p, tag))
     return PPH_RECORD_XREF;
 
-  /* If DATA is a pre-loaded tree node, return a pre-loaded reference
-     marker.  */
-  if (pph_cache_lookup (NULL, data, ix_p, tag))
-    return PPH_RECORD_PREF;
-
   /* DATA is in none of the caches.  It should be pickled out.  */
   return PPH_RECORD_START;
 }
@@ -837,7 +827,7 @@ pph_out_mergeable_tree_vec (pph_stream *stream, VEC(tree,gc)
*v)
 /* Return true if T matches FILTER for STREAM.  */
 
 static inline bool
-pph_tree_matches (tree t, unsigned filter)
+pph_tree_matches (pph_stream *stream, tree t, unsigned filter)
 {
   if ((filter & PPHF_NO_BUILTINS)
       && DECL_P (t)
@@ -849,7 +839,8 @@ pph_tree_matches (tree t, unsigned filter)
     return false;
 
   if ((filter & PPHF_NO_XREFS)
-      && pph_cache_lookup_in_includes (t, NULL, NULL, pph_tree_code_to_tag
(t)))
+      && pph_cache_lookup_in_includes (stream, t, NULL, NULL,
+	                               pph_tree_code_to_tag (t)))
     return false;
 
   return true;
@@ -875,7 +866,7 @@ pph_out_tree_vec_filtered (pph_stream *stream, VEC(tree,gc)
*v, unsigned filter)
 
   /* Collect all the nodes that match the filter.  */
   FOR_EACH_VEC_ELT (tree, v, i, t)
-    if (pph_tree_matches (t, filter))
+    if (pph_tree_matches (stream, t, filter))
       VEC_safe_push (tree, heap, to_write, t);
 
   /* Write them.  */
@@ -984,7 +975,7 @@ pph_out_chain_filtered (pph_stream *stream, tree first,
unsigned filter)
 
   /* Collect all the nodes that match the filter.  */
   for (t = first; t; t = TREE_CHAIN (t))
-    if (pph_tree_matches (t, filter))
+    if (pph_tree_matches (stream, t, filter))
       VEC_safe_push (tree, heap, to_write, t);
 
   /* Write them.  */
@@ -998,7 +989,7 @@ pph_out_chain_filtered (pph_stream *stream, tree first,
unsigned filter)
 
 static void
 pph_out_mergeable_chain_filtered (pph_stream *stream, tree first,
-					unsigned filter)
+				  unsigned filter)
 {
   tree t;
   VEC(tree, heap) *to_write = NULL;
@@ -1013,7 +1004,7 @@ pph_out_mergeable_chain_filtered (pph_stream *stream, tree
first,
 
   /* Collect all the nodes that match the filter.  */
   for (t = first; t; t = TREE_CHAIN (t))
-    if (pph_tree_matches (t, filter))
+    if (pph_tree_matches (stream, t, filter))
       VEC_safe_push (tree, heap, to_write, t);
 
   /* Write them.  */
@@ -2365,3 +2356,12 @@ pph_writer_finish (void)
   pph_stream_close (pph_out_stream);
   pph_out_stream = NULL;
 }
+
+
+/* Add INCLUDE to the list of images included by pph_out_stream.  */
+
+void
+pph_writer_add_include (pph_stream *include)
+{
+  pph_add_include (pph_out_stream, include);
+}
diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c
index 9dd3faf..5a4eec9 100644
--- a/gcc/cp/pph-streamer.c
+++ b/gcc/cp/pph-streamer.c
@@ -33,12 +33,12 @@ along with GCC; see the file COPYING3.  If not see
 #include "cppbuiltin.h"
 #include "streamer-hooks.h"
 
-/* List of PPH images read during parsing.  Images opened during #include
+/* List of PPH images opened for reading.  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).  */
-VEC(pph_stream_ptr, heap) *pph_read_images;
+   them until the end of parsing (when pph_streamer_finish is called).  */
+static VEC(pph_stream_ptr, heap) *pph_read_images = NULL;
 
 /* A cache of pre-loaded common tree nodes.  */
 static pph_cache *pph_preloaded_cache;
@@ -118,7 +118,7 @@ pph_cache_init (pph_cache *cache)
 /* Initialize the pre-loaded cache.  This contains all the common
    tree nodes built by the compiler on startup.  */
 
-void
+static void
 pph_init_preloaded_cache (void)
 {
   pph_preloaded_cache = XCNEW (pph_cache);
@@ -127,6 +127,62 @@ pph_init_preloaded_cache (void)
 }
 
 
+/* Initialize the streamer.  */
+
+void
+pph_streamer_init (void)
+{
+  pph_hooks_init ();
+  pph_init_preloaded_cache ();
+}
+
+
+/* Finalize the streamer.  */
+
+void
+pph_streamer_finish (void)
+{
+  unsigned i;
+  pph_stream *image;
+
+  /* Finalize the writer.  */
+  pph_writer_finish ();
+
+  /* Close any images read during parsing.  */
+  FOR_EACH_VEC_ELT (pph_stream_ptr, pph_read_images, i, image)
+    pph_stream_close (image);
+
+  VEC_free (pph_stream_ptr, heap, pph_read_images);
+}
+
+
+/* If FILENAME has already been read, return the stream associated with it.  */
+
+static pph_stream *
+pph_find_stream_for (const char *filename)
+{
+  pph_stream *include;
+  unsigned i;
+
+  /* FIXME pph, implement a hash map to avoid this linear search.  */
+  FOR_EACH_VEC_ELT (pph_stream_ptr, pph_read_images, i, include)
+    if (strcmp (include->name, filename) == 0)
+      return include;
+
+  return NULL;
+}
+
+
+/* Add STREAM to the list of read images.  */
+
+void
+pph_mark_stream_read (pph_stream *stream)
+{
+  stream->in_memory_p = true;
+  VEC_safe_push (pph_stream_ptr, heap, pph_read_images, stream);
+}
+
+
 /* Create a new PPH stream to be stored on the file called NAME.
    MODE is passed to fopen directly.  */
 
@@ -136,12 +192,19 @@ pph_stream_open (const char *name, const char *mode)
   pph_stream *stream;
   FILE *f;
 
-  stream = NULL;
+  /* If we have already opened a PPH stream named NAME, just return
+     its associated stream.  */
+  stream = pph_find_stream_for (name);
+  if (stream)
+    {
+      gcc_assert (stream->in_memory_p);
+      return stream;
+    }
+
   f = fopen (name, mode);
   if (!f)
     return NULL;
 
-  pph_hooks_init ();
   stream = XCNEW (pph_stream);
   stream->file = f;
   stream->name = xstrdup (name);
@@ -183,13 +246,13 @@ pph_stream_close (pph_stream *stream)
   VEC_free (pph_cache_entry, heap, stream->cache.v);
   pointer_map_destroy (stream->cache.m);
   VEC_free (pph_symtab_entry, heap, stream->symtab.v);
+  VEC_free (pph_stream_ptr, heap, stream->includes);
 
   if (stream->write_p)
     {
       destroy_output_block (stream->encoder.w.ob);
       free (stream->encoder.w.decl_state_stream);
       lto_delete_out_decl_state (stream->encoder.w.out_state);
-      VEC_free (pph_stream_ptr, heap, stream->encoder.w.includes);
     }
   else
     {
@@ -207,6 +270,21 @@ pph_stream_close (pph_stream *stream)
 }
 
 
+/* Add INCLUDE, and the images included by it, to the list of files
+   included by STREAM.  */
+
+void
+pph_add_include (pph_stream *stream, pph_stream *include)
+{
+  pph_stream *include_child;
+  unsigned i;
+
+  VEC_safe_push (pph_stream_ptr, heap, stream->includes, include);
+  FOR_EACH_VEC_ELT (pph_stream_ptr, include->includes, i, include_child)
+    VEC_safe_push (pph_stream_ptr, heap, stream->includes, include_child);
+}
+
+
 /* Data types supported by the PPH tracer.  */
 enum pph_trace_type
 {
@@ -506,8 +584,8 @@ pph_cache_lookup (pph_cache *cache, void *data, unsigned
*ix_p,
 }
 
 
-/* Return true if DATA is in the pickle cache of one of the included
-   images.  TAG is the expected data type TAG for data.
+/* Return true if DATA is in the pickle cache of one of STREAM's
+   included images.  TAG is the expected data type TAG for data.
 
    If DATA is found:
       - the index for INCLUDE_P into IMAGE->INCLUDES is returned in
@@ -522,8 +600,9 @@ pph_cache_lookup (pph_cache *cache, void *data, unsigned
*ix_p,
       - the function returns false.  */
 
 bool
-pph_cache_lookup_in_includes (void *data, unsigned *include_ix_p,
-                              unsigned *ix_p, enum pph_tag tag)
+pph_cache_lookup_in_includes (pph_stream *stream, void *data,
+			      unsigned *include_ix_p, unsigned *ix_p,
+			      enum pph_tag tag)
 {
   unsigned include_ix, ix;
   pph_stream *include;
@@ -536,7 +615,7 @@ pph_cache_lookup_in_includes (void *data, unsigned
*include_ix_p,
      in the cache, so instead of ICEing, we ignore the match so the
      caller is forced to pickle DATA.  */
   found_it = false;
-  FOR_EACH_VEC_ELT (pph_stream_ptr, pph_read_images, include_ix, include)
+  FOR_EACH_VEC_ELT (pph_stream_ptr, stream->includes, include_ix, include)
     if (pph_cache_lookup (&include->cache, data, &ix, PPH_null))
       {
         pph_cache_entry *e = pph_cache_get_entry (&include->cache, ix);
diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h
index 822810a..9143f17 100644
--- a/gcc/cp/pph-streamer.h
+++ b/gcc/cp/pph-streamer.h
@@ -151,13 +151,6 @@ typedef struct pph_stream *pph_stream_ptr;
 DEF_VEC_P(pph_stream_ptr);
 DEF_VEC_ALLOC_P(pph_stream_ptr,heap);
 
-/* 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).  */
-extern VEC(pph_stream_ptr, heap) *pph_read_images;
-
 /* Data structures used to encode and decode trees.  */
 
 /* A PPH stream contains all the data and attributes needed to
@@ -176,11 +169,6 @@ struct pph_stream {
       struct output_block *ob;
       struct lto_out_decl_state *out_state;
       struct lto_output_stream *decl_state_stream;
-
-      /* 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;
     } w;
 
     /* Decoding tables and buffers used to read trees from a file.  */
@@ -205,12 +193,22 @@ struct pph_stream {
   /* Nonzero if the stream was opened for writing.  */
   unsigned int write_p : 1;
 
+  /* Nonzero if the stream has been read and it is available for
+     resolving external references.  */
+  unsigned int in_memory_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;
+
+  /* Transitive closure list of all the images included directly and
+     indirectly by this image.  Note that this list only contains PPH
+     files, not regular text headers.  Regular text headers are embedded
+     in this stream.  */
+  VEC(pph_stream_ptr,heap) *includes;
 };
 
 /* Filter values to avoid emitting certain objects to a PPH file.  */
@@ -225,9 +223,12 @@ extern void pph_dump_min_decl (FILE *file, tree decl);
 extern void pph_dump_namespace (FILE *, tree ns);
 
 /* In pph-streamer.c.  */
-void pph_init_preloaded_cache (void);
+void pph_streamer_init (void);
+void pph_streamer_finish (void);
 pph_stream *pph_stream_open (const char *, const char *);
+void pph_mark_stream_read (pph_stream *);
 void pph_stream_close (pph_stream *);
+void pph_add_include (pph_stream *, pph_stream *);
 void pph_trace_tree (pph_stream *, tree);
 void pph_new_trace_tree (pph_stream *, tree, bool);
 void pph_trace_uint (pph_stream *, unsigned int);
@@ -239,11 +240,12 @@ void pph_trace_chain (pph_stream *, tree);
 void pph_trace_bitpack (pph_stream *, struct bitpack_d *);
 void pph_cache_insert_at (pph_cache *, void *, unsigned, enum pph_tag);
 bool pph_cache_lookup (pph_cache *, void *, unsigned *, enum pph_tag);
-bool pph_cache_lookup_in_includes (void *, unsigned *, unsigned *,
+bool pph_cache_lookup_in_includes (pph_stream *, void *, unsigned *, unsigned
*,
                                    enum pph_tag);
 bool pph_cache_add (pph_cache *, void *, unsigned *, enum pph_tag);
 void pph_cache_sign (pph_cache *, unsigned, unsigned, size_t);
 unsigned pph_get_signature (tree, size_t *);
+void pph_writer_add_include (pph_stream *);
 
 /* In pph-streamer-out.c.  */
 void pph_flush_buffers (pph_stream *);
@@ -259,8 +261,7 @@ void pph_write_location (struct output_block *, location_t);
 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 *);
-void pph_reader_finish (void);
+pph_stream *pph_read_file (const char *);
 
 
 /* Inline functions.  */
@@ -269,7 +270,7 @@ void pph_reader_finish (void);
 /* Return the pickle cache in STREAM corresponding to MARKER.
    if MARKER is PPH_RECORD_IREF, it returns the cache in STREAM itself.
    If MARKER is PPH_RECORD_XREF, it returns the cache in
-   pph_read_images[INCLUDE_IX].
+   STREAM->INCLUDES[INCLUDE_IX].
    If MARKER is a PREF, it returns the preloaded cache.  */
 static inline pph_cache *
 pph_cache_select (pph_stream *stream, enum pph_record_marker marker,
@@ -281,7 +282,7 @@ pph_cache_select (pph_stream *stream, enum pph_record_marker
marker,
       return &stream->cache;
       break;
     case PPH_RECORD_XREF:
-      return &VEC_index (pph_stream_ptr, pph_read_images, include_ix)->cache;
+      return &VEC_index (pph_stream_ptr, stream->includes, include_ix)->cache;
       break;
     case PPH_RECORD_PREF:
       return stream->preloaded_cache;
diff --git a/gcc/cp/pph.c b/gcc/cp/pph.c
index 78a998b..97b5de8 100644
--- a/gcc/cp/pph.c
+++ b/gcc/cp/pph.c
@@ -180,11 +180,21 @@ pph_include_handler (cpp_reader *reader,
       && pph_is_valid_here (name, loc)
       && !cpp_included_before (reader, name, input_location))
     {
+      pph_stream *include;
+
       /* Hack. We do this to mimic what the non-pph compiler does in
 	_cpp_stack_include as our goal is to have identical line_tables.  */
       line_table->highest_location--;
 
-      pph_read_file (pph_file);
+      include = pph_read_file (pph_file);
+
+      /* If we are generating a new PPH image, add the stream we just
+	 read to the list of includes.   This way, the parser will be
+	 able to resolve references to symbols in INCLUDE and its
+	 children.  */
+      if (pph_writer_enabled_p ())
+	pph_writer_add_include (include);
+
       read_text_file_p = false;
     }
 
@@ -225,13 +235,11 @@ pph_init (void)
                            cpp_lt_create (cpp_lt_order, flag_pph_debug/2));
   gcc_assert (table == NULL);
 
+  pph_streamer_init ();
+
   /* If we are generating a PPH file, initialize the writer.  */
   if (pph_writer_enabled_p ())
     pph_writer_init ();
-
-  pph_init_preloaded_cache ();
-
-  pph_read_images = NULL;
 }
 
 
@@ -240,11 +248,8 @@ pph_init (void)
 void
 pph_finish (void)
 {
-  /* Finalize the writer.  */
-  pph_writer_finish ();
-
-  /* Finalize the reader.  */
-  pph_reader_finish ();
+  /* Finalize the streamer.  */
+  pph_streamer_finish ();
 
   /* Close log files.  */
   if (flag_pph_tracer >= 1)
diff --git a/gcc/testsuite/g++.dg/pph/d1symnotinc.cc
b/gcc/testsuite/g++.dg/pph/d1symnotinc.cc
index b432a9c..dbdc2c0 100644
--- a/gcc/testsuite/g++.dg/pph/d1symnotinc.cc
+++ b/gcc/testsuite/g++.dg/pph/d1symnotinc.cc
@@ -1,4 +1,4 @@
-// { dg-error "Cannot open PPH file for reading" }
+// { dg-message ".*fatal error: cannot open PPH file.*" }
 #define NAME v
 #define VALUE 1
 #include "d0symnotinc.h"
diff --git a/gcc/testsuite/g++.dg/pph/x5dynarray7.h
b/gcc/testsuite/g++.dg/pph/x5dynarray7.h
index 5ee5d8c..42b1246 100644
--- a/gcc/testsuite/g++.dg/pph/x5dynarray7.h
+++ b/gcc/testsuite/g++.dg/pph/x5dynarray7.h
@@ -1,6 +1,3 @@
-// { dg-xfail-if "BOGUS" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "wchar.h:1:0: error: PPH file stdio.pph fails macro validation,
_WCHAR_H" "" { xfail *-*-* } 0 }
-
 #ifndef X5DYNARRAY7_H
 #define X5DYNARRAY7_H
 
diff --git a/gcc/testsuite/g++.dg/pph/x6dynarray6.h
b/gcc/testsuite/g++.dg/pph/x6dynarray6.h
index 3e9d8d4..de660c9 100644
--- a/gcc/testsuite/g++.dg/pph/x6dynarray6.h
+++ b/gcc/testsuite/g++.dg/pph/x6dynarray6.h
@@ -1,6 +1,3 @@
-// { dg-xfail-if "BOGUS" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "wchar.h:1:0: error: PPH file stdio.pph fails macro validation,
_WCHAR_H" "" { xfail *-*-* } 0 }
-
 #ifndef X6DYNARRAY6_H
 #define X6DYNARRAY6_H
 
diff --git a/gcc/testsuite/g++.dg/pph/x7dynarray6.cc
b/gcc/testsuite/g++.dg/pph/x7dynarray6.cc
index 0292890..b037b7c 100644
--- a/gcc/testsuite/g++.dg/pph/x7dynarray6.cc
+++ b/gcc/testsuite/g++.dg/pph/x7dynarray6.cc
@@ -1,6 +1,5 @@
-// { dg-xfail-if "BOGUS" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "wchar.h:1:0: error: PPH file stdio.pph fails macro validation,
_WCHAR_H" "" { xfail *-*-* } 0 }
-
+// { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
+// { dg-excess-errors "Embedded text merging problems" }
 #include <algorithm>
 #include <iostream>
 
diff --git a/gcc/testsuite/g++.dg/pph/x7dynarray7.cc
b/gcc/testsuite/g++.dg/pph/x7dynarray7.cc
index 08398be..b25e802 100644
--- a/gcc/testsuite/g++.dg/pph/x7dynarray7.cc
+++ b/gcc/testsuite/g++.dg/pph/x7dynarray7.cc
@@ -1,6 +1,5 @@
-// { dg-xfail-if "BOGUS" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "wchar.h:1:0: error: PPH file stdio.pph fails macro validation,
_WCHAR_H" "" { xfail *-*-* } 0 }
-
+// { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
+// { dg-excess-errors "Embedded text merging problems" }
 #include <algorithm>
 #include <iostream>
 

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

[gcharette1, 2011-10-13 21:56:28]

Just looked at the line_table related sections, but see comments below:

On Tue, Oct 11, 2011 at 4:26 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> Currently, the consistency check done on pre-processor symbols is
> triggering on symbols that are not really problematic (e.g., symbols
> used for double-include guards).
>
> The problem is that in the testsuite, we are refusing to process PPH
> images that fail that test, which means we don't get to test other
> issues.  To avoid this, I changed the error() call to warning().  Seemed
> innocent enough, but there were more problems behind that one:
>
> 1- We do not really try to avoid reading PPH images more than once.
>   This problem is different than the usual double-inclusion guard.
>   For instance, suppose a file foo.pph includes 1.pph, 2.pph and
>   3.pph.  When generating foo.pph, we read all 3 files just once and
>   double-include guards do not need to trigger.  However, if we are
>   later building a TU with:
>        #include 2.pph
>        #include foo.pph
>   we first read 2.pph and when reading foo.pph, we try to read 2.pph
>   again, because it is mentioned in foo.pph's line map table.
>
>   I added a guard in pph_stream_open() so it doesn't try to open the
>   same file more than once, but that meant adjusting some of the
>   assertions while reading the line table.  We should not expect to
>   find foo.pph's line map table exactly like the one we wrote.

That makes sense.

> @@ -328,8 +327,6 @@ pph_in_line_table_and_includes (pph_stream *stream)
>   int entries_offset = line_table->used - PPH_NUM_IGNORED_LINE_TABLE_ENTRIES;
>   enum pph_linetable_marker next_lt_marker = pph_in_linetable_marker (stream);
>
> -  pph_reading_includes++;
> -
>   for (first = true; next_lt_marker != PPH_LINETABLE_END;
>        next_lt_marker = pph_in_linetable_marker (stream))
>     {
> @@ -373,19 +370,33 @@ pph_in_line_table_and_includes (pph_stream *stream)
>          else
>            lm->included_from += entries_offset;
>
> -         gcc_assert (lm->included_from < (int) line_table->used);
> -

This should still hold, it is impossible that included_from points to
an entry that doesn't exist (i.e. beyond line_table->used), but since
we recalculate it on the previous line, adding entries_offset, this
was just a safe check to make sure everything read makes sense.

>          lm->start_location += pph_loc_offset;
>
>          line_table->used++;
>        }
>     }
>
> -  pph_reading_includes--;
> +  /* We used to expect exactly the same number of entries, but files
> +     included from this PPH file may sometimes not be needed.  For
> +     example,
> +
> +       #include "2.pph"
> +       #include "foo.pph"
> +         +-->  #include "1.pph"
> +               #include "2.pph"
> +               #include "3.pph"
> +
> +     When foo.pph was originally created, the line table was built
> +     with inclusions of 1.pph, 2.pph and 3.pph.  But when compiling
> +     the main translation unit, we include 2.pph before foo.pph, so
> +     the inclusion of 2.pph from foo.pph does nothing.  Leaving the
> +     line table in a different shape than the original compilation.
>
> +     Instead of insisting on getting EXPECTED_IN entries, we expect at
> +     most EXPECTED_IN entries.  */
>   {
>     unsigned int expected_in = pph_in_uint (stream);
> -    gcc_assert (line_table->used - used_before == expected_in);
> +    gcc_assert (line_table->used - used_before <= expected_in);

I'm not sure exactly how you skip headers already parsed now (we
didn't used to when I wrote this code and that was the only problem
remaining in the line_table (i.e. duplicate entries for guarded
headers in the non-pph compile)), but couldn't you count the number of
skipped entries and assert (line_table->used - used_before) +
numSkipped == expected_in) ?

I'd have to re-download the code, I've bee following through patches,
but I'm not so sure now exactly how the "guarded headers skipping" is
done, my memorized knowledge of the codebase has diverged I feel..!

A more important note: I think it could be worth having a new flag
that outputs the line_table when done parsing (as a mean to robustly
test it). My way to test it usually was to breakpoint on
varpool_assemble_decl (a random choice, but it was only called after
parsing was done...), both in pph and non-pph compiles and compare the
line_table in gdb.... However, to have a stable test in the long run,
it could be nice to have a flag that asks for an output of the
line_table and then we could checksum and compare the line_table
outputted by the pph and non-pph compiles.

A good test I had found to break in and analyze the line_table was
p4eabi.h as it pretty much had all the problems that I fixed regarding
the line_table (it also has re-includes if I remember correctly, but
that wasn't a problem before as we would not guard out re-includes as
I just mentioned above).

Having such a robust test would be important I feel as, as we saw with
previous bugs, discrepancies in the line_table can result in tricky
unique ID diffs.

If you are now guarding out re-includes the line_table should now be
perfectly identical in pph and non-pph as this was the only thing
remaining I think :)!

Cheers,
Gab

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

[Diego Novillo, 2011-10-14 12:16:07]

On 11-10-13 17:55 , Gabriel Charette wrote:

> I'm not sure exactly how you skip headers already parsed now (we
> didn't used to when I wrote this code and that was the only problem
> remaining in the line_table (i.e. duplicate entries for guarded
> headers in the non-pph compile)), but couldn't you count the number of
> skipped entries and assert (line_table->used - used_before) +
> numSkipped == expected_in) ?

The problem is that the compilation process of foo.h -> foo.pph may 
generate different line tables than a compile that includes foo.pph. 
For instance,

foo.h:
#include "1.pph"
#include "2.pph"
#include "3.pph"

foo.cc:
#include "2.pph"
#include "foo.pph"


When we compile foo.h, the line table incorporates the effects of 
including 2.pph, and that's what we save to foo.pph.  However, when 
compiling foo.cc, the first thing we do is include 2.pph, so when 
processing the include for foo.pph, we will completely skip over 2.pph.

That's why we cannot really have the same line table that we had when we 
generated foo.pph.


Diego.

[gcharette1, 2011-10-15 00:27:41]

Yes, I understand that.

But when the second 2.pph is skipped when reading foo.pph, the reading
of its line_table is also skipped (as foo.pph doesn't contain the
line_table information for 2.h, 2.pph does and adds it when its
included as a child, but if it's skipped, the line_table info for 2.h
should never make it in the line_table), so I don't see why this is an
issue for the line_table (other than the assert about the number of
line table entries read). What I was suggesting is that as far as the
assert is concerned it would be stronger to count the number of
skipped child headers on read and assert num_read+num_skipped ==
num_expected_childs basically (it is still only an assert so no big
deal I guess).

Essentially this patch fixes the last bug we had in the line_table
merging (i.e. that guarded out headers in the non-pph version weren't
guarded out in the pph version) and this is a good thing. I'm just
being picky about weakening asserts!


I still think it would be nice to have a way to test constructs like
the line_table at the end of parsing (maybe a new flag, as I was
suggesting in my previous email, as gcc doesn't allow for modular
testing) and compare pph and non-pph versions. Testing at this level
would potentially be much better than trying to understand tricky test
failures from the ground up. This is beyond the scope of this patch of
course, but something to keep in mind I think.

Gab

On Fri, Oct 14, 2011 at 8:16 AM, Diego Novillo <dnovillo@google.com> wrote:
>
> On 11-10-13 17:55 , Gabriel Charette wrote:
>
>> I'm not sure exactly how you skip headers already parsed now (we
>> didn't used to when I wrote this code and that was the only problem
>> remaining in the line_table (i.e. duplicate entries for guarded
>> headers in the non-pph compile)), but couldn't you count the number of
>> skipped entries and assert (line_table->used - used_before) +
>> numSkipped == expected_in) ?
>
> The problem is that the compilation process of foo.h -> foo.pph may generate
different line tables than a compile that includes foo.pph. For instance,
>
> foo.h:
> #include "1.pph"
> #include "2.pph"
> #include "3.pph"
>
> foo.cc:
> #include "2.pph"
> #include "foo.pph"
>
>
> When we compile foo.h, the line table incorporates the effects of including
2.pph, and that's what we save to foo.pph.  However, when compiling foo.cc, the
first thing we do is include 2.pph, so when processing the include for foo.pph,
we will completely skip over 2.pph.
>
> That's why we cannot really have the same line table that we had when we
generated foo.pph.
>
>
> Diego.

[Diego Novillo, 2011-10-17 13:04:14]

On Fri, Oct 14, 2011 at 20:27, Gabriel Charette <gcharette1@gmail.com> wrote:
> Yes, I understand that.
>
> But when the second 2.pph is skipped when reading foo.pph, the reading
> of its line_table is also skipped (as foo.pph doesn't contain the
> line_table information for 2.h, 2.pph does and adds it when its
> included as a child, but if it's skipped, the line_table info for 2.h
> should never make it in the line_table), so I don't see why this is an
> issue for the line_table (other than the assert about the number of
> line table entries read). What I was suggesting is that as far as the
> assert is concerned it would be stronger to count the number of
> skipped child headers on read and assert num_read+num_skipped ==
> num_expected_childs basically (it is still only an assert so no big
> deal I guess).

Ah, I see what you are saying.  I didn't really bother too much with
that assert.  Since I was not reading the line table again, I figured
both asserts were triggering because of the different values coming
from the skipped file, so I left them out.

>
> Essentially this patch fixes the last bug we had in the line_table
> merging (i.e. that guarded out headers in the non-pph version weren't
> guarded out in the pph version) and this is a good thing. I'm just
> being picky about weakening asserts!
>
>
> I still think it would be nice to have a way to test constructs like
> the line_table at the end of parsing (maybe a new flag, as I was
> suggesting in my previous email, as gcc doesn't allow for modular
> testing) and compare pph and non-pph versions. Testing at this level
> would potentially be much better than trying to understand tricky test
> failures from the ground up. This is beyond the scope of this patch of
> course, but something to keep in mind I think.

Yeah.  I'll come back to it at a later point.


Diego.

[gcharette1, 2011-10-21 03:09:03]

I just thought about something..

Earlier I said that ALL line_table issues were resolved after this
patch (as it ignores the re-included headers that were guarded, as the
non-pph compiler does naturally).

One problem remains however, I'm pretty sure that re-included
non-pph'ed header's line_table entries are still showing up multiple
times (as the direct non-pph childs of a given pph_include have their
line_table entries copied one by one from the pph file).

I think we were talking about somehow remembering guards context in
which DECLs were declared and then ignoring DECLs streamed in if they
belong to a given "header guard type" that was previously seen in a
prior include using the same non-pph header, allowing us to ignore
those DECLs that are re-declared when they should have been guarded
out the second time.

I'm not sure whether there is machinery to handle non-pph re-includes
yet... but... at the very least, I'm pretty sure those non-pph entries
still show up multiple times in the line_table.

Now, we can't just remove/ignore those entries either... doing so
would alter the expected location offset (pph_loc_offset) applied to
all tokens streamed in directly from the pph header.

What we could potentially do is:
- ignore the repeated non-pph entry
- remember the number of locations this entry was "supposed" to take
(call that pph_loc_ignored_offset)
- then for DECLs imported after it we would then need an offset of
pph_loc_offset - pph_loc_ignored_offset, to compensate for the missing
entries in the line_table

The problem here obviously is that I don't think we have a way of
knowing which DECLs come before, inside, and after a given non-pph
header included in the parent pph header which we are currently
reading.

Furthermore, a DECL coming after the non-pph header could potentially
refer to something inside the ignored non-pph header and the
source_location of the referred token would now be invalid (although
that might already be fixed by the cache hit which would redirect that
token reference to the same token in the first included copy of that
same header which wasn't actually skipped as it was first and which is
valid)


On Tue, Oct 11, 2011 at 4:26 PM, Diego Novillo <dnovillo@google.com> wrote:
> @@ -328,8 +327,6 @@ pph_in_line_table_and_includes (pph_stream *stream)
>   int entries_offset = line_table->used - PPH_NUM_IGNORED_LINE_TABLE_ENTRIES;
>   enum pph_linetable_marker next_lt_marker = pph_in_linetable_marker (stream);
>
> -  pph_reading_includes++;
> -
>   for (first = true; next_lt_marker != PPH_LINETABLE_END;
>        next_lt_marker = pph_in_linetable_marker (stream))
>     {
> @@ -373,19 +370,33 @@ pph_in_line_table_and_includes (pph_stream *stream)
>          else
>            lm->included_from += entries_offset;
>

Also, if we do ignore some non-pph entries, the included_from
calculation is going to need some trickier logic as well (it's fine
for the pph includes though as each child calculates it's own offset)

> -         gcc_assert (lm->included_from < (int) line_table->used);
> -

Also, I think this slipped in my previous comment, but I don't see how
this assert could trigger in the current code. If it did trigger
something was definitely wrong as it asserts the offseted
included_from is referring to an entry that is actually in the
line_table...

>          lm->start_location += pph_loc_offset;

Cheers,
Gab


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

[Lawrence Crowl, 2011-10-21 18:44:00]

On 10/20/11, Gabriel Charette <gcharette1@gmail.com> wrote:
> I just thought about something..
>
> Earlier I said that ALL line_table issues were resolved after this
> patch (as it ignores the re-included headers that were guarded, as the
> non-pph compiler does naturally).
>
> One problem remains however, I'm pretty sure that re-included
> non-pph'ed header's line_table entries are still showing up multiple
> times (as the direct non-pph childs of a given pph_include have their
> line_table entries copied one by one from the pph file).
>
> I think we were talking about somehow remembering guards context in
> which DECLs were declared and then ignoring DECLs streamed in if they
> belong to a given "header guard type" that was previously seen in a
> prior include using the same non-pph header, allowing us to ignore
> those DECLs that are re-declared when they should have been guarded
> out the second time.
>
> I'm not sure whether there is machinery to handle non-pph re-includes
> yet... but... at the very least, I'm pretty sure those non-pph entries
> still show up multiple times in the line_table.
>
> Now, we can't just remove/ignore those entries either... doing so
> would alter the expected location offset (pph_loc_offset) applied to
> all tokens streamed in directly from the pph header.
>
> What we could potentially do is:
> - ignore the repeated non-pph entry
> - remember the number of locations this entry was "supposed" to take
> (call that pph_loc_ignored_offset)
> - then for DECLs imported after it we would then need an offset of
> pph_loc_offset - pph_loc_ignored_offset, to compensate for the missing
> entries in the line_table
>
> The problem here obviously is that I don't think we have a way of
> knowing which DECLs come before, inside, and after a given non-pph
> header included in the parent pph header which we are currently
> reading.
>
> Furthermore, a DECL coming after the non-pph header could potentially
> refer to something inside the ignored non-pph header and the
> source_location of the referred token would now be invalid (although
> that might already be fixed by the cache hit which would redirect that
> token reference to the same token in the first included copy of that
> same header which wasn't actually skipped as it was first and which is
> valid)
>
>
> On Tue, Oct 11, 2011 at 4:26 PM, Diego Novillo <dnovillo@google.com> wrote:
>> @@ -328,8 +327,6 @@ pph_in_line_table_and_includes (pph_stream *stream)
>>   int entries_offset = line_table->used -
>> PPH_NUM_IGNORED_LINE_TABLE_ENTRIES;
>>   enum pph_linetable_marker next_lt_marker = pph_in_linetable_marker
>> (stream);
>>
>> -  pph_reading_includes++;
>> -
>>   for (first = true; next_lt_marker != PPH_LINETABLE_END;
>>        next_lt_marker = pph_in_linetable_marker (stream))
>>     {
>> @@ -373,19 +370,33 @@ pph_in_line_table_and_includes (pph_stream *stream)
>>          else
>>            lm->included_from += entries_offset;
>>
>
> Also, if we do ignore some non-pph entries, the included_from
> calculation is going to need some trickier logic as well (it's fine
> for the pph includes though as each child calculates it's own offset)
>
>> -         gcc_assert (lm->included_from < (int) line_table->used);
>> -
>
> Also, I think this slipped in my previous comment, but I don't see how
> this assert could trigger in the current code. If it did trigger
> something was definitely wrong as it asserts the offseted
> included_from is referring to an entry that is actually in the
> line_table...
>
>>          lm->start_location += pph_loc_offset;

I'm wondering if we shouldn't just whitelist the problematic cases
that we know about in the system/standard headers.  It seems that all
others we could reasonably complain to the maintainers of the code.

-- 
Lawrence Crowl

[gcharette1, 2011-10-22 18:22:20]

On Fri, Oct 21, 2011 at 2:43 PM, Lawrence Crowl <crowl@google.com> wrote:

> On 10/20/11, Gabriel Charette <gcharette1@gmail.com> wrote:
> > I just thought about something..
> >
> > Earlier I said that ALL line_table issues were resolved after this
> > patch (as it ignores the re-included headers that were guarded, as the
> > non-pph compiler does naturally).
> >
> > One problem remains however, I'm pretty sure that re-included
> > non-pph'ed header's line_table entries are still showing up multiple
> > times (as the direct non-pph childs of a given pph_include have their
> > line_table entries copied one by one from the pph file).
> >
> > I think we were talking about somehow remembering guards context in
> > which DECLs were declared and then ignoring DECLs streamed in if they
> > belong to a given "header guard type" that was previously seen in a
> > prior include using the same non-pph header, allowing us to ignore
> > those DECLs that are re-declared when they should have been guarded
> > out the second time.
> >
> > I'm not sure whether there is machinery to handle non-pph re-includes
> > yet... but... at the very least, I'm pretty sure those non-pph entries
> > still show up multiple times in the line_table.
> >
> > Now, we can't just remove/ignore those entries either... doing so
> > would alter the expected location offset (pph_loc_offset) applied to
> > all tokens streamed in directly from the pph header.
> >
> > What we could potentially do is:
> > - ignore the repeated non-pph entry
> > - remember the number of locations this entry was "supposed" to take
> > (call that pph_loc_ignored_offset)
> > - then for DECLs imported after it we would then need an offset of
> > pph_loc_offset - pph_loc_ignored_offset, to compensate for the missing
> > entries in the line_table
> >
> > The problem here obviously is that I don't think we have a way of
> > knowing which DECLs come before, inside, and after a given non-pph
> > header included in the parent pph header which we are currently
> > reading.
> >
> > Furthermore, a DECL coming after the non-pph header could potentially
> > refer to something inside the ignored non-pph header and the
> > source_location of the referred token would now be invalid (although
> > that might already be fixed by the cache hit which would redirect that
> > token reference to the same token in the first included copy of that
> > same header which wasn't actually skipped as it was first and which is
> > valid)
> >
> >
> > On Tue, Oct 11, 2011 at 4:26 PM, Diego Novillo <dnovillo@google.com>
> wrote:
> >> @@ -328,8 +327,6 @@ pph_in_line_table_and_includes (pph_stream *stream)
> >>   int entries_offset = line_table->used -
> >> PPH_NUM_IGNORED_LINE_TABLE_ENTRIES;
> >>   enum pph_linetable_marker next_lt_marker = pph_in_linetable_marker
> >> (stream);
> >>
> >> -  pph_reading_includes++;
> >> -
> >>   for (first = true; next_lt_marker != PPH_LINETABLE_END;
> >>        next_lt_marker = pph_in_linetable_marker (stream))
> >>     {
> >> @@ -373,19 +370,33 @@ pph_in_line_table_and_includes (pph_stream
> *stream)
> >>          else
> >>            lm->included_from += entries_offset;
> >>
> >
> > Also, if we do ignore some non-pph entries, the included_from
> > calculation is going to need some trickier logic as well (it's fine
> > for the pph includes though as each child calculates it's own offset)
> >
> >> -         gcc_assert (lm->included_from < (int) line_table->used);
> >> -
> >
> > Also, I think this slipped in my previous comment, but I don't see how
> > this assert could trigger in the current code. If it did trigger
> > something was definitely wrong as it asserts the offseted
> > included_from is referring to an entry that is actually in the
> > line_table...
> >
> >>          lm->start_location += pph_loc_offset;
>
> I'm wondering if we shouldn't just whitelist the problematic cases
> that we know about in the system/standard headers.  It seems that all
> others we could reasonably complain to the maintainers of the code.
>
>
Hmmm I'm not sure when/where you'd want to use that whitelist of headers?
Only allow non-pph headers that are whitelisted, all others would have to be
pph'ed (otherwise we'd output a warning/error)??

That would only fix (i.e. avoid) the problem described above if the only
whitelisted headers are NOT ifdef guarded, and thus re-including them does
create a second line_table entry as expected.

I like the idea of forcing people to write pph'able ifdef guarded headers as
much as possible though.

Gab