diff mbox

[ubsan] Add libcall arguments

Message ID 20130719184530.GE3169@redhat.com
State New
Headers show

Commit Message

Marek Polacek July 19, 2013, 6:45 p.m. UTC 
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  <polacek@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.


	Marek

Comments

Jakub Jelinek July 19, 2013, 6:50 p.m. UTC | #1 
On Fri, Jul 19, 2013 at 08:45:30PM +0200, Marek Polacek wrote:
> > >+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.

uintptr_type_node is a C/C++/ObjC/ObjC++ FE tree.  So, if you use it just
in c-family/c-ubsan.c, that is just fine, but you can't use it in ubsan.c.

> @@ -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;
>  }

Only one underscore for _typeinfo ?

	Jakub
Marek Polacek July 19, 2013, 7:01 p.m. UTC | #2 
On Fri, Jul 19, 2013 at 08:50:42PM +0200, Jakub Jelinek wrote:
> On Fri, Jul 19, 2013 at 08:45:30PM +0200, Marek Polacek wrote:
> > > >+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.
> 
> uintptr_type_node is a C/C++/ObjC/ObjC++ FE tree.  So, if you use it just
> in c-family/c-ubsan.c, that is just fine, but you can't use it in ubsan.c.

In that case I prefer to keep uptr_type around.  Even though I like
uintptr_type_node more.

> > @@ -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;
> >  }
> 
> Only one underscore for _typeinfo ?

I wonder where the _ disappeared.  Will fix.

	Marek
Jason Merrill July 20, 2013, 6:04 a.m. UTC | #3 
On 07/19/2013 03:01 PM, Marek Polacek wrote:
> On Fri, Jul 19, 2013 at 08:50:42PM +0200, Jakub Jelinek wrote:
>> uintptr_type_node is a C/C++/ObjC/ObjC++ FE tree.  So, if you use it just
>> in c-family/c-ubsan.c, that is just fine, but you can't use it in ubsan.c.

Any reason not to move it into the middle-end?

Jason
Marek Polacek July 20, 2013, 8:19 a.m. UTC | #4 
On Sat, Jul 20, 2013 at 02:04:24AM -0400, Jason Merrill wrote:
> On 07/19/2013 03:01 PM, Marek Polacek wrote:
> >On Fri, Jul 19, 2013 at 08:50:42PM +0200, Jakub Jelinek wrote:
> >>uintptr_type_node is a C/C++/ObjC/ObjC++ FE tree.  So, if you use it just
> >>in c-family/c-ubsan.c, that is just fine, but you can't use it in ubsan.c.
> 
> Any reason not to move it into the middle-end?

I don't know, but it seems iffy to pluck out one particular type of
those supplemented when adding the support of stdint.h types.
We'd have to move all of them into the middle-end (from
c_common_nodes_and_builtins).  I'm not sure if it's worth it.

	Marek
Jakub Jelinek July 20, 2013, 2:27 p.m. UTC | #5 
On Sat, Jul 20, 2013 at 10:19:55AM +0200, Marek Polacek wrote:
> On Sat, Jul 20, 2013 at 02:04:24AM -0400, Jason Merrill wrote:
> > On 07/19/2013 03:01 PM, Marek Polacek wrote:
> > >On Fri, Jul 19, 2013 at 08:50:42PM +0200, Jakub Jelinek wrote:
> > >>uintptr_type_node is a C/C++/ObjC/ObjC++ FE tree.  So, if you use it just
> > >>in c-family/c-ubsan.c, that is just fine, but you can't use it in ubsan.c.
> > 
> > Any reason not to move it into the middle-end?
> 
> I don't know, but it seems iffy to pluck out one particular type of
> those supplemented when adding the support of stdint.h types.
> We'd have to move all of them into the middle-end (from
> c_common_nodes_and_builtins).  I'm not sure if it's worth it.

The way uintptr_type_node is constructed relies on C/C++ FE to transform
the string "unsigned long int" etc. defined in the target headers to a type,
but you can't do that in the middle-end.  So, the middle-end can only use
some other type, say build_nonstandard_integer_type (POINTER_SIZE, 1);
which often will be the same type as uintptr_type_node, but perhaps not on
all targets.  It could be worth to create such a type in tree.c and stick it
into global trees and then just use, but right now all the uses are just in
asan.c and ubsan.c.

	Jakub
Jason Merrill July 21, 2013, 12:17 a.m. UTC | #6 
On 07/20/2013 10:27 AM, Jakub Jelinek wrote:
> So, the middle-end can only use
> some other type, say build_nonstandard_integer_type (POINTER_SIZE, 1);
> which often will be the same type as uintptr_type_node, but perhaps not on
> all targets.  It could be worth to create such a type in tree.c and stick it
> into global trees and then just use, but right now all the uses are just in
> asan.c and ubsan.c.

Let's do that; I expect it will be useful in other situations, too.

Jason
Jason Merrill July 22, 2013, 2:09 p.m. UTC | #7 
On 07/19/2013 02:45 PM, Marek Polacek wrote:
>  /* 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;
>  };

What I was looking for was something along the lines of "this hash table 
maps from a TYPE to a ubsan type descriptor VAR_DECL for that type".

Jason
diff mbox

Patch

--- 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;