RFA (hash-map): PATCH to support GTY((cache)) with hash_map

RFA (hash-map): PATCH to support GTY((cache)) with hash_map

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

The hash_map interface is a lot more convenient than that of
hash_table for cases where it makes sense, but there hasn't been a way
to get the ggc_cache_remove behavior with a hash_map.  In other words,
not marking elements during the initial ggc marking phase, but maybe
marking them during the clear_caches phase based on keep_cache_entry.

This patch implements that by:

Adding a ggc_maybe_mx member function to ggc_remove, and overriding
that instead of ggc_mx in ggc_cache_remove.
Calling H::ggc_maybe_mx instead of H::ggc_mx in gt_ggc_mx (hash_table *).
Calling H::ggc_mx in gt_cleare_caches (hash_table *) rather than
relying on an extern declaration of a plain function that cannot be
declared for hash_map::hash_entry.
Adding ggc_maybe_mx and keep_cache_entry to hash_map::hash_entry.
Adding gt_cleare_cache for hash_map.
Adding a boolean constant to the hash-map traits indicating whether we
want the cache behavior above.

I then define a typedef tree_cache_map to use this functionality, and
use it in a few places in the C++ front end.

Tested x86_64-pc-linux-gnu, OK for trunk?

[-- Attachment #2: cache-map.diff --]
[-- Type: text/plain, Size: 11617 bytes --]

commit 29a77538437442915ecb85516e3710c918d0e8ac
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Sep 13 14:33:33 2017 -0400

            Support GTY((cache)) on hash_map.
    
    gcc/
            * hash-traits.h (ggc_remove): Add ggc_maybe_mx member function.
            (ggc_cache_remove): Override it instead of ggc_mx.
            * hash-table.h (gt_ggc_mx): Call it instead of ggc_mx.
            (gt_cleare_cache): Call ggc_mx instead of gt_ggc_mx.
            * hash-map-traits.h (simple_hashmap_traits): Add maybe_mx member.
            (simple_cache_map_traits): Override maybe_mx.
            * hash-map.h (hash_entry): Add ggc_maybe_mx and keep_cache_entry.
            (hash_map): Friend gt_cleare_cache.
            (gt_cleare_cache): New.
            * tree.h (tree_cache_traits): New hash_map traits class.
            (tree_cache_map): New typedef.
    gcc/cp/
            * decl.c (decomp_type_table): Use tree_cache_map.
            * init.c (nsdmi_inst): Likewise.
            * pt.c (defarg_ints): Likewise.
            * cp-objcp-common.c (cp_get_debug_type): Likewise.

diff --git a/gcc/cp/cp-objcp-common.c b/gcc/cp/cp-objcp-common.c
index 183e7f7bf57..27f0b985378 100644
--- a/gcc/cp/cp-objcp-common.c
+++ b/gcc/cp/cp-objcp-common.c
@@ -131,19 +131,7 @@ cxx_types_compatible_p (tree x, tree y)
   return same_type_ignoring_top_level_qualifiers_p (x, y);
 }
 
-struct debug_type_hasher : ggc_cache_ptr_hash<tree_map>
-{
-  static hashval_t hash (tree_map *m) { return tree_map_hash (m); }
-  static bool equal (tree_map *a, tree_map *b) { return tree_map_eq (a, b); }
-
-  static int
-  keep_cache_entry (tree_map *&e)
-  {
-    return ggc_marked_p (e->base.from);
-  }
-};
-
-static GTY((cache)) hash_table<debug_type_hasher> *debug_type_hash;
+static GTY((cache)) tree_cache_map *debug_type_map;
 
 /* Return a type to use in the debug info instead of TYPE, or NULL_TREE to
    keep TYPE.  */
@@ -151,38 +139,29 @@ static GTY((cache)) hash_table<debug_type_hasher> *debug_type_hash;
 tree
 cp_get_debug_type (const_tree type)
 {
+  tree dtype = NULL_TREE;
+
   if (TYPE_PTRMEMFUNC_P (type) && !typedef_variant_p (type))
+    dtype = build_offset_type (TYPE_PTRMEMFUNC_OBJECT_TYPE (type),
+			       TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE (type)));
+
+  /* We cannot simply return the debug type here because the function uses
+     the type canonicalization hashtable, which is GC-ed, so its behavior
+     depends on the actual collection points.  Since we are building these
+     types on the fly for the debug info only, they would not be attached
+     to any GC root and always be swept, so we would make the contents of
+     the debug info depend on the collection points.  */
+  if (dtype)
     {
-      if (debug_type_hash == NULL)
-	debug_type_hash = hash_table<debug_type_hasher>::create_ggc (512);
-
-      /* We cannot simply use build_offset_type here because the function uses
-	 the type canonicalization hashtable, which is GC-ed, so its behavior
-	 depends on the actual collection points.  Since we are building these
-	 types on the fly for the debug info only, they would not be attached
-	 to any GC root and always be swept, so we would make the contents of
-	 the debug info depend on the collection points.  */
-      struct tree_map in, *h;
-
-      in.base.from = CONST_CAST_TREE (type);
-      in.hash = htab_hash_pointer (type);
-      h = debug_type_hash->find_with_hash (&in, in.hash);
-      if (h)
-	return h->to;
-
-      tree t = build_offset_type (TYPE_PTRMEMFUNC_OBJECT_TYPE (type),
-				  TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE (type)));
-
-      h = ggc_alloc<tree_map> ();
-      h->base.from = CONST_CAST_TREE (type);
-      h->hash = htab_hash_pointer (type);
-      h->to = t;
-      *debug_type_hash->find_slot_with_hash (h, h->hash, INSERT) = h;
-
-      return t;
+      tree ktype = CONST_CAST_TREE (type);
+      if (debug_type_map == NULL)
+	debug_type_map = tree_cache_map::create_ggc (512);
+      else if (tree *slot = debug_type_map->get (ktype))
+	return *slot;
+      debug_type_map->put (ktype, dtype);
     }
 
-  return NULL_TREE;
+  return dtype;
 }
 
 /* Return -1 if dwarf ATTR shouldn't be added for DECL, or the attribute
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index f4d6f802cbe..06a4a7d22cc 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5117,7 +5117,6 @@ extern GTY(()) vec<tree, va_gc> *static_decls;
 /* An array of vtable-needing types that have no key function, or have
    an emitted key function.  */
 extern GTY(()) vec<tree, va_gc> *keyed_classes;
-
 \f
 /* Here's where we control how name mangling takes place.  */
 
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 6101bdf2bc8..bfb0ebe9642 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -7303,12 +7303,13 @@ get_tuple_decomp_init (tree decl, unsigned i)
 
 /* It's impossible to recover the decltype of a tuple decomposition variable
    based on the actual type of the variable, so store it in a hash table.  */
-static GTY(()) hash_map<tree,tree> *decomp_type_table;
+
+static GTY((cache)) tree_cache_map *decomp_type_table;
 static void
 store_decomp_type (tree v, tree t)
 {
   if (!decomp_type_table)
-    decomp_type_table = hash_map<tree,tree>::create_ggc (13);
+    decomp_type_table = tree_cache_map::create_ggc (13);
   decomp_type_table->put (v, t);
 }
 
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index b01d662fef2..d6241d29379 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -535,7 +535,7 @@ perform_target_ctor (tree init)
 
 /* Return the non-static data initializer for FIELD_DECL MEMBER.  */
 
-static GTY(()) hash_map<tree, tree> *nsdmi_inst;
+static GTY((cache)) tree_cache_map *nsdmi_inst;
 
 tree
 get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain)
@@ -590,7 +590,7 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain)
 	  if (init != error_mark_node)
 	    {
 	      if (!nsdmi_inst)
-		nsdmi_inst = hash_map<tree,tree>::create_ggc (37);
+		nsdmi_inst = tree_cache_map::create_ggc (37);
 	      nsdmi_inst->put (member, init);
 	    }
 
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index ec7bbc890c6..aa158dc939d 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -12008,7 +12008,7 @@ tsubst_aggr_type (tree t,
     }
 }
 
-static GTY(()) hash_map<tree, tree> *defarg_inst;
+static GTY((cache)) tree_cache_map *defarg_inst;
 
 /* Substitute into the default argument ARG (a default argument for
    FN), which has the indicated TYPE.  */
@@ -12095,7 +12095,7 @@ tsubst_default_argument (tree fn, int parmnum, tree type, tree arg,
   if (arg != error_mark_node && !cp_unevaluated_operand)
     {
       if (!defarg_inst)
-	defarg_inst = hash_map<tree,tree>::create_ggc (37);
+	defarg_inst = tree_cache_map::create_ggc (37);
       defarg_inst->put (parm, arg);
     }
 
diff --git a/gcc/hash-map-traits.h b/gcc/hash-map-traits.h
index 2b5fddf2d09..a92f0cb00f4 100644
--- a/gcc/hash-map-traits.h
+++ b/gcc/hash-map-traits.h
@@ -32,6 +32,7 @@ template <typename H, typename Value>
 struct simple_hashmap_traits
 {
   typedef typename H::value_type key_type;
+  static const bool maybe_mx = true;
   static inline hashval_t hash (const key_type &);
   static inline bool equal_keys (const key_type &, const key_type &);
   template <typename T> static inline void remove (T &);
@@ -97,6 +98,12 @@ simple_hashmap_traits <H, Value>::mark_deleted (T &entry)
   H::mark_deleted (entry.m_key);
 }
 
+template <typename H, typename Value>
+struct simple_cache_map_traits: public simple_hashmap_traits<H,Value>
+{
+  static const bool maybe_mx = false;
+};
+
 /* Implement traits for a hash_map with values of type Value for cases
    in which the key cannot represent empty and deleted slots.  Instead
    record empty and deleted entries in Value.  Derived classes must
diff --git a/gcc/hash-map.h b/gcc/hash-map.h
index 73f1c5427a0..6b8365a9d0a 100644
--- a/gcc/hash-map.h
+++ b/gcc/hash-map.h
@@ -62,6 +62,12 @@ class GTY((user)) hash_map
 	gt_ggc_mx (e.m_value);
       }
 
+    static void ggc_maybe_mx (hash_entry &e)
+      {
+	if (Traits::maybe_mx)
+	  ggc_mx (e);
+      }
+
     static void pch_nx (hash_entry &e)
       {
 	gt_pch_nx (e.m_key);
@@ -74,6 +80,11 @@ class GTY((user)) hash_map
 	pch_nx_helper (e.m_value, op, c);
       }
 
+    static int keep_cache_entry (hash_entry &e)
+      {
+	return ggc_marked_p (e.m_key);
+      }
+
   private:
     template<typename T>
     static void
@@ -237,7 +248,8 @@ private:
 
   template<typename T, typename U, typename V> friend void gt_ggc_mx (hash_map<T, U, V> *);
   template<typename T, typename U, typename V> friend void gt_pch_nx (hash_map<T, U, V> *);
-      template<typename T, typename U, typename V> friend void gt_pch_nx (hash_map<T, U, V> *, gt_pointer_operator, void *);
+  template<typename T, typename U, typename V> friend void gt_pch_nx (hash_map<T, U, V> *, gt_pointer_operator, void *);
+  template<typename T, typename U, typename V> friend void gt_cleare_cache (hash_map<T, U, V> *);
 
   hash_table<hash_entry> m_table;
 };
@@ -260,6 +272,13 @@ gt_pch_nx (hash_map<K, V, H> *h)
 
 template<typename K, typename V, typename H>
 static inline void
+gt_cleare_cache (hash_map<K, V, H> *h)
+{
+  gt_cleare_cache (&h->m_table);
+}
+
+template<typename K, typename V, typename H>
+static inline void
 gt_pch_nx (hash_map<K, V, H> *h, gt_pointer_operator op, void *cookie)
 {
   op (&h->m_table.m_entries, cookie);
diff --git a/gcc/hash-table.h b/gcc/hash-table.h
index 64d3157953c..b86a1d1b278 100644
--- a/gcc/hash-table.h
+++ b/gcc/hash-table.h
@@ -1044,7 +1044,9 @@ gt_ggc_mx (hash_table<E> *h)
 	  || table::is_deleted (h->m_entries[i]))
 	continue;
 
-      E::ggc_mx (h->m_entries[i]);
+      /* Use ggc_maxbe_mx so we don't mark right away for cache tables; we'll
+	 mark in gt_cleare_cache if appropriate.  */
+      E::ggc_maybe_mx (h->m_entries[i]);
     }
 }
 
@@ -1094,7 +1096,6 @@ template<typename H>
 inline void
 gt_cleare_cache (hash_table<H> *h)
 {
-  extern void gt_ggc_mx (typename H::value_type &t);
   typedef hash_table<H> table;
   if (!h)
     return;
@@ -1106,7 +1107,7 @@ gt_cleare_cache (hash_table<H> *h)
 	if (res == 0)
 	  h->clear_slot (&*iter);
 	else if (res != -1)
-	  gt_ggc_mx (*iter);
+	  H::ggc_mx (*iter);
       }
 }
 
diff --git a/gcc/hash-traits.h b/gcc/hash-traits.h
index a5c4f103474..6a613c45811 100644
--- a/gcc/hash-traits.h
+++ b/gcc/hash-traits.h
@@ -235,6 +235,13 @@ struct ggc_remove
     gt_ggc_mx (p);
   }
 
+  /* Overridden in ggc_cache_remove.  */
+  static void
+  ggc_maybe_mx (T &p)
+  {
+    ggc_mx (p);
+  }
+
   static void
   pch_nx (T &p)
   {
@@ -256,7 +263,7 @@ template<typename T>
 struct ggc_cache_remove : ggc_remove<T>
 {
   /* Entries are weakly held because this is for caches.  */
-  static void ggc_mx (T &) {}
+  static void ggc_maybe_mx (T &) {}
 
   static int
   keep_cache_entry (T &e)
diff --git a/gcc/tree.h b/gcc/tree.h
index caa4a69977d..6845953c09c 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -4873,6 +4873,13 @@ struct tree_decl_map_cache_hasher : ggc_cache_ptr_hash<tree_decl_map>
 #define tree_vec_map_hash tree_decl_map_hash
 #define tree_vec_map_marked_p tree_map_base_marked_p
 
+/* A hash_map of two trees for use with GTY((cache)).  Garbage collection for
+   such a map will not mark keys, and will mark values if the key is already
+   marked.  */
+struct tree_cache_traits
+  : simple_cache_map_traits<default_hash_traits<tree>, tree> { };
+typedef hash_map<tree,tree,tree_cache_traits> tree_cache_map;
+
 /* Initialize the abstract argument list iterator object ITER with the
    arguments from CALL_EXPR node EXP.  */
 static inline void

Re: RFA (hash-map): PATCH to support GTY((cache)) with hash_map

[Richard Biener]

On Fri, Sep 15, 2017 at 11:45 PM, Jason Merrill <jason@redhat.com> wrote:
> The hash_map interface is a lot more convenient than that of
> hash_table for cases where it makes sense, but there hasn't been a way
> to get the ggc_cache_remove behavior with a hash_map.  In other words,
> not marking elements during the initial ggc marking phase, but maybe
> marking them during the clear_caches phase based on keep_cache_entry.
>
> This patch implements that by:
>
> Adding a ggc_maybe_mx member function to ggc_remove, and overriding
> that instead of ggc_mx in ggc_cache_remove.
> Calling H::ggc_maybe_mx instead of H::ggc_mx in gt_ggc_mx (hash_table *).
> Calling H::ggc_mx in gt_cleare_caches (hash_table *) rather than
> relying on an extern declaration of a plain function that cannot be
> declared for hash_map::hash_entry.
> Adding ggc_maybe_mx and keep_cache_entry to hash_map::hash_entry.
> Adding gt_cleare_cache for hash_map.
> Adding a boolean constant to the hash-map traits indicating whether we
> want the cache behavior above.
>
> I then define a typedef tree_cache_map to use this functionality, and
> use it in a few places in the C++ front end.
>
> Tested x86_64-pc-linux-gnu, OK for trunk?

Ok.

Richard.

Re: RFA (hash-map): PATCH to support GTY((cache)) with hash_map

[Markus Trippelsdorf]

On 2017.11.14 at 13:32 +0100, Richard Biener wrote:
> On Fri, Sep 15, 2017 at 11:45 PM, Jason Merrill <jason@redhat.com> wrote:
> > The hash_map interface is a lot more convenient than that of
> > hash_table for cases where it makes sense, but there hasn't been a way
> > to get the ggc_cache_remove behavior with a hash_map.  In other words,
> > not marking elements during the initial ggc marking phase, but maybe
> > marking them during the clear_caches phase based on keep_cache_entry.
> >
> > This patch implements that by:
> >
> > Adding a ggc_maybe_mx member function to ggc_remove, and overriding
> > that instead of ggc_mx in ggc_cache_remove.
> > Calling H::ggc_maybe_mx instead of H::ggc_mx in gt_ggc_mx (hash_table *).
> > Calling H::ggc_mx in gt_cleare_caches (hash_table *) rather than
> > relying on an extern declaration of a plain function that cannot be
> > declared for hash_map::hash_entry.
> > Adding ggc_maybe_mx and keep_cache_entry to hash_map::hash_entry.
> > Adding gt_cleare_cache for hash_map.
> > Adding a boolean constant to the hash-map traits indicating whether we
> > want the cache behavior above.
> >
> > I then define a typedef tree_cache_map to use this functionality, and
> > use it in a few places in the C++ front end.
> >
> > Tested x86_64-pc-linux-gnu, OK for trunk?
> 
> Ok.

The patch causes UB in some cases:
 
 gcc/hash-map.h:277:19: runtime error: member access within null pointer of type 'struct hash_map'

-- 
Markus

[PATCH] Fix UB in hash-map.h

[Markus Trippelsdorf]

bootstrap-ubsan shows:
  gcc/hash-map.h:277:19: runtime error: member access within null pointer of type 'struct hash_map'

Fix the issue by returning early.
bootstrap-ubsan on X86_64 and ppc64le. Tested on ppc64le.

OK for trunk?

gcc/
	* hash-map.h (gt_cleare_cache): Avoid UB.

diff --git a/gcc/hash-map.h b/gcc/hash-map.h
index 6b8365a9d0a6..e5630338491a 100644
--- a/gcc/hash-map.h
+++ b/gcc/hash-map.h
@@ -274,6 +274,8 @@ template<typename K, typename V, typename H>
 static inline void
 gt_cleare_cache (hash_map<K, V, H> *h)
 {
+  if (!h)
+    return;
   gt_cleare_cache (&h->m_table);
 }
 
-- 
Markus

Re: [PATCH] Fix UB in hash-map.h

[Richard Biener]

On Sun, Nov 26, 2017 at 10:05 AM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> bootstrap-ubsan shows:
>   gcc/hash-map.h:277:19: runtime error: member access within null pointer of type 'struct hash_map'
>
> Fix the issue by returning early.
> bootstrap-ubsan on X86_64 and ppc64le. Tested on ppc64le.
>
> OK for trunk?

Ok.

> gcc/
>         * hash-map.h (gt_cleare_cache): Avoid UB.
>
> diff --git a/gcc/hash-map.h b/gcc/hash-map.h
> index 6b8365a9d0a6..e5630338491a 100644
> --- a/gcc/hash-map.h
> +++ b/gcc/hash-map.h
> @@ -274,6 +274,8 @@ template<typename K, typename V, typename H>
>  static inline void
>  gt_cleare_cache (hash_map<K, V, H> *h)
>  {
> +  if (!h)
> +    return;
>    gt_cleare_cache (&h->m_table);
>  }
>
> --
> Markus