[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470
--- Comment #30 from Vincent Lefèvre ---
(In reply to Matthias Kretz (Vir) from comment #29)
> Right. But if that's the case wouldn't a warning about the unexpected
> promotion be useful? (which -Wsign-compare happens to provide)
Yes, and FYI, there were such platform-dependent bugs in GNU MPFR in the past
involving additions or subtractions (not comparisons). I added the following in
its README.dev file:
--
Avoid mixing signed and unsigned integer types, as this can lead signed
types to be automatically converted into unsigned types (usual arithmetic
conversions). If such a signed type contains a negative value, the result
may be incorrect on some platforms. With MPFR 2.x, this problem could
arise with mpfr_exp_t, which is signed, and mpfr_prec_t (mp_prec_t),
which was unsigned (it is now signed), meaning that in general, a cast
of a mpfr_prec_t to a mpfr_exp_t was needed.
Note that such bugs are difficult to detect because they may depend on
the platform (e.g., on LP64, 32-bit unsigned int + 64-bit long will give
a signed type, but on ILP32, 32-bit int + 32-bit unsigned long will give
an unsigned type, which may not be what is expected), but also on the
input values. So, do not rely on tests very much. However, if a test
works on 32 bits but fails on 64 bits in the extended exponent range
(or conversely), the cause may be related to the integer types (e.g. a
signness problem or an integer overflow due to different type sizes).
For instance, in MPFR, such issues were fixed in r1992 and r5588.
An example that will fail with 32-bit int and long:
long long umd(void)
{
long a = 1;
unsigned int b = 2;
return a - b;
}
--
So a warning for such operations would also be useful. But I'm not sure about
potential issues with false positives...
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #29 from Matthias Kretz (Vir) --- (In reply to Vincent Lefèvre from comment #28) > (In reply to Matthias Kretz (Vir) from comment #27) > > Fair enough. But how can the compiler be certain that the developer realized > > u and u % 100 is unsigned? Maybe when (s)he wrote the code the expectation > > was for the RHS to be within [-99..99]. > > Indeed. (I never use % when its LHS can be either positive or negative, so > that I didn't think about this case.) FWIW, personally I could use a warning whenever I use % on variables that can potentially be negative. Because my mental model of % N is that I get a result in [0..N-1]. I tried to fix it but my head is stubborn. ;) > Now, the cause of the bug in such a case would be that the user messed up > with the signedness of u, not because he forgot about the promotion rule. > This is something rather different. Right. But if that's the case wouldn't a warning about the unexpected promotion be useful? (which -Wsign-compare happens to provide) To reduce the noise, I won't argue this point any further and let whoever actually implements a fix for the PR decide. :)
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #28 from Vincent Lefèvre --- (In reply to Matthias Kretz (Vir) from comment #27) > Fair enough. But how can the compiler be certain that the developer realized > u and u % 100 is unsigned? Maybe when (s)he wrote the code the expectation > was for the RHS to be within [-99..99]. Indeed. (I never use % when its LHS can be either positive or negative, so that I didn't think about this case.) Now, the cause of the bug in such a case would be that the user messed up with the signedness of u, not because he forgot about the promotion rule. This is something rather different.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #27 from Matthias Kretz (Vir) --- (In reply to Vincent Lefèvre from comment #26) > But if I understand Piotr Engelking's point, UINT_MAX + 1 + s is not in the > range 0..SHRT_MAX mentioned above, thus cannot be equal to u % 100. Said > otherwise, the promotion has no visible effect here (if s is negative, then > the comparison will be false, as if promotion did not occur), so that there > is no need to warn. Fair enough. But how can the compiler be certain that the developer realized u and u % 100 is unsigned? Maybe when (s)he wrote the code the expectation was for the RHS to be within [-99..99]. > That said, this is very specific code, and it might not be worth to handle > it. And IMHO, this is more than what this PR is about. I tend to agree. It's a slightly different problem and IMHO not a 100% obvious false positive. I believe this PR should be about dropping the warning where signed -> unsigned promotion or implicit conversion cannot change the value because the value range of the signed variable is limited to values >= 0.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #26 from Vincent Lefèvre --- (In reply to Matthias Kretz (Vir) from comment #25) > (In reply to Piotr Engelking from comment #24) > > It would be nice if the compiler noticed that rhs is always within > > 0..SHRT_MAX, so the comparison is not surprisingly affected by integer ^^^ > > promotion. > > The problem is the LHS which can be negative and has to be promoted to > `unsigned int`. This promotion changes the value from s to UINT_MAX + 1 + s. > I don't think this example is a case where -Wsign-compare should be silent. But if I understand Piotr Engelking's point, UINT_MAX + 1 + s is not in the range 0..SHRT_MAX mentioned above, thus cannot be equal to u % 100. Said otherwise, the promotion has no visible effect here (if s is negative, then the comparison will be false, as if promotion did not occur), so that there is no need to warn. That said, this is very specific code, and it might not be worth to handle it. And IMHO, this is more than what this PR is about.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #25 from Matthias Kretz (Vir) --- (In reply to Piotr Engelking from comment #24) > It would be nice if the compiler noticed that rhs is always within > 0..SHRT_MAX, so the comparison is not surprisingly affected by integer > promotion. The problem is the LHS which can be negative and has to be promoted to `unsigned int`. This promotion changes the value from s to UINT_MAX + 1 + s. I don't think this example is a case where -Wsign-compare should be silent.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470
Piotr Engelking changed:
What|Removed |Added
CC||inkerman42 at gmail dot com
--- Comment #24 from Piotr Engelking ---
Created attachment 51071
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51071=edit
not useful -Wsign-compare warning involving % operator
Another case where the feature would be useful:
extern int
f(short s, unsigned int u)
{
return s == u % 100;
}
It would be nice if the compiler noticed that rhs is always within 0..SHRT_MAX,
so the comparison is not surprisingly affected by integer promotion.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Eric Gallager changed: What|Removed |Added CC||egallager at gcc dot gnu.org, ||msebor at gcc dot gnu.org --- Comment #23 from Eric Gallager --- (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
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #22 from Matthias Kretz (Vir) --- (In reply to Matthias Kretz (Vir) from comment #21) > However, -O2 would still show the warning. I meant -O0 of course.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Matthias Kretz (Vir) changed: What|Removed |Added CC||kretz at kde dot org --- Comment #21 from Matthias Kretz (Vir) --- 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.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #20 from Andrew Macleod --- No plans at this point to have VRP info in the front end since it requires SSA.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #19 from Manuel López-Ibáñez --- (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.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Andrew Macleod changed: What|Removed |Added CC||amacleod at redhat dot com --- Comment #18 from Andrew Macleod --- The code as shown gets generated right off the bat in SSA as : _1 = ABS_EXPR ; _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 : if (x_3(D) >= 0) goto ; [INV] else goto ; [INV] 2->3 (T) x_3(D) : [0, +INF] int 2->4 (F) x_3(D) : [-INF, 0x] int === BB 3 x_3(D) [0, +INF] int : _5 = (unsigned int) x_3(D); // predicted unlikely by early return (on trees) predictor. goto ; [INV] _5 : [0, 0x7fff] unsigned int === BB 4 x_3(D) [-INF, 0x] int : _1 = -x_3(D); _4 = (unsigned int) _1; _1 : [1, +INF] int _4 : [1, 0x7fff] unsigned int === BB 5 : # _2 = PHI <_5(3), _4(4)> return _2; _2 : [0, 0x7fff] 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.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #17 from Eric Gallager --- I'm wondering if Project Ranger will help this bug at all once it's merged?
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #16 from Manuel López-Ibáñez --- (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.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Eric Gallager changed: What|Removed |Added CC||egall at gwmail dot gwu.edu --- Comment #15 from Eric Gallager --- (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?
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added CC||john.marshall at sanger dot ac.uk --- Comment #14 from Manuel López-Ibáñez manu at gcc dot gnu.org --- *** Bug 66622 has been marked as a duplicate of this bug. ***
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added CC||patrick.pelissier at gmail dot com --- Comment #13 from Manuel López-Ibáñez manu at gcc dot gnu.org --- *** Bug 64518 has been marked as a duplicate of this bug. ***
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added CC||shawn at churchofgit dot com --- Comment #12 from Manuel López-Ibáñez manu at gcc dot gnu.org --- *** Bug 59293 has been marked as a duplicate of this bug. ***
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Bug 38470 depends on bug 23608, which changed state. Bug 23608 Summary: constant propagation (CCP) would improve -Wsign-compare http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23608 What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 --- Comment #11 from Manuel López-Ibáñez manu at gcc dot gnu.org 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
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470
David Stone DeusExSophismata at gmail dot com changed:
What|Removed |Added
CC||DeusExSophismata at gmail
||dot com
--- Comment #10 from David Stone DeusExSophismata at gmail dot com 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.
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added CC||jim at meyering dot net --- Comment #9 from Manuel López-Ibáñez manu at gcc dot gnu.org 2011-06-15 18:15:44 UTC --- *** Bug 49426 has been marked as a duplicate of this bug. ***
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
--- Comment #6 from manu at gcc dot gnu dot org 2010-02-24 14:11 --- 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. -- manu at gcc dot gnu dot org changed: What|Removed |Added BugsThisDependsOn||23608 Summary|dataflow would improve -|value range propagation |Wsign-compare |(VRP) would improve -Wsign- ||compare http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
--- Comment #7 from manu at gcc dot gnu dot org 2010-02-24 14:12 --- (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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470
[Bug c/38470] value range propagation (VRP) would improve -Wsign-compare
--- Comment #8 from m dot j dot thayer at googlemail dot com 2010-02-24 14:16 --- 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 :) -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38470
