From: Alexander Ivchenko <aivchenk@gmail.com>
To: Ilya Enkovich <enkovich.gnu@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: Pointer Bounds Checker and trailing arrays (PR68270)
Date: Wed, 21 Dec 2016 20:27:00 -0000 [thread overview]
Message-ID: <CACysShhYhEUkcYOMpX33CtoSKELoq+GioaWEKsWq4ycYYeTEkQ@mail.gmail.com> (raw)
In-Reply-To: <CAMbmDYZVkZBPAW659Y4rhH98x9neENJYTQrU+5psYe2+xv7X3A@mail.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)
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
>> 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)
next prev parent reply other threads:[~2016-12-21 19:19 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-25 12:47 Alexander Ivchenko
2016-11-25 21:28 ` Ilya Enkovich
2016-12-20 14:55 ` Alexander Ivchenko
2016-12-21 18:03 ` Ilya Enkovich
2016-12-21 20:27 ` Alexander Ivchenko [this message]
2016-12-22 4:16 ` Ilya Enkovich
2016-12-27 13:46 ` Alexander Ivchenko
2017-01-04 10:25 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACysShhYhEUkcYOMpX33CtoSKELoq+GioaWEKsWq4ycYYeTEkQ@mail.gmail.com \
--to=aivchenk@gmail.com \
--cc=enkovich.gnu@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).