From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 8A4913857C4A for ; Thu, 17 Mar 2022 07:31:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8A4913857C4A Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 6D02321116; Thu, 17 Mar 2022 07:31:13 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (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 663E3A3B81; Thu, 17 Mar 2022 07:31:13 +0000 (UTC) Date: Thu, 17 Mar 2022 08:31:13 +0100 (CET) From: Richard Biener To: Jakub Jelinek cc: Jason Merrill , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] gimplify: Emit clobbers for TARGET_EXPR_SLOT vars later [PR103984] In-Reply-To: Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Thu, 17 Mar 2022 07:31:16 -0000 On Wed, 16 Mar 2022, Jakub Jelinek wrote: > Hi! > > As mentioned in the PR, we emit a bogus uninitialized warning but > easily could emit wrong-code for it or similar testcases too. > The bug is that we emit clobber for a TARGET_EXPR_SLOT too early: > D.2499.e = B::qux (&h); [return slot optimization] > D.2516 = 1; > try > { > B::B (&D.2498, &h); > try > { > _2 = baz (&D.2498); > D.2499.f = _2; > D.2516 = 0; > try > { > try > { > bar (&D.2499); > } > finally > { > C::~C (&D.2499); > } > } > finally > { > D.2499 = {CLOBBER(eol)}; > } > } > finally > { > D.2498 = {CLOBBER(eol)}; > } > } > catch > { > if (D.2516 != 0) goto ; else goto ; > : > A::~A (&D.2499.e); > goto ; > : > : > } > The CLOBBER for D.2499 is essentially only emitted on the non-exceptional > path, if B::B or baz throws, then there is no CLOBBER for it but there > is a conditional destructor A::~A (&D.2499.e). Now, ehcleanup1 > sink_clobbers optimization assumes that clobbers in the EH cases are > emitted after last use and so sinks the D.2499 = {CLOBBER(eol)}; later, > so we then have > # _3 = PHI <1(3), 0(9)> > : > D.2499 ={v} {CLOBBER(eol)}; > D.2498 ={v} {CLOBBER(eol)}; > if (_3 != 0) > goto ; [INV] > else > goto ; [INV] > > : > _35 = D.2499.a; > if (&D.2499.b != _35) > where that _35 = D.2499.a comes from inline expansion of the A::~A dtor, > and that is a load from a clobbered memory. > > Now, what the gimplifier sees in this case is a CLEANUP_POINT_EXPR with > somewhere inside of it a TARGET_EXPR for D.2499 (with the C::~C (&D.2499) > cleanup) which in its TARGET_EXPR_INITIAL has another TARGET_EXPR for > D.2516 bool flag which has CLEANUP_EH_ONLY which performs that conditional > A::~A (&D.2499.e) call. > The following patch ensures that CLOBBERs (and asan poisoning) are emitted > after even those gimple_push_cleanup pushed cleanups from within the > TARGET_EXPR_INITIAL gimplification (i.e. the last point where the slot could > be in theory used). In my first version of the patch I've done it by just > moving the > /* Add a clobber for the temporary going out of scope, like > gimplify_bind_expr. */ > if (gimplify_ctxp->in_cleanup_point_expr > && needs_to_live_in_memory (temp)) > { > ... > } > block earlier in gimplify_target_expr, but that regressed a couple of tests > where temp is marked TREE_ADDRESSABLE only during (well, very early during > that) the gimplification of TARGET_EXPR_INITIAL, so we didn't emit e.g. on > pr80032.C or stack2.C tests any clobbers for the slots and thus stack slot > reuse wasn't performed. > So that we don't regress those tests, this patch gimplifies > TARGET_EXPR_INITIAL as before, but doesn't emit it directly into pre_p, > emits it into a temporary sequence. Then emits the CLOBBER cleanup > into pre_p, then asan poisoning if needed, then appends the > TARGET_EXPR_INITIAL temporary sequence and finally adds TARGET_EXPR_CLEANUP > gimple_push_cleanup. The earlier a GIMPLE_WCE appears in the sequence, the > outer try/finally or try/catch it is. > So, with this patch the part of the testcase in gimple dump cited above > looks instead like: > try > { > D.2499.e = B::qux (&h); [return slot optimization] > D.2516 = 1; > try > { > try > { > B::B (&D.2498, &h); > _2 = baz (&D.2498); > D.2499.f = _2; > D.2516 = 0; > try > { > bar (&D.2499); > } > finally > { > C::~C (&D.2499); > } > } > finally > { > D.2498 = {CLOBBER(eol)}; > } > } > catch > { > if (D.2516 != 0) goto ; else goto ; > : > A::~A (&D.2499.e); > goto ; > : > : > } > } > finally > { > D.2499 = {CLOBBER(eol)}; > } > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? LGTM. Thanks, Richard. > 2022-03-16 Jakub Jelinek > > PR middle-end/103984 > * gimplify.cc (gimplify_target_expr): Gimplify type sizes and > TARGET_EXPR_INITIAL into a temporary sequence, then push clobbers > and asan unpoisioning, then append the temporary sequence and > finally the TARGET_EXPR_CLEANUP clobbers. > > * g++.dg/opt/pr103984.C: New test. > > --- gcc/gimplify.cc.jj 2022-03-16 10:55:58.426014897 +0100 > +++ gcc/gimplify.cc 2022-03-16 15:41:06.122913465 +0100 > @@ -6997,17 +6997,17 @@ gimplify_target_expr (tree *expr_p, gimp > > if (init) > { > - tree cleanup = NULL_TREE; > + gimple_seq init_pre_p = NULL; > > /* TARGET_EXPR temps aren't part of the enclosing block, so add it > to the temps list. Handle also variable length TARGET_EXPRs. */ > if (!poly_int_tree_p (DECL_SIZE (temp))) > { > if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp))) > - gimplify_type_sizes (TREE_TYPE (temp), pre_p); > + gimplify_type_sizes (TREE_TYPE (temp), &init_pre_p); > /* FIXME: this is correct only when the size of the type does > not depend on expressions evaluated in init. */ > - gimplify_vla_decl (temp, pre_p); > + gimplify_vla_decl (temp, &init_pre_p); > } > else > { > @@ -7022,12 +7022,14 @@ gimplify_target_expr (tree *expr_p, gimp > /* If TARGET_EXPR_INITIAL is void, then the mere evaluation of the > expression is supposed to initialize the slot. */ > if (VOID_TYPE_P (TREE_TYPE (init))) > - ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none); > + ret = gimplify_expr (&init, &init_pre_p, post_p, is_gimple_stmt, > + fb_none); > else > { > tree init_expr = build2 (INIT_EXPR, void_type_node, temp, init); > init = init_expr; > - ret = gimplify_expr (&init, pre_p, post_p, is_gimple_stmt, fb_none); > + ret = gimplify_expr (&init, &init_pre_p, post_p, is_gimple_stmt, > + fb_none); > init = NULL; > ggc_free (init_expr); > } > @@ -7037,18 +7039,9 @@ gimplify_target_expr (tree *expr_p, gimp > TARGET_EXPR_INITIAL (targ) = NULL_TREE; > return GS_ERROR; > } > - if (init) > - gimplify_and_add (init, pre_p); > > - /* If needed, push the cleanup for the temp. */ > - if (TARGET_EXPR_CLEANUP (targ)) > - { > - if (CLEANUP_EH_ONLY (targ)) > - gimple_push_cleanup (temp, TARGET_EXPR_CLEANUP (targ), > - CLEANUP_EH_ONLY (targ), pre_p); > - else > - cleanup = TARGET_EXPR_CLEANUP (targ); > - } > + if (init) > + gimplify_and_add (init, &init_pre_p); > > /* Add a clobber for the temporary going out of scope, like > gimplify_bind_expr. */ > @@ -7079,8 +7072,13 @@ gimplify_target_expr (tree *expr_p, gimp > } > } > } > - if (cleanup) > - gimple_push_cleanup (temp, cleanup, false, pre_p); > + > + gimple_seq_add_seq (pre_p, init_pre_p); > + > + /* If needed, push the cleanup for the temp. */ > + if (TARGET_EXPR_CLEANUP (targ)) > + gimple_push_cleanup (temp, TARGET_EXPR_CLEANUP (targ), > + CLEANUP_EH_ONLY (targ), pre_p); > > /* Only expand this once. */ > TREE_OPERAND (targ, 3) = init; > --- gcc/testsuite/g++.dg/opt/pr103984.C.jj 2022-03-16 15:28:54.756742692 +0100 > +++ gcc/testsuite/g++.dg/opt/pr103984.C 2022-03-16 15:28:54.756742692 +0100 > @@ -0,0 +1,31 @@ > +// PR middle-end/103984 > +// { dg-do compile { target c++11 } } > +// { dg-options "-O2 -Wuninitialized" } > + > +struct A { > + char *a; > + char b[4]; > + A (); > + A (const A &); > + A (const char *); > + A (const char *, const char *); > + [[gnu::always_inline]] ~A () { if (a != b) delete a; } > +}; > +struct B { > + const char *c = nullptr; > + const char *d = nullptr; > + A qux () const { return A (c, d); } > + B (const char *x) : c(x), d(x) { d += __builtin_strlen (x); } > + B (const B &x) { c = x.c; d = x.d; } > +}; > +struct C { A e; int f; }; > +extern int baz (B); > +void bar (C &&); > + > +void > +foo (char **x) > +{ > + const A g ("foo"); > + const B h = x[0]; > + bar (C { h.qux (), baz (h) }); > +} > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)