Bug 213307

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.

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).

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?

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?

(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?

Created attachment 402766 [details] LLIntAssembly.h Yusuke asked me to attach the generated LLIntAssembly.h

Created attachment 402781 [details] Patch

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).

(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!

(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().

(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 :)

Created attachment 402795 [details] Patch Patch for landing

Created attachment 402797 [details] Patch Patch for landing

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

<rdar://problem/64780677>

<rdar://problem/64780679>