[RFC] middle-end/46476 - resurrect -Wunreachable-code

  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
  

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];
>>>    }
>>>
>>
>>
>>
>