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

[Richard Biener <rguenther@suse.de>]

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)