Bug 194007 - [JSC][BigEndians] Several JSC stress tests failing
Summary: [JSC][BigEndians] Several JSC stress tests failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tomas Popela
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-29 23:03 PST by Tomas Popela
Modified: 2020-03-19 10:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.68 KB, patch)
2019-01-30 04:19 PST, Tomas Popela
no flags Details | Formatted Diff | Diff
Patch (25.00 KB, patch)
2019-03-14 06:43 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff
Patch for landing (25.21 KB, patch)
2020-03-19 10:22 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 2019-01-29 23:03:49 PST
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: assert@dataview-jit-get.js:5:24
stress/dataview-jit-get.js.default: test1@dataview-jit-get.js:28:15
stress/dataview-jit-get.js.default: global code@dataview-jit-get.js: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: assert@dataview-jit-neuter.js:5:24
stress/dataview-jit-neuter.js.default: test@dataview-jit-neuter.js:20:15
stress/dataview-jit-neuter.js.default: global code@dataview-jit-neuter.js: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: assert@dataview-jit-set.js:5:24
stress/dataview-jit-set.js.default: test@dataview-jit-set.js:64:15
stress/dataview-jit-set.js.default: global code@dataview-jit-set.js: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: assert@dataview-get-cse.js:5:24
stress/dataview-get-cse.js.default: test3@dataview-get-cse.js:57:15
stress/dataview-get-cse.js.default: global code@dataview-get-cse.js: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: assert@dataview-jit-set-nan.js:7:24
stress/dataview-jit-set-nan.js.default: test@dataview-jit-set-nan.js:33:15
stress/dataview-jit-set-nan.js.default: global code@dataview-jit-set-nan.js: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?
Comment 1 Mark Lam 2019-01-30 01:46:51 PST
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.
Comment 2 Tomas Popela 2019-01-30 04:19:57 PST
Created attachment 360569 [details]
Patch
Comment 3 Tomas Popela 2019-01-30 04:20:20 PST
(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!
Comment 4 Dominik Inführ 2019-01-30 06:31:13 PST
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?
Comment 5 Mark Lam 2019-01-30 06:31:57 PST
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?
Comment 6 Mark Lam 2019-01-30 06:32:39 PST
(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.
Comment 7 Tomas Popela 2019-01-30 21:46:52 PST
(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.
Comment 8 Mark Lam 2019-01-30 21:59:45 PST
(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.
Comment 9 Tomas Popela 2019-01-31 04:33:58 PST
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 :)..
Comment 10 Saam Barati 2019-01-31 08:40:21 PST
(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.
Comment 11 Tomas Popela 2019-02-01 02:00:21 PST
(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.
Comment 12 Saam Barati 2019-02-04 08:58:13 PST
(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.
Comment 13 Saam Barati 2019-02-04 08:59:44 PST
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
Comment 14 Mark Lam 2019-02-04 10:03:47 PST
(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.
Comment 15 Saam Barati 2019-02-04 11:26:01 PST
You can also write an isBegEndian function solely in JS. You don’t need the help of C++
Comment 16 Mark Lam 2019-02-04 11:26:46 PST
(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!
Comment 17 Tomas Popela 2019-02-18 04:22:09 PST
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?
Comment 18 Tomas Popela 2019-03-14 06:41:44 PDT
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.
Comment 19 Tomas Popela 2019-03-14 06:43:02 PDT
Created attachment 364657 [details]
Patch
Comment 20 Michael Catanzaro 2020-03-19 09:50:12 PDT
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.
Comment 21 Tomas Popela 2020-03-19 10:08:13 PDT
(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
Comment 22 Tomas Popela 2020-03-19 10:22:16 PDT
Created attachment 393992 [details]
Patch for landing
Comment 23 EWS 2020-03-19 10:44:28 PDT
Committed r258710: <https://trac.webkit.org/changeset/258710>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393992 [details].
Comment 24 Radar WebKit Bug Importer 2020-03-19 10:45:23 PDT
<rdar://problem/60640238>