| Message ID | 1448318933-23235-1-git-send-email-tbsaunde+gcc@tbsaunde.org |
|---|---|
| State | New |
| Headers | show |
On Mon, 23 Nov 2015, tbsaunde+gcc@tbsaunde.org wrote: > From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> > > Hi, > > This fixes several leaks where a hash_map user expected deleting the map to > destruct the value part of entries as well as the key. A couple of these bugs > have already been fixed, but there are more of them for example some of the > sanitizer code, and tree-if-conv.c). The expectation that hash_map should > destruct values seems to be pretty obviously correct, so we should fix that to > fix the existing bugs and prevent future ones (I also seem to remember that > working at some point, but could be incorrect). > > I checked all the existing hash_map users and couldn't find any value types > other than auto_vec with destructors, so its the only one with a non trivial > destructor. So the only effected case auto_vec is fixed by this patch and no > expectations are broken. > > bootstrapped + regtested on x86_64-linux-gnu, ok? Ok. Thanks, Richard. > Trev > > gcc/ChangeLog: > > 2015-11-20 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> > > * hash-map-traits.h (simple_hashmap_traits ::remove): call > destructors on values that are being removed. > * mem-stats.h (hash_map): Pass type of values to > simple_hashmap_traits. > * tree-sra.c (sra_deinitialize): Remove work around for hash > maps not destructing values. > * genmatch.c (sinfo_hashmap_traits): Adjust. > * tree-ssa-uncprop.c (val_ssa_equiv_hash_traits): Likewise. > --- > gcc/genmatch.c | 3 ++- > gcc/hash-map-traits.h | 32 +++++++++++++++++--------------- > gcc/mem-stats.h | 3 ++- > gcc/tree-sra.c | 6 ------ > gcc/tree-ssa-uncprop.c | 3 ++- > 5 files changed, 23 insertions(+), 24 deletions(-) > > diff --git a/gcc/genmatch.c b/gcc/genmatch.c > index 3a20a48..76c8f1f 100644 > --- a/gcc/genmatch.c > +++ b/gcc/genmatch.c > @@ -1397,7 +1397,8 @@ struct sinfo > unsigned cnt; > }; > > -struct sinfo_hashmap_traits : simple_hashmap_traits <pointer_hash <dt_simplify> > > +struct sinfo_hashmap_traits : simple_hashmap_traits<pointer_hash<dt_simplify>, > + sinfo *> > { > static inline hashval_t hash (const key_type &); > static inline bool equal_keys (const key_type &, const key_type &); > diff --git a/gcc/hash-map-traits.h b/gcc/hash-map-traits.h > index 2225426..773ac1b 100644 > --- a/gcc/hash-map-traits.h > +++ b/gcc/hash-map-traits.h > @@ -28,7 +28,7 @@ along with GCC; see the file COPYING3. If not see > /* Implement hash_map traits for a key with hash traits H. Empty and > deleted map entries are represented as empty and deleted keys. */ > > -template <typename H> > +template <typename H, typename Value> > struct simple_hashmap_traits > { > typedef typename H::value_type key_type; > @@ -41,56 +41,58 @@ struct simple_hashmap_traits > template <typename T> static inline void mark_deleted (T &); > }; > > -template <typename H> > +template <typename H, typename Value> > inline hashval_t > -simple_hashmap_traits <H>::hash (const key_type &h) > +simple_hashmap_traits <H, Value>::hash (const key_type &h) > { > return H::hash (h); > } > > -template <typename H> > +template <typename H, typename Value> > inline bool > -simple_hashmap_traits <H>::equal_keys (const key_type &k1, const key_type &k2) > +simple_hashmap_traits <H, Value>::equal_keys (const key_type &k1, > + const key_type &k2) > { > return H::equal (k1, k2); > } > > -template <typename H> > +template <typename H, typename Value> > template <typename T> > inline void > -simple_hashmap_traits <H>::remove (T &entry) > +simple_hashmap_traits <H, Value>::remove (T &entry) > { > H::remove (entry.m_key); > + entry.m_value.~Value (); > } > > -template <typename H> > +template <typename H, typename Value> > template <typename T> > inline bool > -simple_hashmap_traits <H>::is_empty (const T &entry) > +simple_hashmap_traits <H, Value>::is_empty (const T &entry) > { > return H::is_empty (entry.m_key); > } > > -template <typename H> > +template <typename H, typename Value> > template <typename T> > inline bool > -simple_hashmap_traits <H>::is_deleted (const T &entry) > +simple_hashmap_traits <H, Value>::is_deleted (const T &entry) > { > return H::is_deleted (entry.m_key); > } > > -template <typename H> > +template <typename H, typename Value> > template <typename T> > inline void > -simple_hashmap_traits <H>::mark_empty (T &entry) > +simple_hashmap_traits <H, Value>::mark_empty (T &entry) > { > H::mark_empty (entry.m_key); > } > > -template <typename H> > +template <typename H, typename Value> > template <typename T> > inline void > -simple_hashmap_traits <H>::mark_deleted (T &entry) > +simple_hashmap_traits <H, Value>::mark_deleted (T &entry) > { > H::mark_deleted (entry.m_key); > } > diff --git a/gcc/mem-stats.h b/gcc/mem-stats.h > index a6489b5..2c68ca7 100644 > --- a/gcc/mem-stats.h > +++ b/gcc/mem-stats.h > @@ -3,7 +3,8 @@ > > /* Forward declaration. */ > template<typename Key, typename Value, > - typename Traits = simple_hashmap_traits<default_hash_traits<Key> > > > + typename Traits = simple_hashmap_traits<default_hash_traits<Key>, > + Value> > > class hash_map; > > #define LOCATION_LINE_EXTRA_SPACE 30 > diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c > index 2835c99..c4fea5b 100644 > --- a/gcc/tree-sra.c > +++ b/gcc/tree-sra.c > @@ -674,12 +674,6 @@ sra_deinitialize (void) > assign_link_pool.release (); > obstack_free (&name_obstack, NULL); > > - /* TODO: hash_map does not support traits that can release > - value type of the hash_map. */ > - for (hash_map<tree, auto_vec<access_p> >::iterator it = > - base_access_vec->begin (); it != base_access_vec->end (); ++it) > - (*it).second.release (); > - > delete base_access_vec; > } > > diff --git a/gcc/tree-ssa-uncprop.c b/gcc/tree-ssa-uncprop.c > index be6c209d..23b4ca2 100644 > --- a/gcc/tree-ssa-uncprop.c > +++ b/gcc/tree-ssa-uncprop.c > @@ -277,7 +277,8 @@ struct equiv_hash_elt > > /* Value to ssa name equivalence hashtable helpers. */ > > -struct val_ssa_equiv_hash_traits : simple_hashmap_traits <tree_operand_hash> > +struct val_ssa_equiv_hash_traits : simple_hashmap_traits <tree_operand_hash, > + vec<tree> > > { > template<typename T> static inline void remove (T &); > }; >
tbsaunde+gcc@tbsaunde.org writes: > -template <typename H> > +template <typename H, typename Value> > template <typename T> > inline void > -simple_hashmap_traits <H>::remove (T &entry) > +simple_hashmap_traits <H, Value>::remove (T &entry) > { > H::remove (entry.m_key); > + entry.m_value.~Value (); > } This is just repeating my IRC comment really, but doesn't this mean that we're calling the destructor on an object that was never constructed? I.e. nothing ever calls placement new on the entry, the m_key, or the m_value. Thanks, Richard
On Tue, Dec 01, 2015 at 07:43:35PM +0000, Richard Sandiford wrote: > tbsaunde+gcc@tbsaunde.org writes: > > -template <typename H> > > +template <typename H, typename Value> > > template <typename T> > > inline void > > -simple_hashmap_traits <H>::remove (T &entry) > > +simple_hashmap_traits <H, Value>::remove (T &entry) > > { > > H::remove (entry.m_key); > > + entry.m_value.~Value (); > > } > > This is just repeating my IRC comment really, but doesn't this mean that > we're calling the destructor on an object that was never constructed? > I.e. nothing ever calls placement new on the entry, the m_key, or the > m_value. I believe you are correct that placement new is not called. I'd say its a bug waiting to happen given that the usage of auto_vec seems to demonstrate that people expect objects to be initialized and destroyed. However for now all values are either POD, or auto_vec and in either case the current 0 initialization has the same effect as the constructor. So There may be a theoretical problem with how we initialize values that will become real when somebody adds a constructor that doesn't just 0 initialize. So it should probably be improved at some point, but it doesn't seem necessary to mess with it at this point instead of next stage 1. Trev > > Thanks, > Richard
On Wed, 2 Dec 2015, Trevor Saunders wrote: > On Tue, Dec 01, 2015 at 07:43:35PM +0000, Richard Sandiford wrote: > > tbsaunde+gcc@tbsaunde.org writes: > > > -template <typename H> > > > +template <typename H, typename Value> > > > template <typename T> > > > inline void > > > -simple_hashmap_traits <H>::remove (T &entry) > > > +simple_hashmap_traits <H, Value>::remove (T &entry) > > > { > > > H::remove (entry.m_key); > > > + entry.m_value.~Value (); > > > } > > > > This is just repeating my IRC comment really, but doesn't this mean that > > we're calling the destructor on an object that was never constructed? > > I.e. nothing ever calls placement new on the entry, the m_key, or the > > m_value. > > I believe you are correct that placement new is not called. I'd say its > a bug waiting to happen given that the usage of auto_vec seems to > demonstrate that people expect objects to be initialized and destroyed. > However for now all values are either POD, or auto_vec and in either > case the current 0 initialization has the same effect as the > constructor. So There may be a theoretical problem with how we > initialize values that will become real when somebody adds a constructor > that doesn't just 0 initialize. So it should probably be improved at > some point, but it doesn't seem necessary to mess with it at this point > instead of next stage 1. Agreed. You'll also need a more elaborate allocator/constructor scheme for this considering the case where no default constructor is available. See how alloc-pool.h tries to dance around this using a "raw" allocate and a operator new... Richard. > Trev > > > > > Thanks, > > Richard > >
Richard Biener <rguenther@suse.de> writes: > On Wed, 2 Dec 2015, Trevor Saunders wrote: >> On Tue, Dec 01, 2015 at 07:43:35PM +0000, Richard Sandiford wrote: >> > tbsaunde+gcc@tbsaunde.org writes: >> > > -template <typename H> >> > > +template <typename H, typename Value> >> > > template <typename T> >> > > inline void >> > > -simple_hashmap_traits <H>::remove (T &entry) >> > > +simple_hashmap_traits <H, Value>::remove (T &entry) >> > > { >> > > H::remove (entry.m_key); >> > > + entry.m_value.~Value (); >> > > } >> > >> > This is just repeating my IRC comment really, but doesn't this mean that >> > we're calling the destructor on an object that was never constructed? >> > I.e. nothing ever calls placement new on the entry, the m_key, or the >> > m_value. >> >> I believe you are correct that placement new is not called. I'd say its >> a bug waiting to happen given that the usage of auto_vec seems to >> demonstrate that people expect objects to be initialized and destroyed. >> However for now all values are either POD, or auto_vec and in either >> case the current 0 initialization has the same effect as the >> constructor. So There may be a theoretical problem with how we >> initialize values that will become real when somebody adds a constructor >> that doesn't just 0 initialize. So it should probably be improved at >> some point, but it doesn't seem necessary to mess with it at this point >> instead of next stage 1. > > Agreed. OK. I was just worried that (IIRC) we had cases where for: a.~foo () a.x = ...; the assignment to a.x was removed as dead since the object had been destroyed. Maybe that could happen again if we don't have an explicit constructor to create a new object. Thanks, Richard > You'll also need a more elaborate allocator/constructor > scheme for this considering the case where no default constructor > is available. See how alloc-pool.h tries to dance around this > using a "raw" allocate and a operator new... > > Richard.
On Wed, Dec 02, 2015 at 09:35:13AM +0000, Richard Sandiford wrote: > Richard Biener <rguenther@suse.de> writes: > > On Wed, 2 Dec 2015, Trevor Saunders wrote: > >> On Tue, Dec 01, 2015 at 07:43:35PM +0000, Richard Sandiford wrote: > >> > tbsaunde+gcc@tbsaunde.org writes: > >> > > -template <typename H> > >> > > +template <typename H, typename Value> > >> > > template <typename T> > >> > > inline void > >> > > -simple_hashmap_traits <H>::remove (T &entry) > >> > > +simple_hashmap_traits <H, Value>::remove (T &entry) > >> > > { > >> > > H::remove (entry.m_key); > >> > > + entry.m_value.~Value (); > >> > > } > >> > > >> > This is just repeating my IRC comment really, but doesn't this mean that > >> > we're calling the destructor on an object that was never constructed? > >> > I.e. nothing ever calls placement new on the entry, the m_key, or the > >> > m_value. > >> > >> I believe you are correct that placement new is not called. I'd say its > >> a bug waiting to happen given that the usage of auto_vec seems to > >> demonstrate that people expect objects to be initialized and destroyed. > >> However for now all values are either POD, or auto_vec and in either > >> case the current 0 initialization has the same effect as the > >> constructor. So There may be a theoretical problem with how we > >> initialize values that will become real when somebody adds a constructor > >> that doesn't just 0 initialize. So it should probably be improved at > >> some point, but it doesn't seem necessary to mess with it at this point > >> instead of next stage 1. > > > > Agreed. > > OK. I was just worried that (IIRC) we had cases where for: > > a.~foo () > a.x = ...; > > the assignment to a.x was removed as dead since the object had been > destroyed. Maybe that could happen again if we don't have an explicit > constructor to create a new object. It seems possible though it seems rather unlikely since you'd have to insert a new element with the same hash as the one you just removed, and the compiler would have to figure that out. On the other hand I suppose the existing code may already be invalid since it operates on objects without constructing them. Trev > > Thanks, > Richard > > > You'll also need a more elaborate allocator/constructor > > scheme for this considering the case where no default constructor > > is available. See how alloc-pool.h tries to dance around this > > using a "raw" allocate and a operator new... > > > > Richard. >
diff --git a/gcc/genmatch.c b/gcc/genmatch.c index 3a20a48..76c8f1f 100644 --- a/gcc/genmatch.c +++ b/gcc/genmatch.c @@ -1397,7 +1397,8 @@ struct sinfo unsigned cnt; }; -struct sinfo_hashmap_traits : simple_hashmap_traits <pointer_hash <dt_simplify> > +struct sinfo_hashmap_traits : simple_hashmap_traits<pointer_hash<dt_simplify>, + sinfo *> { static inline hashval_t hash (const key_type &); static inline bool equal_keys (const key_type &, const key_type &); diff --git a/gcc/hash-map-traits.h b/gcc/hash-map-traits.h index 2225426..773ac1b 100644 --- a/gcc/hash-map-traits.h +++ b/gcc/hash-map-traits.h @@ -28,7 +28,7 @@ along with GCC; see the file COPYING3. If not see /* Implement hash_map traits for a key with hash traits H. Empty and deleted map entries are represented as empty and deleted keys. */ -template <typename H> +template <typename H, typename Value> struct simple_hashmap_traits { typedef typename H::value_type key_type; @@ -41,56 +41,58 @@ struct simple_hashmap_traits template <typename T> static inline void mark_deleted (T &); }; -template <typename H> +template <typename H, typename Value> inline hashval_t -simple_hashmap_traits <H>::hash (const key_type &h) +simple_hashmap_traits <H, Value>::hash (const key_type &h) { return H::hash (h); } -template <typename H> +template <typename H, typename Value> inline bool -simple_hashmap_traits <H>::equal_keys (const key_type &k1, const key_type &k2) +simple_hashmap_traits <H, Value>::equal_keys (const key_type &k1, + const key_type &k2) { return H::equal (k1, k2); } -template <typename H> +template <typename H, typename Value> template <typename T> inline void -simple_hashmap_traits <H>::remove (T &entry) +simple_hashmap_traits <H, Value>::remove (T &entry) { H::remove (entry.m_key); + entry.m_value.~Value (); } -template <typename H> +template <typename H, typename Value> template <typename T> inline bool -simple_hashmap_traits <H>::is_empty (const T &entry) +simple_hashmap_traits <H, Value>::is_empty (const T &entry) { return H::is_empty (entry.m_key); } -template <typename H> +template <typename H, typename Value> template <typename T> inline bool -simple_hashmap_traits <H>::is_deleted (const T &entry) +simple_hashmap_traits <H, Value>::is_deleted (const T &entry) { return H::is_deleted (entry.m_key); } -template <typename H> +template <typename H, typename Value> template <typename T> inline void -simple_hashmap_traits <H>::mark_empty (T &entry) +simple_hashmap_traits <H, Value>::mark_empty (T &entry) { H::mark_empty (entry.m_key); } -template <typename H> +template <typename H, typename Value> template <typename T> inline void -simple_hashmap_traits <H>::mark_deleted (T &entry) +simple_hashmap_traits <H, Value>::mark_deleted (T &entry) { H::mark_deleted (entry.m_key); } diff --git a/gcc/mem-stats.h b/gcc/mem-stats.h index a6489b5..2c68ca7 100644 --- a/gcc/mem-stats.h +++ b/gcc/mem-stats.h @@ -3,7 +3,8 @@ /* Forward declaration. */ template<typename Key, typename Value, - typename Traits = simple_hashmap_traits<default_hash_traits<Key> > > + typename Traits = simple_hashmap_traits<default_hash_traits<Key>, + Value> > class hash_map; #define LOCATION_LINE_EXTRA_SPACE 30 diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index 2835c99..c4fea5b 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -674,12 +674,6 @@ sra_deinitialize (void) assign_link_pool.release (); obstack_free (&name_obstack, NULL); - /* TODO: hash_map does not support traits that can release - value type of the hash_map. */ - for (hash_map<tree, auto_vec<access_p> >::iterator it = - base_access_vec->begin (); it != base_access_vec->end (); ++it) - (*it).second.release (); - delete base_access_vec; } diff --git a/gcc/tree-ssa-uncprop.c b/gcc/tree-ssa-uncprop.c index be6c209d..23b4ca2 100644 --- a/gcc/tree-ssa-uncprop.c +++ b/gcc/tree-ssa-uncprop.c @@ -277,7 +277,8 @@ struct equiv_hash_elt /* Value to ssa name equivalence hashtable helpers. */ -struct val_ssa_equiv_hash_traits : simple_hashmap_traits <tree_operand_hash> +struct val_ssa_equiv_hash_traits : simple_hashmap_traits <tree_operand_hash, + vec<tree> > { template<typename T> static inline void remove (T &); };
From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> Hi, This fixes several leaks where a hash_map user expected deleting the map to destruct the value part of entries as well as the key. A couple of these bugs have already been fixed, but there are more of them for example some of the sanitizer code, and tree-if-conv.c). The expectation that hash_map should destruct values seems to be pretty obviously correct, so we should fix that to fix the existing bugs and prevent future ones (I also seem to remember that working at some point, but could be incorrect). I checked all the existing hash_map users and couldn't find any value types other than auto_vec with destructors, so its the only one with a non trivial destructor. So the only effected case auto_vec is fixed by this patch and no expectations are broken. bootstrapped + regtested on x86_64-linux-gnu, ok? Trev gcc/ChangeLog: 2015-11-20 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> * hash-map-traits.h (simple_hashmap_traits ::remove): call destructors on values that are being removed. * mem-stats.h (hash_map): Pass type of values to simple_hashmap_traits. * tree-sra.c (sra_deinitialize): Remove work around for hash maps not destructing values. * genmatch.c (sinfo_hashmap_traits): Adjust. * tree-ssa-uncprop.c (val_ssa_equiv_hash_traits): Likewise. --- gcc/genmatch.c | 3 ++- gcc/hash-map-traits.h | 32 +++++++++++++++++--------------- gcc/mem-stats.h | 3 ++- gcc/tree-sra.c | 6 ------ gcc/tree-ssa-uncprop.c | 3 ++- 5 files changed, 23 insertions(+), 24 deletions(-)