Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AIX - failure in test-timers-args.js #5089

Closed
mhdawson opened this issue Feb 4, 2016 · 8 comments
Closed

AIX - failure in test-timers-args.js #5089

mhdawson opened this issue Feb 4, 2016 · 8 comments
Assignees
Labels
test Issues and PRs related to the tests. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. v8 engine Issues and PRs related to the V8 dependency.

Comments

@mhdawson
Copy link
Member

mhdawson commented Feb 4, 2016

Failure on AIX in newly added CI runs

https://ci.nodejs.org/job/node-test-commit-aix/33/nodes=aix61-ppc64/console

not ok 769 test-timers-args.js


duration_ms: 0.897
...

@mhdawson mhdawson added the master label Feb 4, 2016
@mhdawson mhdawson self-assigned this Feb 4, 2016
@Trott Trott added the test Issues and PRs related to the tests. label Feb 4, 2016
@mscdex mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Feb 5, 2016
@mhdawson
Copy link
Member Author

@gireeshpunathil has investigated and the net is that there is a problem in the instructions being generated with respect to power 6 support. We've not seen it so far because our internal machines have all be power 7 so far.

@gireeshpunathil debug log

Recreatable easily in the CI system with this small code:
bash-4.3$ cat t.js

function bar() {}
for(var i=0;i<process.argv[2];i++) {
setTimeout(bar, 0);
console.log(i);
}

bash-4.3$ ./node t.js 2000
0
1
..
1676
1677
Illegal instruction (core dumped)
bash-4.3$

Investigating further.

Looks like this is a JIT bug. The crash occurs soon after we compile exports.setTimeout function:

bash-4.3$ ./node --trace_opt t.js 2000
1674
1675
[marking 0xef1fa27d <JS Function exports.setTimeout (SharedFunctionInfo ef10967d)> for recompilation, reason: hot and stable, ICs with typeinfo: 12/27 (44%), generic ICs: 0/27 (0%)]
1676
[didn't find optimized code in optimized code map for 0xef10967d ]
[compiling method 0xef1fa27d <JS Function exports.setTimeout (SharedFunctionInfo ef10967d)> using TurboFan]
[optimizing 0xef1fa27d <JS Function exports.setTimeout (SharedFunctionInfo ef10967d)> - took 6.714, 0.000, 0.000 ms]
[didn't find optimized code in optimized code map for 0xef10967d ]
[completed optimizing 0xef1fa27d <JS Function exports.setTimeout (SharedFunctionInfo ef10967d)>]
1677
Illegal instruction (core dumped)

If we skip this method from being optimize compiled, the crash is avoided:
bash-4.3$ ./node --hydrogen_filter=-exports.setTimeout --trace_opt t.js 2000
0
1
..
1117
[didn't find optimized code in optimized code map for 0xef1096f9 ]
[disabled optimization for 0xef1096f9 , reason: Optimization disabled by filter]
1118
..
2000
bash-4.3$

Next, trying to compare the code generated in good (dev) and bad (ci) machine.


The code generated seem to be the same, the notable difference is that all our dev systems are power7 while ci is hosted on power6.
Inference: Turbofan generated wrong code for setTimeout function, or not compatible for power6.
todo:

  1. Check whether this fails in another power6 system to confirm this assumption.
  2. Inspect the generated code and check its validity in power6 and 7.

Looks like only the first instruction in a sequence is invalid, while the rest if properly formed.
0xef3a4dc8 (???) 7c60585e Invalid opcode.
0xef3a4dcc (???) 2c030000 cmpi cr0,0x0,r3,0x0
0xef3a4dd0 (???) 40820014 bne 0xef3a4de4 (???)
0xef3a4dd4 (???) 7c651b78 mr r5,r3
0xef3a4dd8 (???) 807d0020 lwz r3,0x20(r29)
0xef3a4ddc (???) 38800000 li r4,0x0
0xef3a4de0 (???) 480000a0 b 0xef3a4e80 (???)
0xef3a4de4 (???) 80bfffe lwz r5,-28(r31)
0xef3a4de8 (???) 8065000b lwz r3,0xb(r5)
0xef3a4dec (???) 80c3002f lwz r6,0x2f(r3)
0xef3a4df0 (???) 90dfffe0 stw r6,-32(r31)
0xef3a4df4 (???) 7cc43378 mr r4,r6
0xef3a4df8 (???) 807d0018 lwz r3,0x18(r29)
0xef3a4dfc (???) 7cbe2b78 mr r30,r5
0xef3a4e00 (???) 7c671b78 mr r7,r3
0xef3a4e04 (???) 819c0048 lwz r12,0x48(r28)
0xef3a4e08 (???) 7d8903a6 mtctr r12
0xef3a4e0c (???) 4e800421 bctrl

...


Looks like isel (Integer Select) is the instruction which causes SIGILL in power6. Tihs instruction is decoded thus in power7:
0xef39fec8 552 7c60585e isel r3, r0, r11

Inspecting whether this is supported in power6.

bash-4.3$ hostname
dal-pax
bash-4.3$ cat foo.cpp
int main()
{
__asm("isel 0, 1, 2, 2");
}

bash-4.3$ g++ foo.cpp
.bash-4.3$ ./a.out
bash-4.3$

bash-4.3$ hostname
sovma188
bash-4.3$ ./a.out
Illegal instruction (core dumped)
bash-4.3$

Clearly, this is not supported on power6, on which CI is hosted.

@mhdawson mhdawson added the v8 engine Issues and PRs related to the V8 dependency. label Feb 16, 2016
@mhdawson
Copy link
Member Author

Talked to @mtbrandy and he will add the code to V8 master to handle that case properly on Power 6. Once in I'll propose a floating patch to fix in master until we pick up a level of V8 that includes the fix.

@mtbrandy
Copy link

Fix committed to V8 master here.
Backported to 4.9 and 4.8.

mhdawson added a commit to mhdawson/io.js that referenced this issue Feb 17, 2016
Float v8 patch, which has been committed to v8 master and
backported to 4.8 and 4.9 in google repos, onto 4.8 v8 in
deps to resolve nodejs#5089

Original title/commit from google repos for 4.8 is:
 PPC: [turbofan] Support for CPU models lacking isel.
 v8/v8@2e4da65
@mhdawson
Copy link
Member Author

Putting together PR to pull into node tree, just testing now and will submit PR when https://ci.nodejs.org/job/node-test-commit-aix/48/ completes.

@mhdawson
Copy link
Member Author

@mhdawson
Copy link
Member Author

V8 test run looks good. Some unrelated issue I need to figure out in non-ppc runs, but ppc one ran fine and the only v8 file changed is for ppc so we're good.

@mhdawson
Copy link
Member Author

PR #5293

@mhdawson
Copy link
Member Author

node-test-pull-request CI run - https://ci.nodejs.org/job/node-test-pull-request/1695/

mtbrandy pushed a commit to ibmruntimes/node that referenced this issue Feb 18, 2016
Float v8 patch, which has been committed to v8 master and
backported to 4.8 and 4.9 in google repos, onto 4.8 v8 in
deps to resolve nodejs/node#5089

Original title/commit from google repos for 4.8 is:
 PPC: [turbofan] Support for CPU models lacking isel.
 v8/v8@2e4da65

PR-URL: nodejs/node#5293
Fixes: nodejs/node#5089
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
richardlau pushed a commit to ibmruntimes/node that referenced this issue Feb 18, 2016
Float v8 patch, which has been committed to v8 master and
backported to 4.8 and 4.9 in google repos, onto 4.8 v8 in
deps to resolve nodejs/node#5089

Original title/commit from google repos for 4.8 is:
 PPC: [turbofan] Support for CPU models lacking isel.
 v8/v8@2e4da65

PR-URL: nodejs/node#5293
Fixes: nodejs/node#5089
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Bysmyyr pushed a commit to Bysmyyr/v8-crosswalk that referenced this issue Feb 19, 2016
Merged 789ad2f

PPC: [turbofan] Support for CPU models lacking isel.
(See nodejs/node#5089)

R=hablich@chromium.org, adamk@chromium.org
BUG=

Review URL: https://codereview.chromium.org/1705293002 .

Cr-Commit-Position: refs/branch-heads/4.8@{crosswalk-project#24}
Cr-Branched-From: 10449d4-refs/heads/4.8.271@{#1}
Cr-Branched-From: 2ebd5fc-refs/heads/master@{#31941}
rvagg pushed a commit that referenced this issue Feb 21, 2016
Float v8 patch, which has been committed to v8 master and
backported to 4.8 and 4.9 in google repos, onto 4.8 v8 in
deps to resolve #5089

Original title/commit from google repos for 4.8 is:
 PPC: [turbofan] Support for CPU models lacking isel.
 v8/v8@2e4da65

PR-URL: #5293
Fixes: #5089
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
GnorTech pushed a commit to nwjs/v8 that referenced this issue Feb 29, 2016
Merged 789ad2f

PPC: [turbofan] Support for CPU models lacking isel.
(See nodejs/node#5089)

R=hablich@chromium.org
BUG=

Review URL: https://codereview.chromium.org/1711453002 .

Cr-Commit-Position: refs/branch-heads/4.9@{#26}
Cr-Branched-From: 2fea296-refs/heads/4.9.385@{#1}
Cr-Branched-From: 0c1430a-refs/heads/master@{#33306}
MylesBorins pushed a commit that referenced this issue Mar 17, 2016
Float v8 patch, which has been committed to v8 master and
backported to 4.8 and 4.9 in google repos, onto 4.8 v8 in
deps to resolve #5089

Original title/commit from google repos for 4.8 is:
 PPC: [turbofan] Support for CPU models lacking isel.
 v8/v8@2e4da65

PR-URL: #5293
Fixes: #5089
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this issue Mar 21, 2016
Float v8 patch, which has been committed to v8 master and
backported to 4.8 and 4.9 in google repos, onto 4.8 v8 in
deps to resolve #5089

Original title/commit from google repos for 4.8 is:
 PPC: [turbofan] Support for CPU models lacking isel.
 v8/v8@2e4da65

PR-URL: #5293
Fixes: #5089
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants