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

[v19.x] deps: V8: cherry-pick 9ec4e9095a25 #47535

Conversation

kleisauke
Copy link

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 <[email protected]>
Reviewed-by: Maya Lekova <[email protected]>
Commit-Queue: Maya Lekova <[email protected]>
Commit-Queue: Jakob Kummerow <[email protected]>
Cr-Commit-Position: refs/heads/main@{#85501}

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v19.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 13, 2023
@kleisauke
Copy link
Author

For reference, this was fixed in v18.16.0 with PR #47092. The upcoming v20.0.0 doesn't need this as V8 has been updated to 11.3 (f226350).

@richardlau
Copy link
Member

I've just landed #47239 on v19.x-staging to fix our V8 CI for Node.js 19.x (nodejs/build#3221). Could you rebase onto the current v19.x-staging to pick up that change, then I'll kick off the CI runs? v8_embedder_string in common.gypi will need to be bumped by 1 as #47239 also bumped it up.

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 <[email protected]>
    Reviewed-by: Maya Lekova <[email protected]>
    Commit-Queue: Maya Lekova <[email protected]>
    Commit-Queue: Jakob Kummerow <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90
@kleisauke kleisauke force-pushed the cherrypick-v8-9ec4e9095a25-v19 branch from 1b6d976 to 00fd5ee Compare April 13, 2023 12:19
@kleisauke
Copy link
Author

Rebased and bumped the v8_embedder_string as requested.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2023
targos pushed a commit that referenced this pull request May 1, 2023
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 <[email protected]>
    Reviewed-by: Maya Lekova <[email protected]>
    Commit-Queue: Maya Lekova <[email protected]>
    Commit-Queue: Jakob Kummerow <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90
PR-URL: #47535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@targos
Copy link
Member

targos commented May 1, 2023

Landed in 8283313

@targos targos closed this May 1, 2023
@kleisauke kleisauke deleted the cherrypick-v8-9ec4e9095a25-v19 branch May 1, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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
Development

Successfully merging this pull request may close these issues.

5 participants