From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16058 invoked by alias); 21 Dec 2016 18:00:36 -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 15333 invoked by uid 89); 21 Dec 2016 18:00:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=integer_cst, INTEGER_CST, sb, aivchenk@gmail.com X-HELO: mail-vk0-f42.google.com Received: from mail-vk0-f42.google.com (HELO mail-vk0-f42.google.com) (209.85.213.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Dec 2016 18:00:31 +0000 Received: by mail-vk0-f42.google.com with SMTP id p9so153301660vkd.3 for ; Wed, 21 Dec 2016 10:00:31 -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=ZpSq7tbmKOEe1xPzz7jwoOgTKmIW0SxECNk0i8Oug3w=; b=lTHM5SSEBnqbp4WLFvndQQAoZfFPN2P/+wkUfDDjfP8fpeSXT8FOjDxA7T7BUG1jQx RHvfwnL0AtsbPvvAyXNTLEB9vug14Vbc0ALJIbQZrbWuSa8tPPq5//94gvwoX2lLHGdr TXw+ejNLe3B5k13o3iatc7KTtIAT0ItGLsd0yUzOrLNlB+x8aJ+y1cG8oxx4Du+F+w5B 6BAgpOSmnAfMeVkjPdtpymE+ayuF8HwCESN/8zgxwaLxgDXx5k7LPqAT+JQ3Za6I2Cpm VVmtUoDPiYZ5TqJyAfgJzEjmbpVbBmZufDx1XsJHiNG+WyMAh7vK32OwQP6v3dsioMrF 9iKQ== X-Gm-Message-State: AIkVDXJPJ+/OsNMABW8Xbpd8i5jHN3NyAZTgaxJ06D+r6pb54OnfLE6WAv3xlQeeXfGp4TLswYMNiUZ0nfK7bA== X-Received: by 10.31.3.155 with SMTP id f27mr2015644vki.64.1482343229342; Wed, 21 Dec 2016 10:00:29 -0800 (PST) MIME-Version: 1.0 Received: by 10.159.35.103 with HTTP; Wed, 21 Dec 2016 10:00:28 -0800 (PST) In-Reply-To: References: From: Ilya Enkovich Date: Wed, 21 Dec 2016 18:03:00 -0000 Message-ID: Subject: Re: Pointer Bounds Checker and trailing arrays (PR68270) To: Alexander Ivchenko Cc: GCC Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg01797.txt.bz2 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 > 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)