Bug 142575

Summary: [ARM] REGRESSION(181077): jsc-layout-tests.yaml/js/script-tests/array-length-shortening.js fails on AArch64
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: fpizlo, ggaren, msaboff, ossy, ysuzuki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645, 141351    
Attachments:
Description Flags
Patch none

Csaba Osztrogonác
Reported 2015-03-11 02:25:23 PDT
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
Attachments
Patch (29.23 KB, patch)
2015-03-16 04:52 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1 2015-03-11 02:26:18 PDT
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
Comment 2 2015-03-11 05:02:52 PDT
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
Comment 3 2015-03-15 23:03:54 PDT
Thank you for your notification, I've now noticed this. I'll investigate it.
Yusuke Suzuki
Comment 4 2015-03-16 01:01:18 PDT
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
Comment 5 2015-03-16 04:52:20 PDT
Created attachment 248721 [details] Patch
Yusuke Suzuki
Comment 6 2015-03-16 04:57:02 PDT
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
Comment 7 2015-03-16 05:08:19 PDT
(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
Comment 8 2015-03-16 06:29:09 PDT
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: testLengthShortening@pseudo-array-length-shorten.js:11:24 stress/pseudo-array-length-shorten.js.no-llint: global code@pseudo-array-length-shorten.js: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: testLengthShortening@pseudo-array-length-shorten.js:11:24 stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: global code@pseudo-array-length-shorten.js: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: testLengthShortening@pseudo-array-length-shorten.js:11:24 stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: global code@pseudo-array-length-shorten.js: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: testLengthShortening@pseudo-array-length-shorten.js:9:24 stress/pseudo-array-length-shorten.js.dfg-eager: global code@pseudo-array-length-shorten.js: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: testLengthShortening@pseudo-array-length-shorten.js:11:24 stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: global code@pseudo-array-length-shorten.js: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: testLengthShortening@pseudo-array-length-shorten.js:9:24 stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: global code@pseudo-array-length-shorten.js: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: testLengthShortening@pseudo-array-length-shorten.js:9:24 stress/pseudo-array-length-shorten.js.ftl-eager: global code@pseudo-array-length-shorten.js: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: testLengthShortening@pseudo-array-length-shorten.js:9:24 stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: global code@pseudo-array-length-shorten.js:28:21 stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: ERROR: Unexpected exit code: 3
Yusuke Suzuki
Comment 9 2015-03-16 06:38:08 PDT
(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: > testLengthShortening@pseudo-array-length-shorten.js:11:24 > stress/pseudo-array-length-shorten.js.no-llint: global > code@pseudo-array-length-shorten.js: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: > testLengthShortening@pseudo-array-length-shorten.js:11:24 > stress/pseudo-array-length-shorten.js.ftl-no-cjit-validate: global > code@pseudo-array-length-shorten.js: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: > testLengthShortening@pseudo-array-length-shorten.js:11:24 > stress/pseudo-array-length-shorten.js.no-cjit-validate-phases: global > code@pseudo-array-length-shorten.js: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: > testLengthShortening@pseudo-array-length-shorten.js:9:24 > stress/pseudo-array-length-shorten.js.dfg-eager: global > code@pseudo-array-length-shorten.js: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: > testLengthShortening@pseudo-array-length-shorten.js:11:24 > stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: global > code@pseudo-array-length-shorten.js: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: > testLengthShortening@pseudo-array-length-shorten.js:9:24 > stress/pseudo-array-length-shorten.js.dfg-eager-no-cjit-validate: global > code@pseudo-array-length-shorten.js: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: > testLengthShortening@pseudo-array-length-shorten.js:9:24 > stress/pseudo-array-length-shorten.js.ftl-eager: global > code@pseudo-array-length-shorten.js: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: > testLengthShortening@pseudo-array-length-shorten.js:9:24 > stress/pseudo-array-length-shorten.js.ftl-eager-no-cjit: global > code@pseudo-array-length-shorten.js: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
Comment 10 2015-03-16 11:05:20 PDT
(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
Comment 12 2015-03-19 02:17:14 PDT
(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: > testLengthShortening@pseudo-array-length-shorten.js:11:24 > stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: global > code@pseudo-array-length-shorten.js: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
Comment 13 2015-03-19 04:00:11 PDT
(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: > > testLengthShortening@pseudo-array-length-shorten.js:11:24 > > stress/pseudo-array-length-shorten.js.ftl-no-cjit-no-inline-validate: global > > code@pseudo-array-length-shorten.js: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
Comment 14 2015-03-20 09:18:50 PDT
(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
Comment 15 2015-03-27 08:45:12 PDT
(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
Comment 16 2015-03-27 11:03:12 PDT
(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
Comment 17 2015-03-27 11:04:58 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.