Bug 142575 – [ARM] REGRESSION(181077): jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js fails on AArch64

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Csaba Osztrogonác]

It fails on AArch64 Linux with enabled DFG JIT: (pass in other cases)
FAIL: jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit
FAIL: jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-ftl-eager-no-cjit

jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: DIFF FAILURE!
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: --- ../.tests/jsc-layout-tests.yaml/js/array-length-shortening-expected.txt  2015-03-10 05:27:44.000000000 -0700
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: +++ ../jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit.out 2015-03-11 00:52:59.320000000 -0700
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: @@ -5,21 +5,21 @@
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: +FAIL count should be 1. Was 2.
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: +FAIL count should be 1. Was 2.
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: +FAIL count should be 1. Was 5.
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: +FAIL count should be 1. Was 5.
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: +FAIL count should be 1. Was 2.
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: +FAIL count should be 1. Was 2.
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: -PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: -PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: -PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: -PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: -PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: -PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: -PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit: +FAIL count should be 1. Was 2.
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1
jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js.layout-dfg-eager-no-cjit:  PASS count is 1

[Csaba Osztrogonác]

It must be a regression, because I haven't seen this failure 
previously, near ~ 1-2 weeks before. I'm going to do a bisect.

[Csaba Osztrogonác]

I finished with the bisecting, http://trac.webkit.org/changeset/181077 
is the culprit. It passed before r181077 and started to fail from r181077.

Didn't you notice the same failure on iOS AArch64 too?

[Yusuke Suzuki]

Thank you for your notification, I've now noticed this.
I'll investigate it.

[Yusuke Suzuki]

Seeing the result,

1. builtin arrays work fine. pseudo arrays created by Object.create(Array.prototype) fail.
2. They behave like array.length is not shortened.

And major changes introduced by this patch is,

1. Enumeration phase includes pushTry/popTryAndEmitCatch to catch thrown errors & execute iterator.return method.
2. next() is rewritten in JavaScript. So there's chance to inline it. (previously, this function is written in ASM as intrinsic)

Since there's no JIT code changes, I think that I missed something in (1), BytecodeGenerator change.

Filip, does it fail on iOS Aarch64 build?
I don't have any Aarch64 devices, I cannot check it...

[Yusuke Suzuki]

Created attachment 248721 [details]
Patch

[Yusuke Suzuki]

Seeing the ForOfNode implementation, I found several issues that I missed.
This patch fixes several issues in the existing ForOfNode/BytecodeGenerator.

However, I'm not sure that this patch can solve ARM64 issues originally reported here.

ossy, could you apply this patch in your local ARM64 environment?
This patch includes "attempt to fix".

[Csaba Osztrogonác]

(In reply to comment #6)
> Seeing the ForOfNode implementation, I found several issues that I missed.
> This patch fixes several issues in the existing ForOfNode/BytecodeGenerator.
> 
> However, I'm not sure that this patch can solve ARM64 issues originally
> reported here.
> 
> ossy, could you apply this patch in your local ARM64 environment?
> This patch includes "attempt to fix".

Sure, will check soon.

[Csaba Osztrogonác]

Unfortunately it didn't fix the original issue on AArch64 Linux
and the new stress/pseudo-array-length-shorten.js fails too:

stress/pseudo-array-length-shorten.js.no-llint: Exception: Error: bad value 1
stress/pseudo-array-length-shorten.js.no-llint: [e-mail]:11:24
stress/pseudo-array-length-shorten.js.no-llint: global [e-mail]:28:21
stress/pseudo-array-length-shorten.js.no-llint: ERROR: Unexpected exit code: 3

stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: Exception: Error: bad value 1
stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: [e-mail]:11:24
stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: global [e-mail]:55:21
stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: ERROR: Unexpected exit code: 3

stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: Exception: Error: bad value 1
stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: [e-mail]:11:24
stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: global [e-mail]:55:21
stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: ERROR: Unexpected exit code: 3

stress/pseudo-array-length-shorten.js.dfg-eager: Exception: Error: bad value 2
stress/pseudo-array-length-shorten.js.dfg-eager: [e-mail]:9:24
stress/pseudo-array-length-shorten.js.dfg-eager: global [e-mail]:28:21
stress/pseudo-array-length-shorten.js.dfg-eager: ERROR: Unexpected exit code: 3

stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: Exception: Error: bad value 1
stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: [e-mail]:11:24
stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: global [e-mail]:55:21
stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: ERROR: Unexpected exit code: 3

stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: Exception: Error: bad value 2
stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: [e-mail]:9:24
stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: global [e-mail]:28:21
stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: ERROR: Unexpected exit code: 3

stress/pseudo-array-length-shorten.js.ftl-eager: Exception: Error: bad value 2
stress/pseudo-array-length-shorten.js.ftl-eager: [e-mail]:9:24
stress/pseudo-array-length-shorten.js.ftl-eager: global [e-mail]:28:21
stress/pseudo-array-length-shorten.js.ftl-eager: ERROR: Unexpected exit code: 3

stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: Exception: Error: bad value 2
stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: [e-mail]:9:24
stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: global [e-mail]:28:21
stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: ERROR: Unexpected exit code: 3

[Yusuke Suzuki]

(In reply to comment #8)
> Unfortunately it didn't fix the original issue on AArch64 Linux
> and the new stress/pseudo-array-length-shorten.js fails too:
> 
> stress/pseudo-array-length-shorten.js.no-llint: Exception: Error: bad value 1
> stress/pseudo-array-length-shorten.js.no-llint:
> [e-mail]:11:24
> stress/pseudo-array-length-shorten.js.no-llint: global
> [e-mail]:28:21
> stress/pseudo-array-length-shorten.js.no-llint: ERROR: Unexpected exit code:
> 3
> 
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: Exception:
> Error: bad value 1
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate:
> [e-mail]:11:24
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: global
> [e-mail]:55:21
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: ERROR:
> Unexpected exit code: 3
> 
> stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: Exception:
> Error: bad value 1
> stress/pseudo-array-length-shorten.js.no-cjit-validate-phases:
> [e-mail]:11:24
> stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: global
> [e-mail]:55:21
> stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: ERROR:
> Unexpected exit code: 3
> 
> stress/pseudo-array-length-shorten.js.dfg-eager: Exception: Error: bad value
> 2
> stress/pseudo-array-length-shorten.js.dfg-eager:
> [e-mail]:9:24
> stress/pseudo-array-length-shorten.js.dfg-eager: global
> [e-mail]:28:21
> stress/pseudo-array-length-shorten.js.dfg-eager: ERROR: Unexpected exit
> code: 3
> 
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate:
> Exception: Error: bad value 1
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate:
> [e-mail]:11:24
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: global
> [e-mail]:55:21
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: ERROR:
> Unexpected exit code: 3
> 
> stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: Exception:
> Error: bad value 2
> stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate:
> [e-mail]:9:24
> stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: global
> [e-mail]:28:21
> stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: ERROR:
> Unexpected exit code: 3
> 
> stress/pseudo-array-length-shorten.js.ftl-eager: Exception: Error: bad value
> 2
> stress/pseudo-array-length-shorten.js.ftl-eager:
> [e-mail]:9:24
> stress/pseudo-array-length-shorten.js.ftl-eager: global
> [e-mail]:28:21
> stress/pseudo-array-length-shorten.js.ftl-eager: ERROR: Unexpected exit
> code: 3
> 
> stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: Exception: Error:
> bad value 2
> stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit:
> [e-mail]:9:24
> stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: global
> [e-mail]:28:21
> stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: ERROR: Unexpected
> exit code: 3

Thank you!
This new stress test is just moved & modified version of originally failed one.
So still reported issue is not fixed by this patch.
I'll check it more!

[Michael Saboff]

(In reply to comment #4)
> Seeing the result,
> 
> 1. builtin arrays work fine. pseudo arrays created by
> Object.create(Array.prototype) fail.
> 2. They behave like array.length is not shortened.
> 
> And major changes introduced by this patch is,
> 
> 1. Enumeration phase includes pushTry/popTryAndEmitCatch to catch thrown
> errors & execute iterator.return method.
> 2. next() is rewritten in JavaScript. So there's chance to inline it.
> (previously, this function is written in ASM as intrinsic)
> 
> Since there's no JIT code changes, I think that I missed something in (1),
> BytecodeGenerator change.
> 
> Filip, does it fail on iOS Aarch64 build?
> I don't have any Aarch64 devices, I cannot check it...

It does fail on iOS AArch64.

[Yusuke Suzuki]

I'll try it.
http://webkit.sed.hu/blog/20140816/quickndirty-set-aarch64-ubuntu-1404-vm-qemu

[Yusuke Suzuki]

(In reply to comment #10)
> It does fail on iOS AArch64.

Thank you for your report!
Seeing the results, especially,

> stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate:
> Exception: Error: bad value 1
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate:
> [e-mail]:11:24
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: global
> [e-mail]:55:21
> stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: ERROR:
> Unexpected exit code: 3

is quite odd. It checks array.length === 1. Since array.length is set to 1 explicitly, this should be 1.

I'm planning to check it on QEMU aarch64 emurators.

[Csaba Osztrogonác]

(In reply to comment #12)
> (In reply to comment #10)
> > It does fail on iOS AArch64.
> 
> Thank you for your report!
> Seeing the results, especially,
> 
> > stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate:
> > Exception: Error: bad value 1
> > stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate:
> > [e-mail]:11:24
> > stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: global
> > [e-mail]:55:21
> > stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: ERROR:
> > Unexpected exit code: 3
> 
> is quite odd. It checks array.length === 1. Since array.length is set to 1
> explicitly, this should be 1.
> 
> I'm planning to check it on QEMU aarch64 emurators.

Building and debugging in qemu is extremely slow. Fortunately we have
real hardware now for testing, so if you have a patch with debug infos
and or tell me which command line argument should I pass JSC, I willingly
try patches and send debug infos.

[Yusuke Suzuki]

(In reply to comment #13)
> 
> Building and debugging in qemu is extremely slow. Fortunately we have
> real hardware now for testing, so if you have a patch with debug infos
> and or tell me which command line argument should I pass JSC, I willingly
> try patches and send debug infos.

Great! Thank you.
I'm now planning to identify the root cause of this. To do that, try and error is required; I need to pass variaous input data (input script) and check the result.
So at first, I'll set up the environment and perform try and error on QEMU.

After I identified the root cause of this, I need to check it is correct on some actual machines.
At that time, I'd appreciate it if you would help :)

[Yusuke Suzuki]

(In reply to comment #13)

Hi! Could you try the tests with http://trac.webkit.org/changeset/182058 patch?
I think that the above change may fix this issue.

[Csaba Osztrogonác]

(In reply to comment #15)
> (In reply to comment #13)
> 
> Hi! Could you try the tests with http://trac.webkit.org/changeset/182058
> patch?
> I think that the above change may fix this issue.

I checked it, it passes now and unskipped by https://trac.webkit.org/changeset/182070

If the change/improvement you proposed here is still needed, it would be 
better to do it in another bug report. This title is invalid, r181077 is
innocent, it only revealed this bug.

*** This bug has been marked as a duplicate of bug 142792 ***

[Yusuke Suzuki]

(In reply to comment #16)
> I checked it, it passes now and unskipped by
> https://trac.webkit.org/changeset/182070
> 
> If the change/improvement you proposed here is still needed, it would be 
> better to do it in another bug report. This title is invalid, r181077 is
> innocent, it only revealed this bug.
> 
> *** This bug has been marked as a duplicate of bug 142792 ***

Great! I'm happy to hear that :D