| Message ID | CACysShhYhEUkcYOMpX33CtoSKELoq+GioaWEKsWq4ycYYeTEkQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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)
Committed as r243936. Thank you, Alexander 2016-12-22 2:47 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > 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)
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)
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)