public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Fix for PR79987
@ 2017-04-04 15:07 Alexander Ivchenko
  2017-04-04 15:34 ` Jeff Law
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Ivchenko @ 2017-04-04 15:07 UTC (permalink / raw)
  To: Ilya Enkovich, marxin; +Cc: GCC Patches

Hi,

When creating static bounds for foo below we end up with:

*((unsigned long *) &__chkp_bounds_of_foo + 8) =
~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo +
18446744073709551615));

This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P
(TREE_TYPE (*expr_p)));

Is it OK?

gcc/ChangeLog:

2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>

        * tree-chkp.c (chkp_get_bounds_for_decl_addr):
assigning zero bounds to void variables


gcc/testsuite/ChangeLog:

2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>

        * gcc.target/i386/mpx/PR79987.c: New test.


diff --git a/gcc/testsuite/gcc.target/i386/mpx/PR79987.c
b/gcc/testsuite/gcc.target/i386/mpx/PR79987.c
new file mode 100644
index 0000000..14a32f3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/PR79987.c
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx" } */
+
+extern void foo;
+void *bar = &foo;
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index b1ff218..8a16025 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -3146,6 +3146,11 @@ chkp_get_bounds_for_decl_addr (tree decl)
       && !flag_chkp_incomplete_type)
       return chkp_get_zero_bounds ();

+  /* For void variables use zero bounds as well - we cannot
+     possibly get the size of the void.  */
+  if (VOID_TYPE_P (TREE_TYPE (decl)))
+      return chkp_get_zero_bounds ();
+
   if (flag_chkp_use_static_bounds
       && VAR_P (decl)
       && (TREE_STATIC (decl)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Fix for PR79987
  2017-04-04 15:07 Fix for PR79987 Alexander Ivchenko
@ 2017-04-04 15:34 ` Jeff Law
  2017-04-08 19:59   ` Ilya Enkovich
  0 siblings, 1 reply; 7+ messages in thread
From: Jeff Law @ 2017-04-04 15:34 UTC (permalink / raw)
  To: Alexander Ivchenko, Ilya Enkovich, marxin; +Cc: GCC Patches

On 04/04/2017 09:07 AM, Alexander Ivchenko wrote:
> Hi,
>
> When creating static bounds for foo below we end up with:
>
> *((unsigned long *) &__chkp_bounds_of_foo + 8) =
> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo +
> 18446744073709551615));
>
> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P
> (TREE_TYPE (*expr_p)));
>
> Is it OK?
>
> gcc/ChangeLog:
>
> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>
>         * tree-chkp.c (chkp_get_bounds_for_decl_addr):
> assigning zero bounds to void variables
>
>
> gcc/testsuite/ChangeLog:
>
> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>
>         * gcc.target/i386/mpx/PR79987.c: New test.
I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT 
it's not a regression.

Jeff

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Fix for PR79987
  2017-04-04 15:34 ` Jeff Law
@ 2017-04-08 19:59   ` Ilya Enkovich
  2017-05-09 15:41     ` Alexander Ivchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Ilya Enkovich @ 2017-04-08 19:59 UTC (permalink / raw)
  To: Jeff Law; +Cc: Alexander Ivchenko, marxin, GCC Patches

2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>:
> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote:
>>
>> Hi,
>>
>> When creating static bounds for foo below we end up with:
>>
>> *((unsigned long *) &__chkp_bounds_of_foo + 8) =
>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo +
>> 18446744073709551615));
>>
>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P
>> (TREE_TYPE (*expr_p)));
>>
>> Is it OK?
>>
>> gcc/ChangeLog:
>>
>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>
>>         * tree-chkp.c (chkp_get_bounds_for_decl_addr):
>> assigning zero bounds to void variables
>>
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>
>>         * gcc.target/i386/mpx/PR79987.c: New test.
>
> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's
> not a regression.
>
> Jeff
>

Hi,

If we delay it for GCC8 anyway then I think we may fix it in a better way. If we
cannot detect size of a variable then size relocations may be used. It is done
already for arrays with unknown size and also can be done for void vars.

Thanks,
Ilya

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Fix for PR79987
  2017-04-08 19:59   ` Ilya Enkovich
@ 2017-05-09 15:41     ` Alexander Ivchenko
       [not found]       ` <CAMbmDYYj++Trk1JHs0dXBR4NfLCaKNnzLHN1PBF7a_96YCJGWg@mail.gmail.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Ivchenko @ 2017-05-09 15:41 UTC (permalink / raw)
  To: Ilya Enkovich; +Cc: Jeff Law, marxin, GCC Patches

If we use chkp_generate_extern_var_bounds for void variable just as
for arrays with unknown size, we will create the following gimple seq:

# VUSE <.MEM>
__size_tmp.0 = __builtin_ia32_sizeof (foo);
__size_tmp.1_3 = __size_tmp.0;

However, this will fail in verify_gimple_call:

tree arg = gimple_call_arg (stmt, i);
if ((is_gimple_reg_type (TREE_TYPE (arg))
     && !is_gimple_val (arg))
    || (!is_gimple_reg_type (TREE_TYPE (arg))
        && !is_gimple_lvalue (arg)))
  {
    error ("invalid argument to gimple call");
    debug_generic_expr (arg);
    return true;
  }
..here the TREE_TYPE(arg)==void. Any ideas for a good workaround ?

Alexander



2017-04-08 21:59 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
> 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>:
>> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote:
>>>
>>> Hi,
>>>
>>> When creating static bounds for foo below we end up with:
>>>
>>> *((unsigned long *) &__chkp_bounds_of_foo + 8) =
>>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo +
>>> 18446744073709551615));
>>>
>>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P
>>> (TREE_TYPE (*expr_p)));
>>>
>>> Is it OK?
>>>
>>> gcc/ChangeLog:
>>>
>>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>>
>>>         * tree-chkp.c (chkp_get_bounds_for_decl_addr):
>>> assigning zero bounds to void variables
>>>
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>>
>>>         * gcc.target/i386/mpx/PR79987.c: New test.
>>
>> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's
>> not a regression.
>>
>> Jeff
>>
>
> Hi,
>
> If we delay it for GCC8 anyway then I think we may fix it in a better way. If we
> cannot detect size of a variable then size relocations may be used. It is done
> already for arrays with unknown size and also can be done for void vars.
>
> Thanks,
> Ilya

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Fix for PR79987
       [not found]             ` <CACysShip_WNDryk1kUiJCO0Mpc_8SVNRkByTdmzEC0xfxFuviw@mail.gmail.com>
@ 2017-05-11 18:41               ` Ilya Enkovich
  2017-06-09 12:22                 ` Alexander Ivchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Ilya Enkovich @ 2017-05-11 18:41 UTC (permalink / raw)
  To: Alexander Ivchenko, gcc-patches

2017-05-11 0:05 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
> Didn't quite get your point. I though that the idea is to hit this code:
>
>       bounds = chkp_generate_extern_var_bounds (decl);
> ... to get the builtin_sizeof call on this variable and hence to get
> the size relocation. What exactly do you mean by reproducing the
> problem? Currently the compiler just ICE on the testcase :)

There are various ways to build bounds. On of them is used by default
and causes ICE. With your patch another way is chosen causing another
ICE. But you can get this another ICE by simply using compiler flag.
Obviously both ways should be fixed.

You don't have to hit chkp_generate_extern_var_bounds to use size
relocation. If you create static bounds var then it is initialized later
in chkp_output_static_bounds and size relocations can appear there
either.

Thanks
Ilya

>
> Alexander
>
> 2017-05-10 22:51 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>> 2017-05-10 22:08 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>> Hi,
>>>
>>> something like that:
>>>
>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>>> index b1ff218..be06d71 100644
>>> --- a/gcc/tree-chkp.c
>>> +++ b/gcc/tree-chkp.c
>>> @@ -3148,6 +3148,7 @@ chkp_get_bounds_for_decl_addr (tree decl)
>>>
>>>    if (flag_chkp_use_static_bounds
>>>        && VAR_P (decl)
>>> +      && !VOID_TYPE_P (TREE_TYPE (decl))
>>>        && (TREE_STATIC (decl)
>>>               || DECL_EXTERNAL (decl)
>>>               || TREE_PUBLIC (decl))
>>> @@ -3162,10 +3163,11 @@ chkp_get_bounds_for_decl_addr (tree decl)
>>>        gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
>>>      }
>>>    else if (!DECL_SIZE (decl)
>>> -      || (chkp_variable_size_type (TREE_TYPE (decl))
>>> -         && (TREE_STATIC (decl)
>>> -             || DECL_EXTERNAL (decl)
>>> -             || TREE_PUBLIC (decl))))
>>> +          || VOID_TYPE_P (TREE_TYPE (decl))
>>> +          || (chkp_variable_size_type (TREE_TYPE (decl))
>>> +              && (TREE_STATIC (decl)
>>> +                  || DECL_EXTERNAL (decl)
>>> +                  || TREE_PUBLIC (decl))))
>>>      {
>>>        gcc_assert (VAR_P (decl));
>>>        bounds = chkp_generate_extern_var_bounds (decl);
>>
>> Looking one more time into this issue I now think that bounds generation
>> already uses size relocation and this is not a part to fix.
>>
>> BTW this patch just restricts static bounds creation for void vars which
>> is not what you need. I think you should be able to reproduce the problem
>> without any patch by just using -fno-chkp-use-static-bounds.
>>
>> Looks like gimple doesn't allow void vars as call arguments. I don't know
>> what is the best way to handle this restriction. The simplest thing a can
>> think of is to change builtin call so it gets address of a decl instead of
>> a decl.
>>
>> Thanks,
>> Ilya
>>
>>>
>>> Thanks,
>>> Alexander
>>>
>>> 2017-05-10 20:55 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>> Hi,
>>>>
>>>> Please share a patch you use.
>>>>
>>>> Thanks,
>>>> Ilya
>>>>
>>>> 2017-05-09 18:39 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>>>> If we use chkp_generate_extern_var_bounds for void variable just as
>>>>> for arrays with unknown size, we will create the following gimple seq:
>>>>>
>>>>> # VUSE <.MEM>
>>>>> __size_tmp.0 = __builtin_ia32_sizeof (foo);
>>>>> __size_tmp.1_3 = __size_tmp.0;
>>>>>
>>>>> However, this will fail in verify_gimple_call:
>>>>>
>>>>> tree arg = gimple_call_arg (stmt, i);
>>>>> if ((is_gimple_reg_type (TREE_TYPE (arg))
>>>>>      && !is_gimple_val (arg))
>>>>>     || (!is_gimple_reg_type (TREE_TYPE (arg))
>>>>>         && !is_gimple_lvalue (arg)))
>>>>>   {
>>>>>     error ("invalid argument to gimple call");
>>>>>     debug_generic_expr (arg);
>>>>>     return true;
>>>>>   }
>>>>> ..here the TREE_TYPE(arg)==void. Any ideas for a good workaround ?
>>>>>
>>>>> Alexander
>>>>>
>>>>>
>>>>>
>>>>> 2017-04-08 21:59 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>>>> 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>:
>>>>>>> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> When creating static bounds for foo below we end up with:
>>>>>>>>
>>>>>>>> *((unsigned long *) &__chkp_bounds_of_foo + 8) =
>>>>>>>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo +
>>>>>>>> 18446744073709551615));
>>>>>>>>
>>>>>>>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P
>>>>>>>> (TREE_TYPE (*expr_p)));
>>>>>>>>
>>>>>>>> Is it OK?
>>>>>>>>
>>>>>>>> gcc/ChangeLog:
>>>>>>>>
>>>>>>>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>>>>>>>
>>>>>>>>         * tree-chkp.c (chkp_get_bounds_for_decl_addr):
>>>>>>>> assigning zero bounds to void variables
>>>>>>>>
>>>>>>>>
>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>
>>>>>>>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>>>>>>>
>>>>>>>>         * gcc.target/i386/mpx/PR79987.c: New test.
>>>>>>>
>>>>>>> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's
>>>>>>> not a regression.
>>>>>>>
>>>>>>> Jeff
>>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> If we delay it for GCC8 anyway then I think we may fix it in a better way. If we
>>>>>> cannot detect size of a variable then size relocations may be used. It is done
>>>>>> already for arrays with unknown size and also can be done for void vars.
>>>>>>
>>>>>> Thanks,
>>>>>> Ilya

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Fix for PR79987
  2017-05-11 18:41               ` Ilya Enkovich
@ 2017-06-09 12:22                 ` Alexander Ivchenko
  2017-06-09 17:46                   ` Ilya Enkovich
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Ivchenko @ 2017-06-09 12:22 UTC (permalink / raw)
  To: Ilya Enkovich; +Cc: gcc-patches

Hi Ilya,

I tried changing builtin call so it gets address of a decl instead of
a decl, but it looked very unnatural and I hit some other problems
implementing that. Keeping in mind the exposure of this problem, I
think it is not worth it. I propose to reconsider the first and the
simplest patch in this thread: just returning zero bounds for void
vars. The standard does not specify that size anyways. What do you
think?

Alexander



2017-05-11 20:37 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
> 2017-05-11 0:05 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>> Didn't quite get your point. I though that the idea is to hit this code:
>>
>>       bounds = chkp_generate_extern_var_bounds (decl);
>> ... to get the builtin_sizeof call on this variable and hence to get
>> the size relocation. What exactly do you mean by reproducing the
>> problem? Currently the compiler just ICE on the testcase :)
>
> There are various ways to build bounds. On of them is used by default
> and causes ICE. With your patch another way is chosen causing another
> ICE. But you can get this another ICE by simply using compiler flag.
> Obviously both ways should be fixed.
>
> You don't have to hit chkp_generate_extern_var_bounds to use size
> relocation. If you create static bounds var then it is initialized later
> in chkp_output_static_bounds and size relocations can appear there
> either.
>
> Thanks
> Ilya
>
>>
>> Alexander
>>
>> 2017-05-10 22:51 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>> 2017-05-10 22:08 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>>> Hi,
>>>>
>>>> something like that:
>>>>
>>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>>>> index b1ff218..be06d71 100644
>>>> --- a/gcc/tree-chkp.c
>>>> +++ b/gcc/tree-chkp.c
>>>> @@ -3148,6 +3148,7 @@ chkp_get_bounds_for_decl_addr (tree decl)
>>>>
>>>>    if (flag_chkp_use_static_bounds
>>>>        && VAR_P (decl)
>>>> +      && !VOID_TYPE_P (TREE_TYPE (decl))
>>>>        && (TREE_STATIC (decl)
>>>>               || DECL_EXTERNAL (decl)
>>>>               || TREE_PUBLIC (decl))
>>>> @@ -3162,10 +3163,11 @@ chkp_get_bounds_for_decl_addr (tree decl)
>>>>        gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
>>>>      }
>>>>    else if (!DECL_SIZE (decl)
>>>> -      || (chkp_variable_size_type (TREE_TYPE (decl))
>>>> -         && (TREE_STATIC (decl)
>>>> -             || DECL_EXTERNAL (decl)
>>>> -             || TREE_PUBLIC (decl))))
>>>> +          || VOID_TYPE_P (TREE_TYPE (decl))
>>>> +          || (chkp_variable_size_type (TREE_TYPE (decl))
>>>> +              && (TREE_STATIC (decl)
>>>> +                  || DECL_EXTERNAL (decl)
>>>> +                  || TREE_PUBLIC (decl))))
>>>>      {
>>>>        gcc_assert (VAR_P (decl));
>>>>        bounds = chkp_generate_extern_var_bounds (decl);
>>>
>>> Looking one more time into this issue I now think that bounds generation
>>> already uses size relocation and this is not a part to fix.
>>>
>>> BTW this patch just restricts static bounds creation for void vars which
>>> is not what you need. I think you should be able to reproduce the problem
>>> without any patch by just using -fno-chkp-use-static-bounds.
>>>
>>> Looks like gimple doesn't allow void vars as call arguments. I don't know
>>> what is the best way to handle this restriction. The simplest thing a can
>>> think of is to change builtin call so it gets address of a decl instead of
>>> a decl.
>>>
>>> Thanks,
>>> Ilya
>>>
>>>>
>>>> Thanks,
>>>> Alexander
>>>>
>>>> 2017-05-10 20:55 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>>> Hi,
>>>>>
>>>>> Please share a patch you use.
>>>>>
>>>>> Thanks,
>>>>> Ilya
>>>>>
>>>>> 2017-05-09 18:39 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>>>>> If we use chkp_generate_extern_var_bounds for void variable just as
>>>>>> for arrays with unknown size, we will create the following gimple seq:
>>>>>>
>>>>>> # VUSE <.MEM>
>>>>>> __size_tmp.0 = __builtin_ia32_sizeof (foo);
>>>>>> __size_tmp.1_3 = __size_tmp.0;
>>>>>>
>>>>>> However, this will fail in verify_gimple_call:
>>>>>>
>>>>>> tree arg = gimple_call_arg (stmt, i);
>>>>>> if ((is_gimple_reg_type (TREE_TYPE (arg))
>>>>>>      && !is_gimple_val (arg))
>>>>>>     || (!is_gimple_reg_type (TREE_TYPE (arg))
>>>>>>         && !is_gimple_lvalue (arg)))
>>>>>>   {
>>>>>>     error ("invalid argument to gimple call");
>>>>>>     debug_generic_expr (arg);
>>>>>>     return true;
>>>>>>   }
>>>>>> ..here the TREE_TYPE(arg)==void. Any ideas for a good workaround ?
>>>>>>
>>>>>> Alexander
>>>>>>
>>>>>>
>>>>>>
>>>>>> 2017-04-08 21:59 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>>>>> 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>:
>>>>>>>> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> When creating static bounds for foo below we end up with:
>>>>>>>>>
>>>>>>>>> *((unsigned long *) &__chkp_bounds_of_foo + 8) =
>>>>>>>>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo +
>>>>>>>>> 18446744073709551615));
>>>>>>>>>
>>>>>>>>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P
>>>>>>>>> (TREE_TYPE (*expr_p)));
>>>>>>>>>
>>>>>>>>> Is it OK?
>>>>>>>>>
>>>>>>>>> gcc/ChangeLog:
>>>>>>>>>
>>>>>>>>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>>>>>>>>
>>>>>>>>>         * tree-chkp.c (chkp_get_bounds_for_decl_addr):
>>>>>>>>> assigning zero bounds to void variables
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>>
>>>>>>>>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>>>>>>>>
>>>>>>>>>         * gcc.target/i386/mpx/PR79987.c: New test.
>>>>>>>>
>>>>>>>> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's
>>>>>>>> not a regression.
>>>>>>>>
>>>>>>>> Jeff
>>>>>>>>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> If we delay it for GCC8 anyway then I think we may fix it in a better way. If we
>>>>>>> cannot detect size of a variable then size relocations may be used. It is done
>>>>>>> already for arrays with unknown size and also can be done for void vars.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ilya

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Fix for PR79987
  2017-06-09 12:22                 ` Alexander Ivchenko
@ 2017-06-09 17:46                   ` Ilya Enkovich
  0 siblings, 0 replies; 7+ messages in thread
From: Ilya Enkovich @ 2017-06-09 17:46 UTC (permalink / raw)
  To: Alexander Ivchenko; +Cc: gcc-patches

2017-06-09 15:21 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
> Hi Ilya,
>
> I tried changing builtin call so it gets address of a decl instead of
> a decl, but it looked very unnatural and I hit some other problems

What is unnatural in passing a pointer to a function to get its bounds?
Please show your patch and describe problems you hit.

> implementing that. Keeping in mind the exposure of this problem, I
> think it is not worth it. I propose to reconsider the first and the
> simplest patch in this thread: just returning zero bounds for void
> vars. The standard does not specify that size anyways. What do you
> think?

I think declaring void var is similar to declaring static array with no
size and we should handle these cases similarly.

Thanks,
Ilya

>
> Alexander
>
>
>
> 2017-05-11 20:37 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>> 2017-05-11 0:05 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>> Didn't quite get your point. I though that the idea is to hit this code:
>>>
>>>       bounds = chkp_generate_extern_var_bounds (decl);
>>> ... to get the builtin_sizeof call on this variable and hence to get
>>> the size relocation. What exactly do you mean by reproducing the
>>> problem? Currently the compiler just ICE on the testcase :)
>>
>> There are various ways to build bounds. On of them is used by default
>> and causes ICE. With your patch another way is chosen causing another
>> ICE. But you can get this another ICE by simply using compiler flag.
>> Obviously both ways should be fixed.
>>
>> You don't have to hit chkp_generate_extern_var_bounds to use size
>> relocation. If you create static bounds var then it is initialized later
>> in chkp_output_static_bounds and size relocations can appear there
>> either.
>>
>> Thanks
>> Ilya
>>
>>>
>>> Alexander
>>>
>>> 2017-05-10 22:51 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>> 2017-05-10 22:08 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>>>> Hi,
>>>>>
>>>>> something like that:
>>>>>
>>>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>>>>> index b1ff218..be06d71 100644
>>>>> --- a/gcc/tree-chkp.c
>>>>> +++ b/gcc/tree-chkp.c
>>>>> @@ -3148,6 +3148,7 @@ chkp_get_bounds_for_decl_addr (tree decl)
>>>>>
>>>>>    if (flag_chkp_use_static_bounds
>>>>>        && VAR_P (decl)
>>>>> +      && !VOID_TYPE_P (TREE_TYPE (decl))
>>>>>        && (TREE_STATIC (decl)
>>>>>               || DECL_EXTERNAL (decl)
>>>>>               || TREE_PUBLIC (decl))
>>>>> @@ -3162,10 +3163,11 @@ chkp_get_bounds_for_decl_addr (tree decl)
>>>>>        gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
>>>>>      }
>>>>>    else if (!DECL_SIZE (decl)
>>>>> -      || (chkp_variable_size_type (TREE_TYPE (decl))
>>>>> -         && (TREE_STATIC (decl)
>>>>> -             || DECL_EXTERNAL (decl)
>>>>> -             || TREE_PUBLIC (decl))))
>>>>> +          || VOID_TYPE_P (TREE_TYPE (decl))
>>>>> +          || (chkp_variable_size_type (TREE_TYPE (decl))
>>>>> +              && (TREE_STATIC (decl)
>>>>> +                  || DECL_EXTERNAL (decl)
>>>>> +                  || TREE_PUBLIC (decl))))
>>>>>      {
>>>>>        gcc_assert (VAR_P (decl));
>>>>>        bounds = chkp_generate_extern_var_bounds (decl);
>>>>
>>>> Looking one more time into this issue I now think that bounds generation
>>>> already uses size relocation and this is not a part to fix.
>>>>
>>>> BTW this patch just restricts static bounds creation for void vars which
>>>> is not what you need. I think you should be able to reproduce the problem
>>>> without any patch by just using -fno-chkp-use-static-bounds.
>>>>
>>>> Looks like gimple doesn't allow void vars as call arguments. I don't know
>>>> what is the best way to handle this restriction. The simplest thing a can
>>>> think of is to change builtin call so it gets address of a decl instead of
>>>> a decl.
>>>>
>>>> Thanks,
>>>> Ilya
>>>>
>>>>>
>>>>> Thanks,
>>>>> Alexander
>>>>>
>>>>> 2017-05-10 20:55 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>>>> Hi,
>>>>>>
>>>>>> Please share a patch you use.
>>>>>>
>>>>>> Thanks,
>>>>>> Ilya
>>>>>>
>>>>>> 2017-05-09 18:39 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>>>>>> If we use chkp_generate_extern_var_bounds for void variable just as
>>>>>>> for arrays with unknown size, we will create the following gimple seq:
>>>>>>>
>>>>>>> # VUSE <.MEM>
>>>>>>> __size_tmp.0 = __builtin_ia32_sizeof (foo);
>>>>>>> __size_tmp.1_3 = __size_tmp.0;
>>>>>>>
>>>>>>> However, this will fail in verify_gimple_call:
>>>>>>>
>>>>>>> tree arg = gimple_call_arg (stmt, i);
>>>>>>> if ((is_gimple_reg_type (TREE_TYPE (arg))
>>>>>>>      && !is_gimple_val (arg))
>>>>>>>     || (!is_gimple_reg_type (TREE_TYPE (arg))
>>>>>>>         && !is_gimple_lvalue (arg)))
>>>>>>>   {
>>>>>>>     error ("invalid argument to gimple call");
>>>>>>>     debug_generic_expr (arg);
>>>>>>>     return true;
>>>>>>>   }
>>>>>>> ..here the TREE_TYPE(arg)==void. Any ideas for a good workaround ?
>>>>>>>
>>>>>>> Alexander
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 2017-04-08 21:59 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>>>>>> 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>:
>>>>>>>>> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote:
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> When creating static bounds for foo below we end up with:
>>>>>>>>>>
>>>>>>>>>> *((unsigned long *) &__chkp_bounds_of_foo + 8) =
>>>>>>>>>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo +
>>>>>>>>>> 18446744073709551615));
>>>>>>>>>>
>>>>>>>>>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P
>>>>>>>>>> (TREE_TYPE (*expr_p)));
>>>>>>>>>>
>>>>>>>>>> Is it OK?
>>>>>>>>>>
>>>>>>>>>> gcc/ChangeLog:
>>>>>>>>>>
>>>>>>>>>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>>>>>>>>>
>>>>>>>>>>         * tree-chkp.c (chkp_get_bounds_for_decl_addr):
>>>>>>>>>> assigning zero bounds to void variables
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>>>
>>>>>>>>>> 2017-04-04  Alexander Ivchenko  <aivchenk@gmail.com>
>>>>>>>>>>
>>>>>>>>>>         * gcc.target/i386/mpx/PR79987.c: New test.
>>>>>>>>>
>>>>>>>>> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's
>>>>>>>>> not a regression.
>>>>>>>>>
>>>>>>>>> Jeff
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> If we delay it for GCC8 anyway then I think we may fix it in a better way. If we
>>>>>>>> cannot detect size of a variable then size relocations may be used. It is done
>>>>>>>> already for arrays with unknown size and also can be done for void vars.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Ilya

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-06-09 17:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04 15:07 Fix for PR79987 Alexander Ivchenko
2017-04-04 15:34 ` Jeff Law
2017-04-08 19:59   ` Ilya Enkovich
2017-05-09 15:41     ` Alexander Ivchenko
     [not found]       ` <CAMbmDYYj++Trk1JHs0dXBR4NfLCaKNnzLHN1PBF7a_96YCJGWg@mail.gmail.com>
     [not found]         ` <CACysShjZboYogD31Ujgt-xpE3jK2co1nUpEzxbbrSWbOV_Z4Tg@mail.gmail.com>
     [not found]           ` <CAMbmDYbHjZEXz3YcTvPQa950sxmR3fa+yYsCPkzhychFtjW4Cw@mail.gmail.com>
     [not found]             ` <CACysShip_WNDryk1kUiJCO0Mpc_8SVNRkByTdmzEC0xfxFuviw@mail.gmail.com>
2017-05-11 18:41               ` Ilya Enkovich
2017-06-09 12:22                 ` Alexander Ivchenko
2017-06-09 17:46                   ` Ilya Enkovich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).