Re: Fix for PR79987

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

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