[RFC] c++/46476 - implement -Wunreachable-code-return

  This implements a subset of -Wunreachable-code, unreachable code
after a return stmt.  Contrary to the previous attemt at CFG
construction time this implements the bits during GIMPLE lowering
where there are still all GIMPLE return stmts in the IL.

The lowering phase keeps track of whether stmts can fallthru
which is used to determine if the following stmt is reachable.
The implementation only considers labels here.

The fallthru flag is transparently extended to allow tracking
a reason for non-fallthruness which is used to mark returns.

This patch runs in to the same stray return/gcc_unreachable as the
previous one and thus requires cleanup across the GCC code base
which seems controversical.  So I'm putting this on hold unless
I receive some OK for cleanup in any way, meaning this isn't
going to make stage3.

Sorry.

Richard.

2021-11-26  Richard Biener  <rguenther@suse.de>

	PR c++/46476
gcc/cp/
	* decl.c (finish_function): Set input_location to
	BUILTINS_LOCATION around the code building the return 0
	for main().
	* cp-gimplify.c (genericize_if_stmt): Avoid optimizing if (true)
	and if (false) when -Wunreachable-code-return is in effect.

gcc/
	* common.opt (Wunreachable-code): Re-enable.
	(Wunreachable-code-return): New diagnostic, enabled by
	-Wextra and -Wunreachable-code.
	* doc/invoke.texi (Wunreachable-code): Document.
	(Wunreachable-code-return): Likewise.
	* gimple-low.c: Include diagnostic.h.
	(struct cft_reason): New.
	(lower_data::cannot_fallthru): Make a cft_reason.
	(lower_stmt): Diagnose unreachable stmts after a return.
	* Makefile.in (insn-emit.o-warn): Disable
	-Wunreachable-code-return.

gcc/testsuite/
	* c-c++-common/Wunreachable-code-return-1.c: New testcase.
---
 gcc/Makefile.in                               |  1 +
 gcc/common.opt                                |  8 +++--
 gcc/cp/cp-gimplify.c                          |  6 ++--
 gcc/cp/decl.c                                 |  9 +++++-
 gcc/doc/invoke.texi                           | 13 ++++++++
 gcc/gimple-low.c                              | 30 ++++++++++++++++---
 .../c-c++-common/Wunreachable-code-return-1.c |  9 ++++++
 7 files changed, 67 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-return-1.c
  

On 11/26/2021 5:18 AM, Richard Biener via Gcc-patches wrote:
> This implements a subset of -Wunreachable-code, unreachable code
> after a return stmt.  Contrary to the previous attemt at CFG
> construction time this implements the bits during GIMPLE lowering
> where there are still all GIMPLE return stmts in the IL.
>
> The lowering phase keeps track of whether stmts can fallthru
> which is used to determine if the following stmt is reachable.
> The implementation only considers labels here.
>
> The fallthru flag is transparently extended to allow tracking
> a reason for non-fallthruness which is used to mark returns.
>
> This patch runs in to the same stray return/gcc_unreachable as the
> previous one and thus requires cleanup across the GCC code base
> which seems controversical.  So I'm putting this on hold unless
> I receive some OK for cleanup in any way, meaning this isn't
> going to make stage3.
>
> Sorry.
>
> Richard.
>
> 2021-11-26  Richard Biener  <rguenther@suse.de>
>
> 	PR c++/46476
> gcc/cp/
> 	* decl.c (finish_function): Set input_location to
> 	BUILTINS_LOCATION around the code building the return 0
> 	for main().
> 	* cp-gimplify.c (genericize_if_stmt): Avoid optimizing if (true)
> 	and if (false) when -Wunreachable-code-return is in effect.
>
> gcc/
> 	* common.opt (Wunreachable-code): Re-enable.
> 	(Wunreachable-code-return): New diagnostic, enabled by
> 	-Wextra and -Wunreachable-code.
> 	* doc/invoke.texi (Wunreachable-code): Document.
> 	(Wunreachable-code-return): Likewise.
> 	* gimple-low.c: Include diagnostic.h.
> 	(struct cft_reason): New.
> 	(lower_data::cannot_fallthru): Make a cft_reason.
> 	(lower_stmt): Diagnose unreachable stmts after a return.
> 	* Makefile.in (insn-emit.o-warn): Disable
> 	-Wunreachable-code-return.
>
> gcc/testsuite/
> 	* c-c++-common/Wunreachable-code-return-1.c: New testcase.
I wouldn't object to this moving forward.  I've already ACK'd the cleanups.

Jeff
  

On 11/26/21 5:18 AM, Richard Biener via Gcc-patches wrote:
> This implements a subset of -Wunreachable-code, unreachable code
> after a return stmt.  Contrary to the previous attemt at CFG
> construction time this implements the bits during GIMPLE lowering
> where there are still all GIMPLE return stmts in the IL.
> 
> The lowering phase keeps track of whether stmts can fallthru
> which is used to determine if the following stmt is reachable.
> The implementation only considers labels here.
> 
> The fallthru flag is transparently extended to allow tracking
> a reason for non-fallthruness which is used to mark returns.
> 
> This patch runs in to the same stray return/gcc_unreachable as the
> previous one and thus requires cleanup across the GCC code base
> which seems controversical.  So I'm putting this on hold unless
> I receive some OK for cleanup in any way, meaning this isn't
> going to make stage3.

This isn't meant as an objection to the patch per se, just as
data points suggesting there's room for improvement.  I do think
at least some of those should be considered for GCC 12 if the patch
goes in.  I see just one trivial test which seems a bit light.
I would recommend beefing it up to exercise some the cases below.

I tested the patch with Glibc (no warnings) and Binutils/GDB.
The latter shows over 900 warnings (600 unique ones) in 46
files, so it might be a useful test bed.  Lots of those, maybe
most, are for a break after a return, suggesting that it might
be worthwhile to treat such inoccuous case specially (e.g., only
warn for a break after a return at level 2).

Some other instances suggest other possible improvements.
For example:

/src/binutils-gdb/libiberty/lrealpath.c: In function ‘lrealpath’:
/src/binutils-gdb/libiberty/lrealpath.c:113:3: warning: statement after 
return is not reachable [-Wunreachable-code-return]
   113 |   {
       |   ^
/src/binutils-gdb/libiberty/lrealpath.c:115:5: warning: statement after 
return is not reachable [-Wunreachable-code-return]
   115 |     long path_max = pathconf ("/", _PC_PATH_MAX);
       |     ^~~~
/src/binutils-gdb/libiberty/lrealpath.c:115:21: warning: statement after 
return is not reachable [-Wunreachable-code-return]
   115 |     long path_max = pathconf ("/", _PC_PATH_MAX);
       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think one of them is a true positive but the others all look
like noise.  None of the locations is very useful.  It might be
something to look into.  I would suggest to point the warning
to the first unreachable statement (other instances point to
it already) and add a note pointing to the statement that makes
the former unreachable.

Another example below shows that the warning triggers more than
once for the same statement, suggestiung it's missing some
suppression (e.g., a call to suppress_warning()).

/src/binutils-gdb/bfd/bfdio.c:167:3: warning: statement after return is 
not reachable [-Wunreachable-code-return]
   167 |   return close_on_exec (fopen (filename, modes));
       |   ^~~~~~
/src/binutils-gdb/bfd/bfdio.c:167:10: warning: statement after return is 
not reachable [-Wunreachable-code-return]
   167 |   return close_on_exec (fopen (filename, modes));
       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

There are some other "unusual" cases worth a look, such as missing
context of any kind except for like and column:

elfnn-riscv.c:3346:7: warning: statement after return is not reachable 
[-Wunreachable-code-return]
elfnn-riscv.c:3349:7: warning: statement after return is not reachable 
[-Wunreachable-code-return]
elfnn-riscv.c:3352:7: warning: statement after return is not reachable 
[-Wunreachable-code-return]
elfnn-riscv.c:3355:7: warning: statement after return is not reachable 
[-Wunreachable-code-return]

I also tried a few test cases of my own that might be worth
handling at some point (not necessarily in the first iteration).

struct __jmp_buf_tag { };
typedef struct __jmp_buf_tag jmp_buf[1];

void f ();

void test_return ()
{
   return;
   f ();   // warning here (good)
}

extern __attribute__ ((noreturn)) void fnoret ();

void test_noreturn ()
{
   fnoret ();
   f ();   // missing warning
}

void test_throw ()
{
   throw "";
   f ();   // missing warning
}

jmp_buf jmpbuf;

void test_longjmp ()
{
   __builtin_longjmp (jmpbuf, 1);
   f ();   // missing warning
}

Please see a few more comments on the code changes inline.

> 
> Sorry.
> 
> Richard.
> 
> 2021-11-26  Richard Biener  <rguenther@suse.de>
> 
> 	PR c++/46476
> gcc/cp/
> 	* decl.c (finish_function): Set input_location to
> 	BUILTINS_LOCATION around the code building the return 0
> 	for main().
> 	* cp-gimplify.c (genericize_if_stmt): Avoid optimizing if (true)
> 	and if (false) when -Wunreachable-code-return is in effect.
> 
> gcc/
> 	* common.opt (Wunreachable-code): Re-enable.
> 	(Wunreachable-code-return): New diagnostic, enabled by
> 	-Wextra and -Wunreachable-code.
> 	* doc/invoke.texi (Wunreachable-code): Document.
> 	(Wunreachable-code-return): Likewise.
> 	* gimple-low.c: Include diagnostic.h.
> 	(struct cft_reason): New.
> 	(lower_data::cannot_fallthru): Make a cft_reason.
> 	(lower_stmt): Diagnose unreachable stmts after a return.
> 	* Makefile.in (insn-emit.o-warn): Disable
> 	-Wunreachable-code-return.
> 
> gcc/testsuite/
> 	* c-c++-common/Wunreachable-code-return-1.c: New testcase.
> ---
>   gcc/Makefile.in                               |  1 +
>   gcc/common.opt                                |  8 +++--
>   gcc/cp/cp-gimplify.c                          |  6 ++--
>   gcc/cp/decl.c                                 |  9 +++++-
>   gcc/doc/invoke.texi                           | 13 ++++++++
>   gcc/gimple-low.c                              | 30 ++++++++++++++++---
>   .../c-c++-common/Wunreachable-code-return-1.c |  9 ++++++
>   7 files changed, 67 insertions(+), 9 deletions(-)
>   create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-return-1.c
> 
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index a4344d67f44..71d326ff98c 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -222,6 +222,7 @@ libgcov-merge-tool.o-warn = -Wno-error
>   gimple-match.o-warn = -Wno-unused
>   generic-match.o-warn = -Wno-unused
>   dfp.o-warn = -Wno-strict-aliasing
> +insn-emit.o-warn = -Wno-unreachable-code-return
>   
>   # All warnings have to be shut off in stage1 if the compiler used then
>   # isn't gcc; configure determines that.  WARN_CFLAGS will be either
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 755e1a233b7..486ea36c8e5 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -806,8 +806,12 @@ 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
> +Catch all for -Wunreachable-code-*.
> +
> +Wunreachable-code-return

-Wunreachable-code-return sounds a bit awkward to me.  I would
suggest -Wunreachable-return instead (especially if you think
we might add other similar variations).

> +Common Var(warn_unreachable_code_return) Warning EnabledBy(Wunreachable-code || Wextra)
> +Warn about unreachable statements after a return.
>   
>   Wunused
>   Common Var(warn_unused) Init(0) Warning
> diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
> index c1691c3e073..55e03abba43 100644
> --- a/gcc/cp/cp-gimplify.c
> +++ b/gcc/cp/cp-gimplify.c
> @@ -167,9 +167,11 @@ genericize_if_stmt (tree *stmt_p)
>        the then_ block regardless of whether else_ has side-effects or not.  */
>     if (IF_STMT_CONSTEVAL_P (stmt))
>       stmt = else_;
> -  else if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_))
> +  else if (!warn_unreachable_code_return
> +	   && integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_))
>       stmt = then_;
> -  else if (integer_zerop (cond) && !TREE_SIDE_EFFECTS (then_))
> +  else if (!warn_unreachable_code_return
> +	   && integer_zerop (cond) && !TREE_SIDE_EFFECTS (then_))
>       stmt = else_;
>     else
>       stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_);
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 56f80775ca0..cb81a794e5b 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -17573,7 +17573,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;

That does seem like a hack.  Won't it compromise other warnings?
Would calling suppress_warning (retstmt, OPT_Wunreachable_return)
on the return statement work?  (Similar to what finish_return_stmt
already does for OPT_Wreturn_type.)

> +	  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 3bddfbaae6a..7d7f2e7fddc 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -400,6 +400,7 @@ Objective-C and Objective-C++ Dialects}.
>   -Wsystem-headers  -Wtautological-compare  -Wtrampolines  -Wtrigraphs @gol
>   -Wtsan -Wtype-limits  -Wundef @gol
>   -Wuninitialized  -Wunknown-pragmas @gol
> +-Wunreachable-code -Wunreachable-code-return @gol
>   -Wunsuffixed-float-constants  -Wunused @gol
>   -Wunused-but-set-parameter  -Wunused-but-set-variable @gol
>   -Wunused-const-variable  -Wunused-const-variable=@var{n} @gol
> @@ -5721,6 +5722,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-return @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{)}}
> @@ -6865,6 +6867,17 @@ This warning is enabled by default for C and C++ programs.
>   Warn when @code{__sync_fetch_and_nand} and @code{__sync_nand_and_fetch}
>   built-in functions are used.  These functions changed semantics in GCC 4.4.
>   
> +@item -Wunreachable-code
> +@opindex Wunreachable-code
> +@opindex Wno-unreachable-code
> +Warn about unreachable code.  Enables @option{-Wunreachable-code-return}.
> +
> +@item -Wunreachable-code-return
> +@opindex Wunreachable-code-return
> +@opindex Wno-unreachable-code-return
> +Warn about unreachable code after a return stmt.

The word is "statement" and the @code{return} keyword should be
quoted.  I would also suggest to consider clarifying the text by
saying it warns for the first statement that's rendered unreachable
by a prior return statement (i.e., make clear it doesn't warn for
all unreachable code, or any arbitrary piece of it).

> Enabled by
> +@option{-Wunreachable-code} and @option{-Wextra}.
> +
>   @item -Wunused-but-set-parameter
>   @opindex Wunused-but-set-parameter
>   @opindex Wno-unused-but-set-parameter
> diff --git a/gcc/gimple-low.c b/gcc/gimple-low.c
> index 7e39c22df44..f0eb82f72f6 100644
> --- a/gcc/gimple-low.c
> +++ b/gcc/gimple-low.c
> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.  If not see
>   #include "predict.h"
>   #include "gimple-predict.h"
>   #include "gimple-fold.h"
> +#include "diagnostic.h"
>   
>   /* The differences between High GIMPLE and Low GIMPLE are the
>      following:
> @@ -56,6 +57,22 @@ struct return_statements_t
>   };
>   typedef struct return_statements_t return_statements_t;
>   
> +/* Helper tracking the reason a previous stmt cannot fallthru.  */
> +struct cft_reason
> +{
> +  enum reason { CAN_FALLTHRU = false, UNKNOWN = true, RETURN };
> +  cft_reason () : m_reason (CAN_FALLTHRU) {}
> +  cft_reason (bool b) : m_reason (b ? UNKNOWN : CAN_FALLTHRU) {}
> +  cft_reason (reason r) : m_reason (r) {}
> +  operator bool () const { return m_reason != CAN_FALLTHRU; }
> +  cft_reason& operator|= (const cft_reason& other)
> +    {
> +      if (other.m_reason != m_reason && (bool)other)
> +	m_reason = UNKNOWN;
> +      return *this;
> +    }
> +  reason m_reason;
> +};
>   
>   struct lower_data
>   {
> @@ -67,7 +84,7 @@ struct lower_data
>     vec<return_statements_t> return_statements;
>   
>     /* True if the current statement cannot fall through.  */
> -  bool cannot_fallthru;
> +  cft_reason cannot_fallthru;
>   };
>   
>   static void lower_stmt (gimple_stmt_iterator *, struct lower_data *);
> @@ -84,7 +101,7 @@ static void lower_builtin_posix_memalign (gimple_stmt_iterator *);
>   static unsigned int
>   lower_function_body (void)
>   {
> -  struct lower_data data;
> +  struct lower_data data = {};
>     gimple_seq body = gimple_body (current_function_decl);
>     gimple_seq lowered_body;
>     gimple_stmt_iterator i;
> @@ -96,7 +113,6 @@ lower_function_body (void)
>     gcc_assert (gimple_seq_first (body) == gimple_seq_last (body)
>   	      && gimple_code (gimple_seq_first_stmt (body)) == GIMPLE_BIND);
>   
> -  memset (&data, 0, sizeof (data));
>     data.block = DECL_INITIAL (current_function_decl);
>     BLOCK_SUBBLOCKS (data.block) = NULL_TREE;
>     BLOCK_CHAIN (data.block) = NULL_TREE;
> @@ -249,6 +265,12 @@ lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data)
>   
>     gimple_set_block (stmt, data->block);
>   
> +  if (data->cannot_fallthru.m_reason == cft_reason::RETURN
> +      && gimple_code (stmt) != GIMPLE_LABEL
> +      && LOCATION_LOCUS (gimple_location (stmt)) > BUILTINS_LOCATION)
> +    warning_at (gimple_location (stmt), OPT_Wunreachable_code_return,
> +		"statement after return is not reachable");

As a keyword the word return should be quoted.

Martin

> +
>     switch (gimple_code (stmt))
>       {
>       case GIMPLE_BIND:
> @@ -272,7 +294,7 @@ lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data)
>         else
>   	{
>   	  lower_gimple_return (gsi, data);
> -	  data->cannot_fallthru = true;
> +	  data->cannot_fallthru = cft_reason::RETURN;
>   	}
>         return;
>   
> diff --git a/gcc/testsuite/c-c++-common/Wunreachable-code-return-1.c b/gcc/testsuite/c-c++-common/Wunreachable-code-return-1.c
> new file mode 100644
> index 00000000000..17a35939182
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/Wunreachable-code-return-1.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Wunreachable-code-return" } */
> +
> +void baz();
> +void foo ()
> +{
> +  return;
> +  baz (); /* { dg-warning "statement after return is not reachable" } */
> +}
>
  

On Mon, 29 Nov 2021, Martin Sebor wrote:

> On 11/26/21 5:18 AM, Richard Biener via Gcc-patches wrote:
> > This implements a subset of -Wunreachable-code, unreachable code
> > after a return stmt.  Contrary to the previous attemt at CFG
> > construction time this implements the bits during GIMPLE lowering
> > where there are still all GIMPLE return stmts in the IL.
> > 
> > The lowering phase keeps track of whether stmts can fallthru
> > which is used to determine if the following stmt is reachable.
> > The implementation only considers labels here.
> > 
> > The fallthru flag is transparently extended to allow tracking
> > a reason for non-fallthruness which is used to mark returns.
> > 
> > This patch runs in to the same stray return/gcc_unreachable as the
> > previous one and thus requires cleanup across the GCC code base
> > which seems controversical.  So I'm putting this on hold unless
> > I receive some OK for cleanup in any way, meaning this isn't
> > going to make stage3.
> 
> This isn't meant as an objection to the patch per se, just as
> data points suggesting there's room for improvement.  I do think
> at least some of those should be considered for GCC 12 if the patch
> goes in.  I see just one trivial test which seems a bit light.
> I would recommend beefing it up to exercise some the cases below.

definitely

> I tested the patch with Glibc (no warnings) and Binutils/GDB.
> The latter shows over 900 warnings (600 unique ones) in 46
> files, so it might be a useful test bed.  Lots of those, maybe
> most, are for a break after a return, suggesting that it might
> be worthwhile to treat such inoccuous case specially (e.g., only
> warn for a break after a return at level 2).

I've fixed one bug only after submission which caused quite some
false positives with -g -On, failure to skip debug stmts.

> Some other instances suggest other possible improvements.
> For example:
> 
> /src/binutils-gdb/libiberty/lrealpath.c: In function ‘lrealpath’:
> /src/binutils-gdb/libiberty/lrealpath.c:113:3: warning: statement after return
> is not reachable [-Wunreachable-code-return]
>   113 |   {
>       |   ^
> /src/binutils-gdb/libiberty/lrealpath.c:115:5: warning: statement after return
> is not reachable [-Wunreachable-code-return]
>   115 |     long path_max = pathconf ("/", _PC_PATH_MAX);
>       |     ^~~~
> /src/binutils-gdb/libiberty/lrealpath.c:115:21: warning: statement after
> return is not reachable [-Wunreachable-code-return]
>   115 |     long path_max = pathconf ("/", _PC_PATH_MAX);
>       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I think one of them is a true positive but the others all look
> like noise.  None of the locations is very useful.  It might be
> something to look into.  I would suggest to point the warning
> to the first unreachable statement (other instances point to
> it already) and add a note pointing to the statement that makes
> the former unreachable.
> 
> Another example below shows that the warning triggers more than
> once for the same statement, suggestiung it's missing some
> suppression (e.g., a call to suppress_warning()).
> 
> /src/binutils-gdb/bfd/bfdio.c:167:3: warning: statement after return is not
> reachable [-Wunreachable-code-return]
>   167 |   return close_on_exec (fopen (filename, modes));
>       |   ^~~~~~
> /src/binutils-gdb/bfd/bfdio.c:167:10: warning: statement after return is not
> reachable [-Wunreachable-code-return]
>   167 |   return close_on_exec (fopen (filename, modes));
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> There are some other "unusual" cases worth a look, such as missing
> context of any kind except for like and column:
> 
> elfnn-riscv.c:3346:7: warning: statement after return is not reachable
> [-Wunreachable-code-return]
> elfnn-riscv.c:3349:7: warning: statement after return is not reachable
> [-Wunreachable-code-return]
> elfnn-riscv.c:3352:7: warning: statement after return is not reachable
> [-Wunreachable-code-return]
> elfnn-riscv.c:3355:7: warning: statement after return is not reachable
> [-Wunreachable-code-return]

Yeah, the patch was meant as explorative prototype (and a way to audit
GCC itself), I didn't pay much attention to things like above, I've
also not yet attempted to record the actual stmt causing the warning.

> I also tried a few test cases of my own that might be worth
> handling at some point (not necessarily in the first iteration).
> 
> struct __jmp_buf_tag { };
> typedef struct __jmp_buf_tag jmp_buf[1];
> 
> void f ();
> 
> void test_return ()
> {
>   return;
>   f ();   // warning here (good)
> }
> 
> extern __attribute__ ((noreturn)) void fnoret ();
> 
> void test_noreturn ()
> {
>   fnoret ();
>   f ();   // missing warning
> }
> 
> void test_throw ()
> {
>   throw "";
>   f ();   // missing warning
> }
> 
> jmp_buf jmpbuf;
> 
> void test_longjmp ()
> {
>   __builtin_longjmp (jmpbuf, 1);
>   f ();   // missing warning
> }

All of the last missing cases above would be -Wunreachable-code-ctrl,
they are not after a return ;)

> Please see a few more comments on the code changes inline.
> 
> > 
> > Sorry.
> > 
> > Richard.
> > 
> > 2021-11-26  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR c++/46476
> > gcc/cp/
> >  * decl.c (finish_function): Set input_location to
> >  BUILTINS_LOCATION around the code building the return 0
> >  for main().
> >  * cp-gimplify.c (genericize_if_stmt): Avoid optimizing if (true)
> >  and if (false) when -Wunreachable-code-return is in effect.
> > 
> > gcc/
> >  * common.opt (Wunreachable-code): Re-enable.
> >  (Wunreachable-code-return): New diagnostic, enabled by
> >  -Wextra and -Wunreachable-code.
> >  * doc/invoke.texi (Wunreachable-code): Document.
> >  (Wunreachable-code-return): Likewise.
> >  * gimple-low.c: Include diagnostic.h.
> >  (struct cft_reason): New.
> >  (lower_data::cannot_fallthru): Make a cft_reason.
> >  (lower_stmt): Diagnose unreachable stmts after a return.
> >  * Makefile.in (insn-emit.o-warn): Disable
> >  -Wunreachable-code-return.
> > 
> > gcc/testsuite/
> > 	* c-c++-common/Wunreachable-code-return-1.c: New testcase.
> > ---
> >   gcc/Makefile.in                               |  1 +
> >   gcc/common.opt                                |  8 +++--
> >   gcc/cp/cp-gimplify.c                          |  6 ++--
> >   gcc/cp/decl.c                                 |  9 +++++-
> >   gcc/doc/invoke.texi                           | 13 ++++++++
> >   gcc/gimple-low.c                              | 30 ++++++++++++++++---
> >   .../c-c++-common/Wunreachable-code-return-1.c |  9 ++++++
> >   7 files changed, 67 insertions(+), 9 deletions(-)
> >   create mode 100644 gcc/testsuite/c-c++-common/Wunreachable-code-return-1.c
> > 
> > diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> > index a4344d67f44..71d326ff98c 100644
> > --- a/gcc/Makefile.in
> > +++ b/gcc/Makefile.in
> > @@ -222,6 +222,7 @@ libgcov-merge-tool.o-warn = -Wno-error
> >   gimple-match.o-warn = -Wno-unused
> >   generic-match.o-warn = -Wno-unused
> >   dfp.o-warn = -Wno-strict-aliasing
> > +insn-emit.o-warn = -Wno-unreachable-code-return
> >   
> >   # All warnings have to be shut off in stage1 if the compiler used then
> >   # isn't gcc; configure determines that.  WARN_CFLAGS will be either
> > diff --git a/gcc/common.opt b/gcc/common.opt
> > index 755e1a233b7..486ea36c8e5 100644
> > --- a/gcc/common.opt
> > +++ b/gcc/common.opt
> > @@ -806,8 +806,12 @@ 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
> > +Catch all for -Wunreachable-code-*.
> > +
> > +Wunreachable-code-return
> 
> -Wunreachable-code-return sounds a bit awkward to me.  I would
> suggest -Wunreachable-return instead (especially if you think
> we might add other similar variations).

I've originally mimiced the clang warning name here but I later
realized clang uses it to diagnose unreachable returns rather than
unrechable code _after_ a return ... still -Wunreachable-return
very much sounds like a return stmt is unreachable.

I think it makes sense to differentiate unreachable code following
a stmt that unconditionally transfers control (return, break, continue,
goto) from unreachable code following a stmt that conditionally
transfers control (if, switch, etc.) [what about if (0)?] and
from unreachable code following a call that does not return
normally (abort (), throw, etc.).

Suggestions for a better option name appreciated.  Maybe we can
simply lump it all under -Wunreachable-code at first and split
it later.

> > +Common Var(warn_unreachable_code_return) Warning
> > EnabledBy(Wunreachable-code || Wextra)
> > +Warn about unreachable statements after a return.
> >   
> >   Wunused
> >   Common Var(warn_unused) Init(0) Warning
> > diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
> > index c1691c3e073..55e03abba43 100644
> > --- a/gcc/cp/cp-gimplify.c
> > +++ b/gcc/cp/cp-gimplify.c
> > @@ -167,9 +167,11 @@ genericize_if_stmt (tree *stmt_p)
> >        the then_ block regardless of whether else_ has side-effects or not.
> >     */
> >     if (IF_STMT_CONSTEVAL_P (stmt))
> >       stmt = else_;
> > -  else if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_))
> > +  else if (!warn_unreachable_code_return
> > +	   && integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_))
> >       stmt = then_;
> > -  else if (integer_zerop (cond) && !TREE_SIDE_EFFECTS (then_))
> > +  else if (!warn_unreachable_code_return
> > +	   && integer_zerop (cond) && !TREE_SIDE_EFFECTS (then_))
> >       stmt = else_;
> >     else
> >       stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_);
> > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> > index 56f80775ca0..cb81a794e5b 100644
> > --- a/gcc/cp/decl.c
> > +++ b/gcc/cp/decl.c
> > @@ -17573,7 +17573,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;
> 
> That does seem like a hack.  Won't it compromise other warnings?
> Would calling suppress_warning (retstmt, OPT_Wunreachable_return)
> on the return statement work?  (Similar to what finish_return_stmt
> already does for OPT_Wreturn_type.)

The C frontend does the very same thing.  No, suppress_warning
doesn't seem to work here because gimplification wrecks it,
at least that's what I remember from the original attemt to diagnose
things at CFG construction time, I didn't re-try with this
approach.

> > +	  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 3bddfbaae6a..7d7f2e7fddc 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -400,6 +400,7 @@ Objective-C and Objective-C++ Dialects}.
> >   -Wsystem-headers  -Wtautological-compare  -Wtrampolines  -Wtrigraphs @gol
> >   -Wtsan -Wtype-limits  -Wundef @gol
> >   -Wuninitialized  -Wunknown-pragmas @gol
> > +-Wunreachable-code -Wunreachable-code-return @gol
> >   -Wunsuffixed-float-constants  -Wunused @gol
> >   -Wunused-but-set-parameter  -Wunused-but-set-variable @gol
> >   -Wunused-const-variable  -Wunused-const-variable=@var{n} @gol
> > @@ -5721,6 +5722,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-return @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{)}}
> > @@ -6865,6 +6867,17 @@ This warning is enabled by default for C and C++
> > programs.
> >   Warn when @code{__sync_fetch_and_nand} and @code{__sync_nand_and_fetch}
> >   built-in functions are used.  These functions changed semantics in GCC
> >   4.4.
> >   
> > +@item -Wunreachable-code
> > +@opindex Wunreachable-code
> > +@opindex Wno-unreachable-code
> > +Warn about unreachable code.  Enables @option{-Wunreachable-code-return}.
> > +
> > +@item -Wunreachable-code-return
> > +@opindex Wunreachable-code-return
> > +@opindex Wno-unreachable-code-return
> > +Warn about unreachable code after a return stmt.
> 
> The word is "statement" and the @code{return} keyword should be
> quoted.  I would also suggest to consider clarifying the text by
> saying it warns for the first statement that's rendered unreachable
> by a prior return statement (i.e., make clear it doesn't warn for
> all unreachable code, or any arbitrary piece of it).

OK, will do.

> > Enabled by
> > +@option{-Wunreachable-code} and @option{-Wextra}.
> > +
> >   @item -Wunused-but-set-parameter
> >   @opindex Wunused-but-set-parameter
> >   @opindex Wno-unused-but-set-parameter
> > diff --git a/gcc/gimple-low.c b/gcc/gimple-low.c
> > index 7e39c22df44..f0eb82f72f6 100644
> > --- a/gcc/gimple-low.c
> > +++ b/gcc/gimple-low.c
> > @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.  If not see
> >   #include "predict.h"
> >   #include "gimple-predict.h"
> >   #include "gimple-fold.h"
> > +#include "diagnostic.h"
> >   
> >   /* The differences between High GIMPLE and Low GIMPLE are the
> >      following:
> > @@ -56,6 +57,22 @@ struct return_statements_t
> >   };
> >   typedef struct return_statements_t return_statements_t;
> >   
> > +/* Helper tracking the reason a previous stmt cannot fallthru.  */
> > +struct cft_reason
> > +{
> > +  enum reason { CAN_FALLTHRU = false, UNKNOWN = true, RETURN };
> > +  cft_reason () : m_reason (CAN_FALLTHRU) {}
> > +  cft_reason (bool b) : m_reason (b ? UNKNOWN : CAN_FALLTHRU) {}
> > +  cft_reason (reason r) : m_reason (r) {}
> > +  operator bool () const { return m_reason != CAN_FALLTHRU; }
> > +  cft_reason& operator|= (const cft_reason& other)
> > +    {
> > +      if (other.m_reason != m_reason && (bool)other)
> > +	m_reason = UNKNOWN;
> > +      return *this;
> > +    }
> > +  reason m_reason;
> > +};
> >   
> >   struct lower_data
> >   {
> > @@ -67,7 +84,7 @@ struct lower_data
> >     vec<return_statements_t> return_statements;
> >   
> >     /* True if the current statement cannot fall through.  */
> > -  bool cannot_fallthru;
> > +  cft_reason cannot_fallthru;
> >   };
> >   
> >   static void lower_stmt (gimple_stmt_iterator *, struct lower_data *);
> > @@ -84,7 +101,7 @@ static void lower_builtin_posix_memalign
> > (gimple_stmt_iterator *);
> >   static unsigned int
> >   lower_function_body (void)
> >   {
> > -  struct lower_data data;
> > +  struct lower_data data = {};
> >     gimple_seq body = gimple_body (current_function_decl);
> >     gimple_seq lowered_body;
> >     gimple_stmt_iterator i;
> > @@ -96,7 +113,6 @@ lower_function_body (void)
> >     gcc_assert (gimple_seq_first (body) == gimple_seq_last (body)
> >          && gimple_code (gimple_seq_first_stmt (body)) == GIMPLE_BIND);
> >   -  memset (&data, 0, sizeof (data));
> >     data.block = DECL_INITIAL (current_function_decl);
> >     BLOCK_SUBBLOCKS (data.block) = NULL_TREE;
> >     BLOCK_CHAIN (data.block) = NULL_TREE;
> > @@ -249,6 +265,12 @@ lower_stmt (gimple_stmt_iterator *gsi, struct
> > lower_data *data)
> >   
> >     gimple_set_block (stmt, data->block);
> >   +  if (data->cannot_fallthru.m_reason == cft_reason::RETURN
> > +      && gimple_code (stmt) != GIMPLE_LABEL
> > +      && LOCATION_LOCUS (gimple_location (stmt)) > BUILTINS_LOCATION)
> > +    warning_at (gimple_location (stmt), OPT_Wunreachable_code_return,
> > +		"statement after return is not reachable");
> 
> As a keyword the word return should be quoted.

Fixed.

Thanks,
Richard.
  

On Mon, Nov 29, 2021 at 5:21 PM Martin Sebor via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

> There are some other "unusual" cases worth a look, such as missing
> context of any kind except for like and column:
>
> elfnn-riscv.c:3346:7: warning: statement after return is not reachable
> [-Wunreachable-code-return]
> elfnn-riscv.c:3349:7: warning: statement after return is not reachable
> [-Wunreachable-code-return]
> elfnn-riscv.c:3352:7: warning: statement after return is not reachable
> [-Wunreachable-code-return]
> elfnn-riscv.c:3355:7: warning: statement after return is not reachable
> [-Wunreachable-code-return]
>

That file has "return ...; break;" inside a switch.  Warning for that is
OK, and we shouldn't need a better warning message.  It is pretty obvious
what is wrong.

Jim
  

On 12/9/21 3:58 PM, Jim Wilson wrote:
> On Mon, Nov 29, 2021 at 5:21 PM Martin Sebor via Gcc-patches 
> <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote:
> 
>     There are some other "unusual" cases worth a look, such as missing
>     context of any kind except for like and column:
> 
>     elfnn-riscv.c:3346:7: warning: statement after return is not reachable
>     [-Wunreachable-code-return]
>     elfnn-riscv.c:3349:7: warning: statement after return is not reachable
>     [-Wunreachable-code-return]
>     elfnn-riscv.c:3352:7: warning: statement after return is not reachable
>     [-Wunreachable-code-return]
>     elfnn-riscv.c:3355:7: warning: statement after return is not reachable
>     [-Wunreachable-code-return]
> 
> 
> That file has "return ...; break;" inside a switch.  Warning for that is 
> OK, and we shouldn't need a better warning message.  It is pretty 
> obvious what is wrong.

Yes, it's obvious from code inspection.  What I meant is that
the warnings above don't show the code they refer to as other
messages do, like on the second line in:

/src/binutils-gdb/bfd/elf-attrs.c:450:7: warning: statement after return 
is not reachable [-Wunreachable-code-return]
   450 |       break;
       |       ^~~~~

If they did, no code inspection would be necessary to understand
that the warnings are valid and why (I suggested elsewhere to
add a note pointing to the statement that makes the one in
the warning unreachable; that would help in cases when the first
statement is obscured by a macro, for instance, or far away from
the warning site).

The above were the only warnings I saw in the Binutils build with
this problem (or ever, that I can think of).

Martin