Re: Pointer Bounds Checker and trailing arrays (PR68270)

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Dec 22, 2016 at 12:47 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2016-12-21 22:18 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>> Right.. here is this updated chunk (otherwise no difference in the patch)
>>
>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>> index 2769682..6c7862c 100644
>> --- a/gcc/tree-chkp.c
>> +++ b/gcc/tree-chkp.c
>> @@ -3272,6 +3272,9 @@ chkp_may_narrow_to_field (tree field)
>>  {
>>    return DECL_SIZE (field) && TREE_CODE (DECL_SIZE (field)) == INTEGER_CST
>>      && tree_to_uhwi (DECL_SIZE (field)) != 0
>> +    && !(flag_chkp_flexible_struct_trailing_arrays
>> + && TREE_CODE(TREE_TYPE(field)) == ARRAY_TYPE
>> + && !DECL_CHAIN (field))
>>      && (!DECL_FIELD_OFFSET (field)
>>   || TREE_CODE (DECL_FIELD_OFFSET (field)) == INTEGER_CST)
>>      && (!DECL_FIELD_BIT_OFFSET (field)
>
> OK.

There is array_at_struct_end_p which has a better implementation for
this (the above
considers a TYPE_DECL after the array a field).

Richard.

>>
>> 2016-12-21 21:00 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>> 2016-12-20 17:44 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>>> 2016-11-26 0:28 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>>> 2016-11-25 15:47 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>>>>> Hi,
>>>>>>
>>>>>> The patch below addresses PR68270. could you please take a look?
>>>>>>
>>>>>> 2016-11-25  Alexander Ivchenko  <aivchenk@gmail.com>
>>>>>>
>>>>>>        * c-family/c.opt (flag_chkp_flexible_struct_trailing_arrays):
>>>>>>        Add new option.
>>>>>>        * tree-chkp.c (chkp_parse_array_and_component_ref): Forbid
>>>>>>        narrowing when chkp_parse_array_and_component_ref is used and
>>>>>>        the ARRAY_REF points to an array in the end of the struct.
>>>>>>
>>>>>>
>>>>>>
>>>>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
>>>>>> index 7d8a726..e45d6a2 100644
>>>>>> --- a/gcc/c-family/c.opt
>>>>>> +++ b/gcc/c-family/c.opt
>>>>>> @@ -1166,6 +1166,11 @@ C ObjC C++ ObjC++ LTO RejectNegative Report
>>>>>> Var(flag_chkp_narrow_to_innermost_ar
>>>>>>  Forces Pointer Bounds Checker to use bounds of the innermost arrays in case of
>>>>>>  nested static arryas access.  By default outermost array is used.
>>>>>>
>>>>>> +fchkp-flexible-struct-trailing-arrays
>>>>>> +C ObjC C++ ObjC++ LTO RejectNegative Report
>>>>>> Var(flag_chkp_flexible_struct_trailing_arrays)
>>>>>> +Allow Pointer Bounds Checker to treat all trailing arrays in structures as
>>>>>> +possibly flexible.
>>>>>
>>>>> Words 'allow' and 'possibly' are confusing here. This option is about to force
>>>>> checker to do something, not to give him a choice.
>>>>
>>>> Fixed
>>>>
>>>>> New option has to be documented in invoke.texi. It would also be nice to reflect
>>>>> changes on GCC MPX wiki page.
>>>>
>>>> Done
>>>>> We have an attribute to change compiler behavior when this option is not set.
>>>>> But we have no way to make exceptions when this option is used. Should we
>>>>> add one?
>>>> Something like "bnd_fixed_size" ? Could work. Although the customer
>>>> request did not mention the need for that.
>>>> Can I add it in a separate patch?
>>>>
>>>
>>> Yes.
>>>
>>>>
>>>>>> +
>>>>>>  fchkp-optimize
>>>>>>  C ObjC C++ ObjC++ LTO Report Var(flag_chkp_optimize) Init(-1)
>>>>>>  Allow Pointer Bounds Checker optimizations.  By default allowed
>>>>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>>>>>> index 2769682..40f99c3 100644
>>>>>> --- a/gcc/tree-chkp.c
>>>>>> +++ b/gcc/tree-chkp.c
>>>>>> @@ -3425,7 +3425,9 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
>>>>>>    if (flag_chkp_narrow_bounds
>>>>>>        && !flag_chkp_narrow_to_innermost_arrray
>>>>>>        && (!last_comp
>>>>>> -  || chkp_may_narrow_to_field (TREE_OPERAND (last_comp, 1))))
>>>>>> +  || (chkp_may_narrow_to_field (TREE_OPERAND (last_comp, 1))
>>>>>> +      && !(flag_chkp_flexible_struct_trailing_arrays
>>>>>> +   && array_at_struct_end_p (var)))))
>>>>>
>>>>> This is incorrect place for fix. Consider code
>>>>>
>>>>> struct S {
>>>>>   int a;
>>>>>   int b[10];
>>>>> };
>>>>>
>>>>> struct S s;
>>>>> int *p = s.b;
>>>>>
>>>>> Here you need to compute bounds for p and you want your option to take effect
>>>>> but in this case you won't event reach your new check because there is no
>>>>> ARRAY_REF. And even if we change it to
>>>>>
>>>>> int *p = s.b[5];
>>>>>
>>>>> then it still would be narrowed because s.b would still be written
>>>>> into 'comp_to_narrow'
>>>>> variable. Correct place for fix is in chkp_may_narrow_to_field.
>>>>
>>>> Done
>>>>
>>>>> Also you should consider fchkp-narrow-to-innermost-arrray option. Should it
>>>>> be more powerfull or not? I think fchkp-narrow-to-innermost-arrray shouldn't
>>>>> narrow to variable sized fields. BTW looks like right now bnd_variable_size
>>>>> attribute is ignored by fchkp-narrow-to-innermost-arrray. This is another
>>>>> problem and may be fixed in another patch though.
>>>> The way code works in chkp_parse_array_and_component_ref seems to be
>>>> exactly like you say:  fchkp-narrow-to-innermost-arrray won't narrow
>>>> to variable sized fields. I will create a separate bug for
>>>> bnd_variable_size+ fchkp-narrow-to-innermost-arrray.
>>>>
>>>>> Also patch lacks tests for various situations (with option and without, with
>>>>> ARRAY_REF and without). In case of new attribute and fix for
>>>>> fchkp-narrow-to-innermost-arrray behavior additional tests are required.
>>>> I added three tests for flag_chkp_flexible_struct_trailing_arrays
>>>>
>>>>
>>>>
>>>> The patch is below:
>>>>
>>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
>>>> index 2d47d54..21ad6aa 100644
>>>> --- a/gcc/c-family/c.opt
>>>> +++ b/gcc/c-family/c.opt
>>>> @@ -1188,7 +1188,13 @@ narrowing is on, field bounds are used.
>>>> Otherwise full object bounds are used.
>>>>  fchkp-narrow-to-innermost-array
>>>>  C ObjC C++ ObjC++ LTO RejectNegative Report
>>>> Var(flag_chkp_narrow_to_innermost_arrray)
>>>>  Forces Pointer Bounds Checker to use bounds of the innermost arrays in case of
>>>> -nested static arryas access.  By default outermost array is used.
>>>> +nested static arrays access.  By default outermost array is used.
>>>> +
>>>> +fchkp-flexible-struct-trailing-arrays
>>>> +C ObjC C++ ObjC++ LTO RejectNegative Report
>
> I also noticed this part conflicts with documentation which describes
> both positive and negative flag versions. I think we should allow
> negative version.
>
> Patch is OK with that change and proper testing.
>
> Thanks,
> Ilya
>
>>>> Var(flag_chkp_flexible_struct_trailing_arrays)
>>>> +Forces Pointer Bounds Checker to treat all trailing arrays in structures as
>>>> +possibly flexible.  By default only arrays fields with zero length or that are
>>>> +marked with attribute bnd_variable_size are treated as flexible.
>>>>
>>>>  fchkp-optimize
>>>>  C ObjC C++ ObjC++ LTO Report Var(flag_chkp_optimize) Init(-1)
>>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>>>> index 034ae98..2372c22a 100644
>>>> --- a/gcc/doc/invoke.texi
>>>> +++ b/gcc/doc/invoke.texi
>>>> @@ -10886,6 +10886,13 @@ Forces Pointer Bounds Checker to use narrowed
>>>> bounds for the address of the
>>>>  first field in the structure.  By default a pointer to the first field has
>>>>  the same bounds as a pointer to the whole structure.
>>>>
>>>> +@item -fchkp-flexible-struct-trailing-arrays
>>>> +@opindex fchkp-flexible-struct-trailing-arrays
>>>> +@opindex fno-chkp-flexible-struct-trailing-arrays
>>>> +Forces Pointer Bounds Checker to treat all trailing arrays in structures as
>>>> +possibly flexible.  By default only arrays fields with zero length or that are
>>>> +marked with attribute bnd_variable_size are treated as flexible.
>>>> +
>>>>  @item -fchkp-narrow-to-innermost-array
>>>>  @opindex fchkp-narrow-to-innermost-array
>>>>  @opindex fno-chkp-narrow-to-innermost-array
>>>> diff --git a/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-lbv.c
>>>> b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-lbv.c
>>>> new file mode 100644
>>>> index 0000000..9739920
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-lbv.c
>>>> @@ -0,0 +1,29 @@
>>>> +/* { dg-do run } */
>>>> +/* { dg-shouldfail "bounds violation" } */
>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx
>>>> -fchkp-flexible-struct-trailing-arrays" } */
>>>> +
>>>> +
>>>> +#define SHOULDFAIL
>>>> +
>>>> +#include "mpx-check.h"
>>>> +
>>>> +struct S
>>>> +{
>>>> +  int a;
>>>> +  int p[10];
>>>> +};
>>>> +
>>>> +int rd (int *p, int i)
>>>> +{
>>>> +  int res = p[i];
>>>> +  printf ("%d\n", res);
>>>> +  return res;
>>>> +}
>>>> +
>>>> +int mpx_test (int argc, const char **argv)
>>>> +{
>>>> +  struct S *s = (struct S *)alloca (sizeof(struct S) + sizeof (int)*100);
>>>> +  rd (s->p, -2);
>>>> +
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-nov.c
>>>> b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-nov.c
>>>> new file mode 100644
>>>> index 0000000..f5c8f95
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-nov.c
>>>> @@ -0,0 +1,29 @@
>>>> +/* { dg-do run } */
>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx
>>>> -fchkp-flexible-struct-trailing-arrays" } */
>>>> +
>>>> +
>>>> +#include "mpx-check.h"
>>>> +
>>>> +struct S
>>>> +{
>>>> +  int a;
>>>> +  int p[10];
>>>> +};
>>>> +
>>>> +int rd (int *p, int i)
>>>> +{
>>>> +  int res = p[i];
>>>> +  printf ("%d\n", res);
>>>> +  return res;
>>>> +}
>>>> +
>>>> +int mpx_test (int argc, const char **argv)
>>>> +{
>>>> +  struct S *s = (struct S *)alloca (sizeof(struct S) + sizeof (int)*100);
>>>> +  rd (s->p, 0);
>>>> +  rd (s->p, 99);
>>>> +  s->p[0];
>>>> +  s->p[99];
>>>> +
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-ubv.c
>>>> b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-ubv.c
>>>> new file mode 100644
>>>> index 0000000..8385a5a
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-ubv.c
>>>> @@ -0,0 +1,29 @@
>>>> +/* { dg-do run } */
>>>> +/* { dg-shouldfail "bounds violation" } */
>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx
>>>> -fchkp-flexible-struct-trailing-arrays" } */
>>>> +
>>>> +
>>>> +#define SHOULDFAIL
>>>> +
>>>> +#include "mpx-check.h"
>>>> +
>>>> +struct S
>>>> +{
>>>> +  int a;
>>>> +  int p[10];
>>>> +};
>>>> +
>>>> +int rd (int *p, int i)
>>>> +{
>>>> +  int res = p[i];
>>>> +  printf ("%d\n", res);
>>>> +  return res;
>>>> +}
>>>> +
>>>> +int mpx_test (int argc, const char **argv)
>>>> +{
>>>> +  struct S *s = (struct S *)alloca (sizeof(struct S) + sizeof (int)*100);
>>>> +  rd (s->p, 110);
>>>> +
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>>>> index 2769682..6c5e541 100644
>>>> --- a/gcc/tree-chkp.c
>>>> +++ b/gcc/tree-chkp.c
>>>> @@ -3272,6 +3272,8 @@ chkp_may_narrow_to_field (tree field)
>>>>  {
>>>>    return DECL_SIZE (field) && TREE_CODE (DECL_SIZE (field)) == INTEGER_CST
>>>>      && tree_to_uhwi (DECL_SIZE (field)) != 0
>>>> +    && (!flag_chkp_flexible_struct_trailing_arrays
>>>> + || DECL_CHAIN (field))
>>>
>>> What if it's not array?
>>>
>>> Thanks,
>>> Ilya
>>>
>>>>      && (!DECL_FIELD_OFFSET (field)
>>>>   || TREE_CODE (DECL_FIELD_OFFSET (field)) == INTEGER_CST)
>>>>      && (!DECL_FIELD_BIT_OFFSET (field)