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

V8 Backports Triage #9190

Closed
3 tasks
ofrobots opened this issue Oct 19, 2016 · 12 comments
Closed
3 tasks

V8 Backports Triage #9190

ofrobots opened this issue Oct 19, 2016 · 12 comments
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. v8 engine Issues and PRs related to the V8 dependency.

Comments

@ofrobots
Copy link
Contributor

ofrobots commented Oct 19, 2016

I am going through the list of V8 fixes that potentially need a backport for Node.js. These would be good first contributions / investigations.

/cc @nodejs/lts, @nodejs/v8.

@ofrobots ofrobots added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. v8 engine Issues and PRs related to the V8 dependency. labels Oct 19, 2016
@caitp
Copy link
Contributor

caitp commented Oct 19, 2016

https://bugs.chromium.org/p/v8/issues/detail?id=5363 is not present in v6.x or lower (the old JS implementation did not have this bug, and the fix is already applied when the C++ version was pulled in in September).

@john-yan
Copy link

https://bugs.chromium.org/p/v8/issues/detail?id=5340 is not needed in 5.1.

@bnoordhuis bnoordhuis added the good first issue Issues that are suitable for first-time contributors. label Oct 20, 2016
@maasencioh
Copy link
Contributor

I'm interested in help with this

@ofrobots
Copy link
Contributor Author

@maasencioh Great! You should consult with the guide in nodejs/Release#137. See section on backporting to abandoned branches. I'd be happy to help you through the process.

@maasencioh
Copy link
Contributor

maasencioh commented Oct 20, 2016

Thanks @ofrobots, I was looking at the issue, and it was solved in this commit v8/v8@5af4cd9
I wonder if it actually has sense to backport it to v6, because tail calls are not supported in this version

@ofrobots
Copy link
Contributor Author

@maasencioh By backtails do you mean 'tail calls'? They are not officially supported w/ Node.js v6.x, but are available under a flag. It is indeed not critical to fix this issue, but if the fix is simple enough there is no harm in backporting.

@targos
Copy link
Member

targos commented Oct 24, 2016

@ofrobots v8/v8@5af4cd9 only adds tests for tail call expressions like return continue f();. The change in src/parsing/parser.cc is easy to backport but do you know of a regression test that could be added for it ?

@targos
Copy link
Member

targos commented Jan 8, 2017

ping @ofrobots ^

@ofrobots
Copy link
Contributor Author

@littledan: ^^ do you have a suggestion for a test for the change in praser.(h|cc)?

@littledan
Copy link

The test in test/mjsunit/regress/regress-639270.js is a regression test against implicit tail calls, and the tests in test/mjsunit/es8/syntactic-tail-call-parsing.js and test/message/syntactic-tail-call-generator.js check for explicit tail calls. Note however that none of this affects the default configuration; you'd need to pass experimental flags to trigger the bug path.

@Trott Trott added v8 engine Issues and PRs related to the V8 dependency. and removed v8 engine Issues and PRs related to the V8 dependency. good first issue Issues that are suitable for first-time contributors. labels Jul 15, 2017
@Trott
Copy link
Member

Trott commented Jul 15, 2017

Should this remain open?

@ofrobots
Copy link
Contributor Author

The only remaining issue here is v8:5301. Given that the bug doesn't manifest in the default configuration (--harmony-tailcalls is needed), I would say that this doesn't necessarily need a backport. I am closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

8 participants