FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v18.x] deps: V8: cherry-pick 9ec4e9095a25 by kleisauke · Pull Request #47092 · nodejs/node · GitHub

/ node Public

[v18.x] deps: V8: cherry-pick 9ec4e9095a25 - #47092

Closed
kleisauke wants to merge 1 commit into
nodejs:v18.x-stagingfrom
kleisauke:cherrypick-v8-9ec4e9095a25-v18
ConversationCommits (1)ChecksFiles changed
Closed

[v18.x] deps: V8: cherry-pick 9ec4e9095a25#47092
kleisauke wants to merge 1 commit into
nodejs:v18.x-stagingfrom
kleisauke:cherrypick-v8-9ec4e9095a25-v18

Conversation

kleisauke commented Mar 14, 2023

Copy link
Copy Markdown

Original commit message:

[turbofan] Fix 32-to-64 bit spill slot moves

Other optimizations can create a situation where it is valid to treat a
stack slot as either 32-bit (which is what its value was created as) or
64-bit value (to which it was implicitly zero-extended). So when moving
such a value to a register, we cannot use a 32-bit move instruction just
because the source was annotated as such; we must also take the target
slot's representation into account.

Fixed: chromium:1407594
Bug: chromium:1356461
Change-Id: I00d850c11a020b055e90f6107b604cdd267d9b6c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4197349
Auto-Submit: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90
Refs: #46446 (comment)

deps: V8: cherry-pick 9ec4e9095a25
f571629 
Original commit message:

    [turbofan] Fix 32-to-64 bit spill slot moves

    Other optimizations can create a situation where it is valid to treat a
    stack slot as either 32-bit (which is what its value was created as) or
    64-bit value (to which it was implicitly zero-extended). So when moving
    such a value to a register, we cannot use a 32-bit move instruction just
    because the source was annotated as such; we must also take the target
    slot's representation into account.

    Fixed: chromium:1407594
    Bug: chromium:1356461
    Change-Id: I00d850c11a020b055e90f6107b604cdd267d9b6c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4197349
    Auto-Submit: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Maya Lekova <mslekova@chromium.org>
    Commit-Queue: Maya Lekova <mslekova@chromium.org>
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90

nodejs-github-bot commented Mar 14, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

kleisauke mentioned this pull request Mar 14, 2023
Closed
5 tasks
nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v18.x v8 engine Issues and PRs related to the V8 dependency. labels Mar 14, 2023

atjn commented Mar 22, 2023

Copy link
Copy Markdown

@targos What do we need to do to get this merged?

I don't like to just tag people, but it looks like this patch has accidentally been overlooked. If there is a specific reason you are all ignoring it, please let us know :)
I am specifically asking you (@targos) since you seem to know your way around V8 patches and since you merged the patch that created this issue: #46446

Thanks!

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2023

targos commented Mar 22, 2023

Copy link
Copy Markdown
Member

@atjn It was probably just overlooked indeed. There's no rush, though, as a PR targeting an LTS release line, it would be looked at by a releaser when they prepare the next v18.x release

atjn commented Mar 22, 2023

Copy link
Copy Markdown

I see, thanks again!

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2023

nodejs-github-bot commented Mar 22, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 22, 2023

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5236/

nodejs-github-bot commented Mar 22, 2023

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5236/

kleisauke commented Apr 1, 2023

Copy link
Copy Markdown
Author

This issue also affects Node 19.x, I could open a similiar PR to the v19.x-staging branch if needed/appropriate.

danielleadams pushed a commit that referenced this pull request Apr 2, 2023
deps: V8: cherry-pick 9ec4e9095a25
1ac639a 
Original commit message:

    [turbofan] Fix 32-to-64 bit spill slot moves

    Other optimizations can create a situation where it is valid to treat a
    stack slot as either 32-bit (which is what its value was created as) or
    64-bit value (to which it was implicitly zero-extended). So when moving
    such a value to a register, we cannot use a 32-bit move instruction just
    because the source was annotated as such; we must also take the target
    slot's representation into account.

    Fixed: chromium:1407594
    Bug: chromium:1356461
    Change-Id: I00d850c11a020b055e90f6107b604cdd267d9b6c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4197349
    Auto-Submit: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Maya Lekova <mslekova@chromium.org>
    Commit-Queue: Maya Lekova <mslekova@chromium.org>
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90
PR-URL: #47092
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>

danielleadams commented Apr 2, 2023

Copy link
Copy Markdown
Contributor

Landed in 1ac639a

danielleadams mentioned this pull request Apr 11, 2023
Merged
kleisauke mentioned this pull request Apr 13, 2023
Closed

kleisauke commented Apr 13, 2023

Copy link
Copy Markdown
Author

This issue also affects Node 19.x, I could open a similiar PR to the v19.x-staging branch if needed/appropriate.

I just opened PR #47535 for this.

kleisauke deleted the cherrypick-v8-9ec4e9095a25-v18 branch April 13, 2023 07:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

richardlau richardlau approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL