Re: [PATCH] Fix up PCH with DECL_VALUE_EXPR/DECL_DEBUG_EXPR

[Richard Guenther <rguenther at suse dot de>]

On Wed, 2 Jun 2010, Jakub Jelinek wrote:

> Hi!
> 
> During regtesting PR44367 patch on i686-linux I saw weird PCH errors on
> libstdc++.
> The problem seems to be that DECL_VALUE_EXPR/DECL_DEBUG_EXPR hash tables
> hash using htab_hash_pointer (and even store that hash value into the
> tree_map structure).  When writing PCH, the addresses of the decls in the
> hash table change, but the hash table isn't rehashed, nor ->hash fields
> updated, which results in DECL_HAS_VALUE_EXPR_P being set after PCH read,
> but DECL_VALUE_EXPR for it returning NULL (the right decl is in the hash
> table, but not where the hash lookup will look for it).
> 
> In both these cases (and for shadowed_var_for_decl too) only decls
> are used as from trees, so this patch fixes it by using DECL_UID as hash
> instead of htab_hash_pointer.  Furthermore, I think doing DECL_UID
> on the from tree is cheap enough that it isn't worth wasting 8 bytes per
> decl with DECL_VALUE_EXPR for the cached hash value.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2010-06-02  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree.h (struct tree_decl_map): New type.
> 	(tree_decl_map_eq, tree_decl_map_marked_p): Define.
> 	(tree_decl_map_hash): New prototype.
> 	(debug_expr_for_decl, value_expr_for_decl): Change into
> 	tree_decl_map hashtab from tree_map.
> 	(init_ttree): Adjust initialization.
> 	(tree_decl_map_hash): New function.
> 	(decl_debug_expr_lookup, decl_debug_expr_insert,
> 	decl_value_expr_lookup, decl_value_expr_insert): Adjust.
> cp/
> 	* cp-objcp-common.c (shadowed_var_for_decl): Change into
> 	tree_decl_map hashtab from tree_map.
> 	(decl_shadowed_for_var_lookup, decl_shadowed_for_var_insert): Adjust.
> 	(init_shadowed_var_for_decl): Adjust initialization.
> 
> --- gcc/tree.h.jj	2010-06-01 12:27:48.000000000 +0200
> +++ gcc/tree.h	2010-06-02 19:10:10.000000000 +0200
> @@ -5382,6 +5382,17 @@ struct GTY(()) tree_map {
>  extern unsigned int tree_map_hash (const void *);
>  #define tree_map_marked_p tree_map_base_marked_p
>  
> +/* Map from a decl tree to another tree.  */
> +
> +struct GTY(()) tree_decl_map {
> +  struct tree_map_base base;
> +  tree to;
> +};
> +
> +#define tree_decl_map_eq tree_map_base_eq
> +extern unsigned int tree_decl_map_hash (const void *);
> +#define tree_decl_map_marked_p tree_map_base_marked_p
> +
>  /* Map from a tree to an int.  */
>  
>  struct GTY(()) tree_int_map {
> --- gcc/tree.c.jj	2010-05-28 14:36:04.000000000 +0200
> +++ gcc/tree.c	2010-06-02 19:16:19.000000000 +0200
> @@ -196,10 +196,10 @@ static GTY ((if_marked ("ggc_marked_p"),
>  /* General tree->tree mapping  structure for use in hash tables.  */
>  
>  
> -static GTY ((if_marked ("tree_map_marked_p"), param_is (struct tree_map)))
> +static GTY ((if_marked ("tree_decl_map_marked_p"), param_is (struct tree_decl_map)))
>       htab_t debug_expr_for_decl;
>  
> -static GTY ((if_marked ("tree_map_marked_p"), param_is (struct tree_map)))
> +static GTY ((if_marked ("tree_decl_map_marked_p"), param_is (struct tree_decl_map)))
>       htab_t value_expr_for_decl;
>  
>  static GTY ((if_marked ("tree_priority_map_marked_p"),
> @@ -533,11 +533,11 @@ init_ttree (void)
>    type_hash_table = htab_create_ggc (TYPE_HASH_INITIAL_SIZE, type_hash_hash,
>  				     type_hash_eq, 0);
>  
> -  debug_expr_for_decl = htab_create_ggc (512, tree_map_hash,
> -					 tree_map_eq, 0);
> +  debug_expr_for_decl = htab_create_ggc (512, tree_decl_map_hash,
> +					 tree_decl_map_eq, 0);
>  
> -  value_expr_for_decl = htab_create_ggc (512, tree_map_hash,
> -					 tree_map_eq, 0);
> +  value_expr_for_decl = htab_create_ggc (512, tree_decl_map_hash,
> +					 tree_decl_map_eq, 0);
>    init_priority_for_decl = htab_create_ggc (512, tree_priority_map_hash,
>  					    tree_priority_map_eq, 0);
>  
> @@ -5577,7 +5577,7 @@ tree_map_base_eq (const void *va, const 
>    return (a->from == b->from);
>  }
>  
> -/* Hash a from tree in a tree_map.  */
> +/* Hash a from tree in a tree_base_map.  */
>  
>  unsigned int
>  tree_map_base_hash (const void *item)
> @@ -5595,12 +5595,22 @@ tree_map_base_marked_p (const void *p)
>    return ggc_marked_p (((const struct tree_map_base *) p)->from);
>  }
>  
> +/* Hash a from tree in a tree_map.  */
> +
>  unsigned int
>  tree_map_hash (const void *item)
>  {
>    return (((const struct tree_map *) item)->hash);
>  }
>  
> +/* Hash a from tree in a tree_decl_map.  */
> +
> +unsigned int
> +tree_decl_map_hash (const void *item)
> +{
> +  return DECL_UID (((const struct tree_decl_map *) item)->base.from);
> +}
> +
>  /* Return the initialization priority for DECL.  */
>  
>  priority_type
> @@ -5706,11 +5716,11 @@ print_value_expr_statistics (void)
>  tree
>  decl_debug_expr_lookup (tree from)
>  {
> -  struct tree_map *h, in;
> +  struct tree_decl_map *h, in;
>    in.base.from = from;
>  
> -  h = (struct tree_map *) htab_find_with_hash (debug_expr_for_decl, &in,
> -					       htab_hash_pointer (from));
> +  h = (struct tree_decl_map *)
> +      htab_find_with_hash (debug_expr_for_decl, &in, DECL_UID (from));
>    if (h)
>      return h->to;
>    return NULL_TREE;
> @@ -5721,15 +5731,15 @@ decl_debug_expr_lookup (tree from)
>  void
>  decl_debug_expr_insert (tree from, tree to)
>  {
> -  struct tree_map *h;
> +  struct tree_decl_map *h;
>    void **loc;
>  
> -  h = GGC_NEW (struct tree_map);
> -  h->hash = htab_hash_pointer (from);
> +  h = GGC_NEW (struct tree_decl_map);
>    h->base.from = from;
>    h->to = to;
> -  loc = htab_find_slot_with_hash (debug_expr_for_decl, h, h->hash, INSERT);
> -  *(struct tree_map **) loc = h;
> +  loc = htab_find_slot_with_hash (debug_expr_for_decl, h, DECL_UID (from),
> +				  INSERT);
> +  *(struct tree_decl_map **) loc = h;
>  }
>  
>  /* Lookup a value expression for FROM, and return it if we find one.  */
> @@ -5737,11 +5747,11 @@ decl_debug_expr_insert (tree from, tree 
>  tree
>  decl_value_expr_lookup (tree from)
>  {
> -  struct tree_map *h, in;
> +  struct tree_decl_map *h, in;
>    in.base.from = from;
>  
> -  h = (struct tree_map *) htab_find_with_hash (value_expr_for_decl, &in,
> -					       htab_hash_pointer (from));
> +  h = (struct tree_decl_map *)
> +      htab_find_with_hash (value_expr_for_decl, &in, DECL_UID (from));
>    if (h)
>      return h->to;
>    return NULL_TREE;
> @@ -5752,15 +5762,15 @@ decl_value_expr_lookup (tree from)
>  void
>  decl_value_expr_insert (tree from, tree to)
>  {
> -  struct tree_map *h;
> +  struct tree_decl_map *h;
>    void **loc;
>  
> -  h = GGC_NEW (struct tree_map);
> -  h->hash = htab_hash_pointer (from);
> +  h = GGC_NEW (struct tree_decl_map);
>    h->base.from = from;
>    h->to = to;
> -  loc = htab_find_slot_with_hash (value_expr_for_decl, h, h->hash, INSERT);
> -  *(struct tree_map **) loc = h;
> +  loc = htab_find_slot_with_hash (value_expr_for_decl, h, DECL_UID (from),
> +				  INSERT);
> +  *(struct tree_decl_map **) loc = h;
>  }
>  
>  /* Hashing of types so that we don't make duplicates.
> --- gcc/cp/cp-objcp-common.c.jj	2010-04-16 13:41:33.000000000 +0200
> +++ gcc/cp/cp-objcp-common.c	2010-06-02 19:20:55.000000000 +0200
> @@ -177,7 +177,7 @@ has_c_linkage (const_tree decl)
>    return DECL_EXTERN_C_P (decl);
>  }
>  
> -static GTY ((if_marked ("tree_map_marked_p"), param_is (struct tree_map)))
> +static GTY ((if_marked ("tree_decl_map_marked_p"), param_is (struct tree_decl_map)))
>       htab_t shadowed_var_for_decl;
>  
>  /* Lookup a shadowed var for FROM, and return it if we find one.  */
> @@ -185,11 +185,11 @@ static GTY ((if_marked ("tree_map_marked
>  tree
>  decl_shadowed_for_var_lookup (tree from)
>  {
> -  struct tree_map *h, in;
> +  struct tree_decl_map *h, in;
>    in.base.from = from;
>  
> -  h = (struct tree_map *) htab_find_with_hash (shadowed_var_for_decl, &in,
> -					       htab_hash_pointer (from));
> +  h = (struct tree_decl_map *)
> +      htab_find_with_hash (shadowed_var_for_decl, &in, DECL_UID (from));
>    if (h)
>      return h->to;
>    return NULL_TREE;
> @@ -200,22 +200,22 @@ decl_shadowed_for_var_lookup (tree from)
>  void
>  decl_shadowed_for_var_insert (tree from, tree to)
>  {
> -  struct tree_map *h;
> +  struct tree_decl_map *h;
>    void **loc;
>  
> -  h = GGC_NEW (struct tree_map);
> -  h->hash = htab_hash_pointer (from);
> +  h = GGC_NEW (struct tree_decl_map);
>    h->base.from = from;
>    h->to = to;
> -  loc = htab_find_slot_with_hash (shadowed_var_for_decl, h, h->hash, INSERT);
> -  *(struct tree_map **) loc = h;
> +  loc = htab_find_slot_with_hash (shadowed_var_for_decl, h, DECL_UID (from),
> +				  INSERT);
> +  *(struct tree_decl_map **) loc = h;
>  }
>  
>  void
>  init_shadowed_var_for_decl (void)
>  {
> -  shadowed_var_for_decl = htab_create_ggc (512, tree_map_hash,
> -					   tree_map_eq, 0);
> +  shadowed_var_for_decl = htab_create_ggc (512, tree_decl_map_hash,
> +					   tree_decl_map_eq, 0);
>  }
>  
>  
> 
> 	Jakub
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex