[PATCH][RFC] middle-end/46476 - resurrect -Wunreachable-code
Richard Biener
rguenther@suse.de
Thu Nov 25 07:57:58 GMT 2021
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];
> > }
> >
>
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
More information about the Gcc-patches
mailing list