From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125521 invoked by alias); 27 Dec 2016 13:43:38 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 125504 invoked by uid 89); 27 Dec 2016 13:43:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=aivchenk@gmail.com, aivchenkgmailcom, sb, enkovich.gnu@gmail.com X-HELO: mail-qt0-f170.google.com Received: from mail-qt0-f170.google.com (HELO mail-qt0-f170.google.com) (209.85.216.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 27 Dec 2016 13:43:34 +0000 Received: by mail-qt0-f170.google.com with SMTP id p16so324796692qta.0 for ; Tue, 27 Dec 2016 05:43:34 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=umKBViYf67kItPcQxJmyX+4HDm97J1XD3rTQ9BSQ7iw=; b=qQVnXgY/XJV7xrjElrStNu9+le5NA0Pe2+d4F/jellUWqni87Yc9vVVRecyXriwlcA Kd7/keCg3egPTpDeofrGn9Dp8UMWQavSldw0CHBCFDovOmYEFp1lF9D6iF5GTiQso0/A QniKe9Gy/Dobnq0uhWSa8rRERVGv3BCaWYeKWyyQCbWCnBgAjSJI0yENJyBXlb34B9rg 1t0oqBeGTQRJuppYobNn93UQiAOfhG1Cudu/DleQ26YlZEmq2UNrq2vo91bQrTNMwZO+ zEpWoBd/yDNyO60xOJba3NM0pots/JLASDGyeMeo/GaQTmuz417fZGwZxyDBYA0AvofD NPbA== X-Gm-Message-State: AIkVDXKJTjWDFPCtStx2tBlacaEKeJSf+y9ePRtUlBtqYFu0P30eKU3/58nuDw1JGsnlV9smf/8tMdZQrVZNzA== X-Received: by 10.200.35.110 with SMTP id b43mr33236745qtb.265.1482846212374; Tue, 27 Dec 2016 05:43:32 -0800 (PST) MIME-Version: 1.0 Received: by 10.200.40.132 with HTTP; Tue, 27 Dec 2016 05:42:51 -0800 (PST) In-Reply-To: References: From: Alexander Ivchenko Date: Tue, 27 Dec 2016 13:46:00 -0000 Message-ID: Subject: Re: Pointer Bounds Checker and trailing arrays (PR68270) To: Ilya Enkovich Cc: GCC Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg01921.txt.bz2 Committed as r243936. Thank you, Alexander 2016-12-22 2:47 GMT+03:00 Ilya Enkovich : > 2016-12-21 22:18 GMT+03:00 Alexander Ivchenko : >> 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 : >>> 2016-12-20 17:44 GMT+03:00 Alexander Ivchenko : >>>> 2016-11-26 0:28 GMT+03:00 Ilya Enkovich : >>>>> 2016-11-25 15:47 GMT+03:00 Alexander Ivchenko : >>>>>> Hi, >>>>>> >>>>>> The patch below addresses PR68270. could you please take a look? >>>>>> >>>>>> 2016-11-25 Alexander Ivchenko >>>>>> >>>>>> * 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)