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.
