[pph] Fix streaming of structures inside shared structures

[Diego Novillo, 2012-02-01 01:31:04]

This patch fixes a problem with streaming of symbols and types that
have mutated from one PPH to another.

--- parent.h -----------------------------------------------------------------
#include "child.h"
struct C1 {
    int field1;
    float field2;
};
-----------------------------------------------------------------------------

--- child.h -----------------------------------------------------------------
struct C1;
-----------------------------------------------------------------------------

When we read child.pph from parent.pph, we will notice that struct C1
has mutated, so we read its body again (which fills in the fields,
etc).  However, the TYPE_LANG_DECL structure for C1 is also present in
the pickle cache for child.pph, and we were using it unmodified.

In this case, one of the fields in TYPE_LANG_DECL was
CLASSTYPE_LAZY_DEFAULT_CTOR, which had not been set in child.pph, so
when we tried to use the default ctor from struct C1 during code
generation, we were never finding it.  This was causing the failure in
x1namespace-alias2.cc and two other ICEs in the dynarray tests.

What the patch does is to never insert in the pickle cache any
structures that are embedded in structures that are already inside
other structures that get put in the cache.  We now always write out
the contents of these structures (using the PPH_RECORD_START_NO_CACHE
marker).  This includes the structures: PPH_binding_table,
PPH_function, PPH_lang_decl, PPH_lang_type, PPH_language_function and
PPH_sorted_fields_type.

Since these structures are always embedded in structures that are
already shared, they are never read and allocated more than once,
unless the parent structure is pickled.

Additionally, this simplifies the logic in the reader, since it never
needs to worry about adding these structures to the cache.



cp/ChangeLog.pph
	* name-lookup.c (pph_out_binding_table): Use
	PPH_RECORD_START_NO_CACHE records.
	(pph_in_binding_table): Likewise.
	* pph-in.c (pph_in_language_function): Remove caching code,
	assume that the structure is always in a
	PPH_RECORD_START_NO_CACHE.
	(pph_in_struct_function): Likewise.
	(pph_in_lang_decl): Likewise.
	(pph_in_sorted_fields_type): Likewise.
	(pph_in_lang_type_class): Likewise.
	(pph_in_lang_type): Likewise.
	(pph_in_lang_decl_start): Remove.
	* pph-out.c (pph_get_marker_for): Always return
	PPH_RECORD_START_NO_CACHE for PPH_binding_table, PPH_function,
	PPH_lang_decl, PPH_lang_type, PPH_language_function and
	PPH_sorted_fields_type.
	(pph_out_start_record): Do not write IX for
	PPH_RECORD_START_NO_CACHE.
	(pph_out_language_function): Only support
	PPH_RECORD_START_NO_CACHE or PPH_RECORD_END.
	(pph_out_struct_function): Likewise.
	(pph_out_lang_decl): Likewise.
	(pph_out_sorted_fields_type): Likewise.
	(pph_out_lang_type_class): Likewise.
	(pph_out_lang_type): Likewise.

testsuite/ChangeLog.pph
	* g++.dg/pph/x1namespace-alias2.cc: Mark fixed.
	* g++.dg/pph/x6dynarray4.cc: Remove expected ICE.
	* g++.dg/pph/x7dynarray5.cc: Remove expected ICE.

diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index 463b14d..cbc8ee9 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -6057,7 +6057,7 @@ pph_out_binding_table (pph_stream *stream, binding_table
bt)
     {
       if (bt->chain[i])
 	{
-	  pph_out_record_marker (stream, PPH_RECORD_START, PPH_binding_entry);
+	  pph_out_record_marker (stream, PPH_RECORD_START_NO_CACHE,
PPH_binding_entry);
 	  pph_out_tree (stream, bt->chain[i]->name);
 	  pph_out_tree (stream, bt->chain[i]->type);
 	}
@@ -6083,7 +6083,7 @@ pph_in_binding_table (pph_stream *stream)
       enum pph_tag tag;
       enum pph_record_marker marker = pph_in_record_marker (stream, &tag);
       gcc_assert (tag == PPH_binding_entry);
-      if (marker == PPH_RECORD_START)
+      if (marker == PPH_RECORD_START_NO_CACHE)
 	{
 	  tree name = pph_in_tree (stream);
 	  tree type = pph_in_tree (stream);
diff --git a/gcc/cp/pph-in.c b/gcc/cp/pph-in.c
index 52d21ae..114ceda 100644
--- a/gcc/cp/pph-in.c
+++ b/gcc/cp/pph-in.c
@@ -1300,16 +1300,10 @@ pph_in_language_function (pph_stream *stream)
   marker = pph_in_start_record (stream, &image_ix, &ix, PPH_language_function);
   if (marker == PPH_RECORD_END)
     return NULL;
-  else if (pph_is_reference_marker (marker))
-    return (struct language_function *) pph_cache_find (stream, marker,
-							image_ix, ix,
-							PPH_language_function);
 
-  /* Remove if we start emitting merge keys for this structure.  */
-  gcc_assert (marker == PPH_RECORD_START);
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
-  ALLOC_AND_REGISTER (&stream->cache, ix, PPH_language_function, lf,
-                      ggc_alloc_cleared_language_function ());
+  lf = ggc_alloc_cleared_language_function ();
   lf->base.x_stmt_tree.x_cur_stmt_list = pph_in_tree_vec (stream);
   lf->base.x_stmt_tree.stmts_are_full_exprs_p = pph_in_uint (stream);
   lf->x_cdtor_label = pph_in_tree (stream);
@@ -1353,37 +1347,23 @@ pph_in_struct_function (pph_stream *stream, tree decl)
   marker = pph_in_start_record (stream, &image_ix, &ix, PPH_function);
   if (marker == PPH_RECORD_END)
     return;
-  else if (pph_is_reference_marker (marker))
-    {
-      fn = (struct function *) pph_cache_find (stream, marker, image_ix, ix,
-					       PPH_function);
-      gcc_assert (DECL_STRUCT_FUNCTION (decl) == fn);
-      return;
-    }
 
-  /* Remove if we start emitting merge keys for this structure.  */
-  gcc_assert (marker == PPH_RECORD_START);
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
   /* Possibly allocate a new DECL_STRUCT_FUNCTION for DECL.  */
   t = pph_in_tree (stream);
   gcc_assert (t == decl);
   fn = DECL_STRUCT_FUNCTION (decl);
-  if (!fn)
+  if (fn == NULL)
     {
       /* The DECL_STRUCT_FUNCTION does not already already exists,
          which implies that we are reading an entirely new function
          and not merging into an existing function.  */
-      /* We would normally use ALLOC_AND_REGISTER,
-         but allocate_struct_function does not return a pointer.  */
       allocate_struct_function (decl, false);
       fn = DECL_STRUCT_FUNCTION (decl);
     }
 
-  /* Now register it.  */
-  pph_cache_insert_at (&stream->cache, fn, ix, PPH_function);
-
   /* FIXME pph: For now, accept the new version of the fields when merging.  */
-
   input_struct_function_base (fn, stream->encoder.r.data_in,
 			      stream->encoder.r.ib);
 
@@ -1574,79 +1554,32 @@ pph_in_ld_parm (pph_stream *stream, struct
lang_decl_parm *ldp)
 }
 
 
-/* Read from STREAM the start of a lang_decl record for DECL.  If the
-   caller should do a merge-read, set *IS_MERGE_P to true.  Return
-   lang_decl structure associated with DECL.  If this function returns
-   NULL, it means that the lang_decl record has already been read and
-   nothing else needs to be done.  */
+/* Read language specific data in DECL from STREAM.  */
 
-static struct lang_decl *
-pph_in_lang_decl_start (pph_stream *stream, tree decl, bool *is_merge_p)
+static void
+pph_in_lang_decl (pph_stream *stream, tree decl)
 {
+  struct lang_decl *ld;
+  struct lang_decl_base *ldb;
+  bool is_merge;
   enum pph_record_marker marker;
   unsigned image_ix, ix;
-  struct lang_decl *ld;
 
   marker = pph_in_start_record (stream, &image_ix, &ix, PPH_lang_decl);
   if (marker == PPH_RECORD_END)
-    return NULL;
-  else if (pph_is_reference_marker (marker))
-    {
-      DECL_LANG_SPECIFIC (decl) =
-	(struct lang_decl *) pph_cache_find (stream, marker, image_ix, ix,
-					     PPH_lang_decl);
-      return NULL;
-    }
-  else if (marker == PPH_RECORD_START_MERGE_BODY)
-    {
-      /* If we are about to read the merge body for this lang_decl
-	 structure, the instance we found in the cache, must be the
-	 same one associated with DECL.  */
-      ld = (struct lang_decl *) pph_cache_get (&stream->cache, ix);
-      gcc_assert (ld == DECL_LANG_SPECIFIC (decl));
-      *is_merge_p = true;
-    }
-  else
-    {
-      gcc_assert (marker == PPH_RECORD_START);
+    return;
 
-      /* FIXME pph, we should not be getting a DECL_LANG_SPECIFIC
-	 instance here.  This is being allocated by
-	 pph_in_merge_key_namespace_decl, but this should be the only
-	 place where we allocate it.
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
-	Change the if() below to:
-	          gcc_assert (DECL_LANG_SPECIFIC (decl) == NULL);
-      */
-      if (DECL_LANG_SPECIFIC (decl) == NULL)
-	{
-	  /* Allocate a lang_decl structure for DECL.  */
-	  retrofit_lang_decl (decl);
-	}
-      ld = DECL_LANG_SPECIFIC (decl);
 
-      /* Now register it.  We would normally use ALLOC_AND_REGISTER,
-	 but retrofit_lang_decl does not return a pointer.  */
-      pph_cache_insert_at (&stream->cache, ld, ix, PPH_lang_decl);
-      *is_merge_p = false;
+  is_merge = true;
+  if (DECL_LANG_SPECIFIC (decl) == NULL)
+    {
+      is_merge = false;
+      retrofit_lang_decl (decl);
     }
 
-  return ld;
-}
-
-
-/* Read language specific data in DECL from STREAM.  */
-
-static void
-pph_in_lang_decl (pph_stream *stream, tree decl)
-{
-  struct lang_decl *ld;
-  struct lang_decl_base *ldb;
-  bool is_merge;
-
-  ld = pph_in_lang_decl_start (stream, decl, &is_merge);
-  if (ld == NULL)
-    return;
+  ld = DECL_LANG_SPECIFIC (decl);
 
   /* Read all the fields in lang_decl_base.  */
   ldb = &ld->u.base;
@@ -1724,16 +1657,11 @@ pph_in_sorted_fields_type (pph_stream *stream)
   marker = pph_in_start_record (stream, &image_ix, &ix,
PPH_sorted_fields_type);
   if (marker == PPH_RECORD_END)
     return NULL;
-  else if (pph_is_reference_marker (marker))
-    return (struct sorted_fields_type *)
-	  pph_cache_find (stream, marker, image_ix, ix, PPH_sorted_fields_type);
 
-  /* Remove if we start emitting merge keys for this structure.  */
-  gcc_assert (marker == PPH_RECORD_START);
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
   num_fields = pph_in_uint (stream);
-  ALLOC_AND_REGISTER (&stream->cache, ix, PPH_sorted_fields_type, v,
-                      sorted_fields_type_new (num_fields));
+  v = sorted_fields_type_new (num_fields);
   for (i = 0; i < num_fields; i++)
     v->elts[i] = pph_in_tree (stream);
 
@@ -1804,20 +1732,12 @@ pph_in_lang_type_class (pph_stream *stream, struct
lang_type_class *ltc)
   ltc->vbases = pph_in_tree_vec (stream);
 
   marker = pph_in_start_record (stream, &image_ix, &ix, PPH_binding_table);
-  if (marker == PPH_RECORD_START)
-    {
-      ltc->nested_udts = pph_in_binding_table (stream);
-      pph_cache_insert_at (&stream->cache, ltc->nested_udts, ix,
-                           PPH_binding_table);
-    }
-  else if (pph_is_reference_marker (marker))
-    ltc->nested_udts = (binding_table) pph_cache_find (stream, marker,
-						       image_ix, ix,
-						       PPH_binding_table);
+  if (marker == PPH_RECORD_START_NO_CACHE)
+    ltc->nested_udts = pph_in_binding_table (stream);
   else
     {
-      /* Remove if we start emitting merge keys for this structure.  */
       gcc_assert (marker == PPH_RECORD_END);
+      ltc->nested_udts = NULL;
     }
 
   ltc->as_base = pph_in_tree (stream);
@@ -1856,15 +1776,10 @@ pph_in_lang_type (pph_stream *stream)
   marker = pph_in_start_record (stream, &image_ix, &ix, PPH_lang_type);
   if (marker == PPH_RECORD_END)
     return NULL;
-  else if (pph_is_reference_marker (marker))
-    return (struct lang_type *) pph_cache_find (stream, marker, image_ix, ix,
-						PPH_lang_type);
 
-  /* Remove if we start emitting merge keys for this structure.  */
-  gcc_assert (marker == PPH_RECORD_START);
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
-  ALLOC_AND_REGISTER (&stream->cache, ix, PPH_lang_type, lt,
-                      ggc_alloc_cleared_lang_type (sizeof (struct lang_type)));
+  lt = ggc_alloc_cleared_lang_type (sizeof (struct lang_type));
 
   pph_in_lang_type_header (stream, &lt->u.h);
   if (lt->u.h.is_lang_type_class)
diff --git a/gcc/cp/pph-out.c b/gcc/cp/pph-out.c
index a1ce886..8a984d9 100644
--- a/gcc/cp/pph-out.c
+++ b/gcc/cp/pph-out.c
@@ -478,6 +478,28 @@ pph_get_marker_for (pph_stream *stream, void *data,
unsigned *include_ix_p,
   if (data == NULL)
     return PPH_RECORD_END;
 
+  /* Some structures should never be shared as they are already
+     members of shared structures.  If the parent structure is shared,
+     the inner structure will be shared too.  Additionally, this is a
+     problem when the parent structure has mutated from one PPH to the
+     other.  If we allow the inner structure to be shared, we will
+     read the mutated state of the outer structure, but we will
+     happily re-use the data from the inner one (and miss mutated
+     state in it).  */
+  switch (tag)
+    {
+    case PPH_binding_table:
+    case PPH_function:
+    case PPH_lang_decl:
+    case PPH_lang_type:
+    case PPH_language_function:
+    case PPH_sorted_fields_type:
+      return PPH_RECORD_START_NO_CACHE;
+
+    default:
+      break;
+    }
+
   /* If DATA is a pre-loaded tree node, return a pre-loaded reference
      marker.  */
   if (pph_cache_lookup (NULL, data, ix_p, tag))
@@ -608,9 +630,11 @@ pph_out_start_record (pph_stream *stream, void *data, enum
pph_tag tag)
 
   /* The caller will have to write a physical representation for DATA.  */
   gcc_assert (marker == PPH_RECORD_START
+	      || marker == PPH_RECORD_START_NO_CACHE
 	      || marker == PPH_RECORD_START_MERGE_BODY);
   pph_out_record_marker (stream, marker, tag);
-  pph_out_uint (stream, ix);
+  if (marker != PPH_RECORD_START_NO_CACHE)
+    pph_out_uint (stream, ix);
 
   return marker;
 }
@@ -1404,11 +1428,10 @@ pph_out_language_function (pph_stream *stream, struct
language_function *lf)
   enum pph_record_marker marker;
 
   marker = pph_out_start_record (stream, lf, PPH_language_function);
-  if (pph_is_reference_or_end_marker (marker))
+  if (marker == PPH_RECORD_END)
     return;
 
-  /* Remove if we start emitting merge keys for this structure.  */
-  gcc_assert (marker == PPH_RECORD_START);
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
   pph_out_tree_vec (stream, lf->base.x_stmt_tree.x_cur_stmt_list);
   pph_out_uint (stream, lf->base.x_stmt_tree.stmts_are_full_exprs_p);
@@ -1462,11 +1485,10 @@ pph_out_struct_function (pph_stream *stream, struct
function *fn)
   enum pph_record_marker marker;
 
   marker = pph_out_start_record (stream, fn, PPH_function);
-  if (pph_is_reference_or_end_marker (marker))
+  if (marker == PPH_RECORD_END)
     return;
 
-  /* Remove if we start emitting merge keys for this structure.  */
-  gcc_assert (marker == PPH_RECORD_START);
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
   pph_out_tree (stream, fn->decl);
   output_struct_function_base (stream->encoder.w.ob, fn);
@@ -1638,9 +1660,11 @@ pph_out_lang_decl (pph_stream *stream, tree decl)
 
   ld = DECL_LANG_SPECIFIC (decl);
   marker = pph_out_start_record (stream, ld, PPH_lang_decl);
-  if (pph_is_reference_or_end_marker (marker))
+  if (marker == PPH_RECORD_END)
     return;
 
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
+
   /* Write all the fields in lang_decl_base.  */
   ldb = &ld->u.base;
   pph_out_ld_base (stream, ldb);
@@ -1715,11 +1739,10 @@ pph_out_sorted_fields_type (pph_stream *stream, struct
sorted_fields_type *sft)
   enum pph_record_marker marker;
 
   marker = pph_out_start_record (stream, sft, PPH_sorted_fields_type);
-  if (pph_is_reference_or_end_marker (marker))
+  if (marker == PPH_RECORD_END)
     return;
 
-  /* Remove if we start emitting merge keys for this structure.  */
-  gcc_assert (marker == PPH_RECORD_START);
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
   pph_out_uint (stream, sft->len);
   for (i = 0; i < sft->len; i++)
@@ -1790,10 +1813,9 @@ pph_out_lang_type_class (pph_stream *stream, struct
lang_type_class *ltc)
   {
     enum pph_record_marker marker;
     marker = pph_out_start_record (stream, ltc->nested_udts,
PPH_binding_table);
-    if (!pph_is_reference_or_end_marker (marker))
+    if (marker != PPH_RECORD_END)
       {
-	/* Remove if we start emitting merge keys for this structure.  */
-	gcc_assert (marker == PPH_RECORD_START);
+	gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 	pph_out_binding_table (stream, ltc->nested_udts);
       }
   }
@@ -1830,11 +1852,10 @@ pph_out_lang_type (pph_stream *stream, tree type)
 
   lt = TYPE_LANG_SPECIFIC (type);
   marker = pph_out_start_record (stream, lt, PPH_lang_type);
-  if (pph_is_reference_or_end_marker (marker))
+  if (marker == PPH_RECORD_END)
     return;
 
-  /* Remove if we start emitting merge keys for this structure.  */
-  gcc_assert (marker == PPH_RECORD_START);
+  gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
 
   pph_out_lang_type_header (stream, &lt->u.h);
   if (lt->u.h.is_lang_type_class)
diff --git a/gcc/testsuite/g++.dg/pph/x1namespace-alias2.cc
b/gcc/testsuite/g++.dg/pph/x1namespace-alias2.cc
index 4dbe174..e4c1344 100644
--- a/gcc/testsuite/g++.dg/pph/x1namespace-alias2.cc
+++ b/gcc/testsuite/g++.dg/pph/x1namespace-alias2.cc
@@ -1,6 +1,3 @@
-// pph asm xdiff 34830
-// The assembly difference is due to missing code in function f().
-// The PPH version removes the whole return expression.
 #include "x1namespace-alias1.h"
 #include "x1namespace-alias2.h"
 
diff --git a/gcc/testsuite/g++.dg/pph/x6dynarray4.cc
b/gcc/testsuite/g++.dg/pph/x6dynarray4.cc
index b846312..82bae9d 100644
--- a/gcc/testsuite/g++.dg/pph/x6dynarray4.cc
+++ b/gcc/testsuite/g++.dg/pph/x6dynarray4.cc
@@ -1,5 +1,4 @@
 // { dg-xfail-if "BOGUS MERGING" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus ".*internal compiler error.*" "" { xfail *-*-* } 0 }
 // Too many failures to diagnose.
 
 #include "x6dynarray5.h"
diff --git a/gcc/testsuite/g++.dg/pph/x7dynarray5.cc
b/gcc/testsuite/g++.dg/pph/x7dynarray5.cc
index 8e23edf..740c10d 100644
--- a/gcc/testsuite/g++.dg/pph/x7dynarray5.cc
+++ b/gcc/testsuite/g++.dg/pph/x7dynarray5.cc
@@ -1,5 +1,4 @@
 // { dg-xfail-if "BOGUS POSSIBLY DROPPING SYMBOLS " { "*-*-*" } {
"-fpph-map=pph.map" } }
-// { dg-bogus ".*internal compiler error.*" "" { xfail *-*-* } 0 }
 
 #include "x0dynarray4.h"
 #include "x6dynarray5.h"

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