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

Error ONLY when running babel task on arm64 node v4.4.6 #7417

Closed
TomFreudenberg opened this issue Jun 25, 2016 · 30 comments
Closed

Error ONLY when running babel task on arm64 node v4.4.6 #7417

TomFreudenberg opened this issue Jun 25, 2016 · 30 comments
Assignees
Labels
arm Issues and PRs related to the ARM platform. confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@TomFreudenberg
Copy link

Hi, I have reported an error on the babel issue tracker at https://phabricator.babeljs.io/T7457

but it might also be, that this is an issue on node v4?

The issue described does ONLY occur an the arm64 architecture (I am also using the linoaro dev cluster).

In a short:

running:

mkdir -p /tmp/babel-test

cd /tmp/babel-test

npm install babel-cli babel-preset-es2015 --save-dev

./node_modules/.bin/babel ./node_modules/babel-polyfill/dist/polyfill.js -d lib --presets es2015 --no-babelrc

will return

[BABEL] Note: The code generator has deoptimised the styling of "./node_modules/babel-polyfill/dist/polyfill.js" as it exceeds the max of "100KB".
TypeError: ./node_modules/babel-polyfill/dist/polyfill.js: Cannot read property 'compact' of undefined
at Function.space (/tmp/babel-test/node_modules/babel-generator/lib/buffer.js:139:20)
at Function.ObjectExpression (/tmp/babel-test/node_modules/babel-generator/lib/generators/types.js:62:10)
at /tmp/babel-test/node_modules/babel-generator/lib/printer.js:113:24
at Generator.withSource (/tmp/babel-test/node_modules/babel-generator/lib/buffer.js:293:39)
at Generator.print (/tmp/babel-test/node_modules/babel-generator/lib/printer.js:112:10)
at Generator.printJoin (/tmp/babel-test/node_modules/babel-generator/lib/printer.js:197:12)
at Generator.printList (/tmp/babel-test/node_modules/babel-generator/lib/printer.js:261:17)
at Generator.CallExpression (/tmp/babel-test/node_modules/babel-generator/lib/generators/expressions.js:144:8)
at /tmp/babel-test/node_modules/babel-generator/lib/printer.js:113:24
at Generator.withSource (/tmp/babel-test/node_modules/babel-generator/lib/buffer.js:293:39)

If I use node v6.2.2 arm64 and run the same command, the result is:

[BABEL] Note: The code generator has deoptimised the styling of "./node_modules/babel-polyfill/dist/polyfill.js" as it exceeds the max of "100KB".
./node_modules/babel-polyfill/dist/polyfill.js -> lib/node_modules/babel-polyfill/dist/polyfill.js

Seems to be a kind of memory leak ?

@mscdex
Copy link
Contributor

mscdex commented Jun 25, 2016

This looks more like a babel bug than a node bug. Perhaps one of their userland modules does some sort of node version checking or makes some kind of wrong assumption?

@mscdex mscdex added the v4.x label Jun 25, 2016
@TomFreudenberg
Copy link
Author

Hi Brian ( @mscdex )

I have no clue currently, so thats why I posted in both trackers. Its a pitty, we got nearly everything running on the arm64 and its just the unpacking what is missing now ;-)

Sometimes I hate the jobs ;-)

Thanks for your feedback, hopefully someone from babel tracker will answer too

Tom

@loganfsmyth
Copy link
Contributor

Hey @mscdex, see my response here: https://phabricator.babeljs.io/T7457#79767

There are a bunch of things about this error that make me think it was likely a V8 issue that's been fixed between 4.4.6 and 6.2.2. Primarily, this.format should always be an object, and if it weren't, several functions called before the one in question would have triggered the same error were that the case, combined with the inability to reproduced on x86 and Node 6 ARM64.

@TomFreudenberg
Copy link
Author

Hi Brian, Logan @mscdex @loganfsmyth

if necessary, I can give you access to my linaro development cluster or help you to get an account yourself.

Would be great if this could be get fixed, because meteor will not move soon to node 6.

P.S.: I will run a test on latest node 5 as well - an post result here.

@TomFreudenberg
Copy link
Author

Hi, as expected - it seems to be just an error on the 4.4.6. Doing the same check with node_ v5.12.0-arm64

Just to have this test also with node v5, result is correct working:

PATH=$PATH:/tmp/node5/bin ./node_modules/.bin/babel ./node_modules/babel-polyfill/dist/polyfill.js -d lib --presets es2015 --no-babelrc

BABEL] Note: The code generator has deoptimised the styling of "./node_modules/babel-polyfill/dist/polyfill.js" as it exceeds the max of "100KB".
./node_modules/babel-polyfill/dist/polyfill.js -> lib/node_modules/babel-polyfill/dist/polyfill.js

This works also fine

@MylesBorins
Copy link
Member

Are older versions of v4 working fine?
On Jun 26, 2016 1:17 PM, "Tom Freudenberg" notifications@github.com wrote:

Hi, as expected - it seems to be just an error on the 4.4.6. Doing the
same check with node_ v5.12.0-arm64

Just to have this test also with node v5, result is correct working:

PATH=$PATH:/tmp/node5/bin ./node_modules/.bin/babel ./node_modules/babel-polyfill/dist/polyfill.js -d lib --presets es2015 --no-babelrc

BABEL] Note: The code generator has deoptimised the styling of
"./node_modules/babel-polyfill/dist/polyfill.js" as it exceeds the max of
"100KB".
./node_modules/babel-polyfill/dist/polyfill.js ->
lib/node_modules/babel-polyfill/dist/polyfill.js

This works also fine


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7417 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAecV7iLOI08zhnu3nGT7ZAH-OzECAYIks5qPt5kgaJpZM4I-WUm
.

@TomFreudenberg
Copy link
Author

TomFreudenberg commented Jun 26, 2016

I had currently the same idea and will test. Let me run some and I give feedback

@TomFreudenberg
Copy link
Author

@mscdex @loganfsmyth @thealphanerd

Ok guys, got it

node-v5.0.0-arm64 IS NOT WORKING
node-v5.1.0-arm64 IS OK

@MylesBorins
Copy link
Member

MylesBorins commented Jun 26, 2016 via email

@TomFreudenberg
Copy link
Author

Yes, I know but v4 doesnt work at all, otherwise I have to check for v6

@TomFreudenberg
Copy link
Author

TomFreudenberg commented Jun 26, 2016

@thealphanerd But can't we use the diff between v5.0.0 and v5.1.0 to see what changes might repair that issue?

@MylesBorins
Copy link
Member

MylesBorins commented Jun 26, 2016 via email

@TomFreudenberg
Copy link
Author

Thanks Myles @thealphanerd

I will check the v6 branches to see if there is also one not working

@TomFreudenberg
Copy link
Author

TomFreudenberg commented Jun 26, 2016

Already node-v6.0.0-arm64 is working, but I guess this is clear, because the v6 branch was created sometimes out of v5 branch. In case that it was fixed in this early v5.1.0 it wasn't propably never an issue in v6.x.x

So would be great if you could catch it.

Tom

@TomFreudenberg
Copy link
Author

Just as an additional info: I tested a number of binaries between v4.0.0-arm64 and v4.4.6-arm64.

NONE of those were working, so propably the fix was included between v5.0.0 and v5.1.0.

@mscdex
Copy link
Contributor

mscdex commented Jun 26, 2016

If it is v8-related, it might be this one line change.

@MylesBorins MylesBorins self-assigned this Jun 26, 2016
@TomFreudenberg
Copy link
Author

You mean LINE or this HASH

I can check it out and run a compilation on the arm64

What should I change to the source of 4.4.6 or 5.0.0 ?

@MylesBorins
Copy link
Member

@TomFreudenberg patch on top of 4.4.6 if it is able to land

@mscdex
Copy link
Contributor

mscdex commented Jun 26, 2016

@TomFreudenberg You'd need to open deps/v8/src/arm64/lithium-codegen-arm64.cc and change this to this.

@TomFreudenberg
Copy link
Author

Should I try this on v4.4.6 ?

@mscdex
Copy link
Contributor

mscdex commented Jun 26, 2016

@TomFreudenberg Sure.

@TomFreudenberg
Copy link
Author

I run

./configure --without-snapshot

correct?

@TomFreudenberg
Copy link
Author

make is now running, will take around 15 minutes on the ressources I have on the linaro cluster

Press my thumbs :-)

@TomFreudenberg
Copy link
Author

@mscdex @loganfsmyth @thealphanerd

JIIEEEEHHAAAAA ;-)

It works!

I love to work on OpenSource projects and with so envolved people.

Thanks for your help and time invest!


Would you apply that as patch to next v4.4.7 release ? I just ask to know if I had to run a private fork or to wait until it is include in next stable release.

@MylesBorins
Copy link
Member

@TomFreudenberg I think it may be possible to backport that commit for v4.4.7... I'll dig into it a bit more tomorrow.

I believe this is the commit we want to backport --> v8/v8@102e3e8

@TomFreudenberg
Copy link
Author

Myles @thealphanerd

That would be great. If you need help, just knock me.

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Jun 26, 2016
@mscdex mscdex added arm Issues and PRs related to the ARM platform. confirmed-bug Issues with confirmed bugs. labels Jun 26, 2016
@MylesBorins
Copy link
Member

@TomFreudenberg I'm working on getting this in for today's release, but testing has proven a bit more difficult than expected. I'll update in a couple hours

@TomFreudenberg
Copy link
Author

@thealphanerd Myles - Thanks a lot for your support!

MylesBorins pushed a commit to MylesBorins/node that referenced this issue Jun 28, 2016
This commit backports a fix to a crankshaft bug affects arm64 systems.

Original commit message:

    [arm64] Fix jssp based spill slot accesses in Crankshaft

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

    Cr-Commit-Position: refs/heads/master@{nodejs#31304}

Fixes: nodejs#7417
PR-URL: nodejs#7442
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Member

@TomFreudenberg adding to the release!

MylesBorins pushed a commit that referenced this issue Jun 28, 2016
This commit backports a fix to a crankshaft bug affects arm64 systems.

Original commit message:

    [arm64] Fix jssp based spill slot accesses in Crankshaft

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

    Cr-Commit-Position: refs/heads/master@{#31304}

Fixes: #7417
PR-URL: #7442
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@TomFreudenberg
Copy link
Author

Hi Myels ( @thealphanerd )

Thanks for backport, I can confirm that 4.4.7 is now working an my arm64 system

gibfahn pushed a commit to ibmruntimes/node that referenced this issue Jun 29, 2016
This commit backports a fix to a crankshaft bug affects arm64 systems.

Original commit message:

    [arm64] Fix jssp based spill slot accesses in Crankshaft

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

    Cr-Commit-Position: refs/heads/master@{#31304}

Fixes: nodejs/node#7417
PR-URL: nodejs/node#7442
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Issues and PRs related to the ARM platform. confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants