[Bug middle-end/4210] should not warning with dead code

[Bug middle-end/4210] should not warning with dead code

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xzfcpw+gcc at gmail dot com

--- Comment #23 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 60513 has been marked as a duplicate of this bug. ***

[Bug middle-end/4210] should not warning with dead code

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |brooks at gcc dot gnu.org

--- Comment #24 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 62074 has been marked as a duplicate of this bug. ***

[Bug middle-end/4210] should not warning with dead code

[nisse at lysator dot liu.se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

Niels Möller <nisse at lysator dot liu.se> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nisse at lysator dot liu.se

--- Comment #31 from Niels Möller <nisse at lysator dot liu.se> ---
*** Bug 94773 has been marked as a duplicate of this bug. ***

[Bug middle-end/4210] should not warning with dead code

[nisse at lysator dot liu.se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #32 from Niels Möller <nisse at lysator dot liu.se> ---
I've checked out the gcc sources, to see if I can understand how to move the
warning around. The example input I'm looking at now is  

unsigned 
shift_dead (unsigned x)
{
  if (0)
    return x >> 32;
  else
    return x >> 1;
}

unsigned 
shift_invalid (unsigned x)
{
  return x >> 32;
}

where I'd like gcc -Wall to warn for the second function, but not from the
first (currently, it warns for both). The warning is emitted from
build_binary_op, in gcc/c/c-typeck.c, close to line 11880. Deleting it there
silences the warning for *both* functions above.

I have a few questions. Keep in mind that I only have a very vague
understanding of the different phases in gcc, so any guidance is appreciated. 

1. There's similar code in c_fully_fold_internal, in gcc/c/c-fold.c, close to
line 400. But that code does *not* emit any warning for the example above,
which surprised me a bit. Maybe that's only for the case that both operands to
the shift operator are constants?

2. More importantly, if the warning is deleted from build_binary_op, we need to
add a check for this case, and an appropriate warning, somewhere later in the
compilation process. It has to be done after dead code is identified, i.e., in
a phase processing only non-dead code. And preferably as early as possibly,
when we're still working close to the parse-tree representation. Is there such
a place? Some other functions traversing the tree are c_gimplify_expr
(gcc/c-family/c-gimplify.c) and verify_tree (gcc/c-family/c-common.c), are any
of those called after elimination of dead code?

3. Alternatively, if there's no place after dead code elimination where the
parse tree is still easily available, a different alternative could be to leave
the check for invalid shift counts in c-typeck.c, but instead of emitting a
warning, construct a special tree object representing an expression with
invalid/undefined behavior, and any meta data needed to emit a warning or error
to describe it? Then emission of the warning could be postponed to later, say,
close to the conversion to SSA form?

4. I also wonder what happens if, for some reason, a constant invalid shift
count is passed through all the way to code generation? Most architectures
would represent a constant shift count for a 32-bit value as 5-bit field in the
opcode, and then the invalid shift counts aren't representable at all. Will gcc
silently ignore higher bits, or is it an internal compiler error, or would it
make sense to produce a friendly warning at this late stage of the compilation?

[Bug middle-end/4210] should not warning with dead code

[vincent-gcc at vinc17 dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #33 from Vincent Lefèvre <vincent-gcc at vinc17 dot net> ---
(In reply to Niels Möller from comment #32)
> 4. I also wonder what happens if, for some reason, a constant invalid shift
> count is passed through all the way to code generation? Most architectures
> would represent a constant shift count for a 32-bit value as 5-bit field in
> the opcode, and then the invalid shift counts aren't representable at all.
> Will gcc silently ignore higher bits,

That's undefined behavior, so that GCC can do whatever it wants for the
generated code.

> or is it an internal compiler error,

This would not be acceptable.

[Bug middle-end/4210] should not warn in dead code

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|should not warning with     |should not warn in dead
                   |dead code                   |code
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #34 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Niels Möller from comment #32)

The front ends can eliminate simple subexpressions (as in '0 ? x >> 32 : x >>
1') but they don't do the same for statements.  Moving the warning from the
front end to some later pass would avoid diagnosing code in those cases (it
would also avoid duplicating the same code between different front ends).  The
earliest is probably gimplify.c.  That would avoid warning on statements
rendered dead as a result of constant expressions (as defined by the language)
but not those whose constant value GCC later propagates from prior assignments,
such as in

  const int zero = 0;

  unsigned 
  shift_dead (unsigned x)
  {
    if (zero)
      return x >> 32;
    else
      return x >> 1;
  }

The later the warning is moved the more statements will be eliminated as dead,
but the more other transformations will also be applied that might eliminate
the warning where it might be desirable, or perhaps even introduce it where it
wouldn't be issued otherwise.

[Bug middle-end/4210] should not warn in dead code

[manu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #35 from Manuel López-Ibáñez <manu at gcc dot gnu.org> ---
(In reply to Niels Möller from comment #32)
> 1. There's similar code in c_fully_fold_internal, in gcc/c/c-fold.c, close
> to line 400. But that code does *not* emit any warning for the example
> above, which surprised me a bit. Maybe that's only for the case that both
> operands to the shift operator are constants?

In general, front-ends should avoid emitting warnings while folding
(simplifying) expressions. I think there is an open bug about this but I cannot
find it now.

Of course, if the code is doing the same, it should be factored out into a
common function. A good first patch to submit:
https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps

> 2. More importantly, if the warning is deleted from build_binary_op, we need
> to add a check for this case, and an appropriate warning, somewhere later in
> the compilation process. It has to be done after dead code is identified,
> i.e., in a phase processing only non-dead code. And preferably as early as
> possibly, when we're still working close to the parse-tree representation.
> Is there such a place? Some other functions traversing the tree are
> c_gimplify_expr (gcc/c-family/c-gimplify.c) and verify_tree
> (gcc/c-family/c-common.c), are any of those called after elimination of dead
> code?

There is no such place. Dead code is identified in the middle-end and by then,
there is no parse tree, only GIMPLE and SSA:
https://gcc.gnu.org/onlinedocs/gccint/Passes.html#Passes

Of course, you could add some kind of unreachable code pass to the FE, but that
would require a lot of work from your side. Clang has something similar that
you could use as an inspiration: 

https://github.com/llvm/llvm-project/blob/2946cd701067404b99c39fb29dc9c74bd7193eb3/clang/include/clang/Analysis/Analyses/ReachableCode.h

> 3. Alternatively, if there's no place after dead code elimination where the
> parse tree is still easily available, a different alternative could be to
> leave the check for invalid shift counts in c-typeck.c, but instead of
> emitting a warning, construct a special tree object representing an
> expression with invalid/undefined behavior, and any meta data needed to emit
> a warning or error to describe it? Then emission of the warning could be
> postponed to later, say, close to the conversion to SSA form?

That is still too early, since the dead code elimination happens after SSA.

[Bug middle-end/4210] should not warn in dead code

[nisse at lysator dot liu.se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #36 from Niels Möller <nisse at lysator dot liu.se> ---
(In reply to Martin Sebor from comment #34)
> 
> The front ends can eliminate simple subexpressions (as in '0 ? x >> 32 : x
> >> 1') but they don't do the same for statements.  Moving the warning from
> the front end to some later pass would avoid diagnosing code in those cases
> (it would also avoid duplicating the same code between different front
> ends).  The earliest is probably gimplify.c.  That would avoid warning on
> statements rendered dead as a result of constant expressions (as defined by
> the language) but not [...]

Thanks. So moving the warning to the gimplify pass would be an improvement over
the current situation. Maybe I can try it out, it sounds reasonably simple.

[Bug middle-end/4210] should not warn in dead code

[nisse at lysator dot liu.se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #37 from Niels Möller <nisse at lysator dot liu.se> ---
(In reply to Manuel López-Ibáñez from comment #35)
> There is no such place. Dead code is identified in the middle-end and by
> then, there is no parse tree, only GIMPLE and SSA:
> https://gcc.gnu.org/onlinedocs/gccint/Passes.html#Passes

So if emission of the warning is postponed to after the Tree SSA passes
(https://gcc.gnu.org/onlinedocs/gccint/Tree-SSA-passes.html#Tree-SSA-passes).
Could perhaps be inserted just after (or as part of) pass_remove_useless_stmts?

> > 3. Alternatively, [...] construct a special tree object representing an
> > expression with invalid/undefined behavior, and any meta data needed to emit
> > a warning or error to describe it? Then emission of the warning could be
> > postponed to later, say, close to the conversion to SSA form?
> 
> That is still too early, since the dead code elimination happens after SSA.

Then that special value needs to be passed through the conversions to gimple
and SSA. I imagine it could be treated much like a compile time constant, but
with an annotation saying it's invalid, and why, to be displayed in case the
compiler thinks it needs to generate any code to evaluate it. (And if we go
that route, we should probably do the same for most or all warnings on invalid
operands detected by the frontend).

[Bug middle-end/4210] should not warn in dead code

[nisse at lysator dot liu.se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #38 from Niels Möller <nisse at lysator dot liu.se> ---
Just a brief update.

1. Tried adding fprintf warnings to c_gimplify_expr (btw, what's the right way
to display a pretty warning with line numbers etc in later passes?). But it
seems that's too early, I still get warnings for dead code.

2. The pass_remove_useless_stmts, mentioned in the docs, was deleted in 2009
(see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41573), and I take it it was
obsoleted earlier, since there's no mention of a replacement. So what pass
should I look at that is related to basic control flow analysis, and discarding
unreachable statements?

[Bug middle-end/4210] should not warn in dead code

[lopezibanez at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #39 from Manuel López-Ibáñez <lopezibanez at gmail dot com> ---
I think these questions are more appropriate for the mailing list, since
few people are subscribed to this bug.

You can easily find which pass does something by dumping (-ftree-dump-*)
all of them and comparing them.

On Wed, 6 May 2020, 09:11 nisse at lysator dot liu.se, <
gcc-bugzilla@gcc.gnu.org> wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210
>
> --- Comment #38 from Niels Möller <nisse at lysator dot liu.se> ---
> Just a brief update.
>
> 1. Tried adding fprintf warnings to c_gimplify_expr (btw, what's the right
> way
> to display a pretty warning with line numbers etc in later passes?). But it
> seems that's too early, I still get warnings for dead code.
>
> 2. The pass_remove_useless_stmts, mentioned in the docs, was deleted in
> 2009
> (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41573), and I take it
> it was
> obsoleted earlier, since there's no mention of a replacement. So what pass
> should I look at that is related to basic control flow analysis, and
> discarding
> unreachable statements?
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.

[Bug middle-end/4210] should not warn in dead code

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |egallager at gcc dot gnu.org

--- Comment #40 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Manuel López-Ibáñez from comment #39)
> I think these questions are more appropriate for the mailing list, since
> few people are subscribed to this bug.

There were more previously, but a lot of people got dropped from cc lists all
throughout bugzilla in the process of transferring servers... I was on this one
previously, for example, but now I'm having to re-subscribe... 

> 
> You can easily find which pass does something by dumping (-ftree-dump-*)
> all of them and comparing them.
> 
> On Wed, 6 May 2020, 09:11 nisse at lysator dot liu.se, <
> gcc-bugzilla@gcc.gnu.org> wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210
> >
> > --- Comment #38 from Niels Möller <nisse at lysator dot liu.se> ---
> > Just a brief update.
> >
> > 1. Tried adding fprintf warnings to c_gimplify_expr (btw, what's the right
> > way
> > to display a pretty warning with line numbers etc in later passes?). But it
> > seems that's too early, I still get warnings for dead code.
> >
> > 2. The pass_remove_useless_stmts, mentioned in the docs, was deleted in
> > 2009
> > (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41573), and I take it
> > it was
> > obsoleted earlier, since there's no mention of a replacement. So what pass
> > should I look at that is related to basic control flow analysis, and
> > discarding
> > unreachable statements?
> >
> > --
> > You are receiving this mail because:
> > You are on the CC list for the bug.

[Bug middle-end/4210] should not warn in dead code

[nisse at lysator dot liu.se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #41 from Niels Möller <nisse at lysator dot liu.se> ---
(In reply to Manuel López-Ibáñez from comment #39)

> You can easily find which pass does something by dumping (-ftree-dump-*)
> all of them and comparing them.

It's -ftree-dump-all, and also -fdump-passes was useful. Thanks! I'm now
compiling without optimization to (i) reduce number of passes, and (ii) because
it would be nice to get it right also in absence of optimization options.

It looks like the dead code is eliminated by the "cfg" (control flow graph)
pass, in gcc/tree-cfg.c. In the .cfg dumpfile it says "Removing basic block 3",
and the invalid shift disappears in that dump. That's nice. Immediately after
this pass comes *warn_function_return, implemented in the same file.

It would make sense to me to add a pass to warn about shift operations with
invalid constant operands at about the same place. Is it easy to traverse a
gimple function, and check all expressions? The "*warn_unused_result" pass
seems to do a similar traversal (but examining statements rather than
expressions, and done *before* the cfg pass).

[Bug middle-end/4210] should not warn in dead code

[nisse at lysator dot liu.se]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #42 from Niels Möller <nisse at lysator dot liu.se> ---
Created attachment 48678
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48678&action=edit
Add a new pass for emitting the warning (not working)

Since adding a new pass seems to be the right way, I've given that a try. See
patch. By my printf debugging I can see that the pass is instantiated and the
execute method is invoked twice (same example program):

unsigned 
shift_dead (unsigned x)
{
  if (0)
    return x >> 32;
  else
    return x >> 1;
}

unsigned 
shift_invalid (unsigned x)
{
  return x >> 32;
}

But no further printouts, it seems my loop 

+void
+do_warn_shift_count_range (gimple_seq seq)
+{
+  gimple_stmt_iterator i;
+  for (i = gsi_start (seq); !gsi_end_p (i); gsi_next (&i))

exits before a single iteration. My intention was that it should iterate over
the body of the current function (i.e., fun->gimple_body), which I expect to be
a single return statement after the cfg pass has removed dead code. I'm
probably misunderstanding the data structures.

And what's the easiest way to run the the right compiler process (I guess
that's cc1) under gdb?

[Bug middle-end/4210] should not warn in dead code

[glisse at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=4210

--- Comment #43 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Niels Möller from comment #42)
> And what's the easiest way to run the the right compiler process (I guess
> that's cc1) under gdb?

gcc -c t.c -wrapper gdb,--args