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

Backport fix of Wasm on arm64 (M1) to Node 16 #39327

Closed
surma opened this issue Jul 9, 2021 · 4 comments
Closed

Backport fix of Wasm on arm64 (M1) to Node 16 #39327

surma opened this issue Jul 9, 2021 · 4 comments

Comments

@surma
Copy link

surma commented Jul 9, 2021

Version

16.3

Platform

Darwin SurmBook.local 20.5.0 Darwin Kernel Version 20.5.0: Sat May 8 05:10:31 PDT 2021; root:xnu-7195.121.3~9/RELEASE_ARM64_T8101 arm64

Subsystem

No response

What steps will reproduce the bug?

Detailled reproduction can be found in this crbug: https://crbug.com/1224882

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior?

Should execute the Wasm successfully.

What do you see instead?

E_BAD_ACCESS

Additional information

There are two CLs that landed in V8 that fix this problem:

They will be part of the next v8 release, but it might be worth back-porting them to Node 16 as it is the first release that support arm64 natively (as far as I know?)

@devsnek
Copy link
Member

devsnek commented Jul 9, 2021

These patches don't apply cleanly but I could try to manually apply the logical changes. Dunno what our policy is on that.

@mcollina
Copy link
Member

mcollina commented Jul 9, 2021

I would start with the backport and fix when they do not apply.

@targos
Copy link
Member

targos commented Jul 10, 2021

I'm able to cherry-pick cleanly if I add two other commits. Compiling locally...

targos added a commit to targos/node that referenced this issue Jul 10, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: nodejs#39327
@targos
Copy link
Member

targos commented Jul 10, 2021

#39337

@targos targos closed this as completed in 12aa11c Jul 12, 2021
kodiakhq bot pushed a commit to vercel/next.js that referenced this issue Jul 12, 2021
This PR is a workaround for #24421 by adding an artificial delay when Apple M1 + buggy Node.js is detected.

Node.js 14 is unaffected because it M1 still reports `arch=x64`. Starting in Node.js 16, M1 reports `arch=arm64`.

V8 Bug: https://crbug.com/1224882

Node.js Issue: nodejs/node#39327
targos added a commit that referenced this issue Jul 13, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: #39327

PR-URL: #39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/node that referenced this issue Jul 14, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: nodejs#39327

PR-URL: nodejs#39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/node that referenced this issue Jul 18, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: nodejs#39327

PR-URL: nodejs#39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/node that referenced this issue Jul 20, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: nodejs#39327

PR-URL: nodejs#39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this issue Jul 20, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: #39327

PR-URL: #39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/node that referenced this issue Jul 20, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: nodejs#39327

PR-URL: nodejs#39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this issue Jul 26, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: #39327

PR-URL: #39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this issue Jul 29, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: #39327

PR-URL: #39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Jul 29, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: #39327

PR-URL: #39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Jul 29, 2021
Original commit message:

    [wasm][arm64] Always zero-extend 32 bit offsets, for realz

    We've already been zero-extending 32-bit offset registers since
    https://chromium-review.googlesource.com/c/v8/v8/+/2917612,
    but that patch only covered the case where offset_imm == 0.
    When there is a non-zero offset, we need the same fix.

    Bug: chromium:1224882,v8:11809
    Change-Id: I1908f735929798f411346807fc4f3c79d8e04362
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2998582
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#75500}

Refs: v8/v8@56fe020

Fixes: #39327

PR-URL: #39337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
flybayer pushed a commit to blitz-js/next.js that referenced this issue Aug 19, 2021
This PR is a workaround for vercel#24421 by adding an artificial delay when Apple M1 + buggy Node.js is detected.

Node.js 14 is unaffected because it M1 still reports `arch=x64`. Starting in Node.js 16, M1 reports `arch=arm64`.

V8 Bug: https://crbug.com/1224882

Node.js Issue: nodejs/node#39327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants