Patchwork [ubsan] Use pointer map instead of hash table.

login
register
mail settings
Submitter Marek Polacek
Date Aug. 29, 2013, 12:56 p.m.
Message ID <20130829125658.GA23899@redhat.com>
Download mbox | patch
Permalink /patch/270812/
State New
Headers show

Comments

Marek Polacek - Aug. 29, 2013, 12:56 p.m.
On Wed, Aug 28, 2013 at 03:37:31PM +0200, Jakub Jelinek wrote:
> On Wed, Aug 28, 2013 at 03:30:46PM +0200, Marek Polacek wrote:
> > >From a quick look, it doesn't seem like we do.  (I was searching for
> > something about pointer_map in ggc* and gen* files.)
> 
> If we can't make it GC aware easily, I'm afraid we need to revert that change to
> pointer_map.  Now, the question is if the if_marked stuff can be easily done
> for the previous implementation with C++ish hash table, or if we should just
> use something similar to what tree.c uses for value_expr_for_decl and
> similar (of course, with minor adjustments, because we want to map from
> TYPE_UID rather than DECL_UID of the key to tree).  Or with pointer_map we'd
> need to push both the keys (types) and what they map to into (decls) into
> some GC vector in addition to the pointer_map.

This is an attempt to convert the pointer_map into C style hash map;
it mimics value_expr_for_decl stuff in tree.[ch].  I've built a bunch of
build_distinct_type_copy's that are in fact unused and in the final assembly
I see only the really used ones, so hopefully it works ;).

Tested x86_64-linux, ran bootstrap-ubsan and stage2/3 comparison looks
ok.  Though, I'd appreciate if either of you could take a peek at it
before I commit it.  Thanks,

2013-08-29  Marek Polacek  <polacek@redhat.com>

	* Makefile.in (ubsan.o): Add HASH_TABLE_H and gt-ubsan.h
	dependencies.  Remove pointer-set.h dependency.
	* ubsan.c: Convert to C style hash table.


	Marek
Jakub Jelinek - Aug. 29, 2013, 1:05 p.m.
On Thu, Aug 29, 2013 at 02:56:58PM +0200, Marek Polacek wrote:
> --- gcc/Makefile.in.mp	2013-08-29 14:24:49.839578625 +0200
> +++ gcc/Makefile.in	2013-08-29 14:54:39.354258737 +0200
> @@ -2273,7 +2273,7 @@ tsan.o : $(CONFIG_H) $(SYSTEM_H) $(TREE_
>     intl.h cfghooks.h output.h options.h $(C_COMMON_H) tsan.h asan.h \
>     tree-ssa-propagate.h
>  ubsan.o : ubsan.c ubsan.h $(CONFIG_H) $(SYSTEM_H) $(GIMPLE_H) \
> -   output.h coretypes.h $(TREE_H) $(CGRAPH_H) pointer-set.h \
> +   output.h coretypes.h $(TREE_H) $(CGRAPH_H) $(HASH_TABLE_H) gt-ubsan.h \
>     toplev.h $(C_COMMON_H)
>  tree-ssa-tail-merge.o: tree-ssa-tail-merge.c \
>     $(SYSTEM_H) $(CONFIG_H) coretypes.h $(TM_H) $(BITMAP_H) \
> --- gcc/ubsan.c.mp	2013-08-29 14:24:49.840578629 +0200
> +++ gcc/ubsan.c	2013-08-29 14:24:54.848597440 +0200
> @@ -24,39 +24,71 @@ along with GCC; see the file COPYING3.
>  #include "tree.h"
>  #include "cgraph.h"
>  #include "gimple.h"
> +#include "hash-table.h"

Why not #include "hashtab.h" instead (and corresponding change in
Makefile.in ?

>  #include "pointer-set.h"
>  #include "output.h"
>  #include "toplev.h"
>  #include "ubsan.h"
>  #include "c-family/c-common.h"
>  
> -/* Map a TYPE to an ubsan type descriptor VAR_DECL for that type.  */
> -static pointer_map<tree> *typedesc_map;
> +/* Map from a tree to a VAR_DECL tree.  */
>  
> -/* Insert DECL as the VAR_DECL for TYPE in the TYPEDESC_MAP.  */
> +struct GTY(()) tree_type_map {
> +  struct tree_map_base type;
> +  unsigned int hash;

You use TYPE_UID as hash, and never use hash field, so why you are adding
it?

> +#define tree_type_map_eq tree_map_base_eq
> +#define tree_type_map_marked_p tree_map_base_marked_p
> +
> +static GTY ((if_marked ("tree_type_map_marked_p"), param_is (struct tree_type_map)))
> +     htab_t decl_tree_for_type;
> +
> +/* Initialize the hash table.  */
>  
>  static void
> -insert_decl_for_type (tree decl, tree type)
> +init_hash_table (void)
>  {
> -  *typedesc_map->insert (type) = decl;
> +  decl_tree_for_type = htab_create_ggc (512, tree_decl_map_hash,
> +					tree_decl_map_eq, 0);

tree_type_map_hash and tree_type_map_eq here, right?
You could also just manually inline init_hash_table into the single caller, after
all, it is just one statement.
512 might be too much, how many types we put there typically?  10, 20?

> -/* Find the VAR_DECL for TYPE in TYPEDESC_MAP.  If TYPE does not
> -   exist in the map, return NULL_TREE, otherwise, return the VAR_DECL
> -   we found.  */
> +/* Lookup a VAR_DECL for TYPE, and return it if we find one.  */
>  
> -static tree
> -lookup_decl_for_type (tree type)
> +tree
> +decl_for_type_lookup (tree type)

Why are you dropping the static here?

> +/* Insert a mapping TYPE->DECL in the VAR_DECL for type hashtable.  */
> +
> +void
> +decl_for_type_insert (tree type, tree decl)

Again, can't this be static?

Otherwise looks good.

	Jakub

Patch

--- gcc/Makefile.in.mp	2013-08-29 14:24:49.839578625 +0200
+++ gcc/Makefile.in	2013-08-29 14:54:39.354258737 +0200
@@ -2273,7 +2273,7 @@  tsan.o : $(CONFIG_H) $(SYSTEM_H) $(TREE_
    intl.h cfghooks.h output.h options.h $(C_COMMON_H) tsan.h asan.h \
    tree-ssa-propagate.h
 ubsan.o : ubsan.c ubsan.h $(CONFIG_H) $(SYSTEM_H) $(GIMPLE_H) \
-   output.h coretypes.h $(TREE_H) $(CGRAPH_H) pointer-set.h \
+   output.h coretypes.h $(TREE_H) $(CGRAPH_H) $(HASH_TABLE_H) gt-ubsan.h \
    toplev.h $(C_COMMON_H)
 tree-ssa-tail-merge.o: tree-ssa-tail-merge.c \
    $(SYSTEM_H) $(CONFIG_H) coretypes.h $(TM_H) $(BITMAP_H) \
--- gcc/ubsan.c.mp	2013-08-29 14:24:49.840578629 +0200
+++ gcc/ubsan.c	2013-08-29 14:24:54.848597440 +0200
@@ -24,39 +24,71 @@  along with GCC; see the file COPYING3.
 #include "tree.h"
 #include "cgraph.h"
 #include "gimple.h"
+#include "hash-table.h"
 #include "pointer-set.h"
 #include "output.h"
 #include "toplev.h"
 #include "ubsan.h"
 #include "c-family/c-common.h"
 
-/* Map a TYPE to an ubsan type descriptor VAR_DECL for that type.  */
-static pointer_map<tree> *typedesc_map;
+/* Map from a tree to a VAR_DECL tree.  */
 
-/* Insert DECL as the VAR_DECL for TYPE in the TYPEDESC_MAP.  */
+struct GTY(()) tree_type_map {
+  struct tree_map_base type;
+  unsigned int hash;
+  tree decl;
+};
+
+#define tree_type_map_eq tree_map_base_eq
+#define tree_type_map_marked_p tree_map_base_marked_p
+
+static GTY ((if_marked ("tree_type_map_marked_p"), param_is (struct tree_type_map)))
+     htab_t decl_tree_for_type;
+
+/* Initialize the hash table.  */
 
 static void
-insert_decl_for_type (tree decl, tree type)
+init_hash_table (void)
 {
-  *typedesc_map->insert (type) = decl;
+  decl_tree_for_type = htab_create_ggc (512, tree_decl_map_hash,
+					tree_decl_map_eq, 0);
 }
 
-/* Find the VAR_DECL for TYPE in TYPEDESC_MAP.  If TYPE does not
-   exist in the map, return NULL_TREE, otherwise, return the VAR_DECL
-   we found.  */
+/* Lookup a VAR_DECL for TYPE, and return it if we find one.  */
 
-static tree
-lookup_decl_for_type (tree type)
+tree
+decl_for_type_lookup (tree type)
 {
-  /* If the pointer map is not initialized yet, create it now.  */
-  if (typedesc_map == NULL)
+  /* If the hash table is not initialized yet, create it now.  */
+  if (decl_tree_for_type == NULL)
     {
-      typedesc_map = new pointer_map<tree>;
+      init_hash_table ();
       /* That also means we don't have to bother with the lookup.  */
       return NULL_TREE;
     }
-  tree *t = typedesc_map->contains (type);
-  return t ? *t : NULL_TREE;
+
+  struct tree_type_map *h, in;
+  in.type.from = type;
+
+  h = (struct tree_type_map *)
+      htab_find_with_hash (decl_tree_for_type, &in, TYPE_UID (type));
+  return h ? h->decl : NULL_TREE;
+}
+
+/* Insert a mapping TYPE->DECL in the VAR_DECL for type hashtable.  */
+
+void
+decl_for_type_insert (tree type, tree decl)
+{
+  struct tree_type_map *h;
+  void **slot;
+
+  h = ggc_alloc_tree_type_map ();
+  h->type.from = type;
+  h->decl = decl;
+  slot = htab_find_slot_with_hash (decl_tree_for_type, h, TYPE_UID (type),
+                                  INSERT);
+  *(struct tree_type_map **) slot = h;
 }
 
 /* Helper routine, which encodes a value in the pointer_sized_int_node.
@@ -226,7 +258,7 @@  ubsan_type_descriptor (tree type)
   /* See through any typedefs.  */
   type = TYPE_MAIN_VARIANT (type);
 
-  tree decl = lookup_decl_for_type (type);
+  tree decl = decl_for_type_lookup (type);
   if (decl != NULL_TREE)
     return decl;
 
@@ -282,7 +314,7 @@  ubsan_type_descriptor (tree type)
 
   /* Save the address of the VAR_DECL into the pointer map.  */
   decl = build_fold_addr_expr (decl);
-  insert_decl_for_type (decl, type);
+  decl_for_type_insert (type, decl);
 
   return decl;
 }
@@ -388,3 +420,5 @@  is_ubsan_builtin_p (tree t)
   return strncmp (IDENTIFIER_POINTER (DECL_NAME (t)),
 		  "__builtin___ubsan_", 18) == 0;
 }
+
+#include "gt-ubsan.h"