From patchwork Tue Dec 20 14:44:17 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Ivchenko X-Patchwork-Id: 707467 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3tjgZ2500hz9rxw for ; Wed, 21 Dec 2016 01:45:28 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="ar4hDrPS"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; q=dns; s=default; b=yiQYpo8L4VBJ9s8 vYnrui1CMiLF7KjuuNdADGaJ0EwBnX0TwFWefu6yKnDQTTyi7aNp5QT62IONzMCl td1BPg/lTtgYYL/y+2oK6nbEY2csO+JJk6dSk4Heyl465WCPwx7u7Gj0altSKCUR X9NKOSo31bhl3HotkH04IiTLxyyc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; s=default; bh=Rjzo/txc2veOWCw7D/d5S vfZQ4A=; b=ar4hDrPSLGZ6BpzBEtxMbUKVL5IPszXOixB5+ft+oAec8mpN8shpm vJkpt6yME/IfjZd0i7Pq7ZYskDCs8XfpOxwY8iIIJ6dFGP5i76vF0biBAukmTplG HKRiwuZr/oTg8ROxc7rxOintmIoiesEpSdPrrrsxC5lM8Eay4ZwFiM= Received: (qmail 5338 invoked by alias); 20 Dec 2016 14:45:16 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 5204 invoked by uid 89); 20 Dec 2016 14:45:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=32728, 20161126, 2016-11-26, sb X-HELO: mail-qk0-f172.google.com Received: from mail-qk0-f172.google.com (HELO mail-qk0-f172.google.com) (209.85.220.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 20 Dec 2016 14:44:59 +0000 Received: by mail-qk0-f172.google.com with SMTP id t184so49901913qkd.0 for ; Tue, 20 Dec 2016 06:44:59 -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=geBrwJvbsCRN2IyiHgwdZiNakdis2C3rTkPb9zAjZcg=; b=cisoqkW/wBSyikOVIpg6vTakdt9R6uGDGZbpgvKxo80nLIxM1+JomOR0SC8zsVHz/+ haYrwUMI3TRciBp2BrRJFcn+xd1/d0QPUPDIGlYm90KFb7T+MRhaRgjfR2csw6xLq2K5 J0iXIKDv/hzghAROViUtcJDJaajsn+FiV3uE7ZJbwhRUVngDXbQorhDO0AQTrP82l5sS ZAblkYJdI8PQwuhcq05CG3Va8uvSxaSJgGFVVLPnM6On6ln92LfpjeOqLFA11w+aM5GF IndiiSRQmfWlCRFTk+IJwlT5QqhR9VV93AmCYTlev29aV+WEaf8zj1vdjQE0ee/FicN3 f0wQ== X-Gm-Message-State: AIkVDXL8EI2OeEvfTGZA02N7R3drObRN4sm53kBjIRhK6y02Tg0+MslbeKHToORPD/4R+goSAN9GQ3Naue/1Cw== X-Received: by 10.233.221.130 with SMTP id r124mr8565260qkf.183.1482245097697; Tue, 20 Dec 2016 06:44:57 -0800 (PST) MIME-Version: 1.0 Received: by 10.200.40.132 with HTTP; Tue, 20 Dec 2016 06:44:17 -0800 (PST) In-Reply-To: References: From: Alexander Ivchenko Date: Tue, 20 Dec 2016 17:44:17 +0300 Message-ID: Subject: Re: Pointer Bounds Checker and trailing arrays (PR68270) To: Ilya Enkovich Cc: GCC Patches X-IsSubscribed: yes 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? >> + >> 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)) && (!DECL_FIELD_OFFSET (field) || TREE_CODE (DECL_FIELD_OFFSET (field)) == INTEGER_CST) && (!DECL_FIELD_BIT_OFFSET (field)