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

deps: backport 819b40a from V8 upstream #3937

Closed
wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Nov 20, 2015

Original commit message:

Use baseline code to compute message locations.

This switches Isolate::ComputeLocation to use baseline code when
computing message locations. This unifies locations between optimized
and non-optimized code by always going through the FrameSummary for
location computation.

[email protected]
TEST=message/regress/regress-4266
BUG=v8:4266
LOG=n

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

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

Fixes: #3934

cc @nodejs/v8

Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: nodejs#3934
@targos targos added the v8 engine Issues and PRs related to the V8 dependency. label Nov 20, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Nov 20, 2015

Rubber stamp LGTM

@indutny
Copy link
Member

indutny commented Nov 20, 2015

LGTM

@targos
Copy link
Member Author

targos commented Nov 22, 2015

@bnoordhuis
Copy link
Member

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

@bnoordhuis
Copy link
Member

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

@targos
Copy link
Member Author

targos commented Nov 23, 2015

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

@bnoordhuis
Copy link
Member

Yes, that's the one.

@ofrobots
Copy link
Contributor

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.)

@ofrobots
Copy link
Contributor

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.

@targos
Copy link
Member Author

targos commented Nov 24, 2015

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

@bnoordhuis
Copy link
Member

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.

@targos
Copy link
Member Author

targos commented Nov 24, 2015

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 ?

@targos
Copy link
Member Author

targos commented Nov 24, 2015

@bnoordhuis
Copy link
Member

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.

@targos
Copy link
Member Author

targos commented Nov 24, 2015

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

Yep

@targos
Copy link
Member Author

targos commented Nov 24, 2015

>>> Running tests for x64.debug
No connection to distribution server; running tests locally.
[80:01|% 100|+ 23568|-   0]: Done 

targos added a commit that referenced this pull request Nov 25, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: #3934
PR-URL: #3937
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@targos
Copy link
Member Author

targos commented Nov 25, 2015

Thanks for the help Ben.
Landed on master with ab25589

@targos targos closed this Nov 25, 2015
@targos targos deleted the fix-3934-4.6 branch November 25, 2015 09:56
targos added a commit that referenced this pull request Dec 5, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: #3934
PR-URL: #3937
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@rvagg
Copy link
Member

rvagg commented Jan 15, 2016

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.

@targos
Copy link
Member Author

targos commented Jan 15, 2016

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

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: nodejs#3934
PR-URL: nodejs#3937
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants