Rubber stamp LGTM

LGTM

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

Edit: retrying... https://ci.nodejs.org/job/node-test-pull-request/814/

LGTM although I'd be happier if we could run the V8 test suite on this.

@targos Have you tried getting this accepted in the upstream 4.6 branch?

I'd be happier if we could run the V8 test suite on this.

I'll try to run it locally

Have you tried getting this accepted in the upstream 4.6 branch?

I haven't. Should I follow this guide to do it ? https://github.com/v8/v8/wiki/Merging%20&%20Patching

Yes, that's the one.

I asked the V8 team already about this; it is unlikely to be merge-accepted into the 4.6 branch at this point. (But don't let me discourage you from requesting a merge.)

For more transparency:

4.6 is a tricky beast because we have no longer test coverage for it. We branched a week ago and all our testers have switched to 4.8 and 4.7.

I have one failing test on the 4.6 branch with this change:

/home/mzasso/git/chromium/v8/out/x64.release/cctest --random-seed=-1779142066 --turbo --always-opt test-strings/RobustSubStringStub --nohard-abort --nodead-code-elimination --nofold-constants --testing_serialization_file=/home/mzasso/git/chromium/v8/out/.serdes/serdes_RobustSubStringStub__turbo__always_opt


#
# Fatal error in , line 0
# Unexpected translation type
#

==== C stack trace ===============================

 1: 0x6e2583
 2: 0x7f939a
 3: 0x82d4f7
 4: 0x934f89
 5: 0x934854
 6: 0xb77471
 7: 0xedd3510839b
[1]    9019 abort (core dumped)  /home/mzasso/git/chromium/v8/out/x64.release/cctest --random-seed=-1779142066

Do you mean the test passes without the change from this PR? Apparently it's corrupting optimized stack frames, going by the error message. You may get a more helpful error with a make x64.debug extrachecks=on slowdchecks=on build.

You may get a more helpful error with a make x64.debug extrachecks=on slowdchecks=on build.

That helped a lot, thanks.

I just pushed an update. Sorry I missed this when resolving the cherry-pick conflict. Since it's a turbofan part, our test suite couldn't catch it.

now make x64.release.check passes. Do I need to test something else ?

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

EDIT: run on ARM: https://ci.nodejs.org/job/node-test-commit-arm-fanned/629/

LGTM. The missing bit was the case Runtime::kInlineSubString: in linkage.cc, wasn't it?

now make x64.release.check passes. Do I need to test something else ?

If you're patient, you may want to check make x64.debug.check as well.

The missing bit was the case Runtime::kInlineSubString: in linkage.cc, wasn't it?

Yep

Thanks for the help Ben.
Landed on master with ab25589

After a quick check with v4.x it seems that V8 4.5 did not have this but, tagging as dont-land-on-v4.x but I'm open to correction because perhaps I'm running #3934 wrong.

@rvagg I did another PR for V8 4.5 (#3938). So you're right to tag this as dont-land-on-v4.x