[pph] Fix binding_level's names_size streaming (issue4634071)

[pph] Fix binding_level's names_size streaming (issue4634071)

[Gabriel Charette]

Here is the patch removing names_size.

We found out it was write-only and that we could remove it.

I tested it with a full bootstrap build as well as a full regression test (make check-g++).

(actually one of the pph test fails, but that's because of a line number issue in where
we expect the ICE... I'll have to re-upload the patch as the script already submitted the files...)

2011-06-22  Gabriel Charette  <gchare@google.com>

	* gcc/cp/name-lookup.h (cp_binding_level): Removed unused
	member names_size. Update all users.

diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index 54977ce..297c57e 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -544,7 +544,6 @@ add_decl_to_level (tree decl, cxx_scope *b)
 	 necessary.  */
       TREE_CHAIN (decl) = b->names;
       b->names = decl;
-      b->names_size++;
 
       /* If appropriate, add decl to separate list of statics.  We
 	 include extern variables because they might turn out to be
diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
index 009b5d9..5f266eb 100644
--- a/gcc/cp/name-lookup.h
+++ b/gcc/cp/name-lookup.h
@@ -191,9 +191,6 @@ struct GTY(()) cp_binding_level {
        are wrapped in TREE_LISTs; the TREE_VALUE is the OVERLOAD.  */
     tree names;
 
-    /* Count of elements in names chain.  */
-    size_t names_size;
-
     /* A chain of NAMESPACE_DECL nodes.  */
     tree namespaces;
 
diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index 0e8c6bf..a535cab 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -442,7 +442,6 @@ pph_in_binding_level (pph_stream *stream)
   ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level ());
 
   bl->names = pph_in_chain (stream);
-  bl->names_size = pph_in_uint (stream);
   bl->namespaces = pph_in_chain (stream);
 
   bl->static_decls = pph_in_tree_vec (stream);
diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index f219cef..7a6c516 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -448,7 +448,6 @@ pph_out_binding_level (pph_stream *stream, struct cp_binding_level *bl,
     return;
 
   pph_out_chain_filtered (stream, bl->names, ref_p, NO_BUILTINS);
-  pph_out_uint (stream, bl->names_size);
   pph_out_chain_filtered (stream, bl->namespaces, ref_p, NO_BUILTINS);
 
   pph_out_tree_vec (stream, bl->static_decls, ref_p);

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

Re: [pph] Fix binding_level's names_size streaming (issue4634071)

[dnovillo]

On 2011/06/24 17:35:51, Gabriel Charette wrote:
> This was commited to trunk. Diego can you commit this patch to pph as
well?

Done.  r175387.


Diego.

http://codereview.appspot.com/4634071/

Re: [pph] Fix binding_level's names_size streaming (issue4634071)

[Gabriel Charette]

This was commited to trunk. Diego can you commit this patch to pph as well?

Thanks,
Gab

On Thu, Jun 23, 2011 at 4:07 AM, Diego Novillo <dnovillo@google.com> wrote:
> On Wed, Jun 22, 2011 at 20:17, Gabriel Dos Reis
> <gdr@integrable-solutions.net> wrote:
>> On Wed, Jun 22, 2011 at 7:05 PM, Gabriel Charette <gchare@google.com> wrote:
>>> And it looks like this wasn't sent to anyone directly...
>>> Adding back dnovillo and crowl (Diego I don't think Jason was ever
>>> added to the original message...?)
>>
>> should not this go to mainline too?
>
> Yes, I CC'd Jason in the original thread that started this discussion.
>  Gab, could you send a patch for trunk?  Please CC Jason when you do.
>
>
> Diego.
>

Re: [pph] Fix binding_level's names_size streaming (issue4634071)

[Diego Novillo]

On Wed, Jun 22, 2011 at 20:17, Gabriel Dos Reis
<gdr@integrable-solutions.net> wrote:
> On Wed, Jun 22, 2011 at 7:05 PM, Gabriel Charette <gchare@google.com> wrote:
>> And it looks like this wasn't sent to anyone directly...
>> Adding back dnovillo and crowl (Diego I don't think Jason was ever
>> added to the original message...?)
>
> should not this go to mainline too?

Yes, I CC'd Jason in the original thread that started this discussion.
 Gab, could you send a patch for trunk?  Please CC Jason when you do.


Diego.

Re: [pph] Fix binding_level's names_size streaming (issue4634071)

[Gabriel Dos Reis]

On Wed, Jun 22, 2011 at 7:05 PM, Gabriel Charette <gchare@google.com> wrote:
> And it looks like this wasn't sent to anyone directly...
> Adding back dnovillo and crowl (Diego I don't think Jason was ever
> added to the original message...?)

should not this go to mainline too?


-- Gaby

Re: [pph] Fix binding_level's names_size streaming (issue4634071)

[Gabriel Charette]

And it looks like this wasn't sent to anyone directly...
Adding back dnovillo and crowl (Diego I don't think Jason was ever
added to the original message...?)

Gab

On Wed, Jun 22, 2011 at 5:02 PM, Gabriel Charette <gchare@google.com> wrote:
>
> See previous message. This patch only adds the fix to the dg-bogus comment for the failing pph test.
>
> 2011-06-22  Gabriel Charette  <gchare@google.com>
>
>        * gcc/cp/name-lookup.h (cp_binding_level): Removed unused
>        member names_size. Update all users.
>
> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
> index 54977ce..297c57e 100644
> --- a/gcc/cp/name-lookup.c
> +++ b/gcc/cp/name-lookup.c
> @@ -544,7 +544,6 @@ add_decl_to_level (tree decl, cxx_scope *b)
>         necessary.  */
>       TREE_CHAIN (decl) = b->names;
>       b->names = decl;
> -      b->names_size++;
>
>       /* If appropriate, add decl to separate list of statics.  We
>         include extern variables because they might turn out to be
> diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
> index 009b5d9..5f266eb 100644
> --- a/gcc/cp/name-lookup.h
> +++ b/gcc/cp/name-lookup.h
> @@ -191,9 +191,6 @@ struct GTY(()) cp_binding_level {
>        are wrapped in TREE_LISTs; the TREE_VALUE is the OVERLOAD.  */
>     tree names;
>
> -    /* Count of elements in names chain.  */
> -    size_t names_size;
> -
>     /* A chain of NAMESPACE_DECL nodes.  */
>     tree namespaces;
>
> diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
> index 0e8c6bf..a535cab 100644
> --- a/gcc/cp/pph-streamer-in.c
> +++ b/gcc/cp/pph-streamer-in.c
> @@ -442,7 +442,6 @@ pph_in_binding_level (pph_stream *stream)
>   ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level ());
>
>   bl->names = pph_in_chain (stream);
> -  bl->names_size = pph_in_uint (stream);
>   bl->namespaces = pph_in_chain (stream);
>
>   bl->static_decls = pph_in_tree_vec (stream);
> diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
> index f219cef..7a6c516 100644
> --- a/gcc/cp/pph-streamer-out.c
> +++ b/gcc/cp/pph-streamer-out.c
> @@ -448,7 +448,6 @@ pph_out_binding_level (pph_stream *stream, struct cp_binding_level *bl,
>     return;
>
>   pph_out_chain_filtered (stream, bl->names, ref_p, NO_BUILTINS);
> -  pph_out_uint (stream, bl->names_size);
>   pph_out_chain_filtered (stream, bl->namespaces, ref_p, NO_BUILTINS);
>
>   pph_out_tree_vec (stream, bl->static_decls, ref_p);
> diff --git a/gcc/testsuite/g++.dg/pph/x1template.cc b/gcc/testsuite/g++.dg/pph/x1template.cc
> index 5b1e980..cecefd7 100644
> --- a/gcc/testsuite/g++.dg/pph/x1template.cc
> +++ b/gcc/testsuite/g++.dg/pph/x1template.cc
> @@ -1,5 +1,5 @@
>  // { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
> -// { dg-bogus "x1template.h:18:13: internal compiler error: in resume_scope, at cp/name-lookup.c:1569" "" { xfail *-*-* } 0 }
> +// { dg-bogus "x1template.h:18:13: internal compiler error: in resume_scope, at cp/name-lookup.c:1568" "" { xfail *-*-* } 0 }
>  // { dg-prune-output "In file included from " }
>
>  #include "x1template.h"
>
> --
> This patch is available for review at http://codereview.appspot.com/4634071

[pph] Fix binding_level's names_size streaming (issue4634071)

[Gabriel Charette]

See previous message. This patch only adds the fix to the dg-bogus comment for the failing pph test.

2011-06-22  Gabriel Charette  <gchare@google.com>

	* gcc/cp/name-lookup.h (cp_binding_level): Removed unused
	member names_size. Update all users.

diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index 54977ce..297c57e 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -544,7 +544,6 @@ add_decl_to_level (tree decl, cxx_scope *b)
 	 necessary.  */
       TREE_CHAIN (decl) = b->names;
       b->names = decl;
-      b->names_size++;
 
       /* If appropriate, add decl to separate list of statics.  We
 	 include extern variables because they might turn out to be
diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
index 009b5d9..5f266eb 100644
--- a/gcc/cp/name-lookup.h
+++ b/gcc/cp/name-lookup.h
@@ -191,9 +191,6 @@ struct GTY(()) cp_binding_level {
        are wrapped in TREE_LISTs; the TREE_VALUE is the OVERLOAD.  */
     tree names;
 
-    /* Count of elements in names chain.  */
-    size_t names_size;
-
     /* A chain of NAMESPACE_DECL nodes.  */
     tree namespaces;
 
diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index 0e8c6bf..a535cab 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -442,7 +442,6 @@ pph_in_binding_level (pph_stream *stream)
   ALLOC_AND_REGISTER (stream, ix, bl, ggc_alloc_cleared_cp_binding_level ());
 
   bl->names = pph_in_chain (stream);
-  bl->names_size = pph_in_uint (stream);
   bl->namespaces = pph_in_chain (stream);
 
   bl->static_decls = pph_in_tree_vec (stream);
diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index f219cef..7a6c516 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -448,7 +448,6 @@ pph_out_binding_level (pph_stream *stream, struct cp_binding_level *bl,
     return;
 
   pph_out_chain_filtered (stream, bl->names, ref_p, NO_BUILTINS);
-  pph_out_uint (stream, bl->names_size);
   pph_out_chain_filtered (stream, bl->namespaces, ref_p, NO_BUILTINS);
 
   pph_out_tree_vec (stream, bl->static_decls, ref_p);
diff --git a/gcc/testsuite/g++.dg/pph/x1template.cc b/gcc/testsuite/g++.dg/pph/x1template.cc
index 5b1e980..cecefd7 100644
--- a/gcc/testsuite/g++.dg/pph/x1template.cc
+++ b/gcc/testsuite/g++.dg/pph/x1template.cc
@@ -1,5 +1,5 @@
 // { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "x1template.h:18:13: internal compiler error: in resume_scope, at cp/name-lookup.c:1569" "" { xfail *-*-* } 0 }
+// { dg-bogus "x1template.h:18:13: internal compiler error: in resume_scope, at cp/name-lookup.c:1568" "" { xfail *-*-* } 0 }
 // { dg-prune-output "In file included from " }
 
 #include "x1template.h"

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

Re: [pph] Fix binding_level's names_size streaming (issue4634071)

[Diego Novillo]

On Mon, Jun 20, 2011 at 21:12, Gabriel Charette <gchare@google.com> wrote:

> It appears names_size is not even used anywhere in the code
> (or at least I couldn't find any use of it with `grep "names_size" -R gcc/`.
> Should we just remove it?

Yes, we should remove it.

Jason, the field cp_binding_level.names_size is essentially write-only:

$ grep -r '\<names_size\>' .
./ChangeLog-2002:       names_size and vtables.
./ChangeLog-2002:       (wrapup_globals_for_namespace): Use names_size instead
./cp/ChangeLog-2002:    names_size and vtables.
./cp/ChangeLog-2002:    (wrapup_globals_for_namespace): Use names_size instead
./cp/name-lookup.h:    size_t names_size;
./cp/name-lookup.c:      b->names_size++;

I suppose it's OK to remove it, right?


Diego.

[pph] Fix binding_level's names_size streaming (issue4634071)

[Gabriel Charette]

We were streaming out the original names_size which includes the
built-in names which are not streamed out.

This only streams out the actual number of streamed names as names_size.

It appears names_size is not even used anywhere in the code
(or at least I couldn't find any use of it with `grep "names_size" -R gcc/`.
Should we just remove it?

This doesn't fix any currently failing pph tests.

Tested with bootstrap build and pph regression testing.

2011-06-20  Gabriel Charette  <gchare@google.com>

	* gcc/cp/pph-streamer-out.c (pph_count_filter_match): Added.
	(pph_out_chain_filtered): Use pph_count_filter_match.
	(pph_out_binding_level): Use pph_count_filter_match.
	(pph_out_lang_specific): Fixed space typo.
	(pph_write_tree): Fixed space typo.

diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
index 49847a9..1f9ae1d 100644
--- a/gcc/cp/pph-streamer-out.c
+++ b/gcc/cp/pph-streamer-out.c
@@ -383,6 +383,34 @@ pph_out_label_binding (pph_stream *stream, cp_label_binding *lb, bool ref_p)
 }
 
 
+/* Returns the number of nodes matching FILTER in chain
+   starting with FIRST.  */
+
+static unsigned
+pph_count_filter_match (tree first, enum chain_filter filter)
+{
+  unsigned count;
+  tree t;
+
+  switch (filter)
+    {
+    case NO_BUILTINS:
+      for (t = first, count = 0; t; t = TREE_CHAIN (t))
+	{
+	  if (filter == NO_BUILTINS && DECL_P (t) && DECL_IS_BUILTIN (t))
+	    continue;
+	  count++;
+	}
+      break;
+
+    default:
+      internal_error ("unknown chain_filter used in pph_count_filter_match");
+    }
+
+  return count;
+}
+
+
 /* Output a chain of nodes to STREAM starting with FIRST.  Skip any
    nodes that do not match FILTER.  REF_P is true if nodes in the chain
    should be emitted as references.  */
@@ -402,13 +430,7 @@ pph_out_chain_filtered (pph_stream *stream, tree first, bool ref_p,
       return;
     }
 
-  /* Count all the nodes that match the filter.  */
-  for (t = first, count = 0; t; t = TREE_CHAIN (t))
-    {
-      if (filter == NO_BUILTINS && DECL_P (t) && DECL_IS_BUILTIN (t))
-	continue;
-      count++;
-    }
+  count = pph_count_filter_match (first, filter);
   pph_out_uint (stream, count);
 
   /* Output all the nodes that match the filter.  */
@@ -439,7 +461,7 @@ static void
 pph_out_binding_level (pph_stream *stream, struct cp_binding_level *bl,
 				bool ref_p)
 {
-  unsigned i;
+  unsigned i, streamed_names_size;
   cp_class_binding *cs;
   cp_label_binding *sl;
   struct bitpack_d bp;
@@ -448,7 +470,8 @@ pph_out_binding_level (pph_stream *stream, struct cp_binding_level *bl,
     return;
 
   pph_out_chain_filtered (stream, bl->names, ref_p, NO_BUILTINS);
-  pph_out_uint (stream, bl->names_size);
+  streamed_names_size = pph_count_filter_match (bl->names, NO_BUILTINS);
+  pph_out_uint (stream, streamed_names_size);
   pph_out_chain_filtered (stream, bl->namespaces, ref_p, NO_BUILTINS);
 
   pph_out_tree_vec (stream, bl->static_decls, ref_p);
@@ -714,7 +737,7 @@ pph_out_lang_specific (pph_stream *stream, tree decl, bool ref_p)
   ld = DECL_LANG_SPECIFIC (decl);
   if (!pph_start_record (stream, ld))
     return;
-    
+
   /* Write all the fields in lang_decl_base.  */
   ldb = &ld->u.base;
   pph_out_ld_base (stream, ldb);
@@ -1080,7 +1103,7 @@ pph_write_tree (struct output_block *ob, tree expr, bool ref_p)
           TI_TYPEDEFS_NEEDING_ACCESS_CHECKING (expr), ref_p);
       break;
 
-    case TEMPLATE_PARM_INDEX: 
+    case TEMPLATE_PARM_INDEX:
       {
         template_parm_index *p = TEMPLATE_PARM_INDEX_CAST (expr);
         pph_out_tree_common (stream, expr, ref_p);

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