From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34680 invoked by alias); 2 Feb 2018 21:35:28 -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 34668 invoked by uid 89); 2 Feb 2018 21:35:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=H*i:sk:QeORzXa, H*f:sk:QeORzXa, Hence X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 02 Feb 2018 21:35:26 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DC3B65F7AD; Fri, 2 Feb 2018 21:35:24 +0000 (UTC) Received: from ovpn-116-223.phx2.redhat.com (ovpn-116-223.phx2.redhat.com [10.3.116.223]) by smtp.corp.redhat.com (Postfix) with ESMTP id 44B2A60C91; Fri, 2 Feb 2018 21:35:22 +0000 (UTC) Message-ID: <1517607322.26503.58.camel@redhat.com> Subject: Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136) From: David Malcolm To: Richard Biener , "Joseph S. Myers" Cc: GCC Patches Date: Fri, 02 Feb 2018 21:35:00 -0000 In-Reply-To: References: <1517413159-17727-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00136.txt.bz2 On Thu, 2018-02-01 at 12:05 +0100, Richard Biener wrote: > On Wed, Jan 31, 2018 at 4:39 PM, David Malcolm > wrote: > > PR 84136 reports an ICE within sccvn_dom_walker when handling a > > C/C++ source file that overuses the labels-as-values extension. > > The code in question stores a jump label into a global, and then > > jumps to it from another function, which ICEs after inlining: > > > > void* a; > > > > void foo() { > > if ((a = &&l)) > > return; > > > > l:; > > } > > > > int main() { > > foo(); > > goto *a; > > > > return 0; > > } > > > > This appears to be far beyond what we claim to support in this > > extension - but we shouldn't ICE. > > > > What's happening is that, after inlining, we have usage of a *copy* > > of the label, which optimizes away the if-return logic, turning it > > into an infinite loop. > > > > On entry to the sccvn_dom_walker we have this gimple: > > > > main () > > { > > void * a.0_1; > > > > [count: 0]: > > a = &l; > > > > [count: 0]: > > l: > > a.0_1 = a; > > goto a.0_1; > > } > > > > and: > > edge taken = find_taken_edge (bb, vn_valueize (val)); > > reasonably valueizes the: > > goto a.0_1; > > after the: > > a = &l; > > a.0_1 = a; > > as if it were: > > goto *&l; > > > > find_taken_edge_computed_goto then has: > > > > 2380 dest = label_to_block (val); > > 2381 if (dest) > > 2382 { > > 2383 e = find_edge (bb, dest); > > 2384 gcc_assert (e != NULL); > > 2385 } > > > > which locates dest as a self-jump from block 3 back to itself. > > > > However, the find_edge call returns NULL - it has a predecessor > > edge > > from block 2, but no successor edges. > > > > Hence the assertion fails and we ICE. > > > > A successor edge from the computed goto could have been created by > > make_edges if the label stmt had been in the function, but > > make_edges > > only looks in the current function when handling computed gotos, > > and > > the label only appeared after inlining. > > > > The following patch removes the assertion, fixing the ICE. > > > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. > > > > If that's option (a), there could be some other approaches: > > > > (b) convert the assertion into a warning/error/sorry, on the > > assumption that if we don't detect such an edge then the code > > is > > presumably abusing the labels-as-values feature > > (c) have make_edges detect such a problematic computed goto (maybe > > converting make_edges_bb's return value to an enum and adding a > > 4th > > value - though it's not clear what to do then with it) > > (d) detect this case on inlining and handle it somehow (e.g. adding > > edges for labels that have appeared since make_edges originally > > ran, for computed gotos that have no out-edges) > > (e) do nothing, keeping the assertion, and accept that this is > > going > > to fail on a non-release build > > (f) something else? > > > > Of the above, (d) seems to me to be the most robust solution, but I > > don't know how far we want to go "down the rabbit hole" of handling > > such uses of labels-as-values (beyond not ICE-ing on them). > > > > Thoughts? > > I think you can preserve the assert for ! DECL_NONLOCAL (val) thus > > gcc_assert (e != NULL || DECL_NONLOCAL (val)); > > does the label in this case properly have DECL_NONLOCAL > set? Probably > not given we shouldn't have duplicated it in this case. Indeed, the inlined copy of the label doesn't have DECL_NONLOCAL set: (gdb) p val->decl_common.nonlocal_flag $5 = 0 > So the issue is really > that the FE doesn't set this bit for "escaped" labels... but I'm not > sure how > to easily constrain the extension here. > > The label should be FORCED_LABEL though so that's maybe a weaker > check. It does have FORCED_LABEL set: (gdb) p val->base.side_effects_flag $6 = 1 ...though presumably that's going to be set for just about any label that a computed goto jumps to? Hence this is presumably of little benefit for adjusting the assertion. > Joseph? > > Richard. > > > > > gcc/testsuite/ChangeLog: > > PR tree-optimization/84136 > > * gcc.c-torture/compile/pr84136.c: New test. > > > > gcc/ChangeLog: > > PR tree-optimization/84136 > > * tree-cfg.c (find_taken_edge_computed_goto): Remove > > assertion > > that the result of find_edge is non-NULL. > > --- > > gcc/testsuite/gcc.c-torture/compile/pr84136.c | 15 +++++++++++++++ > > gcc/tree-cfg.c | 5 +---- > > 2 files changed, 16 insertions(+), 4 deletions(-) > > create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr84136.c > > > > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr84136.c > > b/gcc/testsuite/gcc.c-torture/compile/pr84136.c > > new file mode 100644 > > index 0000000..0a70e4e > > --- /dev/null > > +++ b/gcc/testsuite/gcc.c-torture/compile/pr84136.c > > @@ -0,0 +1,15 @@ > > +void* a; > > + > > +void foo() { > > + if ((a = &&l)) > > + return; > > + > > + l:; > > +} > > + > > +int main() { > > + foo(); > > + goto *a; > > + > > + return 0; > > +} > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > > index c5318b9..6b89307 100644 > > --- a/gcc/tree-cfg.c > > +++ b/gcc/tree-cfg.c > > @@ -2379,10 +2379,7 @@ find_taken_edge_computed_goto (basic_block > > bb, tree val) > > > > dest = label_to_block (val); > > if (dest) > > - { > > - e = find_edge (bb, dest); > > - gcc_assert (e != NULL); > > - } > > + e = find_edge (bb, dest); > > > > return e; > > } > > -- > > 1.8.5.3 > >