[pph] Detect state mutation in DECLs/TYPEs [3/8] (issue5167053)

[dnovillo at google dot com (Diego Novillo)]

This patch introduces actual handling of mutated records.

When the reader finds a PPH_RECORD_START_MUTATED record, it knows that
the tree it is about to read does not need to be allocated.  It can be
found in the cache for an external PPH file.  It reads the external
location of the tree, grabs it from the external cache and then reads
on top of that tree.

This could be improved at some point. Technically we only need to read
the fields that changed.

There is some re-factoring in pph_read_namespace_tree and
pph_write_namespace_tree needed to handle the new record type.

Tested on x86_64.  Committed to branch.


Diego.

	* pph-streamer-in.c (pph_in_start_record): Handle
	PPH_RECORD_START_MUTATED.
	(pph_read_namespace_tree): Re-organize to handle
	PPH_RECORD_START_MUTATED.
	* pph-streamer-out.c (pph_out_start_tree_record): Change
	return value to enum pph_record_marker.  Update all users.
	If the signature of the tree node changed, change the marker
	to be PPH_RECORD_START_MUTATED (disabled for now).
	Handle PPH_RECORD_START_MUTATED.
	(pph_write_namespace_tree): Re-organize to handle
	PPH_RECORD_START_MUTATED.
	* gcc/cp/pph-streamer.h (enum pph_record_marker): Rename
	PPH_RECORD_MREF to PPH_RECORD_START_MUTATED.  Update all
	users.
	(pph_in_record_marker): Handle PPH_RECORD_START_MUTATED.

diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index 1fd810f..3cbe168 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -186,11 +186,16 @@ pph_in_start_record (pph_stream *stream, unsigned *include_ix_p,
       || marker == PPH_RECORD_IREF
       || marker == PPH_RECORD_PREF)
     *cache_ix_p = pph_in_uint (stream);
-  else if (marker == PPH_RECORD_XREF)
+  else if (marker == PPH_RECORD_XREF
+           || marker == PPH_RECORD_START_MUTATED)
     {
       *include_ix_p = pph_in_uint (stream);
       *cache_ix_p = pph_in_uint (stream);
     }
+  else if (marker == PPH_RECORD_END || marker == PPH_RECORD_START_NO_CACHE)
+    ; /* Nothing to do.  This record will not need cache updates.  */
+  else
+    gcc_unreachable ();
 
   return marker;
 }
@@ -2084,6 +2089,7 @@ pph_read_tree (struct lto_input_block *ib_unused ATTRIBUTE_UNUSED,
   return pph_read_namespace_tree (stream, NULL);
 }
 
+
 /* Read a tree from the STREAM.  It ENCLOSING_NAMESPACE is not null,
    the tree may be unified with an existing tree in that namespace.  */
 
@@ -2092,7 +2098,6 @@ pph_read_namespace_tree (pph_stream *stream, tree enclosing_namespace)
 {
   struct lto_input_block *ib = stream->encoder.r.ib;
   struct data_in *data_in = stream->encoder.r.data_in;
-
   tree expr;
   enum pph_record_marker marker;
   unsigned image_ix, ix;
@@ -2107,28 +2112,32 @@ pph_read_namespace_tree (pph_stream *stream, tree enclosing_namespace)
       pph_cache *cache = pph_cache_select (stream, marker, image_ix);
       return (tree) pph_cache_get (cache, ix);
     }
-
-  /* We did not find the tree in the pickle cache, allocate the tree by
-     reading the header fields (different tree nodes need to be
-     allocated in different ways).  */
-  tag = streamer_read_record_start (ib);
-  gcc_assert ((unsigned) tag < (unsigned) LTO_NUM_TAGS);
-  if (tag == LTO_builtin_decl)
-    {
-      /* If we are going to read a built-in function, all we need is
-	 the code and class.  */
-      expr = streamer_get_builtin_tree (ib, data_in);
-    }
-  else if (tag == lto_tree_code_to_tag (INTEGER_CST))
+  else if (marker == PPH_RECORD_START
+           || marker == PPH_RECORD_START_NO_CACHE)
     {
-      /* For integer constants we only need the type and its hi/low
-	 words.  */
-      expr = streamer_read_integer_cst (ib, data_in);
-    }
-  else
-    {
-      /* Otherwise, materialize a new node from IB.  This will also read
-         all the language-independent bitfields for the new tree.  */
+      /* This is a new tree that we need to allocate.  Start by
+         reading the header fields, so we know how to allocate it
+         (different tree nodes need to be allocated in different
+         ways).  */
+      tag = streamer_read_record_start (ib);
+      gcc_assert ((unsigned) tag < (unsigned) LTO_NUM_TAGS);
+      if (tag == LTO_builtin_decl)
+        {
+          /* If we are going to read a built-in function, all we need is
+             the code and class.  */
+          gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
+          return streamer_get_builtin_tree (ib, data_in);
+        }
+      else if (tag == lto_tree_code_to_tag (INTEGER_CST))
+        {
+          /* For integer constants we only need the type and its hi/low
+             words.  */
+          gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
+          return streamer_read_integer_cst (ib, data_in);
+        }
+
+      /* Materialize a new node from IB.  This will also read all the
+         language-independent bitfields for the new tree.  */
       expr = pph_read_tree_header (stream, tag);
       if (enclosing_namespace && DECL_P (expr))
         {
@@ -2142,26 +2151,44 @@ pph_read_namespace_tree (pph_stream *stream, tree enclosing_namespace)
               expr = expr;
             }
         }
+    }
 
-      /* Add the new tree to the cache and read its body.  The tree
-         is added to the cache before we read its body to handle
-         circular references and references from children nodes.  */
-      pph_cache_insert_at (&stream->cache, expr, ix);
-      pph_read_tree_body (stream, expr);
-
-      /* If needed, sign the recently materialized tree to detect
-         mutations.  Note that we only need to compute signatures
-         if we are generating a PPH image.  That is the only time
-         where we need to determine whether a tree read from PPH
-         was updated while parsing the header file that we are
-         currently generating.  */
-      if (pph_writer_enabled_p () && tree_needs_signature (expr))
-        {
-          unsigned crc;
-          size_t nbytes;
-          crc = pph_get_signature (expr, &nbytes);
-          pph_cache_sign (&stream->cache, ix, crc, nbytes);
-        }
+  gcc_assert (marker == PPH_RECORD_START
+              || marker == PPH_RECORD_START_MUTATED);
+
+  if (marker == PPH_RECORD_START_MUTATED)
+    {
+      pph_cache *cache;
+
+      /* When reading a mutated tree, we only need to re-read its
+         body, the tree itself is already in the cache for another
+         PPH image.  */
+      gcc_assert (marker == PPH_RECORD_START_MUTATED);
+      cache = pph_cache_select (stream, PPH_RECORD_XREF, image_ix);
+      expr = (tree) pph_cache_get (cache, ix);
+
+      /* Read the internal cache slot where EXPR should be stored at.  */
+      ix = pph_in_uint (stream);
+    }
+
+  /* Add the new tree to the cache and read its body.  The tree
+     is added to the cache before we read its body to handle
+     circular references and references from children nodes.  */
+  pph_cache_insert_at (&stream->cache, expr, ix);
+  pph_read_tree_body (stream, expr);
+
+  /* If needed, sign the recently materialized tree to detect
+     mutations.  Note that we only need to compute signatures
+     if we are generating a PPH image.  That is the only time
+     where we need to determine whether a tree read from PPH
+     was updated while parsing the header file that we are
+     currently generating.  */
+  if (pph_writer_enabled_p () && tree_needs_signature (expr))
+    {
+      unsigned crc;
+      size_t nbytes;
+      crc = pph_get_signature (expr, &nbytes);
+      pph_cache_sign (&stream->cache, ix, crc, nbytes);
     }
 
   return expr;
diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index 4bff1b6..f46c849 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -332,9 +332,11 @@ pph_out_start_record (pph_stream *stream, void *data)
 
 /* Start a record for tree node T in STREAM.  This is like
    pph_out_start_record, but it filters certain special trees that
-   should never be added to the cache.  */
+   should never be added to the cache.  Additionally, instead of
+   returning a boolean value indicating whether pickling should be
+   done, it returns the actual marker used to start the record.  */
 
-static inline bool
+static inline enum pph_record_marker
 pph_out_start_tree_record (pph_stream *stream, tree t)
 {
   unsigned include_ix, ix;
@@ -352,42 +354,64 @@ pph_out_start_tree_record (pph_stream *stream, tree t)
       pph_cache *cache = pph_cache_select (stream, marker, include_ix);
       pph_cache_entry *e = pph_cache_get_entry (cache, ix);
       unsigned crc = pph_get_signature (t, NULL);
-      if (crc != e->crc)
-        if (flag_pph_debug > 1)
-          {
-            fprintf (pph_logfile, "Tree signature changed: %x to %x\n",
-                     e->crc, crc);
-            print_node (pph_logfile, "", t, 4);
-          }
+      if (0 && crc != e->crc)
+        marker = PPH_RECORD_START_MUTATED;
     }
 
+  /* Write a record header according to the value of MARKER.  */
   if (marker == PPH_RECORD_END || pph_is_reference_marker (marker))
+    pph_out_reference_record (stream, marker, include_ix, ix);
+  else if (marker == PPH_RECORD_START)
     {
-      pph_out_reference_record (stream, marker, include_ix, ix);
-      return true;
+      /* We want to prevent some trees from hitting the cache.
+         For example, we do not want to cache regular constants (as
+         their representation is actually smaller than a cache
+         reference), but some constants are special and need to
+         always be shared (e.g., integer_zero_node).  Those special
+         constants are pre-loaded in STREAM->PRELOADED_CACHE.  */
+      if (!pph_cache_should_handle (t))
+        marker = PPH_RECORD_START_NO_CACHE;
+
+      pph_out_record_marker (stream, marker);
+      if (marker == PPH_RECORD_START)
+        {
+          unsigned ix;
+          pph_cache_add (&stream->cache, t, &ix);
+          pph_out_uint (stream, ix);
+        }
     }
-
-  /* We want to prevent some trees from hitting the cache.  Note that
-     this does not prevent us from ever putting these nodes in the
-     cache.
-
-     For example, we do not want to cache regular constants (as their
-     representation is actually smaller than a cache reference), but
-     some constants are special and need to always be shared (e.g.,
-     integer_zero_node).  Those special constants are pre-loaded in
-     STREAM->PRELOADED_CACHE.  */
-  if (pph_cache_should_handle (t))
+  else if (marker == PPH_RECORD_START_MUTATED)
     {
-      unsigned ix;
-      pph_cache_add (&stream->cache, t, &ix);
-      pph_out_record_marker (stream, PPH_RECORD_START);
+      unsigned int internal_ix;
+
+      /* We found T in an external PPH file, but it has mutated since
+         we originally read it.  We are going to write out T again,
+         but the reader should not re-allocate T, rather it should
+         read the contents of T on top of the existing address.
+
+         We also add T to STREAM's internal cache so further
+         references go to it rather than the external version.
+         Note that although we add an entry for T in STREAM's internal
+         cache, the reference we write to the stream is to the
+         external version of T.  This way the reader will get the
+         location of T from the external reference and overwrite it
+         with the contents that we are going to write here.  */
+      pph_cache_add (&stream->cache, t, &internal_ix);
+      pph_out_record_marker (stream, marker);
+
+      /* Write the location of T in the external cache.  */
+      gcc_assert (include_ix != -1u);
+      pph_out_uint (stream, include_ix);
+
+      gcc_assert (ix != -1u);
       pph_out_uint (stream, ix);
+
+      /* Now write the location of the new version of T in the
+         internal cache.  */
+      pph_out_uint (stream, internal_ix);
     }
-  else
-    pph_out_record_marker (stream, PPH_RECORD_START_NO_CACHE);
 
-  /* The caller will have to write a physical representation for T.  */
-  return false;
+  return marker;
 }
 
 
@@ -2075,9 +2099,13 @@ void
 pph_write_namespace_tree (pph_stream *stream, tree expr,
                           tree enclosing_namespace )
 {
+  enum pph_record_marker marker;
+
+  marker = pph_out_start_tree_record (stream, expr);
+
   /* If EXPR is NULL or it already existed in the pickle cache,
      nothing else needs to be done.  */
-  if (pph_out_start_tree_record (stream, expr))
+  if (marker == PPH_RECORD_END || pph_is_reference_marker (marker))
     return;
 
   if (streamer_handle_as_builtin_p (expr))
@@ -2087,6 +2115,7 @@ pph_write_namespace_tree (pph_stream *stream, tree expr,
          startup.  The only builtins that need to be written out are
          BUILT_IN_FRONTEND.  For all other builtins, we simply write
          the class and code.  */
+      gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
       streamer_write_builtin (stream->encoder.w.ob, expr);
     }
   else if (TREE_CODE (expr) == INTEGER_CST)
@@ -2094,25 +2123,36 @@ pph_write_namespace_tree (pph_stream *stream, tree expr,
       /* INTEGER_CST nodes are special because they need their
 	 original type to be materialized by the reader (to implement
 	 TYPE_CACHED_VALUES).  */
+      gcc_assert (marker == PPH_RECORD_START_NO_CACHE);
       streamer_write_integer_cst (stream->encoder.w.ob, expr, false);
     }
-  else
+  else if (marker == PPH_RECORD_START || marker == PPH_RECORD_START_MUTATED)
     {
       /* This is the first time we see EXPR, write it out.  */
-      pph_write_tree_header (stream, expr);
-      if (enclosing_namespace && DECL_P (expr))
+      if (marker == PPH_RECORD_START)
         {
-          /* We may need to unify two declarations.  */
-          /* FIXME crowl: pph_out_location (stream, DECL_SOURCE_LOCATION (expr)); */
-          tree name = DECL_NAME (expr);
-          if (name)
-            pph_out_string_with_length (stream, IDENTIFIER_POINTER (name),
-                                        IDENTIFIER_LENGTH (name));
-          else
-            pph_out_string (stream, NULL);
+          /* We only need to write EXPR's header if it needs to be
+             re-allocated when reading.  If we are writing the mutated
+             state of an existing tree, then we only need to write its
+             body.  */
+          pph_write_tree_header (stream, expr);
+          if (enclosing_namespace && DECL_P (expr))
+            {
+              /* We may need to unify two declarations.  */
+              /* FIXME crowl: pph_out_location (stream, DECL_SOURCE_LOCATION (expr)); */
+              tree name = DECL_NAME (expr);
+              if (name)
+                pph_out_string_with_length (stream, IDENTIFIER_POINTER (name),
+                    IDENTIFIER_LENGTH (name));
+              else
+                pph_out_string (stream, NULL);
+            }
         }
+
       pph_write_tree_body (stream, expr);
     }
+  else
+    gcc_unreachable ();
 }
 
 
diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h
index 9adc908..2f1b390 100644
--- a/gcc/cp/pph-streamer.h
+++ b/gcc/cp/pph-streamer.h
@@ -35,6 +35,18 @@ enum pph_record_marker {
      added to the pickle cache (see pph_cache_should_handle).  */
   PPH_RECORD_START_NO_CACHE,
 
+  /* Start a mutated reference.  This marker indicates that this data
+     already existed in the cache for another PPH image, but it has
+     mutated since it was inserted into the cache:
+
+     - The writer will pickle the object again as if it had not
+       been pickled before.
+
+     - The reader uses this as an indication that it should not
+       allocate a new object, it should simply unpickle the object on
+       top of the already allocated object.  */
+  PPH_RECORD_START_MUTATED,
+
   /* End of record marker.  If a record starts with PPH_RECORD_END, the
      reader should return a NULL pointer.  */
   PPH_RECORD_END,
@@ -56,19 +68,7 @@ enum pph_record_marker {
   /* Preloaded reference. This marker indicates that this data is a preloaded
      node created by the front-end at the beginning of compilation, which we
      do not need to stream out as it will already exist on the way in.  */
-  PPH_RECORD_PREF,
-
-  /* Mutated reference.  This marker indicates that this data already
-     existed in the cache for another PPH image, but it has mutated
-     since it was inserted into the cache:
-
-     - The writer will pickle the object again as if it had not
-       been pickled before.
-
-     - The reader uses this as an indication that it should not
-       allocate a new object, it should simply unpickle the object on
-       top of the already allocated object.  */
-  PPH_RECORD_MREF
+  PPH_RECORD_PREF
 };
 
 /* Line table markers. We only stream line table entries from the parent header
@@ -124,7 +124,7 @@ typedef struct pph_file_header {
    converted into their definition.
 
    When the cache notices a cache hit on a mutated data, it writes a
-   PPH_RECORD_MREF to indicate to the reader that it is about
+   PPH_RECORD_START_MUTATED to indicate to the reader that it is about
    to read an already instantiated tree.  */
 typedef struct pph_cache_entry {
   /* Pointer to cached data.  */
@@ -709,6 +709,7 @@ pph_in_record_marker (pph_stream *stream)
   enum pph_record_marker m = (enum pph_record_marker) pph_in_uchar (stream);
   gcc_assert (m == PPH_RECORD_START
               || m == PPH_RECORD_START_NO_CACHE
+              || m == PPH_RECORD_START_MUTATED
 	      || m == PPH_RECORD_END
 	      || m == PPH_RECORD_IREF
 	      || m == PPH_RECORD_XREF
-- 
1.7.3.1


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