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 <rguent...@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" } */
+}
