diff mbox

[pph] Fix #include table handling (issue5578045)

Message ID 20120126164901.5D93DAE1D4@tobiano.tor.corp.google.com
State New
Headers show

Commit Message

Diego Novillo Jan. 26, 2012, 4:49 p.m. UTC
This patch fixes a bug in the handling of #include files in PPH files.
The bug is a bit convoluted, so an example is easier.  Given the files

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

1.h
#include "2.h"

Assume that all the files are properly double-guarded, and that we are
generating PPH images for all the headers.  The contents of 2.h and
3.h are not important.

When we are generating foo.pph, we read 1.pph, which adds 1.pph and
2.pph to the #include table of foo.pph.  When we finish reading 1.pph,
the include handler is presented with #include "2.h", which it
resolves to 2.pph, which it then reads and adds to the #include table
*again*.

So, when we generated foo.pph, we had 2.pph in the #include table
twice.  This means that 3.pph ends up in the 4th slot of the #include
table.  All the external references to 3.pph are written to address
into that slot.

However, when we are reading foo.pph, the include hanlder is not
active for the files it includes, so 2.pph is only added once to the
table.  Meaning that external references going to 3.pph are looked up
from the wrong slot in the #include table.

All this, because we were allowing duplicate entries in the #include
table.  So, this patch reworks the #include table to only allow a
single entry.

In generating the test case, I found a second bug.  When we have
function bodies inside a header file, we were adding call graph nodes
for the functions, but we were not resetting the DECL_EXTERNAL field
for the FUNCTION_DECL.  These functions were being dropped by IPA
later on (in function_and_variable_visibility).

2012-01-26   Diego Novillo  <dnovillo@google.com>

cp/ChangeLog.pph
	* pph-core.c (pph_include_handler): Call pph_writer_get_stream.
	(pph_add_include): Move to pph-in.c
	(pph_stream_open): Initialize STREAM->INCLUDES.M.
	(pph_stream_close_1): Free STREAM->INCLUDES.M.
	* pph-in.c (pph_in_symtab): For functions we are going to
	expand, set DECL_EXTERNAL to false.
	(pph_add_include_1): New.
	(pph_add_include): Move from pph-core.c.  Call pph_add_include_1.
	(pph_read_file): Add argument PARENT.  Update all callers.
	If PARENT is given, add STREAM to PARENT's #include table.
	* pph-out.c (pph_writer_get_stream): New.
	* pph-streamer.h (struct pph_stream): Change field INCLUDES to
	be a structure with two fields V and M.  Update all users.
	(pph_add_include): Remove.
	(pph_writer_add_include): Remove.
	(pph_read_file): Add second argument.

testsuite/ChangeLog.pph
	* x0multiple-include.h: New.
	* x1multiple-include.h: New.
	* x2multiple-include.h: New.
	* x3multiple-include.h: New.
	* x4multiple-include.cc: New.


--
This patch is available for review at http://codereview.appspot.com/5578045
diff mbox

Patch

diff --gitc/cp/pph-core.c
index 78b6
--- a/gcc/cp/pph-core.c
+++ b/gcc/cp/pph-core.c
@@ -561,7 +561,7 @@  pph_cache_lookup_in_includes (pph_stream *stream, void *data,
      in the cache, so instead of ICEing, we ignore the match to force
      the caller to pickle DATA.  */
   e = NULL;
-  FOR_EACH_VEC_ELT (pph_stream_ptr, stream->includes, include_ix, include)
+  FOR_EACH_VEC_ELT (pph_stream_ptr, stream->includes.v, include_ix, include)
     {
       e = pph_cache_lookup (&include->cache, data, &ix, PPH_null);
       if (e)
@@ -723,18 +723,13 @@  pph_include_handler (cpp_reader *reader,
       && pph_is_valid_here (name, loc)
       && !cpp_included_before (reader, name, input_location))
     {
-      pph_stream *include = pph_read_file (pph_file);
+      /* The stream we are currently generating is private to
+	 the writer.  As an exception, allow access from the
+	 include handler.  */
+      pph_stream *pph_writer_get_stream (void);
+      pph_stream *include = pph_read_file (pph_file, pph_writer_get_stream ());
       if (include)
-	{
-	  /* 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;
-	}
+	read_text_file_p = false;
       else
 	warning_at (loc, OPT_Wmissing_pph,
 		    "cannot open PPH file %s for reading: %m\n"
@@ -890,7 +885,7 @@  pph_dump_includes (FILE *file, pph_stream *stream, unsigned indent)
   if (file == NULL)
     file = stderr;
 
-  FOR_EACH_VEC_ELT (pph_stream_ptr, stream->includes, i, include)
+  FOR_EACH_VEC_ELT (pph_stream_ptr, stream->includes.v, i, include)
     {
       for (j = 0; j < indent; j++)
 	fputc (' ', file);
@@ -900,22 +895,6 @@  pph_dump_includes (FILE *file, pph_stream *stream, unsigned indent)
 }
 
 
-/* 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;
-
-  include->parent = stream;
-  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);
-}
-
-
 /* Handle work need after PPH files have been read,
    but before parsing starts.  */
 
@@ -1082,6 +1061,7 @@  pph_stream_open (const char *name, const char *mode)
   stream->write_p = (strchr (mode, 'w') != NULL);
   pph_cache_init (&stream->cache);
   stream->preloaded_cache = pph_preloaded_cache;
+  stream->includes.m = pointer_set_create ();
 
   /* Register STREAM in the table of all open streams.  Do it before
      initializing the reader or writer since they may need to look it
@@ -1137,7 +1117,8 @@  pph_stream_close_1 (pph_stream *stream, bool flush_p)
   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);
+  VEC_free (pph_stream_ptr, heap, stream->includes.v);
+  pointer_set_destroy (stream->includes.m);
 
   if (stream->write_p)
     {
diff --git a/gcc/cp/pph-in.c b/gcc/cp/pph-in.c
index c85765b..3edd3f5 100644
--- a/gcc/cp/pph-in.c
+++ b/gcc/cp/pph-in.c
@@ -316,7 +316,6 @@  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
@@ -332,11 +331,7 @@  pph_in_include (pph_stream *stream)
 
   /* We should not be trying to include STREAM again.  */
   gcc_assert (strcmp (include_name, stream->name) != 0);
-  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_read_file (include_name, stream);
 
   pph_loc_offset = old_loc_offset;
 }
@@ -2831,6 +2826,7 @@  pph_in_symtab (pph_stream *stream)
 		 to clear the finalized and reachable bits in the
 		 node, otherwise cgraph_finalize_function will toss
 		 out this node.  */
+	      DECL_EXTERNAL (node->decl) = false;
 	      node->local.finalized = false;
 	      node->reachable = false;
 	      cgraph_finalize_function (node->decl, true);
@@ -3014,7 +3010,9 @@  pph_files_read (void)
 }
 
 
-/* Helper for pph_read_file.  Read contents of PPH file in STREAM.  */
+/* Helper for pph_read_file.  Read contents of PPH file in STREAM.
+   Return true if STREAM was read or false if STREAM was already
+   in memory.  */
 
 static void
 pph_read_file_1 (pph_stream *stream)
@@ -3089,11 +3087,39 @@  pph_read_file_1 (pph_stream *stream)
     pph_dump_global_state (pph_logfile, "after pph read");
 }
 
+/* Helper for pph_add_include.  Add INCLUDE to the list of included
+   images in PARENT.  Do nothing if INCLUDE was already in the list.  */
+
+static void
+pph_add_include_1 (pph_stream *parent, pph_stream *include)
+{
+  if (pointer_set_insert (parent->includes.m, include) == 0)
+    VEC_safe_push (pph_stream_ptr, heap, parent->includes.v, include);
+}
+
 
-/* Read PPH file FILENAME.  Return the in-memory pph_stream instance.  */
+/* Add INCLUDE, and the images included by it, to the list of files
+   included by PARENT.  */
+
+static void
+pph_add_include (pph_stream *parent, pph_stream *include)
+{
+  pph_stream *include_child;
+  unsigned i;
+
+  pph_add_include_1 (parent, include);
+  FOR_EACH_VEC_ELT (pph_stream_ptr, include->includes.v, i, include_child)
+    pph_add_include_1 (parent, include_child);
+}
+
+
+/* Read PPH file FILENAME.  Return the in-memory pph_stream instance.
+   PARENT is the PPH file including FILENAME.  If PARENT is not NULL,
+   the new PPH image is added to the list of images included by
+   PARENT.  */
 
 pph_stream *
-pph_read_file (const char *filename)
+pph_read_file (const char *filename, pph_stream *parent)
 {
   pph_stream *stream = pph_stream_open (filename, "rb");
   if (stream)
@@ -3103,7 +3129,14 @@  pph_read_file (const char *filename)
 	 line_tables.  */
       line_table->highest_location--;
 
+      /* Read STREAM and add it to PARENT.  If PARENT is NULL, it
+	 means that STREAM is being read from the toplevel translation
+	 unit and we are not generating a PPH image.  */
       pph_read_file_1 (stream);
+      if (parent)
+	pph_add_include (parent, stream);
+      else
+	gcc_assert (line_table->depth == 1);
     }
 
   return stream;
diff --git a/gcc/cp/pph-out.c b/gcc/cp/pph-out.c
index b500338..f7839d2 100644
--- a/gcc/cp/pph-out.c
+++ b/gcc/cp/pph-out.c
@@ -67,6 +67,19 @@  static pph_stream *pph_out_stream = NULL;
 static GTY(()) VEC(tree,gc) *pph_decls_in_symtab;
 
 
+/* Return the stream we are currently generating.  We declare this function
+   with external linkage so it can be accessed from pph_include_handler,
+   but this really should be private to this file.  */
+
+pph_stream *pph_writer_get_stream (void);
+
+pph_stream *
+pph_writer_get_stream (void)
+{
+  return pph_out_stream;
+}
+
+
 /* Initialize buffers and tables in STREAM for writing.  */
 
 void
@@ -2820,15 +2833,6 @@  pph_writer_finish (void)
 }
 
 
-/* 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);
-}
-
-
 /* Disable PPH generation.  Used when we discover that the file that we
    are currently converting into PPH is not compatible.  This does not
    necessarily stop compilation, so make sure that the PPH file is
diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h
index 04fa7c7..06f7e2f 100644
--- a/gcc/cp/pph-streamer.h
+++ b/gcc/cp/pph-streamer.h
@@ -225,11 +225,13 @@  struct pph_stream {
      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;
+  struct {
+    /* Vector to hold all the images.  */
+    VEC(pph_stream_ptr,heap) *v;
 
-  /* Parent include file.  This points to the PPH stream for the file
-     that immediately includes this one.  */
-  pph_stream_ptr parent;
+    /* Set to prevent adding the same image more than once.  */
+    struct pointer_set_t *m;
+  } includes;
 };
 
 /* Filter values to avoid emitting certain objects to a PPH file.  */
@@ -257,7 +259,6 @@  void pph_stream_set_header_name (pph_stream *, const char *);
 pph_stream *pph_stream_open (const char *, const char *);
 void pph_stream_close (pph_stream *);
 void pph_stream_close_no_flush (pph_stream *);
-void pph_add_include (pph_stream *, pph_stream *);
 void pph_trace_marker (enum pph_record_marker marker, enum pph_tag tag);
 void pph_trace_tree (tree, enum pph_trace_end, enum pph_trace_kind);
 pph_cache_entry *pph_cache_insert_at (pph_cache *, void *, unsigned,
@@ -270,7 +271,6 @@  pph_cache_entry *pph_cache_lookup_in_includes (pph_stream *, void *,
 pph_cache_entry *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-out.c.  */
 void pph_flush_buffers (pph_stream *);
@@ -285,7 +285,7 @@  void pph_disable_output (void);
 /* In pph-in.c.  */
 void pph_init_read (pph_stream *);
 location_t pph_in_location (pph_stream *);
-pph_stream *pph_read_file (const char *);
+pph_stream *pph_read_file (const char *, pph_stream *);
 tree pph_in_tree (pph_stream *stream);
 void pph_reader_init (void);
 void pph_reader_finish (void);
@@ -320,7 +320,7 @@  pph_cache_select (pph_stream *stream, enum pph_record_marker marker,
       break;
     case PPH_RECORD_XREF:
     case PPH_RECORD_START_MUTATED:
-      return &VEC_index (pph_stream_ptr, stream->includes, include_ix)->cache;
+      return &VEC_index (pph_stream_ptr, stream->includes.v, include_ix)->cache;
       break;
     case PPH_RECORD_PREF:
       return stream->preloaded_cache;
diff --git a/gcc/testsuite/g++.dg/pph/x0multiple-include.h b/gcc/testsuite/g++.dg/pph/x0multiple-include.h
new file mode 100644
index 0000000..525778a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pph/x0multiple-include.h
@@ -0,0 +1,5 @@ 
+#ifndef __X0_MULTIPLE_INCLUDE_H
+#define __X0_MULTIPLE_INCLUDE_H
+typedef int my_int;
+extern my_int foo(int, int);
+#endif
diff --git a/gcc/testsuite/g++.dg/pph/x1multiple-include.h b/gcc/testsuite/g++.dg/pph/x1multiple-include.h
new file mode 100644
index 0000000..8eca699
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pph/x1multiple-include.h
@@ -0,0 +1,4 @@ 
+#ifndef __X1_MULTIPLE_INCLUDE_H
+#define __X1_MULTIPLE_INCLUDE_H
+#include "x0multiple-include.h"
+#endif
diff --git a/gcc/testsuite/g++.dg/pph/x2multiple-include.h b/gcc/testsuite/g++.dg/pph/x2multiple-include.h
new file mode 100644
index 0000000..f641cc3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pph/x2multiple-include.h
@@ -0,0 +1,5 @@ 
+#ifndef __X2_MULTIPLE_INCLUDE_H
+#define __X2_MULTIPLE_INCLUDE_H
+typedef float my_float;
+extern my_float bar(int);
+#endif
diff --git a/gcc/testsuite/g++.dg/pph/x3multiple-include.h b/gcc/testsuite/g++.dg/pph/x3multiple-include.h
new file mode 100644
index 0000000..dff38c6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pph/x3multiple-include.h
@@ -0,0 +1,20 @@ 
+#ifndef __X3_MULTIPLE_INCLUDE_H
+#define __X3_MULTIPLE_INCLUDE_H
+
+#include "x1multiple-include.h"
+#include "x0multiple-include.h"
+#include "x2multiple-include.h"
+
+my_float baz (my_int);
+
+my_int foo(int x, int y)
+{
+  return x - y;
+}
+
+my_float bar(int y)
+{
+  return (my_float) y;
+}
+
+#endif
diff --git a/gcc/testsuite/g++.dg/pph/x4multiple-include.cc b/gcc/testsuite/g++.dg/pph/x4multiple-include.cc
new file mode 100644
index 0000000..b2996c1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pph/x4multiple-include.cc
@@ -0,0 +1,8 @@ 
+#include "x3multiple-include.h"
+
+extern "C" { void abort (void); }
+
+my_float baz(my_int x)
+{
+  return bar (foo (x, x));
+}