[pph] Refactor Vectors and Chains

[Lawrence Crowl, 2011-10-17 02:09:08]

Factor the vector and chain output routines to remove boolean control
parameters. The functions pph_out_tree_vec_1 and pph_out_chain_1 split
their conditional parts of their implementation into their use cases,
calling each other as needed.

pph_out_tree_vec - nothing special
pph_out_tree_vec_unchain - unchaining
pph_out_mergeable_tree_vec - merging, unchaining, reversing
pph_out_tree_vec_filtered - filtering
pph_out_chain - nothing special
pph_out_chain_filtered - filtering
pph_out_mergeable_chain_filtered - merging, unchaining, reversing, filtering

This change fixes an ordering bug, but now causes an ICE to surface,
which will be addressed later.

Tested on x64.


Index: gcc/testsuite/ChangeLog.pph

2011-10-16   Lawrence Crowl  <crowl@google.com>

	* g++.dg/pph/x4tmplfuncinln.cc: Change failure to ICE.
	* g++.dg/pph/x4tmplfuncninl.cc: Likewise.
	* g++.dg/pph/z4tmplfuncinln.cc: Likewise.
	* g++.dg/pph/z4tmplfuncninl.cc: Likewise.

Index: gcc/cp/ChangeLog.pph

2011-10-16   Lawrence Crowl  <crowl@google.com>

	* pph-streamer.h (pph_dump_chain): New.
	* pph-streamer.c (pph_trace_tree): Filter expressions from trace.
	Print tree code.
	* pph.c (pph_dump_tree_name): Add print newline.
	(pph_dump_namespace): Remove print space.
	(pph_dump_binding): Factor out dumping a chain.
	(pph_dump_chain): New.
	* pph-streamer-in.c (pph_in_mergeable_binding_level): Use
	pph_dump_chain.
	(pph_read_file_1): Change location of namespace dump.
	* pph-streamer-out.c (pph_tree_matches): Remove redundant test.
	(pph_out_tree_vec_1): Removed.
	(pph_out_tree_vec): Takes core implementation from pph_out_tree_vec_1.
	(pph_out_tree_vec_unchain): New. Takes core implementation from
	pph_out_tree_vec_1.
	(pph_out_mergeable_tree_vec): Likewise.
	(pph_out_tree_vec_filtered): Likewise.
	(chain2vec_filter): Prevent null filter.
	(chain2vec): New.  Handles case above.
	(pph_out_chain_1):  Removed.
	(pph_out_chain):  Takes core implementation from pph_out_chain_1.
	(pph_out_chain_filtered):  New.  Takes core implementation from
	pph_out_chain_1.
	(pph_out_mergeable_chain_filtered): Likewise.


Index: gcc/testsuite/g++.dg/pph/x4tmplfuncninl.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/x4tmplfuncninl.cc	(revision 180048)
+++ gcc/testsuite/g++.dg/pph/x4tmplfuncninl.cc	(working copy)
@@ -1,5 +1,6 @@
-// pph asm xdiff 37887
-// xfail BOGUS DIFF LABEL
+// { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
+// { dg-bogus "a0tmplfuncninl_g.h:12:23: internal compiler error: in
instantiate_decl" "" { xfail *-*-* } 0 }
+// { dg-excess-errors "Template list problems" }
 
 #include "x0tmplfuncninl1.h"
 #include "x0tmplfuncninl2.h"
Index: gcc/testsuite/g++.dg/pph/z4tmplfuncninl.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/z4tmplfuncninl.cc	(revision 180048)
+++ gcc/testsuite/g++.dg/pph/z4tmplfuncninl.cc	(working copy)
@@ -1,5 +1,6 @@
-// pph asm xdiff 05125
-// xfail BOGUS DUPFUN
+// { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
+// { dg-bogus "a0tmplfuncninl_g.h:12:23: internal compiler error: in
instantiate_decl" "" { xfail *-*-* } 0 }
+// { dg-excess-errors "Template list problems" }
 
 #include "x0tmplfuncninl3.h"
 #include "x0tmplfuncninl4.h"
Index: gcc/testsuite/g++.dg/pph/x4tmplfuncinln.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/x4tmplfuncinln.cc	(revision 180048)
+++ gcc/testsuite/g++.dg/pph/x4tmplfuncinln.cc	(working copy)
@@ -1,6 +1,6 @@
-// pph asm xdiff 16845
-// xfail BOGUS DUPFUN
-// double function1<double>(double) is duplicated
+// { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
+// { dg-bogus "a0tmplfuncinln_g.h:17:23: internal compiler error: in
instantiate_decl" "" { xfail *-*-* } 0 }
+// { dg-excess-errors "Template list problems" }
 
 #include "x0tmplfuncinln1.h"
 #include "x0tmplfuncinln2.h"
Index: gcc/testsuite/g++.dg/pph/z4tmplfuncinln.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/z4tmplfuncinln.cc	(revision 180048)
+++ gcc/testsuite/g++.dg/pph/z4tmplfuncinln.cc	(working copy)
@@ -1,5 +1,6 @@
-// pph asm xdiff 65129
-// xfail BOGUS DUPFUNC
+// { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
+// { dg-bogus "a0tmplfuncinln_g.h:17:23: internal compiler error: in
instantiate_decl" "" { xfail *-*-* } 0 }
+// { dg-excess-errors "Template list problems" }
 
 #include "x0tmplfuncinln3.h"
 #include "x0tmplfuncinln4.h"
Index: gcc/cp/pph.c
===================================================================
--- gcc/cp/pph.c	(revision 180048)
+++ gcc/cp/pph.c	(working copy)
@@ -101,7 +101,10 @@ pph_dump_tree_name (FILE *file, tree t, 
   else if (EXPR_P (t))
     fprintf (file, "%s\n", expr_as_string (t, flags));
   else
-    print_generic_expr (file, t, flags);
+    {
+      print_generic_expr (file, t, flags);
+      fprintf (file, "\n");
+    }
 #endif
 }
 
@@ -113,7 +116,7 @@ pph_dump_namespace (FILE *file, tree ns)
 {
   fprintf (file, "namespace ");
   pph_dump_tree_name (file, ns, 0);
-  fprintf (file, " {\n");
+  fprintf (file, "{\n");
   pph_dump_binding (file, NAMESPACE_LEVEL (ns));
   fprintf (file, "}\n");
 }
@@ -124,19 +127,28 @@ pph_dump_namespace (FILE *file, tree ns)
 void
 pph_dump_binding (FILE *file, cp_binding_level *level)
 {
-  tree t, chain;
-
-  for (t = level->names; t; t = chain)
+  tree t, next;
+  pph_dump_chain (file, level->names);
+  for (t = level->namespaces; t; t = next)
     {
-      chain = DECL_CHAIN (t);
+      next = DECL_CHAIN (t);
       if (!DECL_IS_BUILTIN (t))
-        pph_dump_tree_name (file, t, 0);
+        pph_dump_namespace (file, t);
     }
-  for (t = level->namespaces; t; t = chain)
+}
+
+
+/* Dump a CHAIN for PPH.  */
+
+void
+pph_dump_chain (FILE *file, tree chain)
+{
+  tree t, next;
+  for (t = chain; t; t = next)
     {
-      chain = DECL_CHAIN (t);
+      next = DECL_CHAIN (t);
       if (!DECL_IS_BUILTIN (t))
-        pph_dump_namespace (file, t);
+        pph_dump_tree_name (file, t, 0);
     }
 }
 
Index: gcc/cp/pph-streamer-in.c
===================================================================
--- gcc/cp/pph-streamer-in.c	(revision 180048)
+++ gcc/cp/pph-streamer-in.c	(working copy)
@@ -1086,7 +1086,8 @@ pph_in_mergeable_binding_level (cp_bindi
   if (flag_pph_debug >= 3)
     {
       fprintf (pph_logfile, "PPH: Merging into this chain:\n");
-      debug_tree_chain (existing->names);
+      pph_dump_chain (pph_logfile, existing->names);
+      fprintf (pph_logfile, "\n");
     }
   pph_in_mergeable_chain (stream, &existing->names);
   pph_in_mergeable_chain (stream, &existing->namespaces);
@@ -2377,9 +2378,6 @@ pph_read_file_1 (pph_stream *stream)
   /* Read the bindings from STREAM and merge them with the current bindings. 
*/
   pph_in_mergeable_binding_level (scope_chain->bindings, stream);
 
-  if (flag_pph_dump_tree)
-    pph_dump_namespace (pph_logfile, global_namespace);
-
   /* Read and merge the other global state collected during parsing of
      the original header.  */
   file_keyed_classes = pph_in_tree (stream);
@@ -2401,6 +2399,9 @@ pph_read_file_1 (pph_stream *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);
+
+  if (flag_pph_dump_tree)
+    pph_dump_namespace (pph_logfile, global_namespace);
 }
 
 
Index: gcc/cp/pt.c
===================================================================
--- gcc/cp/pt.c	(revision 180048)
+++ gcc/cp/pt.c	(working copy)
@@ -20301,9 +20301,7 @@ pph_dump_spec_entry_slot (void **slot, v
   struct spec_entry *entry = (struct spec_entry *) *slot;
   fprintf (stream, "spec_entry.tmpl: " );
   pph_dump_tree_name (stream, entry->tmpl, 0);
-  fprintf (stream, "spec_entry.args: " );
-  pph_dump_tree_name (stream, entry->args, 0);
-  fprintf (stream, "\nspec_entry.spec: " );
+  fprintf (stream, "spec_entry.spec: " );
   pph_dump_tree_name (stream, entry->spec, 0);
   return 1;
 }
Index: gcc/cp/pph-streamer.c
===================================================================
--- gcc/cp/pph-streamer.c	(revision 180048)
+++ gcc/cp/pph-streamer.c	(working copy)
@@ -345,12 +345,14 @@ pph_trace_tree (tree t, bool mergeable)
     emit = true;
   else if ((mergeable || is_decl) && flag_pph_tracer >= 3)
     emit = true;
-  else if (flag_pph_tracer >= 4)
+  else if (!EXPR_P (t) && flag_pph_tracer >= 4)
     emit = true;
 
   if (emit)
     {
+      enum tree_code code = TREE_CODE (t);
       fprintf (pph_logfile, "PPH: %c%c ", merging, userdef);
+      fprintf (pph_logfile, "%-19s ", pph_tree_code_text (code));
       pph_dump_tree_name (pph_logfile, t, 0);
     }
 }
Index: gcc/cp/pph-streamer.h
===================================================================
--- gcc/cp/pph-streamer.h	(revision 180048)
+++ gcc/cp/pph-streamer.h	(working copy)
@@ -220,6 +220,7 @@ struct pph_stream {
 /* In pph.c  */
 extern const char *pph_tree_code_text (enum tree_code code);
 extern void pph_dump_min_decl (FILE *file, tree decl);
+extern void pph_dump_chain (FILE *, tree chain);
 extern void pph_dump_binding (FILE *, cp_binding_level *level);
 extern void pph_dump_namespace (FILE *, tree ns);
 
Index: gcc/cp/pph-streamer-out.c
===================================================================
--- gcc/cp/pph-streamer-out.c	(revision 180048)
+++ gcc/cp/pph-streamer-out.c	(working copy)
@@ -753,15 +753,17 @@ pph_out_token_cache (pph_stream *f, cp_t
 
 /******************************************************************* vectors */
 
+/* Note that we use the same format used by streamer_write_chain.
+   This is to support pph_out_chain_filtered, which writes the
+   filtered chain as a VEC.  Since the reader always reads chains
+   using streamer_read_chain, we have to write VECs in exactly the
+   same way as tree chains.  */
 
 /* Return true if T matches FILTER for STREAM.  */
 
 static inline bool
 pph_tree_matches (pph_stream *stream, tree t, unsigned filter)
 {
-  if (filter == PPHF_NONE)
-    return true;
-
   if ((filter & PPHF_NO_BUILTINS)
       && DECL_P (t)
       && DECL_IS_BUILTIN (t))
@@ -804,73 +806,56 @@ vec2vec_filter (pph_stream *stream, VEC(
 }
 
 
-/* Write all the trees in VEC V to STREAM.  REVERSE is true if V should
-   be written in reverse.  MERGEABLE is true if the tree nodes in V
-   are mergeable trees (see pph_out_tree_1).  If FILTER is set,
-   only emit the elements in V that match it.  If UNCHAIN is true,
-   clear TREE_CHAIN on every element written out (this is to support
-   writing chains, as they are supposed to be re-chained by the
-   reader).  */
+/* Write all the trees in VEC V to STREAM.  */
 
 static void
-pph_out_tree_vec_1 (pph_stream *stream, VEC(tree,gc) *v, unsigned filter,
-		    bool mergeable, bool reverse, bool unchain)
+pph_out_tree_vec (pph_stream *stream, VEC(tree,gc) *v)
 {
   unsigned i;
   tree t;
-  VEC(tree,heap) *to_write;
-
-  if (filter != PPHF_NONE)
-    to_write = vec2vec_filter (stream, v, filter);
-  else
-    to_write = (VEC(tree,heap) *) v;
+  pph_out_hwi (stream, VEC_length (tree, v));
+  FOR_EACH_VEC_ELT (tree, v, i, t)
+    pph_out_tree (stream, t);
+}
 
-  /* Note that we use the same format used by streamer_write_chain.
-     This is to support pph_out_chain_filtered, which writes the
-     filtered chain as a VEC.  Since the reader always reads chains
-     using streamer_read_chain, we have to write VECs in exactly the
-     same way as tree chains.  */
-  pph_out_hwi (stream, VEC_length (tree, to_write));
 
-  if (!reverse)
-    FOR_EACH_VEC_ELT (tree, to_write, i, t)
-      {
-	tree prev = NULL;
-	if (unchain)
-	  {
-	    prev = TREE_CHAIN (t);
-	    TREE_CHAIN (t) = NULL;
-	  }
-	pph_out_tree_1 (stream, t, mergeable);
-	if (unchain)
-	  TREE_CHAIN (t) = prev;
-      }
-  else
-    FOR_EACH_VEC_ELT_REVERSE (tree, to_write, i, t)
-      {
-	tree prev = NULL;
-	if (unchain)
-	  {
-	    prev = TREE_CHAIN (t);
-	    TREE_CHAIN (t) = NULL;
-	  }
-	pph_out_tree_1 (stream, t, mergeable);
-	if (unchain)
-	  TREE_CHAIN (t) = prev;
-      }
+/* Write all the trees in VEC V to STREAM.
+   Clear TREE_CHAIN on every element written out (this is to support
+   writing chains, as they are supposed to be re-chained by the reader).  */
 
-  /* If we did not have to filter, TO_WRITE == V.  Do not free it!  */
-  if (filter != PPHF_NONE)
-    VEC_free (tree, heap, to_write);
+static void
+pph_out_tree_vec_unchain (pph_stream *stream, VEC(tree,gc) *v)
+{
+  unsigned i;
+  tree t;
+  pph_out_hwi (stream, VEC_length (tree, v));
+  FOR_EACH_VEC_ELT (tree, v, i, t)
+    {
+      tree prev = TREE_CHAIN (t);
+      TREE_CHAIN (t) = NULL;
+      pph_out_tree (stream, t);
+      TREE_CHAIN (t) = prev;
+    }
 }
 
 
-/* Write all the trees in VEC V to STREAM.  */
+/* Write all the mergeable trees in VEC V to STREAM.
+   The trees must go out in declaration order, i.e. reversed.
+   Unchain each before writing and rechain after writing.  */
 
 static void
-pph_out_tree_vec (pph_stream *stream, VEC(tree,gc) *v)
+pph_out_mergeable_tree_vec (pph_stream *stream, VEC(tree,gc) *v)
 {
-  pph_out_tree_vec_1 (stream, v, PPHF_NONE, false, false, false);
+  unsigned i;
+  tree t;
+  pph_out_hwi (stream, VEC_length (tree, v));
+  FOR_EACH_VEC_ELT_REVERSE (tree, v, i, t)
+    {
+      tree prev = TREE_CHAIN (t);
+      TREE_CHAIN (t) = NULL;
+      pph_out_tree_1 (stream, t, /*mergeable=*/true);
+      TREE_CHAIN (t) = prev;
+    }
 }
 
 
@@ -879,7 +864,14 @@ pph_out_tree_vec (pph_stream *stream, VE
 static void
 pph_out_tree_vec_filtered (pph_stream *stream, VEC(tree,gc) *v, unsigned
filter)
 {
-  pph_out_tree_vec_1 (stream, v, filter, false, false, false);
+  if (filter == PPHF_NONE)
+    pph_out_tree_vec (stream, v);
+  else
+    {
+      VEC(tree,heap) *w = vec2vec_filter (stream, v, filter);
+      pph_out_tree_vec (stream, (VEC(tree,gc) *)w);
+      VEC_free (tree, heap, w);
+    }
 }
 
 
@@ -929,6 +921,11 @@ chain2vec_filter (pph_stream *stream, tr
   tree t;
   VEC(tree,heap) *v = NULL;
 
+  /* Do not accept the nil filter.  The caller is responsible for
+     freeing the returned vector and they may inadvertently free
+     a vector they assumed to be allocated by this function.  */
+  gcc_assert (filter != PPHF_NONE);
+
   for (t = chain; t; t = TREE_CHAIN (t))
     if (pph_tree_matches (stream, t, filter))
       VEC_safe_push (tree, heap, v, t);
@@ -937,30 +934,18 @@ chain2vec_filter (pph_stream *stream, tr
 }
 
 
-/* Write a chain of trees to STREAM starting with FIRST (if REVERSE is
-   false) or the last element reachable from FIRST (if REVERSE is
-   true).  If FILTER is given, use it to decide what nodes should be
-   emitted.  MERGEABLE is as in pph_out_tree_1.  */
+/* Convert a CHAIN to a VEC by copying the nodes.  */
 
-static void
-pph_out_chain_1 (pph_stream *stream, tree first, unsigned filter,
-	         bool mergeable, bool reverse)
+static VEC(tree,heap) *
+chain2vec (tree chain)
 {
-  VEC(tree,heap) *vec = NULL;
+  tree t;
+  VEC(tree,heap) *v = NULL;
 
-  /* Unmodified chain writes go directly to the regular tree streamer.  */
-  if (filter == PPHF_NONE && !mergeable && !reverse)
-    {
-      streamer_write_chain (stream->encoder.w.ob, first, false);
-      return;
-    }
+  for (t = chain; t; t = TREE_CHAIN (t))
+    VEC_safe_push (tree, heap, v, t);
 
-  /* For everything else, convert the chain into a VEC and write it
-     out.  Since we have already applied FILTER, there is no need to
-     apply it again.  */
-  vec = chain2vec_filter (stream, first, filter);
-  pph_out_tree_vec_1 (stream, (VEC(tree,gc) *)vec, reverse, mergeable,
-		      PPHF_NONE, true);
+  return v;
 }
 
 
@@ -969,7 +954,7 @@ pph_out_chain_1 (pph_stream *stream, tre
 static void
 pph_out_chain (pph_stream *stream, tree first)
 {
-  pph_out_chain_1 (stream, first, PPHF_NONE, false, false);
+  streamer_write_chain (stream->encoder.w.ob, first, false);
 }
 
 
@@ -979,7 +964,14 @@ pph_out_chain (pph_stream *stream, tree 
 static void
 pph_out_chain_filtered (pph_stream *stream, tree first, unsigned filter)
 {
-  pph_out_chain_1 (stream, first, filter, false, false);
+  if (filter == PPHF_NONE)
+    pph_out_chain (stream, first);
+  else
+    {
+      VEC(tree,heap) *w = chain2vec_filter (stream, first, filter);
+      pph_out_tree_vec_unchain (stream, (VEC(tree,gc) *)w);
+      VEC_free (tree, heap, w);
+    }
 }
 
 
@@ -991,7 +983,13 @@ static void
 pph_out_mergeable_chain_filtered (pph_stream *stream, tree chain,
 				  unsigned filter)
 {
-  pph_out_chain_1 (stream, chain, filter, true, true);
+  VEC(tree,heap) *w;
+  if (filter == PPHF_NONE)
+    w = chain2vec (chain);
+  else
+    w = chain2vec_filter (stream, chain, filter);
+  pph_out_mergeable_tree_vec (stream, (VEC(tree,gc) *)w);
+  VEC_free (tree, heap, w);
 }
 
 
@@ -2247,8 +2245,6 @@ pph_write_file (pph_stream *stream)
 
   /* Emit the bindings for the global namespace.  */
   pph_out_global_binding (stream);
-  if (flag_pph_dump_tree)
-    pph_dump_namespace (pph_logfile, global_namespace);
 
   /* Emit other global state kept by the parser.  FIXME pph, these
      globals should be fields in struct cp_parser.  */
@@ -2262,6 +2258,9 @@ pph_write_file (pph_stream *stream)
 
   /* Emit the symbol table.  */
   pph_out_symtab (stream);
+
+  if (flag_pph_dump_tree)
+    pph_dump_namespace (pph_logfile, global_namespace);
 }
 
 

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

[Diego Novillo, 2011-10-17 12:33:05]

On Sun, Oct 16, 2011 at 22:09, Lawrence Crowl <crowl@google.com> wrote:
> Factor the vector and chain output routines to remove boolean control
> parameters. The functions pph_out_tree_vec_1 and pph_out_chain_1 split
> their conditional parts of their implementation into their use cases,
> calling each other as needed.
>
> pph_out_tree_vec - nothing special
> pph_out_tree_vec_unchain - unchaining
> pph_out_mergeable_tree_vec - merging, unchaining, reversing
> pph_out_tree_vec_filtered - filtering
> pph_out_chain - nothing special
> pph_out_chain_filtered - filtering
> pph_out_mergeable_chain_filtered - merging, unchaining, reversing, filtering

But, you are duplicating code that the previous patch had explicitly
commonized.  Why?  It's easier to keep the core streaming logic in one
function, to have a single point of debugging when dealing with sync
problems.


> This change fixes an ordering bug, but now causes an ICE to surface,
> which will be addressed later.

What ordering bug?


Diego.