Patchwork [ubsan] Introduce pointer_sized_int_node

login
register
mail settings
Submitter Marek Polacek
Date Aug. 26, 2013, 10:15 a.m.
Message ID <20130826101509.GH4968@redhat.com>
Download mbox | patch
Permalink /patch/269855/
State New
Headers show

Comments

Marek Polacek - Aug. 26, 2013, 10:15 a.m.
I noticed I forgot to apply this old patch, already acked by Jason.
It introduces new pointer_sized_int_node, thus we can get rid of
uptr_type function in ubsan, and it allows us to do some clean-up
in asan.c, too.

Tested x86_64-linux, applying to ubsan branch.

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

	* tree.h (enum tree_index): Add TI_POINTER_SIZED_TYPE. 
	(pointer_sized_int_node): Define.
	* tree.c (build_common_tree_nodes): Initialize
	pointer_sized_int_node.
	* ubsan.c (ubsan_encode_value): Use pointer_sized_int_node instead
	calling uptr_type.
	(uptr_type): Remove function.
	* asan.c (asan_global_struct): Use pointer_sized_int_node instead
	calling build_nonstandard_integer_type.
	(initialize_sanitizer_builtins): Likewise.
	(asan_finish_file): Likewise.


	Marek
Richard Guenther - Aug. 27, 2013, 11:48 a.m.
On Mon, Aug 26, 2013 at 12:15 PM, Marek Polacek <polacek@redhat.com> wrote:
> I noticed I forgot to apply this old patch, already acked by Jason.
> It introduces new pointer_sized_int_node, thus we can get rid of
> uptr_type function in ubsan, and it allows us to do some clean-up
> in asan.c, too.
>
> Tested x86_64-linux, applying to ubsan branch.
>
> 2013-08-26  Marek Polacek  <polacek@redhat.com>
>
>         * tree.h (enum tree_index): Add TI_POINTER_SIZED_TYPE.
>         (pointer_sized_int_node): Define.
>         * tree.c (build_common_tree_nodes): Initialize
>         pointer_sized_int_node.
>         * ubsan.c (ubsan_encode_value): Use pointer_sized_int_node instead
>         calling uptr_type.
>         (uptr_type): Remove function.
>         * asan.c (asan_global_struct): Use pointer_sized_int_node instead
>         calling build_nonstandard_integer_type.
>         (initialize_sanitizer_builtins): Likewise.
>         (asan_finish_file): Likewise.
>
> --- gcc/tree.c.mp       2013-07-21 19:54:35.416986756 +0200
> +++ gcc/tree.c  2013-07-21 19:56:58.347562787 +0200
> @@ -9638,6 +9638,8 @@ build_common_tree_nodes (bool signed_cha
>      = build_pointer_type (build_type_variant (void_type_node, 1, 0));
>    fileptr_type_node = ptr_type_node;
>
> +  pointer_sized_int_node = build_nonstandard_integer_type (POINTER_SIZE, 1);
> +
>    float_type_node = make_node (REAL_TYPE);
>    TYPE_PRECISION (float_type_node) = FLOAT_TYPE_SIZE;
>    layout_type (float_type_node);
> --- gcc/ubsan.c.mp      2013-07-21 20:04:59.469653493 +0200
> +++ gcc/ubsan.c 2013-07-21 20:07:00.227178083 +0200
> @@ -123,14 +123,6 @@ ubsan_typedesc_new (tree type, tree decl
>    return desc;
>  }
>
> -/* Build the ubsan uptr type.  */
> -
> -static tree
> -uptr_type (void)
> -{
> -  return build_nonstandard_integer_type (POINTER_SIZE, 1);
> -}
> -
>  /* Helper routine, which encodes a value in the uptr type.
>     Arguments with precision <= POINTER_SIZE are passed directly,
>     the rest is passed by reference.  T is a value we are to encode.  */
> @@ -143,7 +135,7 @@ ubsan_encode_value (tree t)
>      {
>      case INTEGER_TYPE:
>        if (TYPE_PRECISION (type) <= POINTER_SIZE)
> -       return fold_build1 (NOP_EXPR, uptr_type (), t);
> +       return fold_build1 (NOP_EXPR, pointer_sized_int_node, t);
>        else
>         return build_fold_addr_expr (t);
>      case REAL_TYPE:
> @@ -153,7 +145,7 @@ ubsan_encode_value (tree t)
>           {
>             tree itype = build_nonstandard_integer_type (bitsize, true);
>             t = fold_build1 (VIEW_CONVERT_EXPR, itype, t);
> -           return fold_convert (uptr_type (), t);
> +           return fold_convert (pointer_sized_int_node, t);
>           }
>         else
>           {
> --- gcc/tree.h.mp       2013-07-21 19:54:35.441986868 +0200
> +++ gcc/tree.h  2013-07-21 19:56:05.128353854 +0200
> @@ -4227,6 +4227,7 @@ enum tree_index
>    TI_VA_LIST_FPR_COUNTER_FIELD,
>    TI_BOOLEAN_TYPE,
>    TI_FILEPTR_TYPE,
> +  TI_POINTER_SIZED_TYPE,

I'd rather see TI_UINTPTR_TYPE and TI_INTPTR_TYPE (note they might
not be exactly of POINTER_SIZE but larger).

TI_POINTER_SIZED_TYPE is ambiguous, too - what's its signedness?
All around the compiler we use sizetype and ssizetype to munge pointers
(well, not strictly correct as targets may define sizetype to be larger/smaller
than actual pointers).

Richard.

>
>    TI_DFLOAT32_TYPE,
>    TI_DFLOAT64_TYPE,
> @@ -4383,6 +4384,7 @@ extern GTY(()) tree global_trees[TI_MAX]
>  #define va_list_fpr_counter_field      global_trees[TI_VA_LIST_FPR_COUNTER_FIELD]
>  /* The C type `FILE *'.  */
>  #define fileptr_type_node              global_trees[TI_FILEPTR_TYPE]
> +#define pointer_sized_int_node         global_trees[TI_POINTER_SIZED_TYPE]
>
>  #define boolean_type_node              global_trees[TI_BOOLEAN_TYPE]
>  #define boolean_false_node             global_trees[TI_BOOLEAN_FALSE]
> --- gcc/asan.c.mp       2013-07-21 20:07:15.013237456 +0200
> +++ gcc/asan.c  2013-07-21 20:16:10.929376734 +0200
> @@ -1954,7 +1954,7 @@ asan_global_struct (void)
>         = build_decl (UNKNOWN_LOCATION, FIELD_DECL,
>                       get_identifier (field_names[i]),
>                       (i == 0 || i == 3) ? const_ptr_type_node
> -                     : build_nonstandard_integer_type (POINTER_SIZE, 1));
> +                     : pointer_sized_int_node);
>        DECL_CONTEXT (fields[i]) = ret;
>        if (i)
>         DECL_CHAIN (fields[i - 1]) = fields[i];
> @@ -2039,8 +2039,7 @@ initialize_sanitizer_builtins (void)
>                                 ptr_type_node, ptr_type_node, NULL_TREE);
>    tree BT_FN_VOID_PTR_PTRMODE
>      = build_function_type_list (void_type_node, ptr_type_node,
> -                               build_nonstandard_integer_type (POINTER_SIZE,
> -                                                               1), NULL_TREE);
> +                               pointer_sized_int_node, NULL_TREE);
>    tree BT_FN_VOID_INT
>      = build_function_type_list (void_type_node, integer_type_node, NULL_TREE);
>    tree BT_FN_BOOL_VPTR_PTR_IX_INT_INT[5];
> @@ -2197,7 +2196,6 @@ asan_finish_file (void)
>    if (gcount)
>      {
>        tree type = asan_global_struct (), var, ctor;
> -      tree uptr = build_nonstandard_integer_type (POINTER_SIZE, 1);
>        tree dtor_statements = NULL_TREE;
>        vec<constructor_elt, va_gc> *v;
>        char buf[20];
> @@ -2226,15 +2224,16 @@ asan_finish_file (void)
>        varpool_assemble_decl (varpool_node_for_decl (var));
>
>        fn = builtin_decl_implicit (BUILT_IN_ASAN_REGISTER_GLOBALS);
> +      tree gcount_tree = build_int_cst (pointer_sized_int_node, gcount);
>        append_to_statement_list (build_call_expr (fn, 2,
>                                                  build_fold_addr_expr (var),
> -                                                build_int_cst (uptr, gcount)),
> +                                                gcount_tree),
>                                 &asan_ctor_statements);
>
>        fn = builtin_decl_implicit (BUILT_IN_ASAN_UNREGISTER_GLOBALS);
>        append_to_statement_list (build_call_expr (fn, 2,
>                                                  build_fold_addr_expr (var),
> -                                                build_int_cst (uptr, gcount)),
> +                                                gcount_tree),
>                                 &dtor_statements);
>        cgraph_build_static_cdtor ('D', dtor_statements,
>                                  MAX_RESERVED_INIT_PRIORITY - 1);
>
>         Marek
Marek Polacek - Aug. 27, 2013, 12:56 p.m.
On Tue, Aug 27, 2013 at 01:48:29PM +0200, Richard Biener wrote:
> > +  TI_POINTER_SIZED_TYPE,
> 
> I'd rather see TI_UINTPTR_TYPE and TI_INTPTR_TYPE (note they might
> not be exactly of POINTER_SIZE but larger).

We already have [u]intptr_type_node -- but only in c-family/, thus
ubsan.c/asan.c cannot use those nodes.  I can create both
TI_SIGNED_POINTER_SIZED_TYPE and TI_UNSIGNED_POINTER_SIZED_TYPE,
but we currently need only the latter...

> TI_POINTER_SIZED_TYPE is ambiguous, too - what's its signedness?

Unsigned.  But yeah, one can't tell by just looking at the name.

> All around the compiler we use sizetype and ssizetype to munge pointers
> (well, not strictly correct as targets may define sizetype to be larger/smaller
> than actual pointers).

I see.

	Marek
Richard Guenther - Aug. 28, 2013, 10:48 a.m.
On Tue, Aug 27, 2013 at 2:56 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Tue, Aug 27, 2013 at 01:48:29PM +0200, Richard Biener wrote:
>> > +  TI_POINTER_SIZED_TYPE,
>>
>> I'd rather see TI_UINTPTR_TYPE and TI_INTPTR_TYPE (note they might
>> not be exactly of POINTER_SIZE but larger).
>
> We already have [u]intptr_type_node -- but only in c-family/, thus
> ubsan.c/asan.c cannot use those nodes.  I can create both
> TI_SIGNED_POINTER_SIZED_TYPE and TI_UNSIGNED_POINTER_SIZED_TYPE,
> but we currently need only the latter...

So simply move them to the middle-end.  The set of C language types that
define the target ABI should be constructed and maintained by the middle-end
(you need it for various builtins anyway)

Richard.

>> TI_POINTER_SIZED_TYPE is ambiguous, too - what's its signedness?
>
> Unsigned.  But yeah, one can't tell by just looking at the name.
>
>> All around the compiler we use sizetype and ssizetype to munge pointers
>> (well, not strictly correct as targets may define sizetype to be larger/smaller
>> than actual pointers).
>
> I see.
>
>         Marek
Jakub Jelinek - Aug. 28, 2013, 11:10 a.m.
On Wed, Aug 28, 2013 at 12:48:41PM +0200, Richard Biener wrote:
> On Tue, Aug 27, 2013 at 2:56 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Tue, Aug 27, 2013 at 01:48:29PM +0200, Richard Biener wrote:
> >> > +  TI_POINTER_SIZED_TYPE,
> >>
> >> I'd rather see TI_UINTPTR_TYPE and TI_INTPTR_TYPE (note they might
> >> not be exactly of POINTER_SIZE but larger).
> >
> > We already have [u]intptr_type_node -- but only in c-family/, thus
> > ubsan.c/asan.c cannot use those nodes.  I can create both
> > TI_SIGNED_POINTER_SIZED_TYPE and TI_UNSIGNED_POINTER_SIZED_TYPE,
> > but we currently need only the latter...
> 
> So simply move them to the middle-end.  The set of C language types that
> define the target ABI should be constructed and maintained by the middle-end
> (you need it for various builtins anyway)

That is not easily possible.  The thing is, in the C FEs they are created
from the C/C++ type name (for stdint purposes):
  if (INTPTR_TYPE)
    intptr_type_node =
      TREE_TYPE (identifier_global_value (c_get_ident (INTPTR_TYPE)));
  if (UINTPTR_TYPE)
    uintptr_type_node =
      TREE_TYPE (identifier_global_value (c_get_ident (UINTPTR_TYPE)));
and are supposed to match the stdint.h previously used type, because
it is part of ABI etc. (long vs. long long vs. int e.g. when two of these
are the same precision).
So the middle-end uintptr type needs to be something different, while it
ideally should have the same precision, it is not the same language type.

	Jakub
Richard Guenther - Aug. 28, 2013, 11:24 a.m.
On Wed, Aug 28, 2013 at 1:10 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Aug 28, 2013 at 12:48:41PM +0200, Richard Biener wrote:
>> On Tue, Aug 27, 2013 at 2:56 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Tue, Aug 27, 2013 at 01:48:29PM +0200, Richard Biener wrote:
>> >> > +  TI_POINTER_SIZED_TYPE,
>> >>
>> >> I'd rather see TI_UINTPTR_TYPE and TI_INTPTR_TYPE (note they might
>> >> not be exactly of POINTER_SIZE but larger).
>> >
>> > We already have [u]intptr_type_node -- but only in c-family/, thus
>> > ubsan.c/asan.c cannot use those nodes.  I can create both
>> > TI_SIGNED_POINTER_SIZED_TYPE and TI_UNSIGNED_POINTER_SIZED_TYPE,
>> > but we currently need only the latter...
>>
>> So simply move them to the middle-end.  The set of C language types that
>> define the target ABI should be constructed and maintained by the middle-end
>> (you need it for various builtins anyway)
>
> That is not easily possible.  The thing is, in the C FEs they are created
> from the C/C++ type name (for stdint purposes):
>   if (INTPTR_TYPE)
>     intptr_type_node =
>       TREE_TYPE (identifier_global_value (c_get_ident (INTPTR_TYPE)));
>   if (UINTPTR_TYPE)
>     uintptr_type_node =
>       TREE_TYPE (identifier_global_value (c_get_ident (UINTPTR_TYPE)));
> and are supposed to match the stdint.h previously used type, because
> it is part of ABI etc. (long vs. long long vs. int e.g. when two of these
> are the same precision).
> So the middle-end uintptr type needs to be something different, while it
> ideally should have the same precision, it is not the same language type.

But it's the C ABI type - and we do need the C ABI types from within
the middle-end.  Joseph, any idea?

Richard.

>         Jakub
Joseph S. Myers - Aug. 28, 2013, 5:13 p.m.
On Wed, 28 Aug 2013, Richard Biener wrote:

> > That is not easily possible.  The thing is, in the C FEs they are created
> > from the C/C++ type name (for stdint purposes):
> >   if (INTPTR_TYPE)
> >     intptr_type_node =
> >       TREE_TYPE (identifier_global_value (c_get_ident (INTPTR_TYPE)));
> >   if (UINTPTR_TYPE)
> >     uintptr_type_node =
> >       TREE_TYPE (identifier_global_value (c_get_ident (UINTPTR_TYPE)));
> > and are supposed to match the stdint.h previously used type, because
> > it is part of ABI etc. (long vs. long long vs. int e.g. when two of these
> > are the same precision).
> > So the middle-end uintptr type needs to be something different, while it
> > ideally should have the same precision, it is not the same language type.
> 
> But it's the C ABI type - and we do need the C ABI types from within
> the middle-end.  Joseph, any idea?

The possible issues that I see are:

* Types getting determined from strings (that are the values of macros 
such as INTPTR_TYPE) by the front ends.  This is no real problem, in that 
the Fortran front end already emulates what's required in 
get_typenode_from_name.  Of course it would be cleaner for all these 
macros to be defined to evaluate to enum values rather than strings, so 
the middle-end doesn't need such emulation of C type name parsing.  See 
the thread starting at 
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00964.html> for a conversion 
to using enum values for the stdint.h macros - that could be updated for 
current trunk.  Of course the aim would be for other macros such as 
SIZE_TYPE to be converted as well, and indeed to get a conversion to hooks 
(probably a few separate hooks for different groups of types, taking 
appropriate enum values as arguments, rather than just one for all the 
typedefs or a separate hook for every typedef).

* Targets still not having the type information in GCC.  As I said in 
<http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00248.html>, listing those 
targets, it's probably time to deprecate them (and then freely check in 
changes that break them by requiring those types to be defined).

Patch

--- gcc/tree.c.mp	2013-07-21 19:54:35.416986756 +0200
+++ gcc/tree.c	2013-07-21 19:56:58.347562787 +0200
@@ -9638,6 +9638,8 @@  build_common_tree_nodes (bool signed_cha
     = build_pointer_type (build_type_variant (void_type_node, 1, 0));
   fileptr_type_node = ptr_type_node;
 
+  pointer_sized_int_node = build_nonstandard_integer_type (POINTER_SIZE, 1);
+
   float_type_node = make_node (REAL_TYPE);
   TYPE_PRECISION (float_type_node) = FLOAT_TYPE_SIZE;
   layout_type (float_type_node);
--- gcc/ubsan.c.mp	2013-07-21 20:04:59.469653493 +0200
+++ gcc/ubsan.c	2013-07-21 20:07:00.227178083 +0200
@@ -123,14 +123,6 @@  ubsan_typedesc_new (tree type, tree decl
   return desc;
 }
 
-/* Build the ubsan uptr type.  */
-
-static tree
-uptr_type (void)
-{
-  return build_nonstandard_integer_type (POINTER_SIZE, 1);
-}
-
 /* Helper routine, which encodes a value in the uptr type.
    Arguments with precision <= POINTER_SIZE are passed directly,
    the rest is passed by reference.  T is a value we are to encode.  */
@@ -143,7 +135,7 @@  ubsan_encode_value (tree t)
     {
     case INTEGER_TYPE:
       if (TYPE_PRECISION (type) <= POINTER_SIZE)
-	return fold_build1 (NOP_EXPR, uptr_type (), t);
+	return fold_build1 (NOP_EXPR, pointer_sized_int_node, t);
       else
 	return build_fold_addr_expr (t);
     case REAL_TYPE:
@@ -153,7 +145,7 @@  ubsan_encode_value (tree t)
 	  {
 	    tree itype = build_nonstandard_integer_type (bitsize, true);
 	    t = fold_build1 (VIEW_CONVERT_EXPR, itype, t);
-	    return fold_convert (uptr_type (), t);
+	    return fold_convert (pointer_sized_int_node, t);
 	  }
 	else
 	  {
--- gcc/tree.h.mp	2013-07-21 19:54:35.441986868 +0200
+++ gcc/tree.h	2013-07-21 19:56:05.128353854 +0200
@@ -4227,6 +4227,7 @@  enum tree_index
   TI_VA_LIST_FPR_COUNTER_FIELD,
   TI_BOOLEAN_TYPE,
   TI_FILEPTR_TYPE,
+  TI_POINTER_SIZED_TYPE,
 
   TI_DFLOAT32_TYPE,
   TI_DFLOAT64_TYPE,
@@ -4383,6 +4384,7 @@  extern GTY(()) tree global_trees[TI_MAX]
 #define va_list_fpr_counter_field	global_trees[TI_VA_LIST_FPR_COUNTER_FIELD]
 /* The C type `FILE *'.  */
 #define fileptr_type_node		global_trees[TI_FILEPTR_TYPE]
+#define pointer_sized_int_node		global_trees[TI_POINTER_SIZED_TYPE]
 
 #define boolean_type_node		global_trees[TI_BOOLEAN_TYPE]
 #define boolean_false_node		global_trees[TI_BOOLEAN_FALSE]
--- gcc/asan.c.mp	2013-07-21 20:07:15.013237456 +0200
+++ gcc/asan.c	2013-07-21 20:16:10.929376734 +0200
@@ -1954,7 +1954,7 @@  asan_global_struct (void)
 	= build_decl (UNKNOWN_LOCATION, FIELD_DECL,
 		      get_identifier (field_names[i]),
 		      (i == 0 || i == 3) ? const_ptr_type_node
-		      : build_nonstandard_integer_type (POINTER_SIZE, 1));
+		      : pointer_sized_int_node);
       DECL_CONTEXT (fields[i]) = ret;
       if (i)
 	DECL_CHAIN (fields[i - 1]) = fields[i];
@@ -2039,8 +2039,7 @@  initialize_sanitizer_builtins (void)
 				ptr_type_node, ptr_type_node, NULL_TREE);
   tree BT_FN_VOID_PTR_PTRMODE
     = build_function_type_list (void_type_node, ptr_type_node,
-				build_nonstandard_integer_type (POINTER_SIZE,
-								1), NULL_TREE);
+				pointer_sized_int_node, NULL_TREE);
   tree BT_FN_VOID_INT
     = build_function_type_list (void_type_node, integer_type_node, NULL_TREE);
   tree BT_FN_BOOL_VPTR_PTR_IX_INT_INT[5];
@@ -2197,7 +2196,6 @@  asan_finish_file (void)
   if (gcount)
     {
       tree type = asan_global_struct (), var, ctor;
-      tree uptr = build_nonstandard_integer_type (POINTER_SIZE, 1);
       tree dtor_statements = NULL_TREE;
       vec<constructor_elt, va_gc> *v;
       char buf[20];
@@ -2226,15 +2224,16 @@  asan_finish_file (void)
       varpool_assemble_decl (varpool_node_for_decl (var));
 
       fn = builtin_decl_implicit (BUILT_IN_ASAN_REGISTER_GLOBALS);
+      tree gcount_tree = build_int_cst (pointer_sized_int_node, gcount);
       append_to_statement_list (build_call_expr (fn, 2,
 						 build_fold_addr_expr (var),
-						 build_int_cst (uptr, gcount)),
+						 gcount_tree),
 				&asan_ctor_statements);
 
       fn = builtin_decl_implicit (BUILT_IN_ASAN_UNREGISTER_GLOBALS);
       append_to_statement_list (build_call_expr (fn, 2,
 						 build_fold_addr_expr (var),
-						 build_int_cst (uptr, gcount)),
+						 gcount_tree),
 				&dtor_statements);
       cgraph_build_static_cdtor ('D', dtor_statements,
 				 MAX_RESERVED_INIT_PRIORITY - 1);