Bug 38470 - value range propagation (VRP) would improve -Wsign-compare
Summary: value range propagation (VRP) would improve -Wsign-compare
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: unknown
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
: 49426 59293 64518 66622 (view as bug list)
Depends on: 23608
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-10 10:34 UTC by Michael Thayer
Modified: 2020-11-03 04:14 UTC (History)
10 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-02-24 13:59:45


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Thayer 2008-12-10 10:34:29 UTC
This is a request to drop the signed/unsigned comparison warning if the compiler can clearly see that the unsigned value is positive.  E.g. in the following case:

if (foo >= 0 && foo < sizeof(bar)) ...

This would avoid the need for a typecase.  Please correct me if this is still not safe for other reasons.
Comment 1 Manuel López-Ibáñez 2010-02-24 13:16:05 UTC
We need a complete testcase.
Comment 2 Manuel López-Ibáñez 2010-02-24 13:16:56 UTC
We need a complete testcase. See http://gcc.gnu.org/bugs
Comment 3 Paolo Carlini 2010-02-24 13:59:45 UTC
I think submitter ask for something like this:

int f(int foo, double bar)
{
  if (foo >= 0 && foo < sizeof(bar))
    return 1;
  return foo;
}

to not warn with -Wsign-compare.
Comment 4 Manuel López-Ibáñez 2010-02-24 14:03:51 UTC
(In reply to comment #3)
> I think submitter ask for something like this:
[snip]
> to not warn with -Wsign-compare.

That would be nice, but it would require moving the warning to the middle-end or some form of dataflow in the front-end. And no one is working on either, so don't hold your breath.
 

Comment 5 Michael Thayer 2010-02-24 14:07:07 UTC
Comment 3 describes what I meant.  And re comment 4, it is a "would be nice to have", obviously if it is too much pain to do then such is life.  Thanks in any case.
Comment 6 Manuel López-Ibáñez 2010-02-24 14:11:06 UTC
Fixing this is even more unlikely than fixing PR 23608, since the latter only asks for constant propagation, but this one requires value range propagation.
Comment 7 Manuel López-Ibáñez 2010-02-24 14:12:57 UTC
(In reply to comment #5)
> Comment 3 describes what I meant.  And re comment 4, it is a "would be nice to
> have", obviously if it is too much pain to do then such is life.  Thanks in any
> case.

Please, do not understand me wrong. I totally agree it would be nice to have and we should leave this open in case someone steps up to the task (patches welcome, as they say). I was just commenting that you should not expect a quick fix because it is more complex than it seems. And thanks for the report.
Comment 8 Michael Thayer 2010-02-24 14:16:55 UTC
Of course.  I asked this without knowing much about compiler internals, but I do have experience of users asking for "little" features which would involve somewhat more work than they would like to think :)
Comment 9 Manuel López-Ibáñez 2011-06-15 18:15:44 UTC
*** Bug 49426 has been marked as a duplicate of this bug. ***
Comment 10 David Stone 2012-03-24 06:52:04 UTC
Simple self-contained test case:


unsigned f (int x) {
	return (x >= 0) ? x : -x;
}
int main () {
    return 0;
}


It seems like we could at least add a simple improvement that just checks for simple comparisons to 0. That probably catches most code (I often do one set of calculations for non-negative values, and another set for negative values) and avoids a lot of problems of trying to track complex calculations for the signedness of the result. I wouldn't be perfect, but it would be better than it currently is.

Being able to detect cases like this is also an optimization opportunity, because division with positive integers is simpler than division with negative integers, for example. We could reuse code that detects that optimization opportunity for this warning.
Comment 11 Manuel López-Ibáñez 2012-04-16 10:43:44 UTC
(In reply to comment #10)
> It seems like we could at least add a simple improvement that just checks for
> simple comparisons to 0. That probably catches most code (I often do one set of

You are welcome to try: http://gcc.gnu.org/contribute.html
Comment 12 Manuel López-Ibáñez 2013-11-25 23:04:27 UTC
*** Bug 59293 has been marked as a duplicate of this bug. ***
Comment 13 Manuel López-Ibáñez 2015-01-07 11:11:44 UTC
*** Bug 64518 has been marked as a duplicate of this bug. ***
Comment 14 Manuel López-Ibáñez 2015-06-22 10:41:38 UTC
*** Bug 66622 has been marked as a duplicate of this bug. ***
Comment 15 Eric Gallager 2016-05-04 16:28:16 UTC
(In reply to Manuel López-Ibáñez from comment #6)
> Fixing this is even more unlikely than fixing PR 23608, since the latter
> only asks for constant propagation, but this one requires value range
> propagation.

That has since been closed as fixed. So are the chances of this one being fixed next somewhat better now?
Comment 16 Manuel López-Ibáñez 2016-05-04 20:56:01 UTC
(In reply to Eric Gallager from comment #15)
> That has since been closed as fixed. So are the chances of this one being
> fixed next somewhat better now?

Not really. PR23608 fixes the case where the variable is actually a "const" by assuming the initial value cannot be changed. There is no propagation of values; we still warn for "i = 5; i < 5u;"

To fix the general case, someone has to implement some kind of reduced dataflow within the FE. Something that once you see "x >= 0", records this info somewhere, keeps track of any subsequent jumps, then when it sees "x < Unsigned", it tries to see if "x >= 0" was recorded and there is a (unconditional?) path from there to here. Doing this seems to me to require a non-trivial amount of work.

The simplest case (x >= 0 && x < Unsigned) could perhaps be handled by some special (folding?) function that when seeing such expression, sets TREE_NO_WARNING for x, or converts the right-hand to (unsigned)x< Unsigned, or something clever that I cannot even imagine. 

If someone is interested in trying the above, look at warn_logical_operator in c-common.c, which is one of those warnings that looks at logical && expressions and tries to figure out whether to warn. You would need to create a similar function (and call it from wherever warn_logical_operator is called from), but you may need to rewrite the expressions or set TREE_NO_WARNING or do something else to avoid warning later. This seems much less work, but still far from trivial and it will only catch this very specific case.
Comment 17 Eric Gallager 2019-09-23 04:58:53 UTC
I'm wondering if Project Ranger will help this bug at all once it's merged?
Comment 18 Andrew Macleod 2019-09-23 15:08:40 UTC
The code as shown gets generated right off the bat in SSA as

 <bb 2> :
  _1 = ABS_EXPR <x_2(D)>;
  _3 = (unsigned int) _1;
  return _3;

which shouldn't even need the Ranger for.  globally _1 should be [0, MAX] and I wouldn't expect any warning... 

If you tweak the code so it shows the if structure:
	if (x >= 0) 
	    return x ;
	return -x;

Then then ranger could help produce the same info:
=========== BB 2 ============
x_3(D)  [-INF, +INF] int
    <bb 2> :
    if (x_3(D) >= 0)
      goto <bb 3>; [INV]
    else
      goto <bb 4>; [INV]

2->3  (T) x_3(D) :      [0, +INF] int
2->4  (F) x_3(D) :      [-INF, 0xffffffff] int

=========== BB 3 ============
x_3(D)  [0, +INF] int
    <bb 3> :
    _5 = (unsigned int) x_3(D);
    // predicted unlikely by early return (on trees) predictor.
    goto <bb 5>; [INV]

_5 : [0, 0x7fffffff] unsigned int

=========== BB 4 ============
x_3(D)  [-INF, 0xffffffff] int
    <bb 4> :
    _1 = -x_3(D);
    _4 = (unsigned int) _1;

_1 : [1, +INF] int
_4 : [1, 0x7fffffff] unsigned int

=========== BB 5 ============
    <bb 5> :
    # _2 = PHI <_5(3), _4(4)>
    return _2;

_2 : [0, 0x7fffffff] unsigned int


It recognizes that both:
a)  _3(D)  [0, +INF] int is positive before being "cast" to unsigned int _5 
b)  _1 is always positive when the conversion to unsigned int is made in BB4.

So the information would be there if one knew what to look for and how to use it.
Comment 19 Manuel López-Ibáñez 2019-09-23 16:20:07 UTC
(In reply to Andrew Macleod from comment #18)
> So the information would be there if one knew what to look for and how to
> use it.

The issue here is to either have VRP info in the FE (like Clang does) or to move this warning to the middle-end.
Comment 20 Andrew Macleod 2019-09-23 17:23:48 UTC
No plans at this point to have VRP info in the front end since it requires SSA.
Comment 21 Matthias Kretz (Vir) 2020-05-06 08:45:15 UTC
Is it possible to insert a predicated warning-marker into the tree in the front end? Basically "if signed object can be negative, emit OPT_Wsign_compare warning". It could even strengthen the message if it detects that the signed object is guaranteed to be negative.

Then the middle end can either drop the warning from the tree or emit it as suggested by the front end.

Test case, showing that simply predicating the warning with `if (x < 0)` works:
https://godbolt.org/z/iFePwJ. However, -O2 would still show the warning.
Comment 22 Matthias Kretz (Vir) 2020-05-06 08:46:07 UTC
(In reply to Matthias Kretz (Vir) from comment #21)
> However, -O2 would still show the warning.

I meant -O0 of course.
Comment 23 Eric Gallager 2020-11-03 04:14:59 UTC
(In reply to Matthias Kretz (Vir) from comment #21)
> Is it possible to insert a predicated warning-marker into the tree in the
> front end? 

I think Martin Sebor was working on a __builtin_warning() function that would work something like that; cc-ing him