583325 - CheckedInt: beware of signed right shift!

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: beware of arithmetic right shift

In C/C++, AFAIU, doing x >> y with x signed can result in a plain shift or in arithmetic shift, the compiler is free to decide.

The problem is that CheckedInt was doing that, and wrongly assuming that this would always be a plain shift (not an arithmetic shift). As a result, on a compiler that would generate arithmetic shifts, it could produce illegal PRBool values such as 0xffffffff.

This patch fixes that and adds some comments. We still do >> but don't rely anymore on it being a plain shift. Before casting to PRBool, we AND with 1.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: beware of arithmetic right shift, take 2

Thanks to cjones for pointing out that shifting signed integers is just plain undefined, at least in the current c++0x draft spec.

This new version only uses a shift in the unsiged case.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: beware of arithmetic right shift, take 3

Wow, this was more subtle than anticipated. It turns out that x >> y is undefined for negative x, and that x << y is undefined even for positive x in a signed type if the result is non-representable. It also turns out that while signed-to-unsigned conversions are well-defined, unsigned-to-signed conversions are only well-defined if the result is representable.

Coming up with a version that's using only well-defined C++ required to know for each integral type what its unsigned variant is. This is now in integer_traits and the unit test is expanded to cover this.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: beware of arithmetic right shift, take 4

... and so, now that we are shifting only unsigned integers, we are again guaranteed that our mIsValid value is always 0 or 1 so we can do without the bit AND in valid().

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: beware of arithmetic right shift, take 5

this update just fixes a couple of typos in a comment.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 461677 [details] [diff] [review]
beware of arithmetic right shift, take 5

http://hg.mozilla.org/mozilla-central/rev/e31957c56d36

But I forgot to ask for approval! Sorry!