From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 9E47F3857B82 for ; Mon, 4 Jul 2022 06:15:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9E47F3857B82 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 2AA9B20204 for ; Mon, 4 Jul 2022 06:15:02 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 0F21B2C142; Mon, 4 Jul 2022 06:15:02 +0000 (UTC) Date: Mon, 4 Jul 2022 06:15:01 +0000 (UTC) From: Richard Biener To: Martin Jambor cc: GCC Patches Subject: Re: [PATCH] tree-sra: Fix union handling in build_reconstructed_reference (PR 105860) In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jul 2022 06:15:05 -0000 On Fri, 1 Jul 2022, Martin Jambor wrote: > Hi, > > As the testcase in PR 105860 shows, the code that tries to re-use the > handled_component chains in SRA can be horribly confused by unions, > where it thinks it has found a compatible structure under which it can > chain the references, but in fact it found the type it was looking > for elsewhere in a union and generated a write to a completely wrong > part of an aggregate. > > I don't remember whether the plan was to support unions at all in > build_reconstructed_reference but it can work, to an extent, if we > make sure that we start the search only outside the outermost union, > which is what the patch does (and the extra testcase verifies). > > Bootstrapped and tested on x86_64-linux. OK for trunk and then for > release branches? OK, but I'm wondering if the same problem can arise when the handled_component_p includes VIEW_CONVERTs or BIT_FIELD_REFs both of which may pun to a type already seen in a more inner referece. Thus, is the actual problem that build_reconstructed_reference searches for the outermost match of the type rather than the innermost match? So should it search inner-to-outer instead (or for the last match in the current way of searching?) Thanks, Richard. > Thanks, > > Martin > > > gcc/ChangeLog: > > 2022-07-01 Martin Jambor > > PR tree-optimization/105860 > * tree-sra.cc (build_reconstructed_reference): Start expr > traversal only just below the outermost union. > > gcc/testsuite/ChangeLog: > > 2022-07-01 Martin Jambor > > PR tree-optimization/105860 > * gcc.dg/tree-ssa/alias-access-path-13.c: New test. > * gcc.dg/tree-ssa/pr105860.c: Likewise. > --- > .../gcc.dg/tree-ssa/alias-access-path-13.c | 31 +++++++++ > gcc/testsuite/gcc.dg/tree-ssa/pr105860.c | 63 +++++++++++++++++++ > gcc/tree-sra.cc | 13 +++- > 3 files changed, 106 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr105860.c > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c b/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c > new file mode 100644 > index 00000000000..e502a97bc75 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c > @@ -0,0 +1,31 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-fre1" } */ > + > +struct inn > +{ > + int val; > +}; > + > +union foo > +{ > + struct inn inn; > + long int baz; > +} *fooptr; > + > +struct bar > +{ > + union foo foo; > + int val2; > +} *barptr; > + > +int > +test () > +{ > + union foo foo; > + foo.inn.val = 0; > + barptr->val2 = 123; > + *fooptr = foo; > + return barptr->val2; > +} > + > +/* { dg-final { scan-tree-dump-times "return 123" 1 "fre1"} } */ > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c b/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c > new file mode 100644 > index 00000000000..77bcb4a6739 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c > @@ -0,0 +1,63 @@ > +/* { dg-do run } */ > +/* { dg-options "-O1" } */ > + > +struct S1 { > + unsigned int _0; > + unsigned int _1; > +} ; > +struct S2 { > + struct S1 _s1; > + unsigned long _x2; > +} ; > + > +struct ufld_type1 { > + unsigned int _u1t; > + struct S2 _s2; > +} ; > + > +struct ufld_type2 { > + unsigned int _u2t; > + struct S1 _s1; > +} ; > +struct parm_type { > + union { > + struct ufld_type1 var_1; > + struct ufld_type2 var_2; > + } U; > +}; > + > +struct parm_type bad_function( struct parm_type arg0 ) > +{ > + struct parm_type rv; > + struct S2 var4; > + switch( arg0.U.var_2._u2t ) { > + case 4294967041: > + var4._s1 = arg0.U.var_1._s2._s1; > + rv.U.var_1._u1t = 4294967041; > + rv.U.var_1._s2 = var4; > + break; > + case 4294967043: > + rv.U.var_2._u2t = 4294967043; > + rv.U.var_2._s1 = arg0.U.var_2._s1; > + break; > + default: > + break; > + } > + return rv; > +} > + > +int main() { > + struct parm_type val; > + struct parm_type out; > + val.U.var_2._u2t = 4294967043; > + val.U.var_2._s1._0 = 0x01010101; > + val.U.var_2._s1._1 = 0x02020202; > + out = bad_function(val); > + if (val.U.var_2._u2t != 4294967043) > + __builtin_abort (); > + if (out.U.var_2._s1._0 != 0x01010101) > + __builtin_abort (); > + if (val.U.var_2._s1._1 != 0x02020202 ) > + __builtin_abort (); > + return 0; > +} > diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc > index 081c51b58a4..099e8dbe873 100644 > --- a/gcc/tree-sra.cc > +++ b/gcc/tree-sra.cc > @@ -1667,7 +1667,18 @@ build_ref_for_offset (location_t loc, tree base, poly_int64 offset, > static tree > build_reconstructed_reference (location_t, tree base, struct access *model) > { > - tree expr = model->expr, prev_expr = NULL; > + tree expr = model->expr; > + /* We have to make sure to start just below the outermost union. */ > + tree start_expr = expr; > + while (handled_component_p (expr)) > + { > + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (expr, 0))) == UNION_TYPE) > + start_expr = expr; > + expr = TREE_OPERAND (expr, 0); > + } > + > + expr = start_expr; > + tree prev_expr = NULL_TREE; > while (!types_compatible_p (TREE_TYPE (expr), TREE_TYPE (base))) > { > if (!handled_component_p (expr)) > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstra