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

WASM usage causes Node 16.1+ crash on ARMv6 #41402

Closed
adriancable opened this issue Jan 5, 2022 · 6 comments · Fixed by #41457
Closed

WASM usage causes Node 16.1+ crash on ARMv6 #41402

adriancable opened this issue Jan 5, 2022 · 6 comments · Fixed by #41457
Labels
v8 engine Issues and PRs related to the V8 dependency. wasm Issues and PRs related to WebAssembly.

Comments

@adriancable
Copy link

Version

16.1 and above, including 17.x

Platform

Linux

Subsystem

No response

What steps will reproduce the bug?

Lots of packages do something like this: (extract from long)

try {
  wasm = new WebAssembly.Instance(new WebAssembly.Module(new Uint8Array([
    0, 97, 115, 109, 1, 0, 0, 0, 1, 13, 2, 96, 0, 1, 127, 96, 4, 127, 127, 127, 127, 1, 127, 3, 7, 6, 0, 1, 1$
  ])), {}).exports;
} catch (e) {
  // no wasm support :(
}

I understand WASM support for ARMv6 has been removed in Node 16.1+ which is fine. But unfortunately the above code causes a hard crash in Node 16.1+ on ARMv6.

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

No response

What is the expected behavior?

Do not crash - instead throw an exception, so the code can fall back to non-WASM usage. (like long tries to do, but fails because node just crashes.)

What do you see instead?

Hard crash.

#
# Fatal error in , line 0
# Liftoff bailout should not happen. Cause: Armv6 not supported

#
#
#
#FailureMessage Object: 0xb49faf50
Trace/breakpoint trap (core dumped)

Additional information

No response

@tniessen tniessen added the wasm Issues and PRs related to WebAssembly. label Jan 5, 2022
@tniessen
Copy link
Member

tniessen commented Jan 5, 2022

Is there any chance you could see what V8 does without Node.js when loading WebAssembly on the same CPU? Maybe in a recent version of Chrome that includes the same version of V8 as an affected Node.js version?

As a workaround, consider using --no-expose-wasm, which should cause libraries to fall back to JavaScript.

@adriancable
Copy link
Author

adriancable commented Jan 5, 2022

@tniessen - you are right that this is (ultimately) a V8 issue. I have looked at the V8 source - the problem seems to be here (crash happens on line 322) and it's because the check for supported architectures (line 308) for Liftoff doesn't actually match the reality, e.g. ARMV6 isn't supported, but isn't excluded by the check - there may be others. I will file a V8 bug report.

However given V8's lifecycle we should consider also implementing a workaround for this in Node. The typical Node user will not like the hard crash and does not care so much about where the blame lies under the hood.

Regarding your --no-expose-wasm workaround, I am not sure I understand your suggestion. Let's say I am the developer of a package X which is used by package Y which is used by package Z (and therefore it's the end user A of package Z that experiences the crash when running Node). I have no way to influence what command line parameters the end user A uses when running node.

Update: V8 bug report filed here - https://bugs.chromium.org/p/v8/issues/detail?id=12527

@adriancable
Copy link
Author

@tniessen - the V8 guys confirm this is a bug and are getting it fixed, see this CL: https://chromium-review.googlesource.com/c/v8/v8/+/3372915/

Google suggests we fix this in Node's V8 sources as well so we do not need to wait for the upstream change. Can you handle?

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

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402
@targos targos added the v8 engine Issues and PRs related to the V8 dependency. label Jan 10, 2022
@tniessen
Copy link
Member

@adriancable This is addressed by #41457.

@adriancable
Copy link
Author

@tniessen - perfect, I will close this now.

@tniessen
Copy link
Member

Reopening since #41457 still references this and will close it automatically once it is merged :)

@tniessen tniessen reopened this Jan 10, 2022
nodejs-github-bot pushed a commit that referenced this issue Jan 14, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: #41402

PR-URL: #41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos added a commit that referenced this issue Jan 16, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: #41402

PR-URL: #41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this issue Jan 17, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos added a commit to targos/node that referenced this issue Jan 18, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
thedull pushed a commit to thedull/node that referenced this issue Jan 18, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos added a commit that referenced this issue Jan 20, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: #41402

PR-URL: #41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos added a commit to targos/node that referenced this issue Jan 20, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos added a commit to targos/node that referenced this issue Jan 29, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: #41402

PR-URL: #41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos added a commit to targos/node that referenced this issue Feb 1, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos added a commit that referenced this issue Feb 2, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: #41402

PR-URL: #41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
VoltrexKeyva pushed a commit to VoltrexKeyva/node that referenced this issue Feb 3, 2022
Original commit message:

    [liftoff] Allow bailout for missing ARMv7

    The bailout is there explicitly in the code, so we should allow it in
    {CheckBailoutAllowed}.

    R=ahaas@chromium.org

    Bug: v8:12527
    Change-Id: Ifd906afb5f034f05c2bf7d9a28e3ab458549e7ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3372915
    Reviewed-by: Andreas Haas <ahaas@chromium.org>
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#78515}

Refs: v8/v8@3b6b21f

Fixes: nodejs#41402

PR-URL: nodejs#41457
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
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. wasm Issues and PRs related to WebAssembly.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants