Bug 194007 – [JSC][BigEndians] Several JSC stress tests failing

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tomas Popela]

After bug 132333 has been fixed by Mark (thanks again!) I can run the stress suite again on big endian machines. Nearly everything passes, but these fails:

+ '[' -f ./results/failed ']'
+ cat ./results/failed
stress/dataview-jit-get.js.default
stress/dataview-jit-neuter.js.default
stress/dataview-jit-set.js.default
stress/dataview-get-cse.js.default
stress/dataview-jit-set-nan.js.default

Running stress/dataview-jit-get.js.default
stress/dataview-jit-get.js.default: Exception: Error: Bad!
stress/dataview-jit-get.js.default: [e-mail]:5:24
stress/dataview-jit-get.js.default: [e-mail]:28:15
stress/dataview-jit-get.js.default: global [e-mail]:67:6
stress/dataview-jit-get.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/dataview-jit-get.js.default

Running stress/dataview-jit-neuter.js.default
stress/dataview-jit-neuter.js.default: Exception: Error: Bad!
stress/dataview-jit-neuter.js.default: [e-mail]:5:24
stress/dataview-jit-neuter.js.default: [e-mail]:20:15
stress/dataview-jit-neuter.js.default: global [e-mail]:32:5
FAIL: stress/dataview-jit-neuter.js.default

Running stress/dataview-jit-set.js.default
stress/dataview-jit-set.js.default: Exception: Error
stress/dataview-jit-set.js.default: [e-mail]:5:24
stress/dataview-jit-set.js.default: [e-mail]:64:15
stress/dataview-jit-set.js.default: global [e-mail]:112:5
stress/dataview-jit-set.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/dataview-jit-set.js.default

stress/dataview-get-cse.js.default: Exception: Error
stress/dataview-get-cse.js.default: [e-mail]:5:24
stress/dataview-get-cse.js.default: [e-mail]:57:15
stress/dataview-get-cse.js.default: global [e-mail]:60:6
stress/dataview-get-cse.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/dataview-get-cse.js.default

Running stress/dataview-jit-set-nan.js.default
stress/dataview-jit-set-nan.js.default: Exception: Error
stress/dataview-jit-set-nan.js.default: [e-mail]:7:24
stress/dataview-jit-set-nan.js.default: [e-mail]:33:15
stress/dataview-jit-set-nan.js.default: global [e-mail]:52:5
stress/dataview-jit-set-nan.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/dataview-jit-set-nan.js.default

I see that some of these tests do have something to do with endianess (as stated in them). But I'm not sure whether it's broken, or these tests should be skipped when running on CLoop (on x86_64, ppc64le and others they passes - so probably the big endian part is buggy). What makes me thing about skipping them is that when Tools/Scripts/run-jsc-stress-tests --no-jit --memory-limited is involved then all the failing tests are printed prefixed with "Skipped", but they are run in the end.

Can you please Saam (as an author of these tests) clarify what's wrong?

[Mark Lam]

All of these are JIT tests. They say so in their names (note: CSE refers to a JIT optimization).  Hence, they are not relevant to the CLoop.

[Tomas Popela]

Created attachment 360569 [details]
Patch

[Tomas Popela]

(In reply to Mark Lam from comment #1)
> All of these are JIT tests. They say so in their names (note: CSE refers to
> a JIT optimization).  Hence, they are not relevant to the CLoop.

Thank you for the clarification Mark!

[Dominik Inführ]

Not sure these tests should be skipped if `$jitTests` is false, since we do run these tests successfully on little endian both with C_LOOP and disabled DFG.

Couldn't you just disable them just for big endian instead?

[Mark Lam]

Hmmm, I know I said that these tests are for the JIT.  However, they do seem to pass on x86_64.  So I wonder if your errors aren’t real.  Can you investigate first why they fail?

[Mark Lam]

(In reply to Mark Lam from comment #5)
> Hmmm, I know I said that these tests are for the JIT.  However, they do seem
> to pass on x86_64.  So I wonder if your errors aren’t real.  Can you
> investigate first why they fail?

I mean they pass for CLoop on x86_64.

[Tomas Popela]

(In reply to Mark Lam from comment #6)
> (In reply to Mark Lam from comment #5)
> > Hmmm, I know I said that these tests are for the JIT.  However, they do seem
> > to pass on x86_64.  So I wonder if your errors aren’t real.  Can you
> > investigate first why they fail?
> 
> I mean they pass for CLoop on x86_64.

Yes, that's what I wrote in the first comment - they do pass on x86_64, ppc64le on other little endian arches. That's why I wanted some clarification. I will look for the reason why they are failing.

[Mark Lam]

(In reply to Tomas Popela from comment #7)
> (In reply to Mark Lam from comment #6)
> > (In reply to Mark Lam from comment #5)
> > > Hmmm, I know I said that these tests are for the JIT.  However, they do seem
> > > to pass on x86_64.  So I wonder if your errors aren’t real.  Can you
> > > investigate first why they fail?
> > 
> > I mean they pass for CLoop on x86_64.
> 
> Yes, that's what I wrote in the first comment - they do pass on x86_64,
> ppc64le on other little endian arches. That's why I wanted some
> clarification. I will look for the reason why they are failing.

Sorry about that.

[Tomas Popela]

Just a quick check - if I reverse all the expected results in stress/dataview-jit-get.js (the little ones with big) then the test passes on big endians, but fails on little :)..

[Saam Barati]

(In reply to Tomas Popela from comment #9)
> Just a quick check - if I reverse all the expected results in
> stress/dataview-jit-get.js (the little ones with big) then the test passes
> on big endians, but fails on little :)..

This isn’t at all surprising. With typed arrays, JS can observe endianness. So it’s also not surprising that we have tests that are dependent on endianness.

[Tomas Popela]

(In reply to Saam Barati from comment #10)
> (In reply to Tomas Popela from comment #9)
> > Just a quick check - if I reverse all the expected results in
> > stress/dataview-jit-get.js (the little ones with big) then the test passes
> > on big endians, but fails on little :)..
> 
> This isn’t at all surprising. With typed arrays, JS can observe endianness.
> So it’s also not surprising that we have tests that are dependent on
> endianness.

Ok, it's not surprising, but then there is something wrong with the test.

If we will look at parts of dataview-jit-set.js

    function storeLittleEndian(dv, index, value) {
        dv.setInt16(index, value, true);
    }

    let buffer = new ArrayBuffer(2);
    let arr = new Uint16Array(buffer);
    let dv = new DataView(buffer);

    for (let i = 0; i < 10000; ++i) {
        storeLittleEndian(dv, 0, 0xfaba);
        throw new Error("Value: " + arr[0]);
        assert(arr[0] === 0xfaba);

Then on x86_64 it will print:

Exception: Error: Value: 0xfaba

And that's expected. But on real big endian hardware it will print:

Exception: Error: Value: 0xbafa

So it stored 0xfaba not as a little indian value, but as a big endian value (as it's running on big endian arch).

Then the assert will fail on the hardcoded value.

[Saam Barati]

(In reply to Tomas Popela from comment #11)
> (In reply to Saam Barati from comment #10)
> > (In reply to Tomas Popela from comment #9)
> > > Just a quick check - if I reverse all the expected results in
> > > stress/dataview-jit-get.js (the little ones with big) then the test passes
> > > on big endians, but fails on little :)..
> > 
> > This isn’t at all surprising. With typed arrays, JS can observe endianness.
> > So it’s also not surprising that we have tests that are dependent on
> > endianness.
> 
> Ok, it's not surprising, but then there is something wrong with the test.
> 
> If we will look at parts of dataview-jit-set.js
> 
>     function storeLittleEndian(dv, index, value) {
>         dv.setInt16(index, value, true);
>     }
> 
>     let buffer = new ArrayBuffer(2);
>     let arr = new Uint16Array(buffer);
>     let dv = new DataView(buffer);
> 
>     for (let i = 0; i < 10000; ++i) {
>         storeLittleEndian(dv, 0, 0xfaba);
>         throw new Error("Value: " + arr[0]);
>         assert(arr[0] === 0xfaba);
> 
> Then on x86_64 it will print:
> 
> Exception: Error: Value: 0xfaba
> 
> And that's expected. But on real big endian hardware it will print:
> 
> Exception: Error: Value: 0xbafa
> 
> So it stored 0xfaba not as a little indian value, but as a big endian value
> (as it's running on big endian arch).
> 
> Then the assert will fail on the hardcoded value.

Again, this isn’t surprising since it’s doing a single load of the value. It should instead read byte-by-byte.

[Saam Barati]

Comment on attachment 360569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360569&action=review

> JSTests/ChangeLog:8
> +        Skip these tests when JIT is not enabled, as they require it.

This is definitely not right. DataView exists with the JIT off. Why not just fix the test to be endian agnostic? Alternatively, I’m ok with you skipping if it’s not a little endian architecture

[Mark Lam]

(In reply to Saam Barati from comment #12)
> Again, this isn’t surprising since it’s doing a single load of the value. It
> should instead read byte-by-byte.

Tomas, this is how you can fix the test to work for big endian platforms as well:
1. Add a isBigEndian() function to the JSDollarVM object.  See the "getpid" function as examples.
2. In the tests, add a adjustForEndianess(x) function that adjusts swizzle the bytes if $vm.isBigEndian().
3. Modify the tests to swizzle value to set e.g.
       ta[0] = adjustForEndianess(0x01020304);

Note: all JSC tests will automatically be run with the --useDollarVM=true option specified.  Hence, you can use the $vm object in the test.  If you're testing with the jsc manually, you'll need to specify the --useDollarVM=true option yourself at the jsc command line.

[Saam Barati]

You can also write an isBegEndian function solely in JS. You don’t need the help of C++

[Mark Lam]

(In reply to Saam Barati from comment #15)
> You can also write an isBegEndian function solely in JS. You don’t need the
> help of C++

Oh, good point!

[Tomas Popela]

While working on patch I found one case where the suggested solution doesn't work:

"use strict";

function assert(b) {
    if (!b)
        throw new Error;
}

function getIsLittleEndian() {
    let ab = new ArrayBuffer(2);
    let ta = new Int16Array(ab);
    ta[0] = 0x0102;
    let dv = new DataView(ab);
    return dv.getInt16(0, true) === 0x0102;
}

let isLittleEndian = getIsLittleEndian();

function adjustForEndianessFloat32(value) {
    if (isLittleEndian)
        return value;

    let ab = new ArrayBuffer(4);
    let ta = new Float32Array(ab);
    ta[0] = value;
    let dv = new DataView(ab);
    return dv.getFloat32(0, true);
}

function test() {
    function storeLittleEndian(dv, index, value) {
        dv.setFloat32(index, value, true);
    }
    noInline(storeLittleEndian);

    function store(dv, index, value, littleEndian) {
        dv.setFloat32(index, value, littleEndian);
    }
    noInline(store);

    let buffer = new ArrayBuffer(4);
    let arr = new Float32Array(buffer);
    let bits = new Uint32Array(buffer);
    let dv = new DataView(buffer);

    for (let i = 0; i < 10000; ++i) {
        storeLittleEndian(dv, 0, adjustForEndianessFloat32(12912.124123215122));
        assert(arr[0] === 12912.1240234375);
        assert(bits[0] === 0x4649c07f);
    }
}
test();


On big endian machine the arr[0] contains 6.905458702346266e-41 and bits[0] 0xc07f.

If I replace the storeLittleEndian() call with:

store(dv, 0, 12912.124123215122, isLittleEndian);

then it does work, but it's strange that the storeLittleEndian() doesn't.

Is the 12912.124123215122 number somehow special? If I observe what's in arr[0], when I change that number then it's:

input to adjustForEndianessFloat32() : arr[0] value
12912 : 12912
12912.1 : 12912.099609375
12912.12 : 12912.1201171875
12912.124 : 6.905458702346266e-41

Does anyone have an idea?

[Tomas Popela]

I'm gonna upload the patch that I've been using in downstream JSC CI for some time. It still has the problem described in comment 17.

[Tomas Popela]

Created attachment 364657 [details]
Patch

[Michael Catanzaro]

Comment on attachment 364657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364657&action=review

Skimming over this patch, it looks good to me. Hope it still applies more or less cleanly.

> JSTests/stress/dataview-jit-set-nan.js:57
> -        storeLittleEndian(dv, 0, 12912.124123215122);
> +        store(dv, 0, 12912.124123215122, isLittleEndian);
> +        //storeLittleEndian(dv, 0, adjustForEndianessFloat32(12912.124123215122));

Please report a new bug for this, and add a FIXME comment pointing to the bug.

[Tomas Popela]

(In reply to Michael Catanzaro from comment #20)
> Comment on attachment 364657 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364657&action=review
> 
> Skimming over this patch, it looks good to me. Hope it still applies more or
> less cleanly.
> 
> > JSTests/stress/dataview-jit-set-nan.js:57
> > -        storeLittleEndian(dv, 0, 12912.124123215122);
> > +        store(dv, 0, 12912.124123215122, isLittleEndian);
> > +        //storeLittleEndian(dv, 0, adjustForEndianessFloat32(12912.124123215122));
> 
> Please report a new bug for this, and add a FIXME comment pointing to the
> bug.

Opened https://bugs.webkit.org/show_bug.cgi?id=209289

[Tomas Popela]

Created attachment 393992 [details]
Patch for landing

[EWS]

Committed r258710: <https://trac.webkit.org/changeset/258710>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393992 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60640238>