Bug 213307 – REGRESSION(r263035): stress/get-prototype-of.js broken on s390x

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

[Michael Catanzaro]

We have another regression in JSC cloop stress tests. This only affects s390x, which suggests it's probably an endianness issue (this is the only big endian architecture we test). All other architectures are fine. Last known good commit is r263017. First known bad is r263135. I'll poke around and see if I can find a suspicious commit.

Running stress/get-prototype-of.js.default
stress/get-prototype-of.js.default: Exception: Error: Bad
stress/get-prototype-of.js.default: [e-mail]:3:20
stress/get-prototype-of.js.default: global [e-mail]:62:11
stress/get-prototype-of.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/get-prototype-of.js.default
Running stress/get-prototype-of.js.mini-mode
stress/get-prototype-of.js.mini-mode: Exception: Error: Bad
stress/get-prototype-of.js.mini-mode: [e-mail]:3:20
stress/get-prototype-of.js.mini-mode: global [e-mail]:62:11
stress/get-prototype-of.js.mini-mode: ERROR: Unexpected exit code: 3
FAIL: stress/get-prototype-of.js.mini-mode

[Michael Catanzaro]

I've started to bisect this, but it's going to take a while because the machine I have access to is a couple thousand times slower than I could wish for.

[Michael Catanzaro]

OK, well turns out there is no need to bisect it: the problem is that the test is brand new. :P Should have checked for that first. So, from the backtrace, we can see the failing check is this one:

assert(getPrototypeOf(proxy) === proxyPrototype);

And here we reach the limit of my nonexistent JSC knowledge: I cannot begin to guess why getPrototypeOf(proxy) is not returning the expected result.k A few things I do know: s390x uses cloop only, so we can eliminate the entire JIT as a source of problems. (The tests are run with --no-jit.) It also uses 4 KB page size, which is nice as that eliminates potential for trouble there. The only major difference from x86_64/aarch64 likely to introduce software bugs is that s390x is big-endian, so it's likely that the bug is an endianness problem... somewhere. All other JSC stress tests are passing except for this new one (and memory-limited tests, since we run tests with --memory-limited).

[Michael Catanzaro]

BTW, this test was introduced in r263035 "super should not depend on proto." I added a bit of debugging:

diff --git a/JSTests/stress/get-prototype-of.js b/JSTests/stress/get-prototype-of.js
index c55d7028284..179b9e7c61c 100644
--- a/JSTests/stress/get-prototype-of.js
+++ b/JSTests/stress/get-prototype-of.js
@@ -59,6 +59,9 @@ for (let i = 0; i < 1e4; ++i) {
     assert(getPrototypeOf(Object.prototype) === null);
     assert(getPrototypeOf(Object.create(null)) === null);
 
+    if (getPrototypeOf(proxy) !== proxyPrototype)
+        throw new Error(`getPrototypeOf(proxy)==${getPrototypeOf(proxy)}, proxyP
+rototype=${proxyPrototype}`);
     assert(getPrototypeOf(proxy) === proxyPrototype);
     assert(getPrototypeOf(polyProtoObject) === polyProtoObject.constructor.prototype);
 }

At the time of the assertion failure, it prints:

Exception: Error: getPrototypeOf(proxy)==null, proxyPrototype=[object Object]

I guess the new builtin isn't working?

[Alexey Shvayka]

Thank you for detailed report, Michael!

Looks like LLInt fast path check (https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm?rev=263044#L1506) doesn't perform as intended.
Is it possible that bit shift in declaration of OverridesGetPrototypeOutOfLine (https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/JSTypeInfo.h?rev=263044#L69) is at fault?

[Michael Catanzaro]

(In reply to Alexey Shvayka from comment #4)
> Thank you for detailed report, Michael!
> 
> Looks like LLInt fast path check
> (https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/llint/
> LowLevelInterpreter64.asm?rev=263044#L1506) doesn't perform as intended.

So the failing line is:

btinz Structure::m_outOfLineTypeFlags[t2], OverridesGetPrototypeOutOfLine, .opGetPrototypeOfSlow

I see in cloop.rb that btinz tests whether a 32-bit integer is nonzero. I also see in JSTypeInfo.h that OutOfLineTypeFlags is a 16-bit integer, so Structure::m_outOfLineTypeFlags is an array of 16-bit integers. This worries me a bit, but I don't understand the code or llint asm well enough to know whether that's actually a problem.

> Is it possible that bit shift in declaration of
> OverridesGetPrototypeOutOfLine
> (https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/
> JSTypeInfo.h?rev=263044#L69) is at fault?

I'm not sure. I don't see why it would be unsafe, since those shifts are operating on 32-bit integers and shift fewer than 32 bits. Is there something you'd like me to change to test it?

[Michael Catanzaro]

Created attachment 402766 [details]
LLIntAssembly.h

Yusuke asked me to attach the generated LLIntAssembly.h

[Yusuke Suzuki]

Created attachment 402781 [details]
Patch

[Ross Kirsling]

Comment on attachment 402781 [details]
Patch

r=me since the explanation is very clear (not sure if we need to have this confirmed on device before landing though).

[Michael Catanzaro]

(In reply to Ross Kirsling from comment #8)
> (not sure if we need to have this confirmed on device before landing though).

I've just confirmed the fix, the test fails on s390x without this patch and passes with the patch applied. Thanks a bunch, Yusuke!

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #7)
> Created attachment 402781 [details]
> Patch

I wonder if we should the same in AssemblyHelpers::emitLoadPrototype()? FTL implementation already does zero-extend via load16ZeroExt32().

[Yusuke Suzuki]

(In reply to Alexey Shvayka from comment #10)
> (In reply to Yusuke Suzuki from comment #7)
> > Created attachment 402781 [details]
> > Patch
> 
> I wonder if we should the same in AssemblyHelpers::emitLoadPrototype()? FTL
> implementation already does zero-extend via load16ZeroExt32().

Right. We should do that while it does not affect on the results in practice since our JIT requires little-endianness :)

[Yusuke Suzuki]

Created attachment 402795 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Created attachment 402797 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Committed r263546: <https://trac.webkit.org/changeset/263546>

[Radar WebKit Bug Importer]

<rdar://problem/64780677>

[Radar WebKit Bug Importer]

<rdar://problem/64780679>