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.
>
> 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)