Page MenuHomePhabricator
Differential D92504

Bug 1655042: Support Cranelift-based Wasm validation. r=jseward
ClosedPublic
Actions

Authored by cfallin on Oct 5 2020, 9:36 PM.
Tags
Referenced Files
Unknown Object (File)
Wed, Apr 15, 3:05 PM
Unknown Object (File)
Tue, Apr 14, 4:32 PM
Unknown Object (File)
Tue, Apr 14, 4:03 PM
Unknown Object (File)
Tue, Apr 14, 2:46 AM
Unknown Object (File)
Tue, Apr 14, 2:30 AM
Unknown Object (File)
Mon, Apr 13, 11:25 AM
Unknown Object (File)
Mon, Apr 13, 8:58 AM
Unknown Object (File)
Sun, Apr 12, 5:14 PM
View All 29 Files
Subscribers
jandem
reviewbot

Details

Reviewers
jseward
Commits
rMOZILLACENTRALdc2e751e246b: Bug 1655042: Support Cranelift-based Wasm validation. r=jseward
rMOZILLACENTRAL0807415f1ea5: Bug 1655042: Support Cranelift-based Wasm validation. r=jseward
Bugzilla Bug ID
1655042
Summary

This patch is an update of Benjamin Bouvier's WIP patch that supports
Cranelift's new Wasm validator functionality, as introduced in
bytecodealliance/wasmtime#2059.

Previously, Baldrdash allowed SpiderMonkey to validate Wasm function
bodies before invoking Cranelift. The Cranelift PR now causes validation
to happen in the function-body compiler. Because the Cranelift backend
now takes on this responsibility, we no longer need to invoke the
SpiderMonkey function-body validator. This requires some significant new
glue in the Baldrdash bindings to allow Cranelift's Wasm frontend to
access module type information.

Co-authored-by: Benjamin Bouvier <benj@benj.me>

Diff Detail

Event Timeline

cfallin created this revision.Oct 5 2020, 9:36 PM
Herald added a project: secure-revision. · View Herald TranscriptOct 5 2020, 9:36 PM
Harbormaster completed remote builds in B246550: Diff 348763.Oct 5 2020, 9:36 PM
phab-bot published this revision for review.Oct 5 2020, 9:36 PM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
Herald added a subscriber: jandem. · View Herald TranscriptOct 5 2020, 9:36 PM
reviewbot added a subscriber: reviewbot.Oct 5 2020, 10:19 PM

Code analysis found 16 defects in the diff 348763:

  • 11 defects found by clang-tidy
  • 2 defects found by rustfmt (Mozlint)
  • 3 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p js/src/wasm/cranelift/baldrapi.h js/src/wasm/cranelift/clifapi.h js/src/wasm/WasmCraneliftCompile.cpp (C/C++)
  • ./mach lint --warnings --outgoing
  • ./mach static-analysis check --outgoing (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).

You have touched the documentation, you can find it rendered here for a week.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

jseward accepted this revision.Oct 6 2020, 9:11 AM

It looks plausible, is the best I can say given my limited understanding of the SM/CL interface.
A couple of points:

  • it would be good if you could fix as many of the linting/style auto-complaints as is easy to do, before landing.
  • given the complexities of storage management at the C++/Rust interface, and that this patch changes this area, has there been any dynamic analysis (ASan/LSan/Valgrind) to try and ensure that there are no UAFs and no leaks as a result of the changes? Having at least some minimal one-off sanity check be done there, would be nice.
js/src/wasm/WasmCraneliftCompile.cpp
293

How is this related to the validation issue? Is it some kind of ridealong fix?

This revision is now accepted and ready to land.Oct 6 2020, 9:11 AM
Herald added a project: needs-testing-tag. · View Herald TranscriptOct 6 2020, 9:11 AM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other.

cfallin edited projects, added testing-approved; removed needs-testing-tag.Oct 6 2020, 4:53 PM
cfallin updated this revision to Diff 349334.Oct 6 2020, 5:15 PM
cfallin edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B246981: Diff 349334.Oct 6 2020, 5:15 PM
cfallin marked an inline comment as done.Oct 6 2020, 5:19 PM

Thanks, updated!

I'll do an ASan run to be sure this is is good (I don't think I can do a try-push since the config for the asan builds does not have the flag for the Cranelift backend, so I'll do it locally).

js/src/wasm/WasmCraneliftCompile.cpp
293

This flag feeds into the WasmFeatures provided to the validator on the Cranelift side and tells it to accept the atomic-operation instructions as valid (so named because they're part of the "threads proposal" IIRC).

reviewbot added a comment.Oct 6 2020, 5:33 PM

You have touched the documentation, you can find it rendered here for a week.

If you see a problem in this automated review, please report it here.

cfallin marked an inline comment as done.Oct 6 2020, 5:36 PM

Local ASan run (../configure --enable-address-sanitizer --disable-jemalloc --enable-optimize) passes jit-tests with --args="--wasm-compiler=cranelift --no-wasm-simd on x86-64; so I'll go ahead and land!

cfallin updated this revision to Diff 349558.Oct 7 2020, 3:27 AM
Harbormaster completed remote builds in B247143: Diff 349558.Oct 7 2020, 3:27 AM
cfallin updated this revision to Diff 349560.Oct 7 2020, 3:43 AM
Harbormaster completed remote builds in B247145: Diff 349560.Oct 7 2020, 3:43 AM
reviewbot added a comment.Oct 7 2020, 4:54 AM

You have touched the documentation, you can find it rendered here for a week.

If you see a problem in this automated review, please report it here.

Closed by commit rMOZILLACENTRAL0807415f1ea5: Bug 1655042: Support Cranelift-based Wasm validation. r=jseward (authored by cfallin). · Explain WhyOct 7 2020, 5:46 AM
This revision was automatically updated to reflect the committed changes.
cfallin added a commit: rMOZILLACENTRAL0807415f1ea5: Bug 1655042: Support Cranelift-based Wasm validation. r=jseward.
• rmaries reopened this revision.Oct 7 2020, 6:25 AM
This revision is now accepted and ready to land.Oct 7 2020, 6:25 AM
• rmaries added a reverting change: rMOZILLACENTRAL3792ecb5de48: Backed out 2 changesets (bug 1655042, bug 1669055) for build bustages. CLOSED….Oct 7 2020, 6:25 AM
cfallin updated this revision to Diff 349939.Oct 7 2020, 5:14 PM
Harbormaster completed remote builds in B247393: Diff 349939.Oct 7 2020, 5:14 PM
reviewbot added a comment.Oct 7 2020, 5:46 PM

You have touched the documentation, you can find it rendered here for a week.

If you see a problem in this automated review, please report it here.

Closed by commit rMOZILLACENTRALdc2e751e246b: Bug 1655042: Support Cranelift-based Wasm validation. r=jseward (authored by cfallin). · Explain WhyOct 7 2020, 6:29 PM
This revision was automatically updated to reflect the committed changes.
cfallin added a commit: rMOZILLACENTRALdc2e751e246b: Bug 1655042: Support Cranelift-based Wasm validation. r=jseward.

Revision Contents
Changeset List

PathSize
js/
src/
jit-test/
lib/
20 lines
tests/
wasm/
10 lines
12 lines
28 lines
2 lines
gc/
2 lines
4 lines
4 lines
32 lines
12 lines
ref-types/
6 lines
16 lines
20 lines
18 lines
regress/
8 lines
6 lines
simd/
2 lines
2 lines
wasm/
109 lines
1 line
7 lines
cranelift/
22 lines
10 lines
src/
bindings/
276 lines
68 lines
31 lines
60 lines

Diff 349983

View Options

js/src/jit-test/lib/wasm.js

Loading...
View Options

js/src/jit-test/tests/wasm/basic.js

Loading...
View Options

js/src/jit-test/tests/wasm/binary.js

Loading...
View Options

js/src/jit-test/tests/wasm/control-flow.js

Loading...
View Options

js/src/jit-test/tests/wasm/float.js

Loading...
View Options

js/src/jit-test/tests/wasm/gc/disabled.js

Loading...
View Options

js/src/jit-test/tests/wasm/globals.js

Loading...
View Options

js/src/jit-test/tests/wasm/memory.js

Loading...
View Options

js/src/jit-test/tests/wasm/passive-segs-boundary.js

Loading...
View Options

js/src/jit-test/tests/wasm/passive-segs-nonboundary.js

Loading...
View Options

js/src/jit-test/tests/wasm/ref-types/ref-func.js

Loading...
View Options

js/src/jit-test/tests/wasm/ref-types/tables-fill.js

Loading...
View Options

js/src/jit-test/tests/wasm/ref-types/tables-generalized.js

Loading...
View Options

js/src/jit-test/tests/wasm/ref-types/tables-multiple.js

Loading...
View Options

js/src/jit-test/tests/wasm/regress/misc-control-flow.js

Loading...
View Options

js/src/jit-test/tests/wasm/select.js

Loading...
View Options

js/src/jit-test/tests/wasm/simd/disabled.js

Loading...
View Options

js/src/jit-test/tests/wasm/text.js

Loading...
View Options

js/src/wasm/WasmCraneliftCompile.cpp

Loading...
View Options

js/src/wasm/WasmValidate.h

Loading...
View Options

js/src/wasm/WasmValidate.cpp

Loading...
View Options

js/src/wasm/cranelift/baldrapi.h

Loading...
View Options

js/src/wasm/cranelift/clifapi.h

Loading...
View Options

js/src/wasm/cranelift/src/bindings/mod.rs

Loading...
View Options

js/src/wasm/cranelift/src/compile.rs

Loading...
View Options

js/src/wasm/cranelift/src/lib.rs

Loading...
View Options

js/src/wasm/cranelift/src/wasm2clif.rs

Loading...
Privacy · Cookies · Legal