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 1EAEF385828E for ; Tue, 5 Jul 2022 07:34:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1EAEF385828E Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id DB24920036 for ; Tue, 5 Jul 2022 07:34:19 +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 BE5C22C141; Tue, 5 Jul 2022 07:34:19 +0000 (UTC) Date: Tue, 5 Jul 2022 07:34:19 +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=-11.0 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: Tue, 05 Jul 2022 07:34:22 -0000 On Mon, 4 Jul 2022, Martin Jambor wrote: > Hi, > > On Mon, Jul 04 2022, Richard Biener wrote: > > 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. > > SRA already refuses to operate at all on any anything that is accessed > with a reference where a V_C_E is not the outermost handled_component. > Outermost V_C_Es are skipped and the pass works with the underlying > expr. BIT_FIELD_REFs have to be outermost and they are treated > similarly. So that should be safe. OK. > > 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?) > > I don't think so. In the testcase it found a match where there should > have been none (meaning a crude MEM_REF should be created), any > certainly correct match must be outside of a union COMPONENT_REF and > there should never be more than one. OK, not having looked at the testcase I suspected there would be two matches. I couldn't come up with a case where a single union can confuse things enough .. The patch is OK. Richard. > Thanks, > > Martin > > > > > 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 > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstra