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

[NightStrike <nightstrike at gmail dot com>]

On Wed, Jun 2, 2010 at 3:40 PM, Richard Guenther <rguenther@suse.de> wrote:
> 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
>

Ping: committed?