Message ID | 20111005194938.E11AA1DA1A5@topo.tor.corp.google.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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);