Fix htab lookup bug in reregister_specialization

[Diego Novillo, 2011-10-05 19:49:42]

I found this bug while debugging a failure in pph images with template
classes.  When writing the decl_specializations table, the writer
calls htab_elements() to output the length of the table.  It then
traverses the table with htab_traverse_noresize() to emit all the
entries.

The reader uses that value to know how many entries it needs to read.
However, the table was out of sync wrt empty entries because
reregister_specialization does a lookup using INSERT instead of
NO_INSERT, so it was leaving a bunch of empty entries in it (thanks
Jakub for helping me diagnose this).

Since empty entries are never traversed by htab_traverse, they were
never written out and the reader was trying to read more entries than
there really were.

Jason, I don't think this is affecting any bugs in trunk, but it looks
worth fixing.  OK for trunk?


Diego.


	* pt.c (reregister_specialization): Call htab_find_slot with
	NO_INSERT.

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 04e7767..2366dc9 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -1648,8 +1648,8 @@ reregister_specialization (tree spec, tree tinfo, tree
new_spec)
   elt.args = TI_ARGS (tinfo);
   elt.spec = NULL_TREE;
 
-  slot = (spec_entry **) htab_find_slot (decl_specializations, &elt, INSERT);
-  if (*slot)
+  slot = (spec_entry **) htab_find_slot (decl_specializations, &elt,
NO_INSERT);
+  if (slot && *slot)
     {
       gcc_assert ((*slot)->spec == spec || (*slot)->spec == new_spec);
       gcc_assert (new_spec != NULL_TREE);

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

[jakub_redhat.com, 2011-10-05 20:05:42]

On Wed, Oct 05, 2011 at 03:49:38PM -0400, Diego Novillo wrote:
> Jason, I don't think this is affecting any bugs in trunk, but it looks
> worth fixing.  OK for trunk?

Well, it can cause problems.  Consider 3 entries in the hash table
with the same hash. (1) inserted first, then (2), then (3), then (2)
gets htab_remove_elt (decl_specializations has deletions on it too),
so (2) in the hash table is overwritten with HTAB_DELETED_ENTRY.
Next reregister_specialization is called with the same hash.
It will find the slot (where (2) used to be stored), overwrites
the HTAB_DELETED_ENTRY with HTAB_EMPTY_ENTRY (aka NULL) and return
that slot, but as reregister_specialization doesn't store anything there,
afterwards htab_find won't be able to find (3).

BTW, register_specialization has the same problems.  If slot != NULL and fn
== NULL, it can still return without storing non-NULL into *slot
(when optimize_specialization_lookup_p (tmpl) returns non-zero).
You can do else if (slot != NULL && fn == NULL) htab_clear_slot
(decl_specializations, slot);

And, IMHO slot should be void **, otherwise there is aliasing violation.

> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -1648,8 +1648,8 @@ reregister_specialization (tree spec, tree tinfo, tree
new_spec)
>    elt.args = TI_ARGS (tinfo);
>    elt.spec = NULL_TREE;
>  
> -  slot = (spec_entry **) htab_find_slot (decl_specializations, &elt, INSERT);
> -  if (*slot)
> +  slot = (spec_entry **) htab_find_slot (decl_specializations, &elt,
NO_INSERT);
> +  if (slot && *slot)
>      {
>        gcc_assert ((*slot)->spec == spec || (*slot)->spec == new_spec);
>        gcc_assert (new_spec != NULL_TREE);

If you never insert, it should be htab_find only (with spec_entry * instead
of spec_entry ** variable).

	Jakub

[jason_redhat.com, 2011-10-05 21:03:49]

On 10/05/2011 04:05 PM, Jakub Jelinek wrote:
> BTW, register_specialization has the same problems.  If slot != NULL and fn
> == NULL, it can still return without storing non-NULL into *slot
> (when optimize_specialization_lookup_p (tmpl) returns non-zero).
> You can do else if (slot != NULL&&  fn == NULL) htab_clear_slot
(decl_specializations, slot);

I don't think there's a problem in that function; if fn == NULL, then we 
store spec in its place.

> If you never insert, it should be htab_find only (with spec_entry * instead
> of spec_entry ** variable).

Makes sense.

Jason

[jakub_redhat.com, 2011-10-05 21:15:41]

On Wed, Oct 05, 2011 at 05:03:45PM -0400, Jason Merrill wrote:
> On 10/05/2011 04:05 PM, Jakub Jelinek wrote:
> >BTW, register_specialization has the same problems.  If slot != NULL and fn
> >== NULL, it can still return without storing non-NULL into *slot
> >(when optimize_specialization_lookup_p (tmpl) returns non-zero).
> >You can do else if (slot != NULL&&  fn == NULL) htab_clear_slot
(decl_specializations, slot);
> 
> I don't think there's a problem in that function; if fn == NULL,
> then we store spec in its place.

If optimize_specialization_lookup_p (tmpl) doesn't change return value in
between the two calls, then you are right.  But perhaps in that case
you could avoid the second call and use slot != NULL instead.

	Jakub

[jason_redhat.com, 2011-10-06 01:44:39]

On 10/05/2011 05:15 PM, Jakub Jelinek wrote:
> If optimize_specialization_lookup_p (tmpl) doesn't change return value in
> between the two calls, then you are right.  But perhaps in that case
> you could avoid the second call and use slot != NULL instead.

That makes sense too.

Jason