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 <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).
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.
