Bug 4210 - should not warn in dead code
Summary: should not warn in dead code
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 3.0.1
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
: 8419 26516 28106 30343 40114 44842 60513 62074 79479 85962 94773 (view as bug list)
Depends on:
Blocks:
 
Reported: 2001-09-03 11:36 UTC by gustav
Modified: 2020-06-05 11:14 UTC (History)
17 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2005-12-24 19:53:42


Attachments
Add a new pass for emitting the warning (not working) (1.79 KB, patch)
2020-06-04 21:02 UTC, Niels Möller
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description gustav 2001-09-03 11:36:00 UTC
gcc outputs a warning about shifts >= width of type inside
dead code when that code is dead because of a const
variable. This can easily happen in autogenerated code, or
when you do operations on preprocessor defines.

Release:
GNU C version 3.0.1 20010626 (prerelease) (i386-pc-linux-gnu)

How-To-Repeat:
Compile the following with -Wall -O2:

int rol0(int a)
{
        const int b = 0;
	if (b) {
		a = (a << b) | (a >> (8 * sizeof a - b));
        }
	return a;
}
Comment 1 Wolfgang Bangerth 2002-11-05 07:43:05 UTC
State-Changed-From-To: open->analyzed
State-Changed-Why: Confirmed. I may be wrong, but I believe Toon Moene has
    recently filed a similar bug (recently=it's number should be
    in the high 8000s) where the dead code was actually created
    by a template parameter, instead of a plain constant. Whoever
    looks at this one might want to look for the other report
    as well.
Comment 2 Wolfgang Bangerth 2002-11-06 16:12:43 UTC
From: Wolfgang Bangerth <bangerth@ticam.utexas.edu>
To: gcc-gnats@gcc.gnu.org
Cc:  
Subject: Re: c/4210: bad warning with dead code
Date: Wed, 6 Nov 2002 16:12:43 -0600 (CST)

 I was wrong on the name of the originator, the range of numbers, and maybe 
 more of the duplicate I suspected. It had templates in it, though :-)
 
 In any case, the duplicate was c++/8419, which I closed because it is 
 almost an exact duplicate.
 
 W.
 
 -------------------------------------------------------------------------
 Wolfgang Bangerth              email:           bangerth@ticam.utexas.edu
                                www: http://www.ticam.utexas.edu/~bangerth
Comment 3 Andrew Pinski 2003-05-24 23:14:41 UTC
stills warns on mainline (20030524).
Comment 4 Andrew Pinski 2003-05-31 12:28:16 UTC
I think the problem is the warning happens before dead code elimation happens.
Comment 5 Andrew Pinski 2003-08-11 12:14:59 UTC
suspending as the problem is that the warning comes before striping of dead code and this will 
not fixed for a while.
Comment 6 Mattias Engdegård 2003-08-11 12:29:57 UTC
Is there a technical reason why the bug is difficult to fix? Otherwise I might
look at it myself.
Comment 7 Andrew Pinski 2003-08-11 12:35:55 UTC
As I said the point at which the shift error happens is before GCC knows that the code is dead code 
at all.  The warning happens when generating the trees while dead code dection happens after 
that.
Comment 8 Mattias Engdegård 2003-08-11 12:51:42 UTC
Fine. Since this is about the only warning that I get from dead code, I assumed
that there was a mechanism already in place that silences other warnings from
removed code, but perhaps this is exactly what is missing :)
Or could the test be moved to a later phase (though it's obviously a frontend
issue)?
I'm just looking for an opportunity to do it myself if it's not too complicated.
Comment 9 Mattias Engdegård 2003-09-02 18:56:41 UTC
In gcc 3.3, warning in dead code also occurs in another case. Testcase:

unsigned f(unsigned x)
{
        if (sizeof x > 4)
                return x >= 1ull << 32 ? 1 : 0;
        return 0;
}

yields:
foo.c:4: warning: comparison is always false due to limited range of data type

This is a regression from 3.2.2. [Should I open a new bug for it?]
Comment 10 Wolfgang Bangerth 2003-09-02 21:08:03 UTC
Subject: Re:  should not warning with dead code

> This is a regression from 3.2.2. [Should I open a new bug for it?]

Yes, always.
W.

-------------------------------------------------------------------------
Wolfgang Bangerth              email:            bangerth@ices.utexas.edu
                               www: http://www.ices.utexas.edu/~bangerth/

Comment 11 Andrew Pinski 2003-09-03 11:31:41 UTC
*** Bug 12150 has been marked as a duplicate of this bug. ***
Comment 12 Andrew Pinski 2006-03-01 19:02:05 UTC
*** Bug 26516 has been marked as a duplicate of this bug. ***
Comment 13 joseph@codesourcery.com 2006-03-01 23:01:41 UTC
Subject: Re:  should not warning with dead code

A workaround is to use ? : and statement expressions instead of "if".  
This way, the front-end setting of skip_evaluation disables these 
warnings.  (skip_evaluation can't be set for if (0) because you can jump 
into if (0), whereas jumps into statement expressions are not permitted.  
Thus in general you need to parse the whole function body to tell if the 
if (0) is dead.)

Comment 14 Wolfgang Bangerth 2006-03-01 23:05:45 UTC
But that's
a) clearly a kludge,
b) may not help in the future if our optimizers become more elaborate
c) doesn't work with code where the code guarded by the if(0) is more
   than a single statement.
It would definitely be better to have this fixed in the right place...

W.
Comment 15 joseph@codesourcery.com 2006-03-01 23:22:50 UTC
Subject: Re:  should not warning with dead code

On Wed, 1 Mar 2006, bangerth at dealii dot org wrote:

> c) doesn't work with code where the code guarded by the if(0) is more
>    than a single statement.

It does; I've used it to eliminate all these warnings in glibc's soft-fp 
code.  Use statement expressions, i.e. surround the whole if body with ({ 
}).

Comment 16 Wolfgang Bangerth 2006-03-01 23:25:17 UTC
> It does; I've used it to eliminate all these warnings in glibc's soft-fp 
> code.  Use statement expressions, i.e. surround the whole if body with ({ 
> }).

Ugh. Do we really want to advocate serious code obfuscation to avoid warnings?
W.


Comment 17 Mattias Engdegård 2006-03-02 09:22:28 UTC
We have resorted to case-by-case workarounds, usually a cast which would have been an identity operation had the condition been true. That is,

if (sizeof x == 8)
        return x << 32 | x;

would have its second line mutated to

        return (unsigned long long)x << 32 | x;

The cast is always a no-op unless it occurs in dead code.
Comment 18 Andrew Pinski 2006-06-20 19:05:38 UTC
*** Bug 28106 has been marked as a duplicate of this bug. ***
Comment 19 Andrew Pinski 2007-01-01 19:03:13 UTC
*** Bug 30343 has been marked as a duplicate of this bug. ***
Comment 20 Andrew Pinski 2009-05-12 14:49:49 UTC
*** Bug 40114 has been marked as a duplicate of this bug. ***
Comment 21 Manuel López-Ibáñez 2010-07-06 17:24:00 UTC
*** Bug 44842 has been marked as a duplicate of this bug. ***
Comment 22 Manuel López-Ibáñez 2010-07-06 17:28:11 UTC
The way Clang gets this right is to perform some very-fast bitmap common constant propagation in the FE. I personally think this would be helpful if implemented correctly, even if it slows down the FE a little. But do not wait for a fix soon because this is not trivial to fix and no one has both the desire and the free time to work on fixing this.
Comment 23 Andrew Pinski 2014-03-13 02:10:05 UTC
*** Bug 60513 has been marked as a duplicate of this bug. ***
Comment 24 Andrew Pinski 2014-08-09 06:09:14 UTC
*** Bug 62074 has been marked as a duplicate of this bug. ***
Comment 25 Paul Eggert 2017-02-15 22:00:23 UTC
I'd like this bug to be changed from SUSPENDED to CONFIRMED, given that it's continuing to be a problem (e.g., bug#79479).

Also, I'd like to suggest what I hope is a simple fix. In 2006 Joseph wrote "skip_evaluation can't be set for if (0) because you can jump into if (0), whereas jumps into statement expressions are not permitted". So, how about if we merely set skip_evaluation for "if (0)" when the then-part lacks labels? This should be an easy test, as it shouldn't require parsing the whole function body. The test might still generate false alarms for code containing gotos, but in practice such gotos are rare, so the proposed change should be a significant improvement even if it's not perfect.
Comment 26 Martin Sebor 2017-02-15 23:08:17 UTC
*** Bug 79479 has been marked as a duplicate of this bug. ***
Comment 27 Eric Gallager 2018-02-20 12:02:41 UTC
(In reply to Paul Eggert from comment #25)
> I'd like this bug to be changed from SUSPENDED to CONFIRMED, given that it's
> continuing to be a problem (e.g., bug#79479).

Well, NEW, but OK.
Comment 28 Andrew Pinski 2018-05-29 02:10:53 UTC
*** Bug 85962 has been marked as a duplicate of this bug. ***
Comment 29 Eric Gallager 2019-03-02 01:11:02 UTC
(In reply to Paul Eggert from comment #25)
> I'd like this bug to be changed from SUSPENDED to CONFIRMED, given that it's
> continuing to be a problem (e.g., bug#79479).
> 
> Also, I'd like to suggest what I hope is a simple fix. In 2006 Joseph wrote
> "skip_evaluation can't be set for if (0) because you can jump into if (0),
> whereas jumps into statement expressions are not permitted". So, how about
> if we merely set skip_evaluation for "if (0)" when the then-part lacks
> labels? This should be an easy test, as it shouldn't require parsing the
> whole function body. The test might still generate false alarms for code
> containing gotos, but in practice such gotos are rare, so the proposed
> change should be a significant improvement even if it's not perfect.

Joseph, what do you think about this solution?
Comment 30 joseph@codesourcery.com 2019-03-02 12:13:33 UTC
At the point where the then block starts being processed (and, thus, 
warnings may be given) it wouldn't be known whether there are labels in 
there or not; cf. the discussion in bug 68193 regarding such warnings and 
_Generic.  If you did things based on settings on entry to if (0), you'd 
need some way to end that setting (i.e. restore that from the containing 
block) early on encountering a label (allowing appropriately for nested if 
(0), of course).
Comment 31 Niels Möller 2020-04-26 21:28:14 UTC
*** Bug 94773 has been marked as a duplicate of this bug. ***
Comment 32 Niels Möller 2020-05-04 20:44:32 UTC
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?
Comment 33 Vincent Lefèvre 2020-05-05 02:17:52 UTC
(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.
Comment 34 Martin Sebor 2020-05-05 15:43:52 UTC
(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.
Comment 35 Manuel López-Ibáñez 2020-05-05 17:54:34 UTC
(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.
Comment 36 Niels Möller 2020-05-05 18:26:21 UTC
(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.
Comment 37 Niels Möller 2020-05-05 18:39:28 UTC
(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).
Comment 38 Niels Möller 2020-05-06 08:10:17 UTC
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?
Comment 39 Manuel López-Ibáñez 2020-05-06 15:37:00 UTC
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.
Comment 40 Eric Gallager 2020-05-06 16:27:14 UTC
(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.
Comment 41 Niels Möller 2020-05-06 18:44:08 UTC
(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).
Comment 42 Niels Möller 2020-06-04 21:02:12 UTC
Created attachment 48678 [details]
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?
Comment 43 Marc Glisse 2020-06-05 11:14:01 UTC
(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