On Thu, Jul 18, 2013 at 03:47:28PM -0400, Jason Merrill wrote:
> On 07/05/2013 10:04 AM, Marek Polacek wrote:
> >+/* This type represents an entry in the hash table. */
>
> Please describe the hash table more up here. What are you tracking?
Ok, I've added two more comments.
> >+ hashval_t h = iterative_hash_object (data->type, 0);
> >+ h = iterative_hash_object (data->decl, h);
>
> If you hash the decl as well as the type, the find_slot in
> ubsan_type_descriptor will almost never find an existing entry.
Oops. Fixed.
> >+uptr_type (void)
> >+{
> >+ return build_nonstandard_integer_type (POINTER_SIZE, 1);
>
> Why not use uintptr_type_node?
I suppose I could. I just followed suit what asan.c does. I didn't
address this in this patch, but I can, if you want to.
> >I have yet to handle
> >freeing the hash table, but I think I'll need the GTY machinery for
> >this (ubsan is not a pass, so I can't just call it at the end of the
> >pas). Or maybe just create a destructor and use append_to_statement_list.
>
> That won't work; append_to_statement_list is for things that happen
> at runtime, but freeing the hash table is something that needs to
> happen in the compiler.
Yeah, indeed.
> >+/* This routine returns a magic number for TYPE.
> >+ ??? This is probably too ugly. Tweak it. */
> >+
> >+static unsigned short
> >+get_tinfo_for_type (tree type)
>
> Why map from size to some magic number rather than use the size
> directly? Also, "tinfo" sounds to me like something to do with C++
> type_info.
It's what the ubsan library wants; however, I rewrote & renamed
that functions as per Jakub's suggestion.
Thanks for the review. Does it look ok?
Ran ubsan testsuite, both -m64/-m32.
2013-07-19 Marek Polacek <pola...@redhat.com>
* ubsan.c (struct ubsan_typedesc): Add comments.
(ubsan_typedesc_hasher::hash): Don't hash the VAR_DECL element.
(ubsan_typedesc_hasher::equal): Adjust comment.
(ubsan_typedesc_get_alloc_pool): Remove comment.
(empty_ubsan_typedesc_hash_table): Remove function.
(ubsan_source_location_type): Remove bogus comment.
(get_tinfo_for_type): Remove function.
(get_ubsan_type_info_for_type): New function.
(ubsan_type_descriptor): Use ASM_GENERATE_INTERNAL_LABEL instead of
ASM_FORMAT_PRIVATE_NAME. Use TYPE_MAIN_VARIANT of the type.
(ubsan_create_data): Likewise.
--- gcc/ubsan.c.mp 2013-07-19 18:42:42.896249415 +0200
+++ gcc/ubsan.c 2013-07-19 20:34:11.459792189 +0200
@@ -34,7 +34,10 @@ along with GCC; see the file COPYING3.
/* This type represents an entry in the hash table. */
struct ubsan_typedesc
{
+ /* This represents the type of a variable. */
tree type;
+
+ /* This is the VAR_DECL of the type. */
tree decl;
};
@@ -56,9 +59,7 @@ struct ubsan_typedesc_hasher
inline hashval_t
ubsan_typedesc_hasher::hash (const ubsan_typedesc *data)
{
- hashval_t h = iterative_hash_object (data->type, 0);
- h = iterative_hash_object (data->decl, h);
- return h;
+ return iterative_hash_object (data->type, 0);
}
/* Compare two data types. */
@@ -67,8 +68,8 @@ inline bool
ubsan_typedesc_hasher::equal (const ubsan_typedesc *d1,
const ubsan_typedesc *d2)
{
- /* ??? Here, the types should have identical __typekind,
- _typeinfo and __typename. Is this enough? */
+ /* Here, the types should have identical __typekind,
+ _typeinfo and __typename. */
return d1->type == d2->type;
}
@@ -93,7 +94,6 @@ ubsan_typedesc_get_alloc_pool ()
ubsan_typedesc_alloc_pool = create_alloc_pool ("ubsan_typedesc",
sizeof (ubsan_typedesc),
10);
- // XXX But where do we free this? We'll need GTY machinery.
return ubsan_typedesc_alloc_pool;
}
@@ -123,18 +123,6 @@ ubsan_typedesc_new (tree type, tree decl
return desc;
}
-/* Clear all entries from the type descriptor hash table. */
-
-#if 0
-static void
-empty_ubsan_typedesc_hash_table ()
-{
- // XXX But when do we call this?
- if (ubsan_typedesc_ht.is_created ())
- ubsan_typedesc_ht.empty ();
-}
-#endif
-
/* Build the ubsan uptr type. */
static tree
@@ -245,7 +233,6 @@ ubsan_source_location_type (void)
{
fields[i] = build_decl (UNKNOWN_LOCATION, FIELD_DECL,
get_identifier (field_names[i]),
- //(i == 0) ? const_string_type_node
(i == 0) ? build_pointer_type (const_char_type)
: unsigned_type_node);
DECL_CONTEXT (fields[i]) = ret;
@@ -291,34 +278,16 @@ ubsan_source_location (location_t loc)
return ctor;
}
-/* This routine returns a magic number for TYPE.
- ??? This is probably too ugly. Tweak it. */
+/* This routine returns a magic number for TYPE. */
static unsigned short
-get_tinfo_for_type (tree type)
+get_ubsan_type_info_for_type (tree type)
{
- unsigned short tinfo;
+ int prec = exact_log2 (TYPE_PRECISION (type));
+ if (prec == -1)
+ error ("unexpected size of type %qT", type);
- switch (GET_MODE_SIZE (TYPE_MODE (type)))
- {
- case 4:
- tinfo = 5;
- break;
- case 8:
- tinfo = 6;
- break;
- case 16:
- tinfo = 7;
- break;
- default:
- error ("unexpected size of type %qT", type);
- }
-
- tinfo <<= 1;
-
- /* The MSB here says whether the value is signed or not. */
- tinfo |= !TYPE_UNSIGNED (type);
- return tinfo;
+ return (prec << 1) | !TYPE_UNSIGNED (type);
}
/* Helper routine that returns ADDR_EXPR of a VAR_DECL of a type
@@ -333,6 +302,9 @@ ubsan_type_descriptor (tree type)
ubsan_typedesc d;
ubsan_typedesc_init (&d, type, NULL);
+ /* See through any typedefs. */
+ type = TYPE_MAIN_VARIANT (type);
+
ubsan_typedesc **slot = ht.find_slot (&d, INSERT);
if (*slot != NULL)
/* We have the VAR_DECL in the table. Return it. */
@@ -353,7 +325,7 @@ ubsan_type_descriptor (tree type)
{
/* For INTEGER_TYPE, this is 0x0000. */
tkind = 0x000;
- tinfo = get_tinfo_for_type (type);
+ tinfo = get_ubsan_type_info_for_type (type);
}
else if (TREE_CODE (type) == REAL_TYPE)
/* We don't have float support yet. */
@@ -362,9 +334,9 @@ ubsan_type_descriptor (tree type)
gcc_unreachable ();
/* Create a new VAR_DECL of type descriptor. */
- char *tmp_name;
+ char tmp_name[32];
static unsigned int type_var_id_num;
- ASM_FORMAT_PRIVATE_NAME (tmp_name, ".Lubsan_type", type_var_id_num++);
+ ASM_GENERATE_INTERNAL_LABEL (tmp_name, "Lubsan_type", type_var_id_num++);
tree decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier
(tmp_name),
dtype);
TREE_STATIC (decl) = 1;
@@ -446,9 +418,9 @@ ubsan_create_data (const char *name, loc
va_end (args);
/* Now, fill in the type. */
- char *tmp_name;
+ char tmp_name[32];
static unsigned int ubsan_var_id_num;
- ASM_FORMAT_PRIVATE_NAME (tmp_name, ".Lubsan_data", ubsan_var_id_num++);
+ ASM_GENERATE_INTERNAL_LABEL (tmp_name, "Lubsan_data", ubsan_var_id_num++);
tree var = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (tmp_name),
ret);
TREE_STATIC (var) = 1;
Marek
