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

[Richard Biener]

On Thu, 25 Nov 2021, Richard Biener wrote:

> On Wed, 24 Nov 2021, Jason Merrill wrote:
> 
> > On 11/24/21 11:15, Marek Polacek wrote:
> > > On Wed, Nov 24, 2021 at 04:21:31PM +0100, Richard Biener via Gcc-patches
> > > wrote:
> > >> This resurrects -Wunreachable-code and implements a warning for
> > >> trivially unreachable code as of CFG construction.  Most problematic
> > >> with this is the C/C++ frontend added 'return 0;' stmt in main
> > >> which the patch handles for C++ like the C frontend already does
> > >> by using BUILTINS_LOCATION.
> > >>
> > >> Another problem for future enhancement is that after CFG construction
> > >> we no longer can point to the stmt making a stmt unreachable, so
> > >> this implementation tries to warn on the first unreachable
> > >> statement of a region.  It might be possible to retain a pointer
> > >> to the stmt that triggered creation of a basic-block but I'm not
> > >> sure how reliable that would be.
> > >>
> > >> So this is really a simple attempt for now, triggered by myself
> > >> running into such a coding error.  As always, the perfect is the
> > >> enemy of the good.
> > >>
> > >> It does not pass bootstrap (which enables -Wextra), because of the
> > >> situation in g++.dg/Wunreachable-code-5.C where the C++ frontend
> > >> prematurely elides conditions like if (! GATHER_STATISTICS) that
> > >> evaluate to true - oddly enough it does _not_ do this for
> > >> conditions evaluating to false ... (one of the
> > >> c-c++-common/Wunreachable-code-2.c cases).
> > > 
> > > I've taken a look into the C++ thing.  This is genericize_if_stmt:
> > > if we have
> > > 
> > >    if (0)
> > >      return;
> > > 
> > > then cond is integer_zerop, then_ is a return_expr, but since it has
> > > TREE_SIDE_EFFECTS, we create a COND_EXPR.  For
> > > 
> > >    if (!0)
> > >       return;
> > > 
> > > we do
> > >   170   else if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_))
> > >   171     stmt = then_;
> > > which elides the if completely.
> > > 
> > > So it seems it would help if we avoided eliding the if stmt if
> > > -Wunreachable-code is in effect.  I'd be happy to make that change,
> > > if it sounds sane.
> 
> Yes, that seems to work.
> 
> > Sure.
> > 
> > Currently the front end does various constant folding as part of
> > genericization, as I recall because there were missed optimizations without
> > it.  Is this particular one undesirable because it's at the statement level
> > rather than within an expression?
> 
> It's undesirable because it short-circuits control flow and thus
> 
>   if (0)
>     return;
>   foo ();
> 
> becomes
> 
>   return;
>   foo ();
> 
> which looks exactly like a case we want to diagnose (very likely a 
> programming error).
> 
> So yes, it applies to the statement level and there only to control
> statements.

So another case in GCC is

  if (WORDS_BIG_ENDIAN == BYTES_BIG_ENDIAN)
...
  else
    {
      /* Assert that we're only dealing with the PDP11 case.  */
      gcc_assert (!BYTES_BIG_ENDIAN);
      gcc_assert (WORDS_BIG_ENDIAN);

      cpp_define (pfile, "__BYTE_ORDER__=__ORDER_PDP_ENDIAN__");

where that macro expands to

      ((void)(!(!0) ? fancy_abort 
("/home/rguenther/src/trunk/gcc/cppbuiltin.c", 180, __FUNCTION__), 0 : 
0));
      ((void)(!(0) ? fancy_abort 
("/home/rguenther/src/trunk/gcc/cppbuiltin.c", 181, __FUNCTION__), 0 : 
0));

      cpp_define (pfile, "__BYTE_ORDER__=__ORDER_PDP_ENDIAN__");

and the frontend elides the COND_EXPRs making the cpp_define
unreachable.  That's only exposed because we no longer elide the
if (1) guardingt this else path ...

Also this is a case where we definitely do not want to
diagnose that either the else or the true path is statically
unreachable.

Richard.