Bug 68166 - Unsigned bit shift fails under certain conditions in 32 bit builds
Summary: Unsigned bit shift fails under certain conditions in 32 bit builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-15 08:54 PDT by keith.thorne
Modified: 2011-09-16 17:19 PDT (History)
2 users (show)

See Also:


Attachments
test case (285 bytes, text/html)
2011-09-16 00:43 PDT, Alexey Proskuryakov
no flags Details
Fix (6.94 KB, patch)
2011-09-16 14:28 PDT, Gavin Barraclough
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description keith.thorne 2011-09-15 08:54:11 PDT
A code snippet will explain it best:

        var i = 0,
            value = -1698987653.3475556;

        var j = (value >>> (i * 8)) % 256;

(I know that the value is a weird number, but it is what I had in this case).

So, in the above code snippet on Webkit for windows, the formula will return -2 as the answer, which is wrong, as it should return 254 (as it does on all other browsers).
If, however, you do this:

       var value = -1698987653.3475556;

       var j = (value >>> (0 * 8)) % 256;

Note that I have just replaced the i with a 0, the returned value is correct. If the initial value is positive, then the first code snippet works as expected, so the point is, with a negative value and an assigned variable instead of a hard coded integer, the unsigned right shift will fail.

NOTE: the modular is simply there because I had it in my code, it does not really make a difference to the bug itself
NOTE 2: I did not try this with an unsigned left shift
NOTE 3: A signed right shift works as expected.
Comment 1 Alexey Proskuryakov 2011-09-15 16:00:56 PDT
> So, in the above code snippet on Webkit for windows, the formula will return -2 as the answer, 
> which is wrong, as it should return 254 (as it does on all other browsers).

Are you seeing this with a WebKit nightly, or with shipping Safari/WebKit 5.1?

On a Mac, I'm getting 123 everywhere, including Firefox.
Comment 2 Gavin Barraclough 2011-09-15 23:06:04 PDT
(-1698987653.3475556 >>> 0) % 256

I believe evaluates as:

((uint32_t)(int32_t)-1698987653.3475556) % 256
((uint32_t)-1698987653) % 256
0x9ABB817Bu % 256
0x7Bu
123

So as Alexey says, 123 is the correct result.
Are you sure you were seeing -2 or 254 from this test case?
Comment 3 keith.thorne 2011-09-16 00:09:56 PDT
Okay, my bad, I should have double checked the snippet in the console before posting. This here snippet breaks as it should, in the console as well:

var encodeNumber = function(target, bytes, value) {
    var i = 0;

    for( ; i < bytes; i++ ) {
        target.push((value >>> (i * 8)) % 256);
    }
}

var target = [];
var bytes = 4;
var value = -1699021058.648747;
encodeNumber(target, bytes, value);
console.log(target);

(This mirrors more the code that I am working with). The first value of the array (-2) is the problem value.
Comment 4 keith.thorne 2011-09-16 00:11:00 PDT
And a quick addendum...this is only on Windows. Works fine on the Mac
Comment 5 Alexey Proskuryakov 2011-09-16 00:43:03 PDT
Created attachment 107616 [details]
test case

Attaching same test for easier verification.

I can reproduce this in Safari 5.0.5 on Mac OS X when running in 32-bit mode. Haven't tested ToT.
Comment 6 Gavin Barraclough 2011-09-16 13:58:02 PDT
Gottcha.  Hmmm, this repros with just:

var value = -1699021058.1;
var i = 0;
print(value >>> i);

(prints -1699021058, the result of unsigned right shift should never be negative).
Comment 7 Gavin Barraclough 2011-09-16 14:28:33 PDT
Created attachment 107722 [details]
Fix
Comment 8 Gavin Barraclough 2011-09-16 14:32:53 PDT
I should probably add a test case for this. :-(
Comment 9 Geoffrey Garen 2011-09-16 15:35:06 PDT
Comment on attachment 107722 [details]
Fix

r=me, but please check in with a test, and be careful when conflict-resolving with r95324.
Comment 10 Gavin Barraclough 2011-09-16 17:19:32 PDT
Fixed in r95338