Message ID | n15rnp6-9019-sn81-q356-83r818n8s5o2@fhfr.qr |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7E53D385780E for <patchwork@sourceware.org>; Wed, 24 Nov 2021 15:22:03 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7E53D385780E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1637767323; bh=sJbSfI7uHldhGeuJySdkxlf5iyi+0RWZBD1C1s2Gtt4=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=CHdiF8m27iB4Zgxfcg9AHPXj0IHdoq/myS/4GJBqbRAlfN0eyUo7VKYqPd+wSSS/K jshk8M2VMcbCHkDC2KI0FAaGQCngRHsoertsFbiLIRv99u5mCLMRjSS26y2JrDiiEv JNUdinYdD4nxajDnpptJl2Xta33gjnxxZKXXZzHY= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id A249A3858C39 for <gcc-patches@gcc.gnu.org>; Wed, 24 Nov 2021 15:21:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A249A3858C39 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 6369F1FD2F; Wed, 24 Nov 2021 15:21:32 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 4582B13F2C; Wed, 24 Nov 2021 15:21:32 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id M95eD3xYnmFvAgAAMHmgww (envelope-from <rguenther@suse.de>); Wed, 24 Nov 2021 15:21:32 +0000 Date: Wed, 24 Nov 2021 16:21:31 +0100 (CET) To: gcc-patches@gcc.gnu.org Subject: [PATCH][RFC] middle-end/46476 - resurrect -Wunreachable-code Message-ID: <n15rnp6-9019-sn81-q356-83r818n8s5o2@fhfr.qr> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.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 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Richard Biener <rguenther@suse.de> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[RFC] middle-end/46476 - resurrect -Wunreachable-code
|
|
Commit Message
Richard Biener
Nov. 24, 2021, 3:21 p.m. UTC
This resurrects -Wunreachable-code and implements a warning for trivially unreachable code as of CFG construction. Most problematic with this is the C/C++ frontend added 'return 0;' stmt in main which the patch handles for C++ like the C frontend already does by using BUILTINS_LOCATION. Another problem for future enhancement is that after CFG construction we no longer can point to the stmt making a stmt unreachable, so this implementation tries to warn on the first unreachable statement of a region. It might be possible to retain a pointer to the stmt that triggered creation of a basic-block but I'm not sure how reliable that would be. So this is really a simple attempt for now, triggered by myself running into such a coding error. As always, the perfect is the enemy of the good. It does not pass bootstrap (which enables -Wextra), because of the situation in g++.dg/Wunreachable-code-5.C where the C++ frontend prematurely elides conditions like if (! GATHER_STATISTICS) that evaluate to true - oddly enough it does _not_ do this for conditions evaluating to false ... (one of the c-c++-common/Wunreachable-code-2.c cases). Richard. 2021-11-24 Richard Biener <rguenther@suse.de> PR middle-end/46476 * common.opt (Wunreachable-code): No longer ignored, add warn_unreachable_code variable, enable with -Wextra. * doc/invoke.texi (Wunreachable-code): Document. (Wextra): Amend. * tree-cfg.c (build_gimple_cfg): Move case label grouping... (execute_build_cfg): ... here after new -Wunreachable-code warnings. (warn_unreachable_code_post_cfg_build): New function. (mark_forward_reachable_blocks): Likewise. (reverse_guess_deadend): Likewise. gcc/cp/ * decl.c (finish_function): Set input_location to BUILTINS_LOCATION around the code building the return 0 for main(). libgomp/ * oacc-plugin.c (GOMP_PLUGIN_acc_default_dim): Remove spurious return. gcc/testsuite/ * c-c++-common/Wunreachable-code-1.c: New testcase. * c-c++-common/Wunreachable-code-2.c: Likewise. * c-c++-common/Wunreachable-code-3.c: Likewise. * gcc.dg/Wunreachable-code-4.c: Likewise. * g++.dg/Wunreachable-code-5.C: Likewise. --- gcc/common.opt | 4 +- gcc/cp/decl.c | 9 +- gcc/doc/invoke.texi | 9 +- .../c-c++-common/Wunreachable-code-1.c | 8 ++ .../c-c++-common/Wunreachable-code-2.c | 8 ++ .../c-c++-common/Wunreachable-code-3.c | 35 ++++++ gcc/testsuite/g++.dg/Wunreachable-code-5.C | 11 ++ gcc/testsuite/gcc.dg/Wunreachable-code-4.c | 10 ++ gcc/tree-cfg.c | 101 +++++++++++++++++- libgomp/oacc-plugin.c | 1 - 10 files changed, 186 insertions(+), 10 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-1.c create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-2.c create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-3.c create mode 100644 gcc/testsuite/g++.dg/Wunreachable-code-5.C create mode 100644 gcc/testsuite/gcc.dg/Wunreachable-code-4.c
Comments
Hello, > +/* Unreachable code in if (0) block. */ > +void baz(int *p) > +{ > + if (0) > + { > + return; /* { dg-bogus "not reachable" } */ Hmm? Why are you explicitely saying that warning here would be bogus? It quite clearly _is_ unreachable, so warning there makes sense. Maybe you want an XFAILed dg-warning if your current implementation fails to warn, and a further XFAILed dg-bogus on the next line? (Or at the very least a comment in the test case that this is actually not what we really want, but rather what current GCCs produce) Ciao, Michael.
On November 24, 2021 4:43:45 PM GMT+01:00, Michael Matz <matz@suse.de> wrote: >Hello, > >> +/* Unreachable code in if (0) block. */ >> +void baz(int *p) >> +{ >> + if (0) >> + { >> + return; /* { dg-bogus "not reachable" } */ > >Hmm? Why are you explicitely saying that warning here would be bogus? Because I don't think we want to warn here. Such code is common from template instantiation or macro expansion. Richard. It >quite clearly _is_ unreachable, so warning there makes sense. Maybe you >want an XFAILed dg-warning if your current implementation fails to warn, >and a further XFAILed dg-bogus on the next line? > >(Or at the very least a comment in the test case that this is actually not >what we really want, but rather what current GCCs produce) > > >Ciao, >Michael.
On Wed, Nov 24, 2021 at 04:21:31PM +0100, Richard Biener via Gcc-patches wrote: > This resurrects -Wunreachable-code and implements a warning for > trivially unreachable code as of CFG construction. Most problematic > with this is the C/C++ frontend added 'return 0;' stmt in main > which the patch handles for C++ like the C frontend already does > by using BUILTINS_LOCATION. > > Another problem for future enhancement is that after CFG construction > we no longer can point to the stmt making a stmt unreachable, so > this implementation tries to warn on the first unreachable > statement of a region. It might be possible to retain a pointer > to the stmt that triggered creation of a basic-block but I'm not > sure how reliable that would be. > > So this is really a simple attempt for now, triggered by myself > running into such a coding error. As always, the perfect is the > enemy of the good. > > It does not pass bootstrap (which enables -Wextra), because of the > situation in g++.dg/Wunreachable-code-5.C where the C++ frontend > prematurely elides conditions like if (! GATHER_STATISTICS) that > evaluate to true - oddly enough it does _not_ do this for > conditions evaluating to false ... (one of the > c-c++-common/Wunreachable-code-2.c cases). I've taken a look into the C++ thing. This is genericize_if_stmt: if we have if (0) return; then cond is integer_zerop, then_ is a return_expr, but since it has TREE_SIDE_EFFECTS, we create a COND_EXPR. For if (!0) return; we do 170 else if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_)) 171 stmt = then_; which elides the if completely. So it seems it would help if we avoided eliding the if stmt if -Wunreachable-code is in effect. I'd be happy to make that change, if it sounds sane. Marek
Hello, On Wed, 24 Nov 2021, Richard Biener wrote: > >> +/* Unreachable code in if (0) block. */ > >> +void baz(int *p) > >> +{ > >> + if (0) > >> + { > >> + return; /* { dg-bogus "not reachable" } */ > > > >Hmm? Why are you explicitely saying that warning here would be bogus? > > Because I don't think we want to warn here. Such code is common from > template instantiation or macro expansion. Like all code with an (const-propagated) explicit 'if (0)', which is of course the reason why -Wunreachable-code is a challenge. IOW: I could accept your argument but then wonder why you want to warn about the second statement of the guarded block. The situation was: if (0) { return; // (1) don't warn here? whatever++; // (2) but warn here? } That seems even more confusing. So you don't want to warn about unreachable code (the 'return') but you do want to warn about unreachable code within unreachable code (point (2) is unreachable because of the if(0) and because of the return). If your worry is macro/template expansion resulting if(0)'s then I don't see why you would only disable warnings for some of the statements therein. It seems we are actually interested in code unreachable via fallthrough or labels, not in all unreachable code, so maybe the warning is mis-named. Btw. what does the code now do about this situation: if (0) { something++; // 1 return; // 2 somethingelse++; // 3 } does it warn at (1) or not? (I assume it unconditionally warns at (3)) Ciao, Michael.
On 11/24/21 8:21 AM, Richard Biener via Gcc-patches wrote: > This resurrects -Wunreachable-code and implements a warning for > trivially unreachable code as of CFG construction. Most problematic > with this is the C/C++ frontend added 'return 0;' stmt in main > which the patch handles for C++ like the C frontend already does > by using BUILTINS_LOCATION. > > Another problem for future enhancement is that after CFG construction > we no longer can point to the stmt making a stmt unreachable, so > this implementation tries to warn on the first unreachable > statement of a region. It might be possible to retain a pointer > to the stmt that triggered creation of a basic-block but I'm not > sure how reliable that would be. > > So this is really a simple attempt for now, triggered by myself > running into such a coding error. As always, the perfect is the > enemy of the good. > > It does not pass bootstrap (which enables -Wextra), because of the > situation in g++.dg/Wunreachable-code-5.C where the C++ frontend > prematurely elides conditions like if (! GATHER_STATISTICS) that > evaluate to true - oddly enough it does _not_ do this for > conditions evaluating to false ... (one of the > c-c++-common/Wunreachable-code-2.c cases). I'm very much in favor of reviving the warning, even in its current simplistic form. I especially welcome the suggestion to enhance it in the future, including adjusting its schedule among other passes (or adding other, later invocations). It would be overly constraining to consider this placement ideal or set in stone. Among possible enhancements worth considering is handling constant conditionals like: int f (void) { if (1) return 0; else return 1; <<< warn } int g (void) { if (1) return 0; return 1; <<< warn also in C (not just in C++) } By the way, a related feature that would be useful and that's been requested in the past is warning for stores with no effect, as in: int i; i = 1; i = 2; <<< warn here The detection of the simple cases like the one above can also be almost trivially implemented. Martin > > Richard. > > 2021-11-24 Richard Biener <rguenther@suse.de> > > PR middle-end/46476 > * common.opt (Wunreachable-code): No longer ignored, > add warn_unreachable_code variable, enable with -Wextra. > * doc/invoke.texi (Wunreachable-code): Document. > (Wextra): Amend. > * tree-cfg.c (build_gimple_cfg): Move case label grouping... > (execute_build_cfg): ... here after new -Wunreachable-code > warnings. > (warn_unreachable_code_post_cfg_build): New function. > (mark_forward_reachable_blocks): Likewise. > (reverse_guess_deadend): Likewise. > > gcc/cp/ > * decl.c (finish_function): Set input_location to > BUILTINS_LOCATION around the code building the return 0 > for main(). > > libgomp/ > * oacc-plugin.c (GOMP_PLUGIN_acc_default_dim): Remove spurious > return. > > gcc/testsuite/ > * c-c++-common/Wunreachable-code-1.c: New testcase. > * c-c++-common/Wunreachable-code-2.c: Likewise. > * c-c++-common/Wunreachable-code-3.c: Likewise. > * gcc.dg/Wunreachable-code-4.c: Likewise. > * g++.dg/Wunreachable-code-5.C: Likewise. > --- > gcc/common.opt | 4 +- > gcc/cp/decl.c | 9 +- > gcc/doc/invoke.texi | 9 +- > .../c-c++-common/Wunreachable-code-1.c | 8 ++ > .../c-c++-common/Wunreachable-code-2.c | 8 ++ > .../c-c++-common/Wunreachable-code-3.c | 35 ++++++ > gcc/testsuite/g++.dg/Wunreachable-code-5.C | 11 ++ > gcc/testsuite/gcc.dg/Wunreachable-code-4.c | 10 ++ > gcc/tree-cfg.c | 101 +++++++++++++++++- > libgomp/oacc-plugin.c | 1 - > 10 files changed, 186 insertions(+), 10 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-1.c > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-2.c > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-3.c > create mode 100644 gcc/testsuite/g++.dg/Wunreachable-code-5.C > create mode 100644 gcc/testsuite/gcc.dg/Wunreachable-code-4.c > > diff --git a/gcc/common.opt b/gcc/common.opt > index 755e1a233b7..0a58cb8a668 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -806,8 +806,8 @@ Common Var(warn_maybe_uninitialized) Warning EnabledBy(Wuninitialized) > Warn about maybe uninitialized automatic variables. > > Wunreachable-code > -Common Ignore Warning > -Does nothing. Preserved for backward compatibility. > +Common Var(warn_unreachable_code) Warning EnabledBy(Wextra) > +Warn about trivially unreachable code. > > Wunused > Common Var(warn_unused) Init(0) Warning > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > index 588094b1db6..26325e41efa 100644 > --- a/gcc/cp/decl.c > +++ b/gcc/cp/decl.c > @@ -17571,7 +17571,14 @@ finish_function (bool inline_p) > { > /* Make it so that `main' always returns 0 by default. */ > if (DECL_MAIN_P (current_function_decl)) > - finish_return_stmt (integer_zero_node); > + { > + /* Hack. We don't want the middle-end to warn that this return > + is unreachable, so we mark its location as special. */ > + auto saved_il = input_location; > + input_location = BUILTINS_LOCATION; > + finish_return_stmt (integer_zero_node); > + input_location = saved_il; > + } > > if (use_eh_spec_block (current_function_decl)) > finish_eh_spec_block (TYPE_RAISES_EXCEPTIONS > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 36fe96b441b..62643e51915 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -267,7 +267,7 @@ in the following sections. > -Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol > -Wsized-deallocation -Wsuggest-final-methods @gol > -Wsuggest-final-types -Wsuggest-override @gol > --Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol > +-Wno-terminate -Wunreachable-code -Wuseless-cast -Wno-vexing-parse @gol > -Wvirtual-inheritance @gol > -Wno-virtual-move-assign -Wvolatile -Wzero-as-null-pointer-constant} > > @@ -4357,6 +4357,12 @@ annotations. > Warn about overriding virtual functions that are not marked with the > @code{override} keyword. > > +@item -Wunreachable-code > +@opindex Wunreachable-code > +@opindex Wno-unreachable-code > +Warn about code that is trivially unreachable before optimizing. > +@option{-Wunreachable-code} is enabled by @option{-Wextra}. > + > @item -Wuseless-cast @r{(C++ and Objective-C++ only)} > @opindex Wuseless-cast > @opindex Wno-useless-cast > @@ -5713,6 +5719,7 @@ name is still supported, but the newer name is more descriptive.) > -Wredundant-move @r{(only for C++)} @gol > -Wtype-limits @gol > -Wuninitialized @gol > +-Wunreachable-code @gol > -Wshift-negative-value @r{(in C++03 and in C99 and newer)} @gol > -Wunused-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)} @gol > -Wunused-but-set-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)}} > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-1.c b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c > new file mode 100644 > index 00000000000..0ca6c00e7fb > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c > @@ -0,0 +1,8 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +int main() > +{ > + /* Avoid warning on the auto-added duplicate return 0. */ > + return 0; > +} > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-2.c b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c > new file mode 100644 > index 00000000000..836d8c30cb2 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c > @@ -0,0 +1,8 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +int main() > +{ > + return 0; > + return 0; /* { dg-warning "not reachable" } */ > +} > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-3.c b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c > new file mode 100644 > index 00000000000..a01e4119c6f > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c > @@ -0,0 +1,35 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +/* Unreachable region entry has a predecessor (backedge). */ > +void foo() > +{ > + return; > + for (int i = 0; i < 5; ++i) /* { dg-warning "not reachable" } */ > + ; > +} > + > +/* Unreachable region not backwards reachable from exit. */ > +void bar1() > +{ > + return; > + __builtin_abort (); /* { dg-warning "not reachable" } */ > +} > +void bar2() > +{ > + return; > + /* Here we end up with a BB without any statements and an edge > + to itself. A location might be obtainable from the edges > + goto_locus. */ > + for (;;); /* { dg-warning "not reachable" "" { xfail *-*-* } } */ > +} > + > +/* Unreachable code in if (0) block. */ > +void baz(int *p) > +{ > + if (0) > + { > + return; /* { dg-bogus "not reachable" } */ > + *p = 0; /* { dg-warning "not reachable" } */ > + } > +} > diff --git a/gcc/testsuite/g++.dg/Wunreachable-code-5.C b/gcc/testsuite/g++.dg/Wunreachable-code-5.C > new file mode 100644 > index 00000000000..832cb6d865f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/Wunreachable-code-5.C > @@ -0,0 +1,11 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +void baz(); > +void bar2() > +{ > + if (! 0) > + return; > + /* The C++ frontend elides the if above. */ > + baz (); /* { dg-bogus "not reachable" "" { xfail *-*-* } } */ > +} > diff --git a/gcc/testsuite/gcc.dg/Wunreachable-code-4.c b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c > new file mode 100644 > index 00000000000..997ec08fb41 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c > @@ -0,0 +1,10 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +void baz(); > +void bar2() > +{ > + if (! 0) > + return; > + baz (); /* { dg-bogus "not reachable" } */ > +} > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > index 8ed8c69b5b1..5a9507d0e44 100644 > --- a/gcc/tree-cfg.c > +++ b/gcc/tree-cfg.c > @@ -239,11 +239,6 @@ build_gimple_cfg (gimple_seq seq) > /* To speed up statement iterator walks, we first purge dead labels. */ > cleanup_dead_labels (); > > - /* Group case nodes to reduce the number of edges. > - We do this after cleaning up dead labels because otherwise we miss > - a lot of obvious case merging opportunities. */ > - group_case_labels (); > - > /* Create the edges of the flowgraph. */ > discriminator_per_locus = new hash_table<locus_discrim_hasher> (13); > make_edges (); > @@ -362,6 +357,94 @@ replace_loop_annotate (void) > } > } > > +/* Mark BB and blocks forward reachable from it as BB_REACHABLE. */ > + > +static void > +mark_forward_reachable_blocks (basic_block bb) > +{ > + bb->flags |= BB_REACHABLE; > + edge_iterator ei; > + edge e; > + FOR_EACH_EDGE (e, ei, bb->succs) > + if (!(e->dest->flags & BB_REACHABLE)) > + mark_forward_reachable_blocks (e->dest); > +} > + > +/* Guess a reverse dead end from BB. */ > + > +static basic_block > +reverse_guess_deadend (basic_block bb) > +{ > + /* Quick and simple minded. */ > + if (EDGE_COUNT (bb->preds) == 0) > + return bb; > + > + /* DFS walk. */ > + auto_bb_flag visited (cfun); > + auto_bb_flag on_worklist (cfun); > + auto_vec<basic_block, 64> bbs_to_cleanup; > + auto_vec<basic_block, 64> worklist; > + worklist.quick_push (bb); > + bb->flags |= on_worklist; > + bbs_to_cleanup.safe_push (bb); > + while (!worklist.is_empty ()) > + { > + bb = worklist.pop (); > + bb->flags &= ~on_worklist; > + bb->flags |= visited; > + bool all_preds_visited = true; > + edge_iterator ei; > + edge e; > + FOR_EACH_EDGE (e, ei, bb->preds) > + if (!(e->src->flags & visited)) > + { > + if (!(e->src->flags & on_worklist)) > + { > + worklist.safe_push (e->src); > + e->src->flags |= on_worklist; > + bbs_to_cleanup.safe_push (e->src); > + } > + all_preds_visited = false; > + } > + /* Found one deadend. */ > + if (all_preds_visited) > + break; > + } > + for (auto bb2 : bbs_to_cleanup) > + bb2->flags &= ~(on_worklist|visited); > + return bb; > +} > + > +/* Warn about trivially unreachable code. */ > + > +static void > +warn_unreachable_code_post_cfg_build (void) > +{ > + find_unreachable_blocks (); > + basic_block bb; > + FOR_EACH_BB_FN (bb, cfun) > + { > + if ((bb->flags & BB_REACHABLE)) > + continue; > + /* Try to find the entry to the unreachable region. */ > + bb = reverse_guess_deadend (bb); > + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); > + gsi_next (&gsi)) > + { > + /* Suppress warnings on BUILTINS_LOCATION which is used by the > + C and C++ frontends to emit the artifical return in main. */ > + if (LOCATION_LOCUS (gimple_location (gsi_stmt (gsi))) > + > BUILTINS_LOCATION) > + warning_at (gimple_location (gsi_stmt (gsi)), > + OPT_Wunreachable_code, "statement is not reachable"); > + break; > + } > + /* Mark blocks reachable from here. And even better make > + sure to process entries to unreachable regions first. */ > + mark_forward_reachable_blocks (bb); > + } > +} > + > static unsigned int > execute_build_cfg (void) > { > @@ -374,6 +457,14 @@ execute_build_cfg (void) > fprintf (dump_file, "Scope blocks:\n"); > dump_scope_blocks (dump_file, dump_flags); > } > + > + if (warn_unreachable_code) > + warn_unreachable_code_post_cfg_build (); > + > + /* Group case nodes. We did this prior to materializing edges in > + build_gimple_cfg to reduce the number of edges, that interferes > + with the -Wunreachable-code diagnostics above. */ > + group_case_labels (); > cleanup_tree_cfg (); > > bb_to_omp_idx.release (); > diff --git a/libgomp/oacc-plugin.c b/libgomp/oacc-plugin.c > index e25b462eff0..98166fe5cd1 100644 > --- a/libgomp/oacc-plugin.c > +++ b/libgomp/oacc-plugin.c > @@ -62,7 +62,6 @@ GOMP_PLUGIN_acc_default_dim (unsigned int i) > if (i >= GOMP_DIM_MAX) > { > gomp_fatal ("invalid dimension argument: %d", i); > - return -1; > } > return goacc_default_dims[i]; > } >
On Wed, Nov 24, 2021 at 10:22 AM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > This resurrects -Wunreachable-code and implements a warning for > trivially unreachable code as of CFG construction. Most problematic > with this is the C/C++ frontend added 'return 0;' stmt in main > which the patch handles for C++ like the C frontend already does > by using BUILTINS_LOCATION. > > Another problem for future enhancement is that after CFG construction > we no longer can point to the stmt making a stmt unreachable, so > this implementation tries to warn on the first unreachable > statement of a region. It might be possible to retain a pointer > to the stmt that triggered creation of a basic-block but I'm not > sure how reliable that would be. > > So this is really a simple attempt for now, triggered by myself > running into such a coding error. As always, the perfect is the > enemy of the good. > > It does not pass bootstrap (which enables -Wextra), because of the > situation in g++.dg/Wunreachable-code-5.C where the C++ frontend > prematurely elides conditions like if (! GATHER_STATISTICS) that > evaluate to true - oddly enough it does _not_ do this for > conditions evaluating to false ... (one of the > c-c++-common/Wunreachable-code-2.c cases). > > Richard. There are several bugs about reviving -Wunreachable-code open, all for different aspects of it. Do we want to consider making it an umbrella flag that's split into multiple sub-options? Bug 46476, which you mentioned, was suggested to be -Wunreachable-code-return specifically: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46476 Meanwhile, there's also bug 92479, for a warning to be named -Wunreachable-code-break: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92479 Then there's also bug 82100, which doesn't have a name suggested for it yet: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82100 I think having separate flags for the 3 of these that are all enabled by -Wunreachable-code as an umbrella would be good. Eric > > 2021-11-24 Richard Biener <rguenther@suse.de> > > PR middle-end/46476 > * common.opt (Wunreachable-code): No longer ignored, > add warn_unreachable_code variable, enable with -Wextra. > * doc/invoke.texi (Wunreachable-code): Document. > (Wextra): Amend. > * tree-cfg.c (build_gimple_cfg): Move case label grouping... > (execute_build_cfg): ... here after new -Wunreachable-code > warnings. > (warn_unreachable_code_post_cfg_build): New function. > (mark_forward_reachable_blocks): Likewise. > (reverse_guess_deadend): Likewise. > > gcc/cp/ > * decl.c (finish_function): Set input_location to > BUILTINS_LOCATION around the code building the return 0 > for main(). > > libgomp/ > * oacc-plugin.c (GOMP_PLUGIN_acc_default_dim): Remove spurious > return. > > gcc/testsuite/ > * c-c++-common/Wunreachable-code-1.c: New testcase. > * c-c++-common/Wunreachable-code-2.c: Likewise. > * c-c++-common/Wunreachable-code-3.c: Likewise. > * gcc.dg/Wunreachable-code-4.c: Likewise. > * g++.dg/Wunreachable-code-5.C: Likewise. > --- > gcc/common.opt | 4 +- > gcc/cp/decl.c | 9 +- > gcc/doc/invoke.texi | 9 +- > .../c-c++-common/Wunreachable-code-1.c | 8 ++ > .../c-c++-common/Wunreachable-code-2.c | 8 ++ > .../c-c++-common/Wunreachable-code-3.c | 35 ++++++ > gcc/testsuite/g++.dg/Wunreachable-code-5.C | 11 ++ > gcc/testsuite/gcc.dg/Wunreachable-code-4.c | 10 ++ > gcc/tree-cfg.c | 101 +++++++++++++++++- > libgomp/oacc-plugin.c | 1 - > 10 files changed, 186 insertions(+), 10 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-1.c > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-2.c > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-3.c > create mode 100644 gcc/testsuite/g++.dg/Wunreachable-code-5.C > create mode 100644 gcc/testsuite/gcc.dg/Wunreachable-code-4.c > > diff --git a/gcc/common.opt b/gcc/common.opt > index 755e1a233b7..0a58cb8a668 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -806,8 +806,8 @@ Common Var(warn_maybe_uninitialized) Warning EnabledBy(Wuninitialized) > Warn about maybe uninitialized automatic variables. > > Wunreachable-code > -Common Ignore Warning > -Does nothing. Preserved for backward compatibility. > +Common Var(warn_unreachable_code) Warning EnabledBy(Wextra) > +Warn about trivially unreachable code. > > Wunused > Common Var(warn_unused) Init(0) Warning > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > index 588094b1db6..26325e41efa 100644 > --- a/gcc/cp/decl.c > +++ b/gcc/cp/decl.c > @@ -17571,7 +17571,14 @@ finish_function (bool inline_p) > { > /* Make it so that `main' always returns 0 by default. */ > if (DECL_MAIN_P (current_function_decl)) > - finish_return_stmt (integer_zero_node); > + { > + /* Hack. We don't want the middle-end to warn that this return > + is unreachable, so we mark its location as special. */ > + auto saved_il = input_location; > + input_location = BUILTINS_LOCATION; > + finish_return_stmt (integer_zero_node); > + input_location = saved_il; > + } > > if (use_eh_spec_block (current_function_decl)) > finish_eh_spec_block (TYPE_RAISES_EXCEPTIONS > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 36fe96b441b..62643e51915 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -267,7 +267,7 @@ in the following sections. > -Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol > -Wsized-deallocation -Wsuggest-final-methods @gol > -Wsuggest-final-types -Wsuggest-override @gol > --Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol > +-Wno-terminate -Wunreachable-code -Wuseless-cast -Wno-vexing-parse @gol > -Wvirtual-inheritance @gol > -Wno-virtual-move-assign -Wvolatile -Wzero-as-null-pointer-constant} > > @@ -4357,6 +4357,12 @@ annotations. > Warn about overriding virtual functions that are not marked with the > @code{override} keyword. > > +@item -Wunreachable-code > +@opindex Wunreachable-code > +@opindex Wno-unreachable-code > +Warn about code that is trivially unreachable before optimizing. > +@option{-Wunreachable-code} is enabled by @option{-Wextra}. > + > @item -Wuseless-cast @r{(C++ and Objective-C++ only)} > @opindex Wuseless-cast > @opindex Wno-useless-cast > @@ -5713,6 +5719,7 @@ name is still supported, but the newer name is more descriptive.) > -Wredundant-move @r{(only for C++)} @gol > -Wtype-limits @gol > -Wuninitialized @gol > +-Wunreachable-code @gol > -Wshift-negative-value @r{(in C++03 and in C99 and newer)} @gol > -Wunused-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)} @gol > -Wunused-but-set-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)}} > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-1.c b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c > new file mode 100644 > index 00000000000..0ca6c00e7fb > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c > @@ -0,0 +1,8 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +int main() > +{ > + /* Avoid warning on the auto-added duplicate return 0. */ > + return 0; > +} > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-2.c b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c > new file mode 100644 > index 00000000000..836d8c30cb2 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c > @@ -0,0 +1,8 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +int main() > +{ > + return 0; > + return 0; /* { dg-warning "not reachable" } */ > +} > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-3.c b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c > new file mode 100644 > index 00000000000..a01e4119c6f > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c > @@ -0,0 +1,35 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +/* Unreachable region entry has a predecessor (backedge). */ > +void foo() > +{ > + return; > + for (int i = 0; i < 5; ++i) /* { dg-warning "not reachable" } */ > + ; > +} > + > +/* Unreachable region not backwards reachable from exit. */ > +void bar1() > +{ > + return; > + __builtin_abort (); /* { dg-warning "not reachable" } */ > +} > +void bar2() > +{ > + return; > + /* Here we end up with a BB without any statements and an edge > + to itself. A location might be obtainable from the edges > + goto_locus. */ > + for (;;); /* { dg-warning "not reachable" "" { xfail *-*-* } } */ > +} > + > +/* Unreachable code in if (0) block. */ > +void baz(int *p) > +{ > + if (0) > + { > + return; /* { dg-bogus "not reachable" } */ > + *p = 0; /* { dg-warning "not reachable" } */ > + } > +} > diff --git a/gcc/testsuite/g++.dg/Wunreachable-code-5.C b/gcc/testsuite/g++.dg/Wunreachable-code-5.C > new file mode 100644 > index 00000000000..832cb6d865f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/Wunreachable-code-5.C > @@ -0,0 +1,11 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +void baz(); > +void bar2() > +{ > + if (! 0) > + return; > + /* The C++ frontend elides the if above. */ > + baz (); /* { dg-bogus "not reachable" "" { xfail *-*-* } } */ > +} > diff --git a/gcc/testsuite/gcc.dg/Wunreachable-code-4.c b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c > new file mode 100644 > index 00000000000..997ec08fb41 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c > @@ -0,0 +1,10 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wunreachable-code" } */ > + > +void baz(); > +void bar2() > +{ > + if (! 0) > + return; > + baz (); /* { dg-bogus "not reachable" } */ > +} > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > index 8ed8c69b5b1..5a9507d0e44 100644 > --- a/gcc/tree-cfg.c > +++ b/gcc/tree-cfg.c > @@ -239,11 +239,6 @@ build_gimple_cfg (gimple_seq seq) > /* To speed up statement iterator walks, we first purge dead labels. */ > cleanup_dead_labels (); > > - /* Group case nodes to reduce the number of edges. > - We do this after cleaning up dead labels because otherwise we miss > - a lot of obvious case merging opportunities. */ > - group_case_labels (); > - > /* Create the edges of the flowgraph. */ > discriminator_per_locus = new hash_table<locus_discrim_hasher> (13); > make_edges (); > @@ -362,6 +357,94 @@ replace_loop_annotate (void) > } > } > > +/* Mark BB and blocks forward reachable from it as BB_REACHABLE. */ > + > +static void > +mark_forward_reachable_blocks (basic_block bb) > +{ > + bb->flags |= BB_REACHABLE; > + edge_iterator ei; > + edge e; > + FOR_EACH_EDGE (e, ei, bb->succs) > + if (!(e->dest->flags & BB_REACHABLE)) > + mark_forward_reachable_blocks (e->dest); > +} > + > +/* Guess a reverse dead end from BB. */ > + > +static basic_block > +reverse_guess_deadend (basic_block bb) > +{ > + /* Quick and simple minded. */ > + if (EDGE_COUNT (bb->preds) == 0) > + return bb; > + > + /* DFS walk. */ > + auto_bb_flag visited (cfun); > + auto_bb_flag on_worklist (cfun); > + auto_vec<basic_block, 64> bbs_to_cleanup; > + auto_vec<basic_block, 64> worklist; > + worklist.quick_push (bb); > + bb->flags |= on_worklist; > + bbs_to_cleanup.safe_push (bb); > + while (!worklist.is_empty ()) > + { > + bb = worklist.pop (); > + bb->flags &= ~on_worklist; > + bb->flags |= visited; > + bool all_preds_visited = true; > + edge_iterator ei; > + edge e; > + FOR_EACH_EDGE (e, ei, bb->preds) > + if (!(e->src->flags & visited)) > + { > + if (!(e->src->flags & on_worklist)) > + { > + worklist.safe_push (e->src); > + e->src->flags |= on_worklist; > + bbs_to_cleanup.safe_push (e->src); > + } > + all_preds_visited = false; > + } > + /* Found one deadend. */ > + if (all_preds_visited) > + break; > + } > + for (auto bb2 : bbs_to_cleanup) > + bb2->flags &= ~(on_worklist|visited); > + return bb; > +} > + > +/* Warn about trivially unreachable code. */ > + > +static void > +warn_unreachable_code_post_cfg_build (void) > +{ > + find_unreachable_blocks (); > + basic_block bb; > + FOR_EACH_BB_FN (bb, cfun) > + { > + if ((bb->flags & BB_REACHABLE)) > + continue; > + /* Try to find the entry to the unreachable region. */ > + bb = reverse_guess_deadend (bb); > + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); > + gsi_next (&gsi)) > + { > + /* Suppress warnings on BUILTINS_LOCATION which is used by the > + C and C++ frontends to emit the artifical return in main. */ > + if (LOCATION_LOCUS (gimple_location (gsi_stmt (gsi))) > + > BUILTINS_LOCATION) > + warning_at (gimple_location (gsi_stmt (gsi)), > + OPT_Wunreachable_code, "statement is not reachable"); > + break; > + } > + /* Mark blocks reachable from here. And even better make > + sure to process entries to unreachable regions first. */ > + mark_forward_reachable_blocks (bb); > + } > +} > + > static unsigned int > execute_build_cfg (void) > { > @@ -374,6 +457,14 @@ execute_build_cfg (void) > fprintf (dump_file, "Scope blocks:\n"); > dump_scope_blocks (dump_file, dump_flags); > } > + > + if (warn_unreachable_code) > + warn_unreachable_code_post_cfg_build (); > + > + /* Group case nodes. We did this prior to materializing edges in > + build_gimple_cfg to reduce the number of edges, that interferes > + with the -Wunreachable-code diagnostics above. */ > + group_case_labels (); > cleanup_tree_cfg (); > > bb_to_omp_idx.release (); > diff --git a/libgomp/oacc-plugin.c b/libgomp/oacc-plugin.c > index e25b462eff0..98166fe5cd1 100644 > --- a/libgomp/oacc-plugin.c > +++ b/libgomp/oacc-plugin.c > @@ -62,7 +62,6 @@ GOMP_PLUGIN_acc_default_dim (unsigned int i) > if (i >= GOMP_DIM_MAX) > { > gomp_fatal ("invalid dimension argument: %d", i); > - return -1; > } > return goacc_default_dims[i]; > } > -- > 2.31.1
On 11/24/21 11:15, Marek Polacek wrote: > On Wed, Nov 24, 2021 at 04:21:31PM +0100, Richard Biener via Gcc-patches wrote: >> This resurrects -Wunreachable-code and implements a warning for >> trivially unreachable code as of CFG construction. Most problematic >> with this is the C/C++ frontend added 'return 0;' stmt in main >> which the patch handles for C++ like the C frontend already does >> by using BUILTINS_LOCATION. >> >> Another problem for future enhancement is that after CFG construction >> we no longer can point to the stmt making a stmt unreachable, so >> this implementation tries to warn on the first unreachable >> statement of a region. It might be possible to retain a pointer >> to the stmt that triggered creation of a basic-block but I'm not >> sure how reliable that would be. >> >> So this is really a simple attempt for now, triggered by myself >> running into such a coding error. As always, the perfect is the >> enemy of the good. >> >> It does not pass bootstrap (which enables -Wextra), because of the >> situation in g++.dg/Wunreachable-code-5.C where the C++ frontend >> prematurely elides conditions like if (! GATHER_STATISTICS) that >> evaluate to true - oddly enough it does _not_ do this for >> conditions evaluating to false ... (one of the >> c-c++-common/Wunreachable-code-2.c cases). > > I've taken a look into the C++ thing. This is genericize_if_stmt: > if we have > > if (0) > return; > > then cond is integer_zerop, then_ is a return_expr, but since it has > TREE_SIDE_EFFECTS, we create a COND_EXPR. For > > if (!0) > return; > > we do > 170 else if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_)) > 171 stmt = then_; > which elides the if completely. > > So it seems it would help if we avoided eliding the if stmt if > -Wunreachable-code is in effect. I'd be happy to make that change, > if it sounds sane. Sure. Currently the front end does various constant folding as part of genericization, as I recall because there were missed optimizations without it. Is this particular one undesirable because it's at the statement level rather than within an expression? Jason
On Wed, 24 Nov 2021, Jason Merrill wrote: > On 11/24/21 11:15, Marek Polacek wrote: > > On Wed, Nov 24, 2021 at 04:21:31PM +0100, Richard Biener via Gcc-patches > > wrote: > >> This resurrects -Wunreachable-code and implements a warning for > >> trivially unreachable code as of CFG construction. Most problematic > >> with this is the C/C++ frontend added 'return 0;' stmt in main > >> which the patch handles for C++ like the C frontend already does > >> by using BUILTINS_LOCATION. > >> > >> Another problem for future enhancement is that after CFG construction > >> we no longer can point to the stmt making a stmt unreachable, so > >> this implementation tries to warn on the first unreachable > >> statement of a region. It might be possible to retain a pointer > >> to the stmt that triggered creation of a basic-block but I'm not > >> sure how reliable that would be. > >> > >> So this is really a simple attempt for now, triggered by myself > >> running into such a coding error. As always, the perfect is the > >> enemy of the good. > >> > >> It does not pass bootstrap (which enables -Wextra), because of the > >> situation in g++.dg/Wunreachable-code-5.C where the C++ frontend > >> prematurely elides conditions like if (! GATHER_STATISTICS) that > >> evaluate to true - oddly enough it does _not_ do this for > >> conditions evaluating to false ... (one of the > >> c-c++-common/Wunreachable-code-2.c cases). > > > > I've taken a look into the C++ thing. This is genericize_if_stmt: > > if we have > > > > if (0) > > return; > > > > then cond is integer_zerop, then_ is a return_expr, but since it has > > TREE_SIDE_EFFECTS, we create a COND_EXPR. For > > > > if (!0) > > return; > > > > we do > > 170 else if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_)) > > 171 stmt = then_; > > which elides the if completely. > > > > So it seems it would help if we avoided eliding the if stmt if > > -Wunreachable-code is in effect. I'd be happy to make that change, > > if it sounds sane. Yes, that seems to work. > Sure. > > Currently the front end does various constant folding as part of > genericization, as I recall because there were missed optimizations without > it. Is this particular one undesirable because it's at the statement level > rather than within an expression? It's undesirable because it short-circuits control flow and thus if (0) return; foo (); becomes return; foo (); which looks exactly like a case we want to diagnose (very likely a programming error). So yes, it applies to the statement level and there only to control statements. Richard.
On Wed, 24 Nov 2021, Martin Sebor wrote: > On 11/24/21 8:21 AM, Richard Biener via Gcc-patches wrote: > > This resurrects -Wunreachable-code and implements a warning for > > trivially unreachable code as of CFG construction. Most problematic > > with this is the C/C++ frontend added 'return 0;' stmt in main > > which the patch handles for C++ like the C frontend already does > > by using BUILTINS_LOCATION. > > > > Another problem for future enhancement is that after CFG construction > > we no longer can point to the stmt making a stmt unreachable, so > > this implementation tries to warn on the first unreachable > > statement of a region. It might be possible to retain a pointer > > to the stmt that triggered creation of a basic-block but I'm not > > sure how reliable that would be. > > > > So this is really a simple attempt for now, triggered by myself > > running into such a coding error. As always, the perfect is the > > enemy of the good. > > > > It does not pass bootstrap (which enables -Wextra), because of the > > situation in g++.dg/Wunreachable-code-5.C where the C++ frontend > > prematurely elides conditions like if (! GATHER_STATISTICS) that > > evaluate to true - oddly enough it does _not_ do this for > > conditions evaluating to false ... (one of the > > c-c++-common/Wunreachable-code-2.c cases). > > I'm very much in favor of reviving the warning, even in its > current simplistic form. I especially welcome the suggestion > to enhance it in the future, including adjusting its schedule > among other passes (or adding other, later invocations). It > would be overly constraining to consider this placement ideal > or set in stone. > > Among possible enhancements worth considering is handling > constant conditionals like: > > int f (void) > { > if (1) > return 0; > else > return 1; <<< warn > } > > int g (void) > { > if (1) > return 0; > return 1; <<< warn also in C (not just in C++) > } I think both cases are undesirable to warn on, but both would be possible to implement during parsing and only there they would possibly make sense (you have to consider the if (1) resulting from macro expansion or template instantiation which are the undesirable to warn about cases - not to mention disabled code that wants to remain syntactically correct, something that cannot be achieved with #if 0) > By the way, a related feature that would be useful and that's > been requested in the past is warning for stores with no effect, > as in: > > int i; > i = 1; > i = 2; <<< warn here > > The detection of the simple cases like the one above can also > be almost trivially implemented. Likewise the above can be done during parsing where it's more appearant whether the stmts are the same. Richard. > Martin > > > > > Richard. > > > > 2021-11-24 Richard Biener <rguenther@suse.de> > > > > PR middle-end/46476 > > * common.opt (Wunreachable-code): No longer ignored, > > add warn_unreachable_code variable, enable with -Wextra. > > * doc/invoke.texi (Wunreachable-code): Document. > > (Wextra): Amend. > > * tree-cfg.c (build_gimple_cfg): Move case label grouping... > > (execute_build_cfg): ... here after new -Wunreachable-code > > warnings. > > (warn_unreachable_code_post_cfg_build): New function. > > (mark_forward_reachable_blocks): Likewise. > > (reverse_guess_deadend): Likewise. > > > > gcc/cp/ > > * decl.c (finish_function): Set input_location to > > BUILTINS_LOCATION around the code building the return 0 > > for main(). > > > > libgomp/ > > * oacc-plugin.c (GOMP_PLUGIN_acc_default_dim): Remove spurious > > return. > > > > gcc/testsuite/ > > * c-c++-common/Wunreachable-code-1.c: New testcase. > > * c-c++-common/Wunreachable-code-2.c: Likewise. > > * c-c++-common/Wunreachable-code-3.c: Likewise. > > * gcc.dg/Wunreachable-code-4.c: Likewise. > > * g++.dg/Wunreachable-code-5.C: Likewise. > > --- > > gcc/common.opt | 4 +- > > gcc/cp/decl.c | 9 +- > > gcc/doc/invoke.texi | 9 +- > > .../c-c++-common/Wunreachable-code-1.c | 8 ++ > > .../c-c++-common/Wunreachable-code-2.c | 8 ++ > > .../c-c++-common/Wunreachable-code-3.c | 35 ++++++ > > gcc/testsuite/g++.dg/Wunreachable-code-5.C | 11 ++ > > gcc/testsuite/gcc.dg/Wunreachable-code-4.c | 10 ++ > > gcc/tree-cfg.c | 101 +++++++++++++++++- > > libgomp/oacc-plugin.c | 1 - > > 10 files changed, 186 insertions(+), 10 deletions(-) > > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-1.c > > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-2.c > > create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-3.c > > create mode 100644 gcc/testsuite/g++.dg/Wunreachable-code-5.C > > create mode 100644 gcc/testsuite/gcc.dg/Wunreachable-code-4.c > > > > diff --git a/gcc/common.opt b/gcc/common.opt > > index 755e1a233b7..0a58cb8a668 100644 > > --- a/gcc/common.opt > > +++ b/gcc/common.opt > > @@ -806,8 +806,8 @@ Common Var(warn_maybe_uninitialized) Warning > > EnabledBy(Wuninitialized) > > Warn about maybe uninitialized automatic variables. > > > > Wunreachable-code > > -Common Ignore Warning > > -Does nothing. Preserved for backward compatibility. > > +Common Var(warn_unreachable_code) Warning EnabledBy(Wextra) > > +Warn about trivially unreachable code. > > > > Wunused > > Common Var(warn_unused) Init(0) Warning > > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > > index 588094b1db6..26325e41efa 100644 > > --- a/gcc/cp/decl.c > > +++ b/gcc/cp/decl.c > > @@ -17571,7 +17571,14 @@ finish_function (bool inline_p) > > { > > /* Make it so that `main' always returns 0 by default. */ > > if (DECL_MAIN_P (current_function_decl)) > > - finish_return_stmt (integer_zero_node); > > + { > > + /* Hack. We don't want the middle-end to warn that this return > > + is unreachable, so we mark its location as special. */ > > + auto saved_il = input_location; > > + input_location = BUILTINS_LOCATION; > > + finish_return_stmt (integer_zero_node); > > + input_location = saved_il; > > + } > > > > if (use_eh_spec_block (current_function_decl)) > > finish_eh_spec_block (TYPE_RAISES_EXCEPTIONS > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > > index 36fe96b441b..62643e51915 100644 > > --- a/gcc/doc/invoke.texi > > +++ b/gcc/doc/invoke.texi > > @@ -267,7 +267,7 @@ in the following sections. > > -Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol > > -Wsized-deallocation -Wsuggest-final-methods @gol > > -Wsuggest-final-types -Wsuggest-override @gol > > --Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol > > +-Wno-terminate -Wunreachable-code -Wuseless-cast -Wno-vexing-parse @gol > > -Wvirtual-inheritance @gol > > -Wno-virtual-move-assign -Wvolatile -Wzero-as-null-pointer-constant} > > > > @@ -4357,6 +4357,12 @@ annotations. > > Warn about overriding virtual functions that are not marked with the > > @code{override} keyword. > > > > +@item -Wunreachable-code > > +@opindex Wunreachable-code > > +@opindex Wno-unreachable-code > > +Warn about code that is trivially unreachable before optimizing. > > +@option{-Wunreachable-code} is enabled by @option{-Wextra}. > > + > > @item -Wuseless-cast @r{(C++ and Objective-C++ only)} > > @opindex Wuseless-cast > > @opindex Wno-useless-cast > > @@ -5713,6 +5719,7 @@ name is still supported, but the newer name is more > > descriptive.) > > -Wredundant-move @r{(only for C++)} @gol > > -Wtype-limits @gol > > -Wuninitialized @gol > > +-Wunreachable-code @gol > > -Wshift-negative-value @r{(in C++03 and in C99 and newer)} @gol > > -Wunused-parameter @r{(only with} @option{-Wunused} @r{or} > > @option{-Wall}@r{)} @gol > > -Wunused-but-set-parameter @r{(only with} @option{-Wunused} @r{or} > > @option{-Wall}@r{)}} > > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-1.c > > b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c > > new file mode 100644 > > index 00000000000..0ca6c00e7fb > > --- /dev/null > > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c > > @@ -0,0 +1,8 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-Wunreachable-code" } */ > > + > > +int main() > > +{ > > + /* Avoid warning on the auto-added duplicate return 0. */ > > + return 0; > > +} > > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-2.c > > b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c > > new file mode 100644 > > index 00000000000..836d8c30cb2 > > --- /dev/null > > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c > > @@ -0,0 +1,8 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-Wunreachable-code" } */ > > + > > +int main() > > +{ > > + return 0; > > + return 0; /* { dg-warning "not reachable" } */ > > +} > > diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-3.c > > b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c > > new file mode 100644 > > index 00000000000..a01e4119c6f > > --- /dev/null > > +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c > > @@ -0,0 +1,35 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-Wunreachable-code" } */ > > + > > +/* Unreachable region entry has a predecessor (backedge). */ > > +void foo() > > +{ > > + return; > > + for (int i = 0; i < 5; ++i) /* { dg-warning "not reachable" } */ > > + ; > > +} > > + > > +/* Unreachable region not backwards reachable from exit. */ > > +void bar1() > > +{ > > + return; > > + __builtin_abort (); /* { dg-warning "not reachable" } */ > > +} > > +void bar2() > > +{ > > + return; > > + /* Here we end up with a BB without any statements and an edge > > + to itself. A location might be obtainable from the edges > > + goto_locus. */ > > + for (;;); /* { dg-warning "not reachable" "" { xfail *-*-* } } */ > > +} > > + > > +/* Unreachable code in if (0) block. */ > > +void baz(int *p) > > +{ > > + if (0) > > + { > > + return; /* { dg-bogus "not reachable" } */ > > + *p = 0; /* { dg-warning "not reachable" } */ > > + } > > +} > > diff --git a/gcc/testsuite/g++.dg/Wunreachable-code-5.C > > b/gcc/testsuite/g++.dg/Wunreachable-code-5.C > > new file mode 100644 > > index 00000000000..832cb6d865f > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/Wunreachable-code-5.C > > @@ -0,0 +1,11 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-Wunreachable-code" } */ > > + > > +void baz(); > > +void bar2() > > +{ > > + if (! 0) > > + return; > > + /* The C++ frontend elides the if above. */ > > + baz (); /* { dg-bogus "not reachable" "" { xfail *-*-* } } */ > > +} > > diff --git a/gcc/testsuite/gcc.dg/Wunreachable-code-4.c > > b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c > > new file mode 100644 > > index 00000000000..997ec08fb41 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c > > @@ -0,0 +1,10 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-Wunreachable-code" } */ > > + > > +void baz(); > > +void bar2() > > +{ > > + if (! 0) > > + return; > > + baz (); /* { dg-bogus "not reachable" } */ > > +} > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > > index 8ed8c69b5b1..5a9507d0e44 100644 > > --- a/gcc/tree-cfg.c > > +++ b/gcc/tree-cfg.c > > @@ -239,11 +239,6 @@ build_gimple_cfg (gimple_seq seq) > > /* To speed up statement iterator walks, we first purge dead labels. */ > > cleanup_dead_labels (); > > - /* Group case nodes to reduce the number of edges. > > - We do this after cleaning up dead labels because otherwise we miss > > - a lot of obvious case merging opportunities. */ > > - group_case_labels (); > > - > > /* Create the edges of the flowgraph. */ > > discriminator_per_locus = new hash_table<locus_discrim_hasher> (13); > > make_edges (); > > @@ -362,6 +357,94 @@ replace_loop_annotate (void) > > } > > } > > > > +/* Mark BB and blocks forward reachable from it as BB_REACHABLE. */ > > + > > +static void > > +mark_forward_reachable_blocks (basic_block bb) > > +{ > > + bb->flags |= BB_REACHABLE; > > + edge_iterator ei; > > + edge e; > > + FOR_EACH_EDGE (e, ei, bb->succs) > > + if (!(e->dest->flags & BB_REACHABLE)) > > + mark_forward_reachable_blocks (e->dest); > > +} > > + > > +/* Guess a reverse dead end from BB. */ > > + > > +static basic_block > > +reverse_guess_deadend (basic_block bb) > > +{ > > + /* Quick and simple minded. */ > > + if (EDGE_COUNT (bb->preds) == 0) > > + return bb; > > + > > + /* DFS walk. */ > > + auto_bb_flag visited (cfun); > > + auto_bb_flag on_worklist (cfun); > > + auto_vec<basic_block, 64> bbs_to_cleanup; > > + auto_vec<basic_block, 64> worklist; > > + worklist.quick_push (bb); > > + bb->flags |= on_worklist; > > + bbs_to_cleanup.safe_push (bb); > > + while (!worklist.is_empty ()) > > + { > > + bb = worklist.pop (); > > + bb->flags &= ~on_worklist; > > + bb->flags |= visited; > > + bool all_preds_visited = true; > > + edge_iterator ei; > > + edge e; > > + FOR_EACH_EDGE (e, ei, bb->preds) > > + if (!(e->src->flags & visited)) > > + { > > + if (!(e->src->flags & on_worklist)) > > + { > > + worklist.safe_push (e->src); > > + e->src->flags |= on_worklist; > > + bbs_to_cleanup.safe_push (e->src); > > + } > > + all_preds_visited = false; > > + } > > + /* Found one deadend. */ > > + if (all_preds_visited) > > + break; > > + } > > + for (auto bb2 : bbs_to_cleanup) > > + bb2->flags &= ~(on_worklist|visited); > > + return bb; > > +} > > + > > +/* Warn about trivially unreachable code. */ > > + > > +static void > > +warn_unreachable_code_post_cfg_build (void) > > +{ > > + find_unreachable_blocks (); > > + basic_block bb; > > + FOR_EACH_BB_FN (bb, cfun) > > + { > > + if ((bb->flags & BB_REACHABLE)) > > + continue; > > + /* Try to find the entry to the unreachable region. */ > > + bb = reverse_guess_deadend (bb); > > + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); > > + gsi_next (&gsi)) > > + { > > + /* Suppress warnings on BUILTINS_LOCATION which is used by the > > + C and C++ frontends to emit the artifical return in main. */ > > + if (LOCATION_LOCUS (gimple_location (gsi_stmt (gsi))) > > + > BUILTINS_LOCATION) > > + warning_at (gimple_location (gsi_stmt (gsi)), > > + OPT_Wunreachable_code, "statement is not reachable"); > > + break; > > + } > > + /* Mark blocks reachable from here. And even better make > > + sure to process entries to unreachable regions first. */ > > + mark_forward_reachable_blocks (bb); > > + } > > +} > > + > > static unsigned int > > execute_build_cfg (void) > > { > > @@ -374,6 +457,14 @@ execute_build_cfg (void) > > fprintf (dump_file, "Scope blocks:\n"); > > dump_scope_blocks (dump_file, dump_flags); > > } > > + > > + if (warn_unreachable_code) > > + warn_unreachable_code_post_cfg_build (); > > + > > + /* Group case nodes. We did this prior to materializing edges in > > + build_gimple_cfg to reduce the number of edges, that interferes > > + with the -Wunreachable-code diagnostics above. */ > > + group_case_labels (); > > cleanup_tree_cfg (); > > > > bb_to_omp_idx.release (); > > diff --git a/libgomp/oacc-plugin.c b/libgomp/oacc-plugin.c > > index e25b462eff0..98166fe5cd1 100644 > > --- a/libgomp/oacc-plugin.c > > +++ b/libgomp/oacc-plugin.c > > @@ -62,7 +62,6 @@ GOMP_PLUGIN_acc_default_dim (unsigned int i) > > if (i >= GOMP_DIM_MAX) > > { > > gomp_fatal ("invalid dimension argument: %d", i); > > - return -1; > > } > > return goacc_default_dims[i]; > > } > > > > >
On Wed, 24 Nov 2021, Michael Matz wrote: > Hello, > > On Wed, 24 Nov 2021, Richard Biener wrote: > > > >> +/* Unreachable code in if (0) block. */ > > >> +void baz(int *p) > > >> +{ > > >> + if (0) > > >> + { > > >> + return; /* { dg-bogus "not reachable" } */ > > > > > >Hmm? Why are you explicitely saying that warning here would be bogus? > > > > Because I don't think we want to warn here. Such code is common from > > template instantiation or macro expansion. > > Like all code with an (const-propagated) explicit 'if (0)', which is of > course the reason why -Wunreachable-code is a challenge. OK, so I probably shouldn't have taken -Wunreachable-code but named it somehow differently. We want to diagnose obvious programming mistakes, not (source code) optimization opportunities. So int foo (int i) { return i; i += 1; return i; } should be diagnosed for example but not so int foo (int i) { if (USE_NOOP_FOO) return i; i += 1; return i; } and compiling with -DUSE_NOOP_FOO=1 > IOW: I could > accept your argument but then wonder why you want to warn about the second > statement of the guarded block. The situation was: > > if (0) { > return; // (1) don't warn here? > whatever++; // (2) but warn here? because as said above, the whatever++ will never be reachable even if you change the condition in the if(). See my response to Martin where I said I think if (0) of a block is a good way to comment it out but keep it syntactically correct. > } > > That seems even more confusing. So you don't want to warn about > unreachable code (the 'return') but you do want to warn about unreachable > code within unreachable code (point (2) is unreachable because of the > if(0) and because of the return). If your worry is macro/template > expansion resulting if(0)'s then I don't see why you would only disable > warnings for some of the statements therein. The point is not to disable the warning for some statements therein but to avoid diagnosing following stmts. > It seems we are actually interested in code unreachable via fallthrough or > labels, not in all unreachable code, so maybe the warning is mis-named. Yes, that's definitely the case - I was too lazy to re-use the old option name here. But I don't have a good name at hand, maybe clang has an option covering the cases I'm thinking about. Btw, the diagnostic spotted qsort_chk doing if (CMP (i1, i2)) break; else if (CMP (i2, i1)) return ERR2 (i1, i2); where ERR2 expands to a call to a noreturn void "returning" qsort_chk_error, so the 'return' stmt is not reachable. Not exactly a bug but somewhat difficult to avoid the diagnostic for. I suppose the pointless 'return' is to make it more visible that the loop terminates here (albeit we don't return normally). Likewise we diagnose (c_tree_equal): default: gcc_unreachable (); } /* We can get here with --disable-checking. */ return false; where the 'return false' is never reachable. The return was likely inserted to avoid very strange error paths then the unreachable falls through to some other random function. > Btw. what does the code now do about this situation: > > if (0) { > something++; // 1 > return; // 2 > somethingelse++; // 3 > } > > does it warn at (1) or not? (I assume it unconditionally warns at (3)) It warns at (3). It basically assumes that if (0) might become if (1) in some other configuration and thus the diagnostic is difficult to silence in source. Any suggestion for a better option name? Richard.
On Thu, 25 Nov 2021, Richard Biener wrote: > On Wed, 24 Nov 2021, Michael Matz wrote: > > > Hello, > > > > On Wed, 24 Nov 2021, Richard Biener wrote: > > > > > >> +/* Unreachable code in if (0) block. */ > > > >> +void baz(int *p) > > > >> +{ > > > >> + if (0) > > > >> + { > > > >> + return; /* { dg-bogus "not reachable" } */ > > > > > > > >Hmm? Why are you explicitely saying that warning here would be bogus? > > > > > > Because I don't think we want to warn here. Such code is common from > > > template instantiation or macro expansion. > > > > Like all code with an (const-propagated) explicit 'if (0)', which is of > > course the reason why -Wunreachable-code is a challenge. > > OK, so I probably shouldn't have taken -Wunreachable-code but named > it somehow differently. We want to diagnose obvious programming > mistakes, not (source code) optimization opportunities. So > > int foo (int i) > { > return i; > i += 1; > return i; > } > > should be diagnosed for example but not so > > int foo (int i) > { > if (USE_NOOP_FOO) > return i; > i += 1; > return i; > } > > and compiling with -DUSE_NOOP_FOO=1 > > > IOW: I could > > accept your argument but then wonder why you want to warn about the second > > statement of the guarded block. The situation was: > > > > if (0) { > > return; // (1) don't warn here? > > whatever++; // (2) but warn here? > > because as said above, the whatever++ will never be reachable even if > you change the condition in the if(). See my response to Martin where > I said I think if (0) of a block is a good way to comment it out > but keep it syntactically correct. > > > } > > > > That seems even more confusing. So you don't want to warn about > > unreachable code (the 'return') but you do want to warn about unreachable > > code within unreachable code (point (2) is unreachable because of the > > if(0) and because of the return). If your worry is macro/template > > expansion resulting if(0)'s then I don't see why you would only disable > > warnings for some of the statements therein. > > The point is not to disable the warning for some statements therein > but to avoid diagnosing following stmts. > > > It seems we are actually interested in code unreachable via fallthrough or > > labels, not in all unreachable code, so maybe the warning is mis-named. > > Yes, that's definitely the case - I was too lazy to re-use the old > option name here. But I don't have a good name at hand, maybe clang > has an option covering the cases I'm thinking about. > > Btw, the diagnostic spotted qsort_chk doing > > if (CMP (i1, i2)) > break; > else if (CMP (i2, i1)) > return ERR2 (i1, i2); > > where ERR2 expands to a call to a noreturn void "returning" > qsort_chk_error, so the 'return' stmt is not reachable. Not exactly > a bug but somewhat difficult to avoid the diagnostic for. I suppose > the pointless 'return' is to make it more visible that the loop > terminates here (albeit we don't return normally). > > Likewise we diagnose (c_tree_equal): > > default: > gcc_unreachable (); > } > /* We can get here with --disable-checking. */ > return false; > > where the 'return false' is never reachable. The return was likely > inserted to avoid very strange error paths then the unreachable > falls through to some other random function. It also finds this strange code in label_rtx_for_bb: /* Find the tree label if it is present. */ for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) { glabel *lab_stmt; lab_stmt = dyn_cast <glabel *> (gsi_stmt (gsi)); if (!lab_stmt) break; lab = gimple_label_label (lab_stmt); if (DECL_NONLOCAL (lab)) break; return jump_target_rtx (lab); } diagnosing /home/rguenther/src/trunk/gcc/cfgexpand.c:2476:60: error: statement is not reachable [-Werror=unreachable-code] 2476 | for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) | ~~~~~~~~~^~~~~~ indeed the loop looks pointless. Unless the DECL_NONLOCAL case was meant to continue; Richard.
On Thu, 25 Nov 2021, Richard Biener wrote: > On Wed, 24 Nov 2021, Jason Merrill wrote: > > > On 11/24/21 11:15, Marek Polacek wrote: > > > On Wed, Nov 24, 2021 at 04:21:31PM +0100, Richard Biener via Gcc-patches > > > wrote: > > >> This resurrects -Wunreachable-code and implements a warning for > > >> trivially unreachable code as of CFG construction. Most problematic > > >> with this is the C/C++ frontend added 'return 0;' stmt in main > > >> which the patch handles for C++ like the C frontend already does > > >> by using BUILTINS_LOCATION. > > >> > > >> Another problem for future enhancement is that after CFG construction > > >> we no longer can point to the stmt making a stmt unreachable, so > > >> this implementation tries to warn on the first unreachable > > >> statement of a region. It might be possible to retain a pointer > > >> to the stmt that triggered creation of a basic-block but I'm not > > >> sure how reliable that would be. > > >> > > >> So this is really a simple attempt for now, triggered by myself > > >> running into such a coding error. As always, the perfect is the > > >> enemy of the good. > > >> > > >> It does not pass bootstrap (which enables -Wextra), because of the > > >> situation in g++.dg/Wunreachable-code-5.C where the C++ frontend > > >> prematurely elides conditions like if (! GATHER_STATISTICS) that > > >> evaluate to true - oddly enough it does _not_ do this for > > >> conditions evaluating to false ... (one of the > > >> c-c++-common/Wunreachable-code-2.c cases). > > > > > > I've taken a look into the C++ thing. This is genericize_if_stmt: > > > if we have > > > > > > if (0) > > > return; > > > > > > then cond is integer_zerop, then_ is a return_expr, but since it has > > > TREE_SIDE_EFFECTS, we create a COND_EXPR. For > > > > > > if (!0) > > > return; > > > > > > we do > > > 170 else if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_)) > > > 171 stmt = then_; > > > which elides the if completely. > > > > > > So it seems it would help if we avoided eliding the if stmt if > > > -Wunreachable-code is in effect. I'd be happy to make that change, > > > if it sounds sane. > > Yes, that seems to work. > > > Sure. > > > > Currently the front end does various constant folding as part of > > genericization, as I recall because there were missed optimizations without > > it. Is this particular one undesirable because it's at the statement level > > rather than within an expression? > > It's undesirable because it short-circuits control flow and thus > > if (0) > return; > foo (); > > becomes > > return; > foo (); > > which looks exactly like a case we want to diagnose (very likely a > programming error). > > So yes, it applies to the statement level and there only to control > statements. So another case in GCC is if (WORDS_BIG_ENDIAN == BYTES_BIG_ENDIAN) ... else { /* Assert that we're only dealing with the PDP11 case. */ gcc_assert (!BYTES_BIG_ENDIAN); gcc_assert (WORDS_BIG_ENDIAN); cpp_define (pfile, "__BYTE_ORDER__=__ORDER_PDP_ENDIAN__"); where that macro expands to ((void)(!(!0) ? fancy_abort ("/home/rguenther/src/trunk/gcc/cppbuiltin.c", 180, __FUNCTION__), 0 : 0)); ((void)(!(0) ? fancy_abort ("/home/rguenther/src/trunk/gcc/cppbuiltin.c", 181, __FUNCTION__), 0 : 0)); cpp_define (pfile, "__BYTE_ORDER__=__ORDER_PDP_ENDIAN__"); and the frontend elides the COND_EXPRs making the cpp_define unreachable. That's only exposed because we no longer elide the if (1) guardingt this else path ... Also this is a case where we definitely do not want to diagnose that either the else or the true path is statically unreachable. Richard.
Hello, On Thu, 25 Nov 2021, Richard Biener wrote: > > Yes, that's definitely the case - I was too lazy to re-use the old > > option name here. But I don't have a good name at hand, maybe clang > > has an option covering the cases I'm thinking about. As you asked: I already have difficulties to describe the exact semantics of the warning in sentences, so I don't find a good name either :-) > > Btw, the diagnostic spotted qsort_chk doing > > > > if (CMP (i1, i2)) > > break; > > else if (CMP (i2, i1)) > > return ERR2 (i1, i2); > > > > where ERR2 expands to a call to a noreturn void "returning" > > qsort_chk_error, so the 'return' stmt is not reachable. Not exactly > > a bug but somewhat difficult to avoid the diagnostic for. I suppose > > the pointless 'return' is to make it more visible that the loop > > terminates here (albeit we don't return normally). Tough one. You could also disable the warning when the fallthrough doesn't exist because of a non-returning call. If it's supposed to find obvious programming mistakes it might make sense to regard all function calls the same, like they look, i.e. as function calls that can return. Or it might make sense to not do that for programmers who happen to know about non-returning functions. :-/ > It also finds this strange code in label_rtx_for_bb: So the warning is definitely useful! > indeed the loop looks pointless. Unless the DECL_NONLOCAL case was > meant to continue; It's like that since it was introduced in 2007. It's an invariant that DECL_NONLOCAL labels are first in a BB and are not followed by normal labels, so a 'continue' wouldn't change anything; the loop is useless. Ciao, Michael.
On Thu, 25 Nov 2021, Michael Matz wrote: > Hello, > > On Thu, 25 Nov 2021, Richard Biener wrote: > > > > Yes, that's definitely the case - I was too lazy to re-use the old > > > option name here. But I don't have a good name at hand, maybe clang > > > has an option covering the cases I'm thinking about. > > As you asked: I already have difficulties to describe the exact semantics > of the warning in sentences, so I don't find a good name either :-) It diagnoses some cases of unreachable code so -Wunreachable-code sounded like the obvious fit :P But names can create (wrong) expectation ... clang has -Wunreachable-code{,-aggressive,-break,-fallthrough,-loop-increment,-return} but documentation is very sparse, -break and -return are what -aggressive enables. > > > Btw, the diagnostic spotted qsort_chk doing > > > > > > if (CMP (i1, i2)) > > > break; > > > else if (CMP (i2, i1)) > > > return ERR2 (i1, i2); > > > > > > where ERR2 expands to a call to a noreturn void "returning" > > > qsort_chk_error, so the 'return' stmt is not reachable. Not exactly > > > a bug but somewhat difficult to avoid the diagnostic for. I suppose > > > the pointless 'return' is to make it more visible that the loop > > > terminates here (albeit we don't return normally). > > Tough one. You could also disable the warning when the fallthrough > doesn't exist because of a non-returning call. If it's supposed to find > obvious programming mistakes it might make sense to regard all function > calls the same, like they look, i.e. as function calls that can return. > Or it might make sense to not do that for programmers who happen to know > about non-returning functions. :-/ > > > It also finds this strange code in label_rtx_for_bb: > > So the warning is definitely useful! Yep, also found some more real issues. But I'm not managing it to get clean in a bootstrap due to some remaining issues with early folding exposing unreachable code following gcc_assert()s Richard.
On 11/25/21 12:57 AM, Richard Biener wrote: > On Wed, 24 Nov 2021, Martin Sebor wrote: > >> On 11/24/21 8:21 AM, Richard Biener via Gcc-patches wrote: >>> This resurrects -Wunreachable-code and implements a warning for >>> trivially unreachable code as of CFG construction. Most problematic >>> with this is the C/C++ frontend added 'return 0;' stmt in main >>> which the patch handles for C++ like the C frontend already does >>> by using BUILTINS_LOCATION. >>> >>> Another problem for future enhancement is that after CFG construction >>> we no longer can point to the stmt making a stmt unreachable, so >>> this implementation tries to warn on the first unreachable >>> statement of a region. It might be possible to retain a pointer >>> to the stmt that triggered creation of a basic-block but I'm not >>> sure how reliable that would be. >>> >>> So this is really a simple attempt for now, triggered by myself >>> running into such a coding error. As always, the perfect is the >>> enemy of the good. >>> >>> It does not pass bootstrap (which enables -Wextra), because of the >>> situation in g++.dg/Wunreachable-code-5.C where the C++ frontend >>> prematurely elides conditions like if (! GATHER_STATISTICS) that >>> evaluate to true - oddly enough it does _not_ do this for >>> conditions evaluating to false ... (one of the >>> c-c++-common/Wunreachable-code-2.c cases). >> >> I'm very much in favor of reviving the warning, even in its >> current simplistic form. I especially welcome the suggestion >> to enhance it in the future, including adjusting its schedule >> among other passes (or adding other, later invocations). It >> would be overly constraining to consider this placement ideal >> or set in stone. >> >> Among possible enhancements worth considering is handling >> constant conditionals like: >> >> int f (void) >> { >> if (1) >> return 0; >> else >> return 1; <<< warn >> } >> >> int g (void) >> { >> if (1) >> return 0; >> return 1; <<< warn also in C (not just in C++) >> } > > I think both cases are undesirable to warn on, but both would be > possible to implement during parsing and only there they would > possibly make sense (you have to consider the if (1) resulting > from macro expansion or template instantiation which are the > undesirable to warn about cases - not to mention disabled code > that wants to remain syntactically correct, something that cannot > be achieved with #if 0) The example I gave above (with the constant) was intentionally contrived to illustrate the inconsistency between issuing the warning in C++ but not in C. I would view it as a bug. Not necessarily one that would make me object to your patch as is but certainly one to fix at some point. Using heuristics like "is it the result of macro expansion?" is one way to ameliorate the problem of issuing valid diagnostics for deliberate instances of the construct the warning is meant to detect. But few heuristics are ever perfect. For example, using an early return from a function is an alternate way of disabling the code that follows without actually removing it: void f (void) { ... return; ... intentionally disabled code... return; // warning here } instead of void f (void) { ... if (1) return; ... intentionally disabled code... return; // no warning here? } Why is diagnosing the former desirable and not the latter? (This is a rhetorical question.) In my view, in both cases, when intentional, such code is best documented. One way to document it is by adding a #pragma GCC diagnostic. Another is to recognize some special word in a comment, analogous to /* Fallthrough */ as an alternate spelling of the snynonymous attribute. >> By the way, a related feature that would be useful and that's >> been requested in the past is warning for stores with no effect, >> as in: >> >> int i; >> i = 1; >> i = 2; <<< warn here >> >> The detection of the simple cases like the one above can also >> be almost trivially implemented. > > Likewise the above can be done during parsing where it's more > appearant whether the stmts are the same. You mean the variables are the same. The front end has to work harder to determine that two expressions are equivalent (e.g., memset (&x, 0, sizeof x) and x = (struct X){ ... } both assign a value to x). It doesn't benefit from any of the simplifications done by the gimple transformations. As with any flow-sensitive warnings, where to implement this one is a balancing act between false positives and negatives. Martin > > Richard. > >> Martin >> >>> >>> Richard. >>> >>> 2021-11-24 Richard Biener <rguenther@suse.de> >>> >>> PR middle-end/46476 >>> * common.opt (Wunreachable-code): No longer ignored, >>> add warn_unreachable_code variable, enable with -Wextra. >>> * doc/invoke.texi (Wunreachable-code): Document. >>> (Wextra): Amend. >>> * tree-cfg.c (build_gimple_cfg): Move case label grouping... >>> (execute_build_cfg): ... here after new -Wunreachable-code >>> warnings. >>> (warn_unreachable_code_post_cfg_build): New function. >>> (mark_forward_reachable_blocks): Likewise. >>> (reverse_guess_deadend): Likewise. >>> >>> gcc/cp/ >>> * decl.c (finish_function): Set input_location to >>> BUILTINS_LOCATION around the code building the return 0 >>> for main(). >>> >>> libgomp/ >>> * oacc-plugin.c (GOMP_PLUGIN_acc_default_dim): Remove spurious >>> return. >>> >>> gcc/testsuite/ >>> * c-c++-common/Wunreachable-code-1.c: New testcase. >>> * c-c++-common/Wunreachable-code-2.c: Likewise. >>> * c-c++-common/Wunreachable-code-3.c: Likewise. >>> * gcc.dg/Wunreachable-code-4.c: Likewise. >>> * g++.dg/Wunreachable-code-5.C: Likewise. >>> --- >>> gcc/common.opt | 4 +- >>> gcc/cp/decl.c | 9 +- >>> gcc/doc/invoke.texi | 9 +- >>> .../c-c++-common/Wunreachable-code-1.c | 8 ++ >>> .../c-c++-common/Wunreachable-code-2.c | 8 ++ >>> .../c-c++-common/Wunreachable-code-3.c | 35 ++++++ >>> gcc/testsuite/g++.dg/Wunreachable-code-5.C | 11 ++ >>> gcc/testsuite/gcc.dg/Wunreachable-code-4.c | 10 ++ >>> gcc/tree-cfg.c | 101 +++++++++++++++++- >>> libgomp/oacc-plugin.c | 1 - >>> 10 files changed, 186 insertions(+), 10 deletions(-) >>> create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-1.c >>> create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-2.c >>> create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-3.c >>> create mode 100644 gcc/testsuite/g++.dg/Wunreachable-code-5.C >>> create mode 100644 gcc/testsuite/gcc.dg/Wunreachable-code-4.c >>> >>> diff --git a/gcc/common.opt b/gcc/common.opt >>> index 755e1a233b7..0a58cb8a668 100644 >>> --- a/gcc/common.opt >>> +++ b/gcc/common.opt >>> @@ -806,8 +806,8 @@ Common Var(warn_maybe_uninitialized) Warning >>> EnabledBy(Wuninitialized) >>> Warn about maybe uninitialized automatic variables. >>> >>> Wunreachable-code >>> -Common Ignore Warning >>> -Does nothing. Preserved for backward compatibility. >>> +Common Var(warn_unreachable_code) Warning EnabledBy(Wextra) >>> +Warn about trivially unreachable code. >>> >>> Wunused >>> Common Var(warn_unused) Init(0) Warning >>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>> index 588094b1db6..26325e41efa 100644 >>> --- a/gcc/cp/decl.c >>> +++ b/gcc/cp/decl.c >>> @@ -17571,7 +17571,14 @@ finish_function (bool inline_p) >>> { >>> /* Make it so that `main' always returns 0 by default. */ >>> if (DECL_MAIN_P (current_function_decl)) >>> - finish_return_stmt (integer_zero_node); >>> + { >>> + /* Hack. We don't want the middle-end to warn that this return >>> + is unreachable, so we mark its location as special. */ >>> + auto saved_il = input_location; >>> + input_location = BUILTINS_LOCATION; >>> + finish_return_stmt (integer_zero_node); >>> + input_location = saved_il; >>> + } >>> >>> if (use_eh_spec_block (current_function_decl)) >>> finish_eh_spec_block (TYPE_RAISES_EXCEPTIONS >>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi >>> index 36fe96b441b..62643e51915 100644 >>> --- a/gcc/doc/invoke.texi >>> +++ b/gcc/doc/invoke.texi >>> @@ -267,7 +267,7 @@ in the following sections. >>> -Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol >>> -Wsized-deallocation -Wsuggest-final-methods @gol >>> -Wsuggest-final-types -Wsuggest-override @gol >>> --Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol >>> +-Wno-terminate -Wunreachable-code -Wuseless-cast -Wno-vexing-parse @gol >>> -Wvirtual-inheritance @gol >>> -Wno-virtual-move-assign -Wvolatile -Wzero-as-null-pointer-constant} >>> >>> @@ -4357,6 +4357,12 @@ annotations. >>> Warn about overriding virtual functions that are not marked with the >>> @code{override} keyword. >>> >>> +@item -Wunreachable-code >>> +@opindex Wunreachable-code >>> +@opindex Wno-unreachable-code >>> +Warn about code that is trivially unreachable before optimizing. >>> +@option{-Wunreachable-code} is enabled by @option{-Wextra}. >>> + >>> @item -Wuseless-cast @r{(C++ and Objective-C++ only)} >>> @opindex Wuseless-cast >>> @opindex Wno-useless-cast >>> @@ -5713,6 +5719,7 @@ name is still supported, but the newer name is more >>> descriptive.) >>> -Wredundant-move @r{(only for C++)} @gol >>> -Wtype-limits @gol >>> -Wuninitialized @gol >>> +-Wunreachable-code @gol >>> -Wshift-negative-value @r{(in C++03 and in C99 and newer)} @gol >>> -Wunused-parameter @r{(only with} @option{-Wunused} @r{or} >>> @option{-Wall}@r{)} @gol >>> -Wunused-but-set-parameter @r{(only with} @option{-Wunused} @r{or} >>> @option{-Wall}@r{)}} >>> diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-1.c >>> b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c >>> new file mode 100644 >>> index 00000000000..0ca6c00e7fb >>> --- /dev/null >>> +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c >>> @@ -0,0 +1,8 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-Wunreachable-code" } */ >>> + >>> +int main() >>> +{ >>> + /* Avoid warning on the auto-added duplicate return 0. */ >>> + return 0; >>> +} >>> diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-2.c >>> b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c >>> new file mode 100644 >>> index 00000000000..836d8c30cb2 >>> --- /dev/null >>> +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c >>> @@ -0,0 +1,8 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-Wunreachable-code" } */ >>> + >>> +int main() >>> +{ >>> + return 0; >>> + return 0; /* { dg-warning "not reachable" } */ >>> +} >>> diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-3.c >>> b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c >>> new file mode 100644 >>> index 00000000000..a01e4119c6f >>> --- /dev/null >>> +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c >>> @@ -0,0 +1,35 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-Wunreachable-code" } */ >>> + >>> +/* Unreachable region entry has a predecessor (backedge). */ >>> +void foo() >>> +{ >>> + return; >>> + for (int i = 0; i < 5; ++i) /* { dg-warning "not reachable" } */ >>> + ; >>> +} >>> + >>> +/* Unreachable region not backwards reachable from exit. */ >>> +void bar1() >>> +{ >>> + return; >>> + __builtin_abort (); /* { dg-warning "not reachable" } */ >>> +} >>> +void bar2() >>> +{ >>> + return; >>> + /* Here we end up with a BB without any statements and an edge >>> + to itself. A location might be obtainable from the edges >>> + goto_locus. */ >>> + for (;;); /* { dg-warning "not reachable" "" { xfail *-*-* } } */ >>> +} >>> + >>> +/* Unreachable code in if (0) block. */ >>> +void baz(int *p) >>> +{ >>> + if (0) >>> + { >>> + return; /* { dg-bogus "not reachable" } */ >>> + *p = 0; /* { dg-warning "not reachable" } */ >>> + } >>> +} >>> diff --git a/gcc/testsuite/g++.dg/Wunreachable-code-5.C >>> b/gcc/testsuite/g++.dg/Wunreachable-code-5.C >>> new file mode 100644 >>> index 00000000000..832cb6d865f >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/Wunreachable-code-5.C >>> @@ -0,0 +1,11 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-Wunreachable-code" } */ >>> + >>> +void baz(); >>> +void bar2() >>> +{ >>> + if (! 0) >>> + return; >>> + /* The C++ frontend elides the if above. */ >>> + baz (); /* { dg-bogus "not reachable" "" { xfail *-*-* } } */ >>> +} >>> diff --git a/gcc/testsuite/gcc.dg/Wunreachable-code-4.c >>> b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c >>> new file mode 100644 >>> index 00000000000..997ec08fb41 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c >>> @@ -0,0 +1,10 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-Wunreachable-code" } */ >>> + >>> +void baz(); >>> +void bar2() >>> +{ >>> + if (! 0) >>> + return; >>> + baz (); /* { dg-bogus "not reachable" } */ >>> +} >>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c >>> index 8ed8c69b5b1..5a9507d0e44 100644 >>> --- a/gcc/tree-cfg.c >>> +++ b/gcc/tree-cfg.c >>> @@ -239,11 +239,6 @@ build_gimple_cfg (gimple_seq seq) >>> /* To speed up statement iterator walks, we first purge dead labels. */ >>> cleanup_dead_labels (); >>> - /* Group case nodes to reduce the number of edges. >>> - We do this after cleaning up dead labels because otherwise we miss >>> - a lot of obvious case merging opportunities. */ >>> - group_case_labels (); >>> - >>> /* Create the edges of the flowgraph. */ >>> discriminator_per_locus = new hash_table<locus_discrim_hasher> (13); >>> make_edges (); >>> @@ -362,6 +357,94 @@ replace_loop_annotate (void) >>> } >>> } >>> >>> +/* Mark BB and blocks forward reachable from it as BB_REACHABLE. */ >>> + >>> +static void >>> +mark_forward_reachable_blocks (basic_block bb) >>> +{ >>> + bb->flags |= BB_REACHABLE; >>> + edge_iterator ei; >>> + edge e; >>> + FOR_EACH_EDGE (e, ei, bb->succs) >>> + if (!(e->dest->flags & BB_REACHABLE)) >>> + mark_forward_reachable_blocks (e->dest); >>> +} >>> + >>> +/* Guess a reverse dead end from BB. */ >>> + >>> +static basic_block >>> +reverse_guess_deadend (basic_block bb) >>> +{ >>> + /* Quick and simple minded. */ >>> + if (EDGE_COUNT (bb->preds) == 0) >>> + return bb; >>> + >>> + /* DFS walk. */ >>> + auto_bb_flag visited (cfun); >>> + auto_bb_flag on_worklist (cfun); >>> + auto_vec<basic_block, 64> bbs_to_cleanup; >>> + auto_vec<basic_block, 64> worklist; >>> + worklist.quick_push (bb); >>> + bb->flags |= on_worklist; >>> + bbs_to_cleanup.safe_push (bb); >>> + while (!worklist.is_empty ()) >>> + { >>> + bb = worklist.pop (); >>> + bb->flags &= ~on_worklist; >>> + bb->flags |= visited; >>> + bool all_preds_visited = true; >>> + edge_iterator ei; >>> + edge e; >>> + FOR_EACH_EDGE (e, ei, bb->preds) >>> + if (!(e->src->flags & visited)) >>> + { >>> + if (!(e->src->flags & on_worklist)) >>> + { >>> + worklist.safe_push (e->src); >>> + e->src->flags |= on_worklist; >>> + bbs_to_cleanup.safe_push (e->src); >>> + } >>> + all_preds_visited = false; >>> + } >>> + /* Found one deadend. */ >>> + if (all_preds_visited) >>> + break; >>> + } >>> + for (auto bb2 : bbs_to_cleanup) >>> + bb2->flags &= ~(on_worklist|visited); >>> + return bb; >>> +} >>> + >>> +/* Warn about trivially unreachable code. */ >>> + >>> +static void >>> +warn_unreachable_code_post_cfg_build (void) >>> +{ >>> + find_unreachable_blocks (); >>> + basic_block bb; >>> + FOR_EACH_BB_FN (bb, cfun) >>> + { >>> + if ((bb->flags & BB_REACHABLE)) >>> + continue; >>> + /* Try to find the entry to the unreachable region. */ >>> + bb = reverse_guess_deadend (bb); >>> + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); >>> + gsi_next (&gsi)) >>> + { >>> + /* Suppress warnings on BUILTINS_LOCATION which is used by the >>> + C and C++ frontends to emit the artifical return in main. */ >>> + if (LOCATION_LOCUS (gimple_location (gsi_stmt (gsi))) >>> + > BUILTINS_LOCATION) >>> + warning_at (gimple_location (gsi_stmt (gsi)), >>> + OPT_Wunreachable_code, "statement is not reachable"); >>> + break; >>> + } >>> + /* Mark blocks reachable from here. And even better make >>> + sure to process entries to unreachable regions first. */ >>> + mark_forward_reachable_blocks (bb); >>> + } >>> +} >>> + >>> static unsigned int >>> execute_build_cfg (void) >>> { >>> @@ -374,6 +457,14 @@ execute_build_cfg (void) >>> fprintf (dump_file, "Scope blocks:\n"); >>> dump_scope_blocks (dump_file, dump_flags); >>> } >>> + >>> + if (warn_unreachable_code) >>> + warn_unreachable_code_post_cfg_build (); >>> + >>> + /* Group case nodes. We did this prior to materializing edges in >>> + build_gimple_cfg to reduce the number of edges, that interferes >>> + with the -Wunreachable-code diagnostics above. */ >>> + group_case_labels (); >>> cleanup_tree_cfg (); >>> >>> bb_to_omp_idx.release (); >>> diff --git a/libgomp/oacc-plugin.c b/libgomp/oacc-plugin.c >>> index e25b462eff0..98166fe5cd1 100644 >>> --- a/libgomp/oacc-plugin.c >>> +++ b/libgomp/oacc-plugin.c >>> @@ -62,7 +62,6 @@ GOMP_PLUGIN_acc_default_dim (unsigned int i) >>> if (i >= GOMP_DIM_MAX) >>> { >>> gomp_fatal ("invalid dimension argument: %d", i); >>> - return -1; >>> } >>> return goacc_default_dims[i]; >>> } >>> >> >> >> >
diff --git a/gcc/common.opt b/gcc/common.opt index 755e1a233b7..0a58cb8a668 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -806,8 +806,8 @@ Common Var(warn_maybe_uninitialized) Warning EnabledBy(Wuninitialized) Warn about maybe uninitialized automatic variables. Wunreachable-code -Common Ignore Warning -Does nothing. Preserved for backward compatibility. +Common Var(warn_unreachable_code) Warning EnabledBy(Wextra) +Warn about trivially unreachable code. Wunused Common Var(warn_unused) Init(0) Warning diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 588094b1db6..26325e41efa 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -17571,7 +17571,14 @@ finish_function (bool inline_p) { /* Make it so that `main' always returns 0 by default. */ if (DECL_MAIN_P (current_function_decl)) - finish_return_stmt (integer_zero_node); + { + /* Hack. We don't want the middle-end to warn that this return + is unreachable, so we mark its location as special. */ + auto saved_il = input_location; + input_location = BUILTINS_LOCATION; + finish_return_stmt (integer_zero_node); + input_location = saved_il; + } if (use_eh_spec_block (current_function_decl)) finish_eh_spec_block (TYPE_RAISES_EXCEPTIONS diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 36fe96b441b..62643e51915 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -267,7 +267,7 @@ in the following sections. -Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol -Wsized-deallocation -Wsuggest-final-methods @gol -Wsuggest-final-types -Wsuggest-override @gol --Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol +-Wno-terminate -Wunreachable-code -Wuseless-cast -Wno-vexing-parse @gol -Wvirtual-inheritance @gol -Wno-virtual-move-assign -Wvolatile -Wzero-as-null-pointer-constant} @@ -4357,6 +4357,12 @@ annotations. Warn about overriding virtual functions that are not marked with the @code{override} keyword. +@item -Wunreachable-code +@opindex Wunreachable-code +@opindex Wno-unreachable-code +Warn about code that is trivially unreachable before optimizing. +@option{-Wunreachable-code} is enabled by @option{-Wextra}. + @item -Wuseless-cast @r{(C++ and Objective-C++ only)} @opindex Wuseless-cast @opindex Wno-useless-cast @@ -5713,6 +5719,7 @@ name is still supported, but the newer name is more descriptive.) -Wredundant-move @r{(only for C++)} @gol -Wtype-limits @gol -Wuninitialized @gol +-Wunreachable-code @gol -Wshift-negative-value @r{(in C++03 and in C99 and newer)} @gol -Wunused-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)} @gol -Wunused-but-set-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)}} diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-1.c b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c new file mode 100644 index 00000000000..0ca6c00e7fb --- /dev/null +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-1.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunreachable-code" } */ + +int main() +{ + /* Avoid warning on the auto-added duplicate return 0. */ + return 0; +} diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-2.c b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c new file mode 100644 index 00000000000..836d8c30cb2 --- /dev/null +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-2.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunreachable-code" } */ + +int main() +{ + return 0; + return 0; /* { dg-warning "not reachable" } */ +} diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-3.c b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c new file mode 100644 index 00000000000..a01e4119c6f --- /dev/null +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-3.c @@ -0,0 +1,35 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunreachable-code" } */ + +/* Unreachable region entry has a predecessor (backedge). */ +void foo() +{ + return; + for (int i = 0; i < 5; ++i) /* { dg-warning "not reachable" } */ + ; +} + +/* Unreachable region not backwards reachable from exit. */ +void bar1() +{ + return; + __builtin_abort (); /* { dg-warning "not reachable" } */ +} +void bar2() +{ + return; + /* Here we end up with a BB without any statements and an edge + to itself. A location might be obtainable from the edges + goto_locus. */ + for (;;); /* { dg-warning "not reachable" "" { xfail *-*-* } } */ +} + +/* Unreachable code in if (0) block. */ +void baz(int *p) +{ + if (0) + { + return; /* { dg-bogus "not reachable" } */ + *p = 0; /* { dg-warning "not reachable" } */ + } +} diff --git a/gcc/testsuite/g++.dg/Wunreachable-code-5.C b/gcc/testsuite/g++.dg/Wunreachable-code-5.C new file mode 100644 index 00000000000..832cb6d865f --- /dev/null +++ b/gcc/testsuite/g++.dg/Wunreachable-code-5.C @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunreachable-code" } */ + +void baz(); +void bar2() +{ + if (! 0) + return; + /* The C++ frontend elides the if above. */ + baz (); /* { dg-bogus "not reachable" "" { xfail *-*-* } } */ +} diff --git a/gcc/testsuite/gcc.dg/Wunreachable-code-4.c b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c new file mode 100644 index 00000000000..997ec08fb41 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wunreachable-code-4.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunreachable-code" } */ + +void baz(); +void bar2() +{ + if (! 0) + return; + baz (); /* { dg-bogus "not reachable" } */ +} diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 8ed8c69b5b1..5a9507d0e44 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -239,11 +239,6 @@ build_gimple_cfg (gimple_seq seq) /* To speed up statement iterator walks, we first purge dead labels. */ cleanup_dead_labels (); - /* Group case nodes to reduce the number of edges. - We do this after cleaning up dead labels because otherwise we miss - a lot of obvious case merging opportunities. */ - group_case_labels (); - /* Create the edges of the flowgraph. */ discriminator_per_locus = new hash_table<locus_discrim_hasher> (13); make_edges (); @@ -362,6 +357,94 @@ replace_loop_annotate (void) } } +/* Mark BB and blocks forward reachable from it as BB_REACHABLE. */ + +static void +mark_forward_reachable_blocks (basic_block bb) +{ + bb->flags |= BB_REACHABLE; + edge_iterator ei; + edge e; + FOR_EACH_EDGE (e, ei, bb->succs) + if (!(e->dest->flags & BB_REACHABLE)) + mark_forward_reachable_blocks (e->dest); +} + +/* Guess a reverse dead end from BB. */ + +static basic_block +reverse_guess_deadend (basic_block bb) +{ + /* Quick and simple minded. */ + if (EDGE_COUNT (bb->preds) == 0) + return bb; + + /* DFS walk. */ + auto_bb_flag visited (cfun); + auto_bb_flag on_worklist (cfun); + auto_vec<basic_block, 64> bbs_to_cleanup; + auto_vec<basic_block, 64> worklist; + worklist.quick_push (bb); + bb->flags |= on_worklist; + bbs_to_cleanup.safe_push (bb); + while (!worklist.is_empty ()) + { + bb = worklist.pop (); + bb->flags &= ~on_worklist; + bb->flags |= visited; + bool all_preds_visited = true; + edge_iterator ei; + edge e; + FOR_EACH_EDGE (e, ei, bb->preds) + if (!(e->src->flags & visited)) + { + if (!(e->src->flags & on_worklist)) + { + worklist.safe_push (e->src); + e->src->flags |= on_worklist; + bbs_to_cleanup.safe_push (e->src); + } + all_preds_visited = false; + } + /* Found one deadend. */ + if (all_preds_visited) + break; + } + for (auto bb2 : bbs_to_cleanup) + bb2->flags &= ~(on_worklist|visited); + return bb; +} + +/* Warn about trivially unreachable code. */ + +static void +warn_unreachable_code_post_cfg_build (void) +{ + find_unreachable_blocks (); + basic_block bb; + FOR_EACH_BB_FN (bb, cfun) + { + if ((bb->flags & BB_REACHABLE)) + continue; + /* Try to find the entry to the unreachable region. */ + bb = reverse_guess_deadend (bb); + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); + gsi_next (&gsi)) + { + /* Suppress warnings on BUILTINS_LOCATION which is used by the + C and C++ frontends to emit the artifical return in main. */ + if (LOCATION_LOCUS (gimple_location (gsi_stmt (gsi))) + > BUILTINS_LOCATION) + warning_at (gimple_location (gsi_stmt (gsi)), + OPT_Wunreachable_code, "statement is not reachable"); + break; + } + /* Mark blocks reachable from here. And even better make + sure to process entries to unreachable regions first. */ + mark_forward_reachable_blocks (bb); + } +} + static unsigned int execute_build_cfg (void) { @@ -374,6 +457,14 @@ execute_build_cfg (void) fprintf (dump_file, "Scope blocks:\n"); dump_scope_blocks (dump_file, dump_flags); } + + if (warn_unreachable_code) + warn_unreachable_code_post_cfg_build (); + + /* Group case nodes. We did this prior to materializing edges in + build_gimple_cfg to reduce the number of edges, that interferes + with the -Wunreachable-code diagnostics above. */ + group_case_labels (); cleanup_tree_cfg (); bb_to_omp_idx.release (); diff --git a/libgomp/oacc-plugin.c b/libgomp/oacc-plugin.c index e25b462eff0..98166fe5cd1 100644 --- a/libgomp/oacc-plugin.c +++ b/libgomp/oacc-plugin.c @@ -62,7 +62,6 @@ GOMP_PLUGIN_acc_default_dim (unsigned int i) if (i >= GOMP_DIM_MAX) { gomp_fatal ("invalid dimension argument: %d", i); - return -1; } return goacc_default_dims[i]; }