[PATCH][RFC] middle-end/46476 - resurrect -Wunreachable-code

Richard Biener rguenther@suse.de
Thu Nov 25 08:52:12 GMT 2021


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.


More information about the Gcc-patches mailing list