search

burner / bugzilla_migration_test

0 stars 0 forks source link

Comparing signed to unsigned does not generate an error #1

Open burner opened 18 years ago

burner commented 18 years ago

lio+bugzilla (lionello) reported this on 2006-07-20T06:34:58Z

Transfered from https://issues.dlang.org/show_bug.cgi?id=259

CC List

Description

From the book of Bright, http://www.digitalmars.com/d/expression.html#RelExpression

"It is an error to have one operand be signed and the other unsigned for a <, <=, > or >= expression. Use casts to make both operands signed or both operands unsigned."

...yet...

import std.conv;

int main( char[] args[] ) {

int i = toInt(args[1]);

uint u = toUint(args[2]);

if (i < u)

return 1;

else

return 0;

}

...compiled with "dmd", "dmd -debug" or "dmd -release" does not give an error message, neither at compile time, nor at run time.

burner commented 11 years ago

nick (@ntrel) commented on 2013-08-20T03:11:43Z

(In reply to comment #47)

https://github.com/D-Programming-Language/dmd/pull/1889

Update: that was split into two, see: https://github.com/D-Programming-Language/dmd/pull/1913

burner commented 11 years ago

lio+bugzilla (@lionello) commented on 2013-09-07T12:12:57Z

Also submitted fixes to druntime: https://github.com/D-Programming-Language/druntime/pull/601

(Note: check the pull requests for the latest info)

burner commented 11 years ago

github-bugzilla commented on 2013-09-22T14:11:38Z

Commit pushed to master at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/439d079dce0079cbacaedd91648d85d2c7d660c0 Merge pull request #601 from lionello/bug259

Bug259: fixed signed-unsigned comparisons in druntime

burner commented 10 years ago

bearophile_hugs commented on 2013-12-07T06:48:38Z

If an immutable int x is inside the if branch that asserts it to be not negative, the assignments and comparisons with an uint should not give warnings:

uint z; void main(string[] args) { immutable int x = args.length - 3; if (x >= 0) { uint y = x; // give no errors here if (x > z) {} // give no warning here. } }

burner commented 10 years ago

github-bugzilla commented on 2014-05-22T06:28:04Z

Commit pushed to master at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/8890678458e775e93ade1de307a417b06032ee0f Merge pull request #799 from lionello/bug259

Fixed some signed/unsigned warnings in druntime

burner commented 9 years ago

andrei (@andralex) commented on 2015-03-15T03:41:20Z

What's left to do about this?

burner commented 9 years ago

lio+bugzilla (@lionello) commented on 2015-03-18T11:38:26Z

(In reply to Andrei Alexandrescu from comment #56)

What's left to do about this?

I need to finish some of the static code analysis that I have been adding so we get less false positives. It's the false positives that will turn Walter (and others) off and prevent merging it in.

burner commented 9 years ago

dominikus commented on 2015-04-17T08:49:01Z

The problem is still there, and the behaviour is completely inconsistent, so braking any code isn't a problem I think because I cannot imagine that anybody really relies on the strange behaviour:

unittest { byte a = -3; ubyte b = 2; short c = -3; ushort d = 2; int e = -3; uint f = 2; long g = -3; ulong h = 2;

assert(a < b);
assert(c < d);
assert(e < f); // fails!!
assert(g < h); // fails!!
assert(a < h); // fails!!
assert(b > g);
assert(d > e);

}

So why don't we change to something that simply always works?

int opCmp(T, U)(const(T) a, const(U) b) pure @safe @nogc nothrow if(is(Unqual!T == Unqual!U) || isFloatingPoint!T || isFloatingPoint!U) { // Should be buildin. Naive implementation: return a <= b ? a != b ? -1 : 0 : 1; }

/// Returns negative value if a < b, 0 if they are equal or positive value if a > b. /// This will always yield a correct result, no matter which integral types are compared. /// It uses one extra comparison operation if and only if /// one type is signed and the other unsigned but has bigger max. int opCmp(T, U)(const(T) a, const(U) b) pure @safe @nogc nothrow if(isIntegral!T && isIntegral!U && !is(Unqual!T == Unqual!U)) { static if(T.sizeof == U.sizeof) alias C = Unsigned!T; else alias C = CommonType!(T, U); // this will be the larger type

static if(isSigned!T && isUnsigned!U && T.sizeof <= U.sizeof) { return (a < 0) ? -1 : opCmp(cast(Unsigned!C)a, cast(C)b); } else static if(isUnsigned!T && isSigned!U && T.sizeof >= U.sizeof) { return (b < 0) ? 1 : opCmp(cast(C)a, cast(Unsigned!C)b); } else { // both signed or both unsigned or the unsigned type is smaller // and can therefore be safely cast to the signed type return opCmp(cast(C)a, cast(C)b); } }

burner commented 9 years ago

lio+bugzilla (@lionello) commented on 2015-04-17T09:25:29Z

It's currently using the C integer promotion rules, which are consistent (they're rules after all) but far from simple.

burner commented 9 years ago

dominikus commented on 2015-04-17T11:40:42Z

(In reply to Lionello Lunesu from comment #59)

It's currently using the C integer promotion rules, which are consistent (they're rules after all) but far from simple.

Ah, ok. I see why a<b and c<d work - they are all promoted to int. But never the less: who would be affected by changing the behaviour for int and long? Is really anybody relying on that? And sure that was not a bug from the beginning? I don't think that we really should keep bugs just to be consistend with C. --> this is a case where a compiler warning is good: "Comparing signed with unsigned values. This works in D but may be a performance issue and behaves different from C. Is this intended?" (of course works only after the fix :-)

burner commented 9 years ago

smjg commented on 2015-04-17T12:34:24Z

(In reply to Dominikus Dittes Scherkl from comment #58)

The problem is still there, and the behaviour is completely inconsistent, so braking any code isn't a problem I think because I cannot imagine that anybody really relies on the strange behaviour:

Exactly, because it won't break any code. It will cause code that is already broken to correctly error.

burner commented 8 years ago

jrdemail2000-dlang commented on 2016-04-20T04:03:54Z

Hit this in my code. My bug, but would have been useful if the compiler had informed me there was a signed vs unsigned comparison. Here's the reduced form of what was happening in my code:

void main() {
    int i = -1;
    size_t j = 0;
    assert(i < j);
}

This code compiles, and the assert fails. Except that I didn't have the assert, just the test.

burner commented 8 years ago

dominikus commented on 2016-08-30T20:49:00Z

(In reply to Dominikus Dittes Scherkl from comment #58)

So why don't we change to something that simply always works?

I have meanwhile improved my solution to something more straight forward, with less superfluous casting, and now that the frontend is written in D, it should be no problem to integrate it in the compiler:

int opCmp(T, U)(const(T) a, const(U) b) pure @safe @nogc nothrow if(isIntegral!T && isIntegral!U && is(Unqual!T != Unqual!U)) { static if(isSigned!T && isUnsigned!U && T.sizeof <= U.sizeof) return (a < 0) ? -1 : opCmp(cast(U)a, b); else static if(isUnsigned!T && isSigned!U && T.sizeof >= U.sizeof) return (b < 0) ? 1 : opCmp(a, cast(T)b); else // do interger propagation as ever: return opCmp(cast(CommonType!(T, U))a, cast(CommonType!(T, U))b); }

burner commented 8 years ago

andrei (@andralex) commented on 2016-09-03T13:50:47Z

Time to finalize this. We should be fine with a slight loss in efficiency in mixed sign compare. What active PRs are open at this time?

burner commented 8 years ago

thomas.bockman commented on 2016-10-10T17:33:38Z

(In reply to Andrei Alexandrescu from comment #64)

We should be fine with a slight loss in efficiency in mixed sign compare.

The previously agreed upon solution was to default to warning about each signed/unsigned comparison, but omit the warning if VRP could prove it to be safe. There is no loss of run-time efficiency that way, but some code will break and require an explicit cast to fix.

Are you suggesting that we should just insert an extra comparison to zero as needed, instead?

I'd support that, but I don't know how others (including Walter Bright) would feel about it. The real performance impact should be immeasurably small in nearly all cases (especially with the VRP upgrades discussed below).

What active PRs are open at this time?

My DMD #5229 is a continuation of Lionello's work toward improving VRP sufficiently to minimize code breakage: https://github.com/dlang/dmd/pull/5229

However, it's stuck pending a resolution to another DMD bug: https://issues.dlang.org/show_bug.cgi?id=15585

If #5229 ever gets pulled, one more tricky VRP- related follow-up is required to make the compiler smart enough. After that, actually fixing this bug is trivial: I've had that patch ready to go for about a year now, but can't submit it until the VRP stuff is straightened out because it causes too much breakage.

burner commented 8 years ago

thomas.bockman commented on 2016-10-10T17:35:06Z

Oops! That's the wrong DMD bug. This is the one I actually meant: https://issues.dlang.org/show_bug.cgi?id=14835

burner commented 8 years ago

andrei (@andralex) commented on 2016-10-12T16:21:24Z

(In reply to thomas.bockman from comment #66)

Oops! That's the wrong DMD bug. This is the one I actually meant: https://issues.dlang.org/show_bug.cgi?id=14835

That makes sense. Thanks, and for the update as well.

burner commented 6 years ago

hsteoh commented on 2017-12-15T21:42:45Z

Whoa. This is approaching 12 years now, and even has a preapproved pull. When will the fix actually get merged?!

burner commented 3 years ago

manuelk89 commented on 2021-05-27T09:44:04Z

I hit this 15 years old bug again, and now feel very insecure about the correctness of all integer comparisons in all my code. At least a warning about signed-unsigned comparison would be great, doing the mathematically correct fast & safe integer promotions would be even better. Not sure how to deal with the slower comparisons like long-unsigned that have no direct assembly comparison instruction, maybe just produce an error message.

burner commented 3 years ago

b2.temp commented on 2021-05-27T09:50:25Z

The promotion trick works unless both operand are 64 bits (and considering that cent/ucent are not a thing yet) but that would be a progress. For long cmp ulong a warning could be emitted.

This topic should make its come back on the news group. Even if the current behavior is layered on C semantics that does not mean that it could not change. Following C is not a sacrosant rule.

burner commented 3 years ago

thomas.bockman commented on 2021-05-27T22:30:33Z

(In reply to Manuel König from comment #69)

Not sure how to deal with the slower comparisons like long-unsigned that have no direct assembly comparison instruction, maybe just produce an error message.

Speed is not the issue. The performance cost of a correct signed-unsigned comparison is almost nothing (less than the cost of a single branch mispredict, I believe), and in those very rare cases where the tiny speed boost of doing the comparison incorrectly is really worth it, it is trivial to cast one of the operands to the type of the other.

The real problem is that the current strange behaviour of mixed comparisons is occasionally deliberately used in correct code. D must not be updated in a way that silently breaks all that pre-existing correct code. Hence, a warning (or at least, a long deprecation period) is the only good option for D2.

Introducing such a warning without drowning people in false positives is surprisingly complicated, and is currently stalled waiting for a chain of compiler changes to be completed: https://github.com/dlang/dmd/pull/12311 https://github.com/dlang/dmd/pull/5229 https://github.com/dlang/dmd/pull/1913

burner commented 3 years ago

smjg commented on 2021-06-21T21:14:38Z

(In reply to thomas.bockman from comment #71)

The real problem is that the current strange behaviour of mixed comparisons is occasionally deliberately used in correct code. D must not be updated in a way that silently breaks all that pre-existing correct code. Hence, a warning (or at least, a long deprecation period) is the only good option for D2.

I don't understand. How can code that the spec explicitly forbids possibly be correct?

burner commented 3 years ago

thomas.bockman commented on 2021-06-21T22:05:36Z

(In reply to Stewart Gordon from comment #72)

I don't understand. How can code that the spec explicitly forbids possibly be correct?

Normal programmers, who are not language lawyers, generally consider code to be correct if it reliably does what it is intended to do, in the way it is intended to do it.

They do not know or care what the language spec says, and they assume the compiler can be trusted to enforce such simple rules as "mixed signed-unsigned comparisons are forbidden", if necessary.

And, this is a perfectly rational approach, since both the compiler and the spec are complex, change over time, and sometimes contradict each other. Where there is a conflict, a programmer must satisfy the compiler in order to get work done, while the spec is important only in theory.

Regardless of what the spec says, the de-facto semantics of the D language are that mixed signed-unsigned integer comparisons in D use integer promotion, like other mixed operations.

That can certainly be changed, but the compiler needs to point out code that needs to be updated for the new semantics via warnings, deprecations, or errors.

People need to be able to write code and then move on with their lives, and must not be forced to memorize the language spec and manually review all of the code they have ever written in D every time a new version of the compiler or spec comes out, to avoid silent breakage.