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.
We need a complete testcase.
We need a complete testcase. See http://gcc.gnu.org/bugs
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.
(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 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.
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.
(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.
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 :)
*** Bug 49426 has been marked as a duplicate of this bug. ***
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.
(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 59293 has been marked as a duplicate of this bug. ***
*** Bug 64518 has been marked as a duplicate of this bug. ***
*** Bug 66622 has been marked as a duplicate of this bug. ***
(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?
(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.
I'm wondering if Project Ranger will help this bug at all once it's merged?
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.
(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.
No plans at this point to have VRP info in the front end since it requires SSA.
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.
(In reply to Matthias Kretz (Vir) from comment #21) > However, -O2 would still show the warning. I meant -O0 of course.
(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
Created attachment 51071 [details] 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.
(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.
(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.
(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.
(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.
(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. :)
(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...