Bug 213307 - REGRESSION(r263035): stress/get-prototype-of.js broken on s390x
Summary: REGRESSION(r263035): stress/get-prototype-of.js broken on s390x
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-17 10:54 PDT by Michael Catanzaro
Modified: 2020-06-25 16:51 PDT (History)
11 users (show)

See Also:


Attachments
LLIntAssembly.h (4.09 MB, text/x-chdr)
2020-06-25 11:54 PDT, Michael Catanzaro
no flags Details
Patch (1.85 KB, patch)
2020-06-25 12:08 PDT, Yusuke Suzuki
ross.kirsling: review+
Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2020-06-25 12:41 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2020-06-25 12:49 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-06-17 10:54:23 PDT
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: assert@get-prototype-of.js:3:20
stress/get-prototype-of.js.default: global code@get-prototype-of.js: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: assert@get-prototype-of.js:3:20
stress/get-prototype-of.js.mini-mode: global code@get-prototype-of.js:62:11
stress/get-prototype-of.js.mini-mode: ERROR: Unexpected exit code: 3
FAIL: stress/get-prototype-of.js.mini-mode
Comment 1 Michael Catanzaro 2020-06-23 12:45:31 PDT
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.
Comment 2 Michael Catanzaro 2020-06-23 13:53:29 PDT
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).
Comment 3 Michael Catanzaro 2020-06-23 14:53:42 PDT
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?
Comment 4 Alexey Shvayka 2020-06-23 14:59:40 PDT
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?
Comment 5 Michael Catanzaro 2020-06-23 15:55:32 PDT
(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?
Comment 6 Michael Catanzaro 2020-06-25 11:54:36 PDT
Created attachment 402766 [details]
LLIntAssembly.h

Yusuke asked me to attach the generated LLIntAssembly.h
Comment 7 Yusuke Suzuki 2020-06-25 12:08:56 PDT
Created attachment 402781 [details]
Patch
Comment 8 Ross Kirsling 2020-06-25 12:15:56 PDT
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).
Comment 9 Michael Catanzaro 2020-06-25 12:27:31 PDT
(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!
Comment 10 Alexey Shvayka 2020-06-25 12:27:51 PDT
(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().
Comment 11 Yusuke Suzuki 2020-06-25 12:37:27 PDT
(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 :)
Comment 12 Yusuke Suzuki 2020-06-25 12:41:13 PDT
Created attachment 402795 [details]
Patch

Patch for landing
Comment 13 Yusuke Suzuki 2020-06-25 12:49:05 PDT
Created attachment 402797 [details]
Patch

Patch for landing
Comment 14 Yusuke Suzuki 2020-06-25 16:50:27 PDT
Committed r263546: <https://trac.webkit.org/changeset/263546>
Comment 15 Radar WebKit Bug Importer 2020-06-25 16:51:15 PDT
<rdar://problem/64780677>
Comment 16 Radar WebKit Bug Importer 2020-06-25 16:51:16 PDT
<rdar://problem/64780679>