It seems impossible to me that this issue has not been discussed already, but in the library I fixed only two weeks ago a *very* stupid bug which went unnoticed for *many* months exactly because of this. Couldn't believe that GCC didn't warn even at -Wall -Wextra (and -O2). Thus: int foo(int& num) { return num; ++num; // Warn! } int bar(int num) { return num; ++num; // Warn! return num; } For the record, ICC *does* warn, with -Wall.
Confirmed.
(In reply to comment #1) > Confirmed. Hi Richard, how do you think this could be implemented? Wasn't -Wunreachable-code removed just a few releases ago?
As the *-Wunreachable-code* was removed, why isn't there any *deprecated* message or *warning* shown to the user, when using this option? That leads to the illusion *-Wunreachable-code* is "working" - which is obviously not the case.
*** Bug 55698 has been marked as a duplicate of this bug. ***
*** Bug 65472 has been marked as a duplicate of this bug. ***
Richard, perhaps a less aggressive -Wunreachable-code could be implemented just after (or while) building the control flow graph? It would not try to be smart with constant propagation or guessing branches taken or not taken, just warn for code that immediately follows return, break, continue, goto or throw. See PR69857. Clang does have -Wunreachable-code.
*** Bug 80698 has been marked as a duplicate of this bug. ***
Happy to pay 200 USD bounty on a committed implementation for -Wunreachable-code examples. Even just instructions just after "return" or "break" etc
Clang does also have -Wunreachable-code-break and -Wunreachable-code-return, which are really nice to have because you can turn them into errors separately. But even clang misses a few cases that VS2015 can detect.
Perhaps some new static analysis tools could be added to GCC. I'd be happy to contribute to costs.
(In reply to Jon Grant from comment #11) > Perhaps some new static analysis tools could be added to GCC. I'd be happy > to contribute to costs. I keep finding new warnings that static analysis tools emit that gcc can't, but rarely do these new warnings get implemented in gcc. Gcc developers seem keen to keep away from static analysis area. If you push them, they say they are only a volunteer project. Far more folks run compilers than static analysis tools, so popularising new warnings would help the wider community. In practice locally, I modify gcc compilers to also run static analysis tools. Then developers can't avoid knowing more about their code.
May be simpler to just implement these static analysis checkers outside of a compiler.
(In reply to Jon Grant from comment #13) > May be simpler to just implement these static analysis checkers outside of a > compiler. Or as a plugin to GCC (so that it reuses GCC internals) that is stored in GCC source repository and build alongside. But the main barrier for this is not technical or acceptance, it is leadership and human resources. Someone has to take care of such a project, find developers and commit to maintain it. For various (bad) reasons, people tend to prefer to develop their personal in-house ad-hoc solution, rather than work with others with similar goals and with the GCC community. The end result is that existing solutions are poor, not widely used and often abandoned. On the other hand, there are many examples to follow on how to approach such a project. This includes all the enhancements to diagnostics done by non-core developers, libcc1, the D front-end, every new target added by non-core developers, etc.
I saw Bjarne Stroustrup announced C++ Core Guidelines, as a gitproject which includes a checker. At least it would all be in one place as a project.
(In reply to Manuel López-Ibáñez from comment #14) > But the main barrier for this is not technical or acceptance, it is > leadership and human resources. And the usual time and money. There are plenty of static analysers out there. Unless it is substantially better, why write another one ? My favourite static analyser, cppcheck, says this for the original code: $ ~/cppcheck/trunk/cppcheck --enable=all bug46476.cc [bug46476.cc:5]: (style) Statements following return, break, continue, goto or throw will never be executed. [bug46476.cc:11]: (style) Statements following return, break, continue, goto or throw will never be executed. [bug46476.cc:8]: (style) The function 'bar' is never used. [bug46476.cc:2]: (style) The function 'foo' is never used. $ which pretty much does the job. Running the same static analyser over the source code of a recent gcc found 22 occurrences of this kind of problem. Here they are: $ fgrep "Statements following" cppcheck.20170617.out [trunk/gcc/c/c-decl.c:3211]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/gcc/fortran/arith.c:2009]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/dwarf.c:2709]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/dwarf.c:2758]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/dwarf.c:2892]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/dwarf.c:3025]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/elf.c:448]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/elf.c:493]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/elf.c:967]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/fileline.c:64]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/fileline.c:75]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/pecoff.c:499]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/pecoff.c:564]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libbacktrace/pecoff.c:931]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libcilkrts/runtime/cilk_fiber.cpp:968]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libcilkrts/runtime/scheduler.c:2468]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libcilkrts/runtime/scheduler.c:2550]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libcilkrts/runtime/scheduler.c:2439]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libffi/src/dlmalloc.c:3877]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libgomp/error.c:90]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libgomp/libgomp-plugin.c:79]: (style) Statements following return, break, continue, goto or throw will never be executed. [trunk/libobjc/error.c:41]: (style) Statements following return, break, continue, goto or throw will never be executed. $ Most of them seem to be in libbacktrace. I could look deeper into these and generate some bug reports. That's the usual way to provoke gcc developers into developing a new warning: show that the gcc source code would benefit from it.
(In reply to David Binderman from comment #16) > (In reply to Manuel López-Ibáñez from comment #14) > > But the main barrier for this is not technical or acceptance, it is > > leadership and human resources. > > And the usual time and money. There are plenty of static analysers out there. > Unless it is substantially better, why write another one ? > > My favourite static analyser, cppcheck, says this for the original code: > > $ ~/cppcheck/trunk/cppcheck --enable=all bug46476.cc > [bug46476.cc:5]: (style) Statements following return, break, continue, goto > or throw will never be executed. > [bug46476.cc:11]: (style) Statements following return, break, continue, goto > or throw will never be executed. > [bug46476.cc:8]: (style) The function 'bar' is never used. > [bug46476.cc:2]: (style) The function 'foo' is never used. > $ > > which pretty much does the job. > > Running the same static analyser over the source code of a recent gcc > found 22 occurrences of this kind of problem. > > Here they are: > > $ fgrep "Statements following" cppcheck.20170617.out > [trunk/gcc/c/c-decl.c:3211]: (style) Statements following return, break, > continue, goto or throw will never be executed. > [trunk/gcc/fortran/arith.c:2009]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/dwarf.c:2709]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/dwarf.c:2758]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/dwarf.c:2892]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/dwarf.c:3025]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/elf.c:448]: (style) Statements following return, break, > continue, goto or throw will never be executed. > [trunk/libbacktrace/elf.c:493]: (style) Statements following return, break, > continue, goto or throw will never be executed. > [trunk/libbacktrace/elf.c:967]: (style) Statements following return, break, > continue, goto or throw will never be executed. > [trunk/libbacktrace/fileline.c:64]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/fileline.c:75]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/pecoff.c:499]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/pecoff.c:564]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libbacktrace/pecoff.c:931]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libcilkrts/runtime/cilk_fiber.cpp:968]: (style) Statements following > return, break, continue, goto or throw will never be executed. > [trunk/libcilkrts/runtime/scheduler.c:2468]: (style) Statements following > return, break, continue, goto or throw will never be executed. > [trunk/libcilkrts/runtime/scheduler.c:2550]: (style) Statements following > return, break, continue, goto or throw will never be executed. > [trunk/libcilkrts/runtime/scheduler.c:2439]: (style) Statements following > return, break, continue, goto or throw will never be executed. > [trunk/libffi/src/dlmalloc.c:3877]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libgomp/error.c:90]: (style) Statements following return, break, > continue, goto or throw will never be executed. > [trunk/libgomp/libgomp-plugin.c:79]: (style) Statements following return, > break, continue, goto or throw will never be executed. > [trunk/libobjc/error.c:41]: (style) Statements following return, break, > continue, goto or throw will never be executed. > $ > > Most of them seem to be in libbacktrace. I could look deeper into these > and generate some bug reports. That's the usual way to provoke gcc developers > into developing a new warning: show that the gcc source code would benefit > from it. Dunno how I missed this when I created the new cppcheck meta-bug; adding this as a dependency for it now
We can warn at CFG construction time. Note the ??? though, we'd want to improve here to avoid duplicate diagnostics. Tricky cases: /* Unreachable region entry has a predecessor (backedge). */ void foo() { return; for (int i = 0; i < 5; ++i) ; } /* Unreachable region not backwards reachable from exit. */ void bar1() { return; __builtin_abort (); } void bar2() { return ; for (;;); } /* Unreachable code in if (0) block. */ void baz(int *p) { if (0) { return; *p = 0; } } bootstrap with the prototype currently fails in libgomp: /home/rguenther/src/trunk/libgomp/oacc-plugin.c: In function 'GOMP_PLUGIN_acc_default_dim': /home/rguenther/src/trunk/libgomp/oacc-plugin.c:65:7: error: statement is not reachable [-Werror] 65 | return -1; | ^~~~~~ /home/rguenther/src/trunk/libgomp/oacc-profiling.c: In function 'acc_prof_register': /home/rguenther/src/trunk/libgomp/oacc-profiling.c:354:7: error: statement is not reachable [-Werror] 354 | __builtin_unreachable (); | ^~~~~~~~~~~~~~~~~~~~~ /home/rguenther/src/trunk/libgomp/oacc-profiling.c: In function 'acc_prof_unregister': /home/rguenther/src/trunk/libgomp/oacc-profiling.c:475:7: error: statement is not reachable [-Werror] 475 | __builtin_unreachable (); | ^~~~~~~~~~~~~~~~~~~~~ the latter two are an issue with inital CFG construction I think, where group_case_labels turns void bar (foo x) { <bb 2> : switch (x) <default: <L2>, case 0: <L0>, case 1: <L1>> <bb 3> : <L0>: goto <L2>; <bb 4> : <L1>: __builtin_unreachable (); <bb 5> : <L2>: return; into the following with BB 4 now unreachable. void bar (foo x) { <bb 2> : switch (x) <default: <L2>, case 0: <L0>> <bb 3> : <L0>: goto <L2>; <bb 4> : <L1>: __builtin_unreachable (); <bb 5> : <L2>: return; The C++ FE also warns about the implicit return in main when there's a preceeding one (the C frontend "appropriately" assigns an internal location which supresses the warning). diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index b3a27bcd17c..64ab2607c56 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -242,7 +242,8 @@ build_gimple_cfg (gimple_seq seq) /* 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 (); + /* ??? This interferes with unreachable code diagnostics. */ + //group_case_labels (); /* Create the edges of the flowgraph. */ discriminator_per_locus = new hash_table<locus_discrim_hasher> (13); @@ -374,6 +375,24 @@ execute_build_cfg (void) fprintf (dump_file, "Scope blocks:\n"); dump_scope_blocks (dump_file, dump_flags); } + + find_unreachable_blocks (); + basic_block bb; + FOR_EACH_BB_FN (bb, cfun) + if (!(bb->flags & BB_REACHABLE)) + for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); + gsi_next (&gsi)) + { + if ((LOCATION_LOCUS (gimple_location (gsi_stmt (gsi))) + > BUILTINS_LOCATION) + && !gimple_no_warning_p (gsi_stmt (gsi))) + warning_at (gimple_location (gsi_stmt (gsi)), 0, + "statement is not reachable"); + /* ??? Mark blocks reachable from here. And even better make + sure to process entries to unreachable regions first. */ + break; + } + cleanup_tree_cfg (); bb_to_omp_idx.release ();
Posted RFC patch. The C++ frontend poses some more issues in GCC code for statically true evaluated conditions like if (! GATHER_STATISTICS) { fprintf (stderr, "No RTX statistics\n"); return; } where it elides the if (). It doesn't do that for if (0) though.
(In reply to Richard Biener from comment #18) > /home/rguenther/src/trunk/libgomp/oacc-plugin.c: In function > 'GOMP_PLUGIN_acc_default_dim': > /home/rguenther/src/trunk/libgomp/oacc-plugin.c:65:7: error: statement is > not reachable [-Werror] > 65 | return -1; > | ^~~~~~ (That's correct, and you do address that in the patch posted.) It feels strange to not have a 'return' in a non-'void' function, but that's fine, given 'gomp_fatal' being 'noreturn'. For posterity (only; these "bad" cases have not made it into the patch posted): > /home/rguenther/src/trunk/libgomp/oacc-profiling.c: In function > 'acc_prof_register': > /home/rguenther/src/trunk/libgomp/oacc-profiling.c:354:7: error: statement > is not reachable [-Werror] > 354 | __builtin_unreachable (); > | ^~~~~~~~~~~~~~~~~~~~~ > /home/rguenther/src/trunk/libgomp/oacc-profiling.c: In function > 'acc_prof_unregister': > /home/rguenther/src/trunk/libgomp/oacc-profiling.c:475:7: error: statement > is not reachable [-Werror] > 475 | __builtin_unreachable (); > | ^~~~~~~~~~~~~~~~~~~~~ > > the latter two are an issue with inital CFG construction I think, where > group_case_labels turns > > void bar (foo x) > { > <bb 2> : > switch (x) <default: <L2>, case 0: <L0>, case 1: <L1>> > > <bb 3> : > <L0>: > goto <L2>; > > <bb 4> : > <L1>: > __builtin_unreachable (); > > <bb 5> : > <L2>: > return; > > into the following with BB 4 now unreachable. > > void bar (foo x) > { > <bb 2> : > switch (x) <default: <L2>, case 0: <L0>> > > <bb 3> : > <L0>: > goto <L2>; > > <bb 4> : > <L1>: > __builtin_unreachable (); > > <bb 5> : > <L2>: > return; The source-level situation here is: [...] 256 /* Special cases. */ 257 if (reg == acc_toggle) [...] 274 else if (reg == acc_toggle_per_thread) 275 { [...] 284 /* Silently ignore. */ 285 gomp_debug (0, " ignoring bogus request\n"); 286 return; 287 } [...] 302 switch (reg) 303 { [...] 353 case acc_toggle_per_thread: 354 __builtin_unreachable (); 355 } [...] ..., and similar for the other instance. Here, the point is to (a) enumerate all possible 'enum' values in the 'switch (reg)', but (b) make it clear ('__builtin_unreachable') that we're not expecting 'acc_toggle_per_thread' here, as it has already been handled (plus early 'return') above. In my opinion, we shouldn't diagnose these cases (and you don't, per the patch posted).
*** Bug 80701 has been marked as a duplicate of this bug. ***
Created attachment 51876 [details] -Wunreachable-code at CFG construction time
Created attachment 51877 [details] some fallout in GCC This fixes some fallout appearant when bootstrapping with the patch, mostly style, so not pushed to trunk. There are more unresolved -Werror cases because of the change so I'm not sure that warning at CFG construction is good enough for a narrow scope warning. Instead the summary requested stmt after return could likely be implemented at parsing time without too much hassle (but obviously repeated in every frontend).
See PR50847 re dead code after C++ 'throw'.
Created attachment 51878 [details] -Wunreachable-code-return at GIMPLE lowering time This is an alternative change only implementing -Wunreachable-code-return (-Wunreachable-code-throw should be doable within this framework as well). It does so during GIMPLE lowering where we still have return stmts using the can_fallthru logic already present. The approach has the same issues with premature optimization by the C++ frontend eliding if (0) and if (1) as shown during bootstrap so the relevant hunk is included here, too, likewise the double return in main(). It also warns for void baz(); void foo (int b) { if (b) __builtin_abort (); else return; baz (); } but not for void baz(); void foo (int b) { if (b) return; else __builtin_abort (); baz (); } as the previous stmt here is not the return but the abort() but in both cases baz () is not really "after return" but after an if, but that part of the IL structure is not easily visible. For the same reason implementing -Wunreachable-code-break as supported by clang is difficult (break is just a goto in GIMPLE). At least this patch passes bootstrap and would have found one real issue but not the problematic dead "looping" stmts.
diff --git a/gcc/gimple-low.c b/gcc/gimple-low.c index 18e66450977..dc56e14b605 100644 --- a/gcc/gimple-low.c +++ b/gcc/gimple-low.c @@ -60,7 +60,7 @@ 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 }; + enum reason { CAN_FALLTHRU = false, UNKNOWN = true, RETURN, CTRL }; cft_reason () : m_reason (CAN_FALLTHRU) {} cft_reason (bool b) : m_reason (b ? UNKNOWN : CAN_FALLTHRU) {} cft_reason (reason r) : m_reason (r) {} @@ -272,6 +304,12 @@ lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data) warning_at (gimple_location (stmt), OPT_Wunreachable_code_return, "statement after return is not reachable"); + if (data->cannot_fallthru.m_reason == cft_reason::CTRL + && gimple_code (stmt) != GIMPLE_LABEL + && LOCATION_LOCUS (gimple_location (stmt)) > BUILTINS_LOCATION) + warning_at (gimple_location (stmt), OPT_Wunreachable_code, + "statement after control statement is not reachable"); + switch (gimple_code (stmt)) { case GIMPLE_BIND: @@ -282,7 +320,7 @@ lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data) case GIMPLE_COND: case GIMPLE_GOTO: case GIMPLE_SWITCH: - data->cannot_fallthru = true; + data->cannot_fallthru = cft_reason::CTRL; gsi_next (gsi); return; would then warn about things like the following (via GIMPLE_GOTO handling), also stmts after continue. void baz(); void foo (int b) { switch (b) { case 1: break; baz (); } } Looks like there's no GIMPLE stmt for throw but we have calls to __cxa_throw so we can handle noreturn & throw here covering all throwing but not fall thru stmts or we can match the exact ABI function being called. As said the main issue will be premature IL eliding.
(In reply to Richard Biener from comment #25) > Created attachment 51878 [details] > -Wunreachable-code-return at GIMPLE lowering time ... > At least this patch passes bootstrap and would have found one real issue > but not the problematic dead "looping" stmts. I was mistaken. The patch runs into some of the same stray return/gcc_unreachable stmts as the other patch, even some more.
Created attachment 51894 [details] -Wunreachable-code-return at GIMPLE lowering time Updated patch
Created attachment 51895 [details] -Wunreachable-code-ctrl at GIMPLE lowering time This is the -Wunreachable-code-ctrl (not enabled by -Wextra) patch diagnosing unreachable stmts after a break, continue, goto (or loops without exit via the backedge goto). Note that unlike clang which seems to model the option names after what kind of stmt is detected as unreachable these patches model the option names after what kind of stmt makes other stmts unreachable. Not sure what is more useful in practice [to avoid coding-style issues].
*** Bug 106456 has been marked as a duplicate of this bug. ***