[PATCH 1/2] destroy values as well as keys when removing them from hash maps

[PATCH 1/2] destroy values as well as keys when removing them from hash maps

[tbsaunde+gcc]

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(-)

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 &);
 };
-- 
2.4.0

[PATCH 2/2] remove val_ssa_equiv_hash_traits

[tbsaunde+gcc]

From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org>

Hi,

this is pretty trivial cleanup after the previous patch, but could wait for
next stage 1 if people don't like the very small risk.

boostrappped + regtested on x86_64-linux-gnu, ok?

Trev

gcc/ChangeLog:

2015-11-20  Trevor Saunders  <tbsaunde+gcc@tbsaunde.org>

	* tree-ssa-uncprop.c (struct val_ssa_equiv_hash_traits): Remove.
	(val_ssa_equiv_hash_traits::remove): Likewise.
	(pass_uncprop::execute): Adjust.
---
 gcc/tree-ssa-uncprop.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/gcc/tree-ssa-uncprop.c b/gcc/tree-ssa-uncprop.c
index 23b4ca2..a60184e 100644
--- a/gcc/tree-ssa-uncprop.c
+++ b/gcc/tree-ssa-uncprop.c
@@ -275,27 +275,10 @@ struct equiv_hash_elt
   vec<tree> equivalences;
 };
 
-/* Value to ssa name equivalence hashtable helpers.  */
-
-struct val_ssa_equiv_hash_traits : simple_hashmap_traits <tree_operand_hash,
-							  vec<tree> >
-{
-  template<typename T> static inline void remove (T &);
-};
-
-/* Free an instance of equiv_hash_elt.  */
-
-template<typename T>
-inline void
-val_ssa_equiv_hash_traits::remove (T &elt)
-{
-  elt.m_value.release ();
-}
-
 /* Global hash table implementing a mapping from invariant values
    to a list of SSA_NAMEs which have the same value.  We might be
    able to reuse tree-vn for this code.  */
-static hash_map<tree, vec<tree>, val_ssa_equiv_hash_traits> *val_ssa_equiv;
+static hash_map<tree, auto_vec<tree> > *val_ssa_equiv;
 
 static void uncprop_into_successor_phis (basic_block);
 
@@ -518,8 +501,7 @@ pass_uncprop::execute (function *fun)
   associate_equivalences_with_edges ();
 
   /* Create our global data structures.  */
-  val_ssa_equiv
-    = new hash_map<tree, vec<tree>, val_ssa_equiv_hash_traits> (1024);
+  val_ssa_equiv = new hash_map<tree, auto_vec<tree> > (1024);
 
   /* We're going to do a dominator walk, so ensure that we have
      dominance information.  */
-- 
2.4.0

Re: [PATCH 1/2] destroy values as well as keys when removing them from hash maps

[Richard Biener]

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 &);
>  };
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH 2/2] remove val_ssa_equiv_hash_traits

[Richard Biener]

On Mon, 23 Nov 2015, tbsaunde+gcc@tbsaunde.org wrote:

> From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org>
> 
> Hi,
> 
> this is pretty trivial cleanup after the previous patch, but could wait for
> next stage 1 if people don't like the very small risk.
> 
> boostrappped + regtested on x86_64-linux-gnu, ok?

Ok.

Thanks,
RIchard.

> Trev
> 
> gcc/ChangeLog:
> 
> 2015-11-20  Trevor Saunders  <tbsaunde+gcc@tbsaunde.org>
> 
> 	* tree-ssa-uncprop.c (struct val_ssa_equiv_hash_traits): Remove.
> 	(val_ssa_equiv_hash_traits::remove): Likewise.
> 	(pass_uncprop::execute): Adjust.
> ---
>  gcc/tree-ssa-uncprop.c | 22 ++--------------------
>  1 file changed, 2 insertions(+), 20 deletions(-)
> 
> diff --git a/gcc/tree-ssa-uncprop.c b/gcc/tree-ssa-uncprop.c
> index 23b4ca2..a60184e 100644
> --- a/gcc/tree-ssa-uncprop.c
> +++ b/gcc/tree-ssa-uncprop.c
> @@ -275,27 +275,10 @@ struct equiv_hash_elt
>    vec<tree> equivalences;
>  };
>  
> -/* Value to ssa name equivalence hashtable helpers.  */
> -
> -struct val_ssa_equiv_hash_traits : simple_hashmap_traits <tree_operand_hash,
> -							  vec<tree> >
> -{
> -  template<typename T> static inline void remove (T &);
> -};
> -
> -/* Free an instance of equiv_hash_elt.  */
> -
> -template<typename T>
> -inline void
> -val_ssa_equiv_hash_traits::remove (T &elt)
> -{
> -  elt.m_value.release ();
> -}
> -
>  /* Global hash table implementing a mapping from invariant values
>     to a list of SSA_NAMEs which have the same value.  We might be
>     able to reuse tree-vn for this code.  */
> -static hash_map<tree, vec<tree>, val_ssa_equiv_hash_traits> *val_ssa_equiv;
> +static hash_map<tree, auto_vec<tree> > *val_ssa_equiv;
>  
>  static void uncprop_into_successor_phis (basic_block);
>  
> @@ -518,8 +501,7 @@ pass_uncprop::execute (function *fun)
>    associate_equivalences_with_edges ();
>  
>    /* Create our global data structures.  */
> -  val_ssa_equiv
> -    = new hash_map<tree, vec<tree>, val_ssa_equiv_hash_traits> (1024);
> +  val_ssa_equiv = new hash_map<tree, auto_vec<tree> > (1024);
>  
>    /* We're going to do a dominator walk, so ensure that we have
>       dominance information.  */
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH 1/2] destroy values as well as keys when removing them from hash maps

[Richard Sandiford]

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

Re: [PATCH 1/2] destroy values as well as keys when removing them from hash maps

[Trevor Saunders]

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

Re: [PATCH 1/2] destroy values as well as keys when removing them from hash maps

[Richard Biener]

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>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH 1/2] destroy values as well as keys when removing them from hash maps

[Richard Sandiford]

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.

Re: [PATCH 1/2] destroy values as well as keys when removing them from hash maps

[Trevor Saunders]

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.
>