From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88273 invoked by alias); 28 Apr 2017 13:17:57 -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 88259 invoked by uid 89); 28 Apr 2017 13:17:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 28 Apr 2017 13:17:54 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 31E0AAD18; Fri, 28 Apr 2017 13:17:54 +0000 (UTC) Date: Fri, 28 Apr 2017 13:37:00 -0000 From: Richard Biener To: Alexander Monakov cc: gcc-patches@gcc.gnu.org, bergner@vnet.ibm.com Subject: Re: [PATCH] Fix PR80533 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2017-04/txt/msg01490.txt.bz2 On Fri, 28 Apr 2017, Richard Biener wrote: > On Thu, 27 Apr 2017, Alexander Monakov wrote: > > > On Thu, 27 Apr 2017, Richard Biener wrote: > > > struct q { int n; long o[100]; }; > > > struct r { int n; long o[0]; }; > > > > > > union { > > > struct r r; > > > struct q q; > > > } u; > > > > > > int foo (int i, int j) > > > { > > > long *q = u.r.o; > > > u.r.o[i/j] = 1; > > > return q[2]; > > > } > > > > > > but nothing convinced scheduling to move the load before the store ;) > > > The two memory references are seen as not aliasing though. Stupid > > > scheduler. > > > > On x86 there's an anti-dependence on %eax that prevents the second scheduler > > from performing the breaking motion; with -fschedule-insns, pre-RA scheduler > > is actually able to move the load too early, but then IRA immediately undoes > > that. Also, -fselective-scheduling2 is able to move the load early via renaming > > (and uncovers the miscompile, as nothing undoes it later). > > > > Applying division to the result of the load, rather than the address of the > > store, also produces code demonstrating the miscompile: > > > > struct q { int n; long o[100]; }; > > struct r { int n; long o[0]; }; > > > > union { > > struct r r; > > struct q q; > > } u; > > > > int foo (int i, int j) > > { > > long *q = u.r.o; > > u.r.o[i] = 1; > > return q[2]/j; > > } > > Thanks. It also is still miscompiled because array_at_struct_end_p > is somewhat confused as to what its semantics are supposed to be. > Multiple callers (including the new one) assume when it returns false > they can trust the array domain while in reality when it returns false > it says we can constrain the access even if only because we know an > underlying decls size ... > > It also raises the question whether for > > struct X > { > double x; > int a[1]; > } x; > > x.a is an array at struct end -- there's enough padding to provide > space for x.a[1] even though its size doesn't include that (and > whether it can depends on the alignment of 'double'). This is > sort-of what happens for the union case above -- u.r.o can be > extended into the padding (which is quite large due to u.q.o). > The question is whether we allow such access to padding in general > (not only when the array is at struct end). Likewise do we allow, in > > struct Y > { > struct X[4][4] a; > } y; > > for y.a[i][j].a[k] k == 3? that is, allow the "last" element of > an array to be flexibly extended? Or do we instead allow > y.a[i][j] with j == 4, thus the last dimension of a multidimensional > array to be "over-allocated"? Or do we just allow i > 3? > > There's another PR where array-bound warnings are confused by > the weird answer from array_at_struct_end_p and two other users > I skimmed would generate wrong code because they trust the array > domain after it. > > Looks like I have to refactor that a bit, like with the following > which makes things a bit more explicit, and _not_ allowing to > use padding appart when using flexarrays (though that can't > happen, you can't instantiate those with decls) or the zero-size > array GNU extension. > > It also corrects IMHO wrong behavior with VIEW_CONVERT_EXPRs > and the overly pessimistic handling of arrays of structs with > arrays at struct end or multi-dimensional arrays when not > dealing with the outermost dimension. > > So I'm testing the following. Bootstrapped / tested on x86_64-unknown-linux-gnu. FAIL: g++.dg/warn/Warray-bounds-4.C -std=gnu++11 (test for warnings, line 25) looks like we explicitely want to warn about char[0] (in C++!) ... Richard. > Richard. > > 2017-04-28 Richard Biener > > PR middle-end/80533 > * tree.c (array_at_struct_end_p): Handle arrays at struct > end with size zero more conservatively. Refactor and treat > arrays of arrays or aggregates more strict. Fix > VIEW_CONVERT_EXPR handling. > > * gcc.dg/torture/pr80533.c: New testcase. > > Index: gcc/tree.c > =================================================================== > --- gcc/tree.c (revision 247362) > +++ gcc/tree.c (working copy) > @@ -13222,9 +13230,19 @@ array_ref_up_bound (tree exp) > bool > array_at_struct_end_p (tree ref, bool allow_compref) > { > - if (TREE_CODE (ref) != ARRAY_REF > - && TREE_CODE (ref) != ARRAY_RANGE_REF > - && (!allow_compref || TREE_CODE (ref) != COMPONENT_REF)) > + tree atype; > + > + if (TREE_CODE (ref) == ARRAY_REF > + || TREE_CODE (ref) == ARRAY_RANGE_REF) > + { > + atype = TREE_TYPE (TREE_OPERAND (ref, 0)); > + ref = TREE_OPERAND (ref, 0); > + } > + else if (allow_compref > + && TREE_CODE (ref) == COMPONENT_REF > + && TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 1))) == ARRAY_TYPE) > + atype = TREE_TYPE (TREE_OPERAND (ref, 1)); > + else > return false; > > while (handled_component_p (ref)) > @@ -13232,19 +13250,43 @@ array_at_struct_end_p (tree ref, bool al > /* If the reference chain contains a component reference to a > non-union type and there follows another field the reference > is not at the end of a structure. */ > - if (TREE_CODE (ref) == COMPONENT_REF > - && TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 0))) == RECORD_TYPE) > + if (TREE_CODE (ref) == COMPONENT_REF) > { > - tree nextf = DECL_CHAIN (TREE_OPERAND (ref, 1)); > - while (nextf && TREE_CODE (nextf) != FIELD_DECL) > - nextf = DECL_CHAIN (nextf); > - if (nextf) > - return false; > + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 0))) == RECORD_TYPE) > + { > + tree nextf = DECL_CHAIN (TREE_OPERAND (ref, 1)); > + while (nextf && TREE_CODE (nextf) != FIELD_DECL) > + nextf = DECL_CHAIN (nextf); > + if (nextf) > + return false; > + } > } > + /* If we have a multi-dimensional array we do not consider > + a non-innermost dimension as flex array if the whole > + multi-dimensional array is at struct end. > + Same for an array of aggregates with a trailing array > + member. */ > + else if (TREE_CODE (ref) == ARRAY_REF) > + return false; > + else if (TREE_CODE (ref) == ARRAY_RANGE_REF) > + ; > + /* If we view an underlying object as sth else then what we > + gathered up to now is what we have to rely on. */ > + else if (TREE_CODE (ref) == VIEW_CONVERT_EXPR) > + break; > + else > + gcc_unreachable (); > > ref = TREE_OPERAND (ref, 0); > } > > + /* The array now is at struct end. Treat flexible arrays and > + zero-sized arrays as always subject to extend, even into > + just padding constrained by an underlying decl. */ > + if (! TYPE_SIZE (atype) > + || integer_zerop (TYPE_SIZE (atype))) > + return true; > + > tree size = NULL; > > if (TREE_CODE (ref) == MEM_REF > Index: gcc/testsuite/gcc.dg/torture/pr80533.c > =================================================================== > --- gcc/testsuite/gcc.dg/torture/pr80533.c (nonexistent) > +++ gcc/testsuite/gcc.dg/torture/pr80533.c (working copy) > @@ -0,0 +1,27 @@ > +/* { dg-do run } */ > + > +extern void abort (void); > + > +struct q { int n; long o[100]; }; > +struct r { int n; long o[0]; }; > + > +union { > + struct r r; > + struct q q; > +} u; > + > +int __attribute__((noclone,noinline)) > +foo (int i, int j) > +{ > + long *q = u.r.o; > + u.r.o[i] = 1; > + return q[2]/j; > +} > + > +int > +main() > +{ > + if (foo (2, 1) != 1) > + abort (); > + return 0; > +} > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)