Closed Bug 1598377 Opened 1 year ago Closed 1 year ago

Make validation aware of multi values support

Categories

(Core :: Javascript: WebAssembly, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: bbouvier, Assigned: rhunt)

References

Details

Attachments

(2 files)

By default, multi values is assumed to be enabled during validation (that is, with WebAssembly.Validate). Unfortunately, this is also used for Cranelift, and it appears that while Cranelift says it can't do multi values (b/o wasmMultiValuesEnabled()), the Validate function assumes it can, causing consistency errors in wasm/spec/types.wast.js after bug 1597989 lands.

Instead, validation should ensure that it allows multi values only when it's supported, as we do for reftypes and others.

Priority: -- → P3

Andy, can you make sure this gets addressed asap, so that we can avoid any problems leaking into the next release? We're just a few days away from branching.

Assignee: nobody → wingo
Priority: P3 → P1

Hi, I am not sure I understand this bug. https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/wasm/multi-value/block-validate.js verifies that validation works for multi-value code when wasmMultiValuesEnabled() is true, and types.wast.js verifies that it fails when wasmMultiValuesEnabled() is false. What is the problem? Is cranelift not respecting --enable-multi-value ?

(In reply to Andy Wingo [:wingo] from comment #2)

Hi, I am not sure I understand this bug. https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/wasm/multi-value/block-validate.js verifies that validation works for multi-value code when wasmMultiValuesEnabled() is true, and types.wast.js verifies that it fails when wasmMultiValuesEnabled() is false. What is the problem? Is cranelift not respecting --enable-multi-value ?

One problem is here [1] [2]. Cranelift returns false for multi-value being enabled, yet MaxResults will be > 1 and validation will erroneously succeed here. I believe there may be other places in the validation code that assume ENABLE_WASM_MULTI_VALUE implies all compilers support multi-value.

[1] https://searchfox.org/mozilla-central/rev/0678172d5b5c681061b904c776b668489e3355b0/js/src/jit-test/tests/wasm/spec/type.wast.js#20
[2] https://searchfox.org/mozilla-central/rev/0678172d5b5c681061b904c776b668489e3355b0/js/src/wasm/WasmConstants.h#584

Nice find. In general this situation is a real nightmare. My latest rant on this broad complex of problems is bug 1566427 but there are lots of variations.

(Edit: fix link)

proper bug link: bug 1566427

Taking this if there's no objection, as I'm working on SpiderMonkey/CL integration at the moment.

Assignee: wingo → rhunt

Ryan, can this land?

Edit: should it land before we branch, even?

Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/f7fb3affa4fb
Only allow function types with one result when CL is enabled. r=lth

Sorry I lost track of this with the holiday, this can land. I believe this patch only fixes an issue that occurs when CL is enabled. So I don't think it needs to be in a specific branch. I may be misunderstanding something though.

Flags: needinfo?(rhunt)

That is very odd. A single test failure in OSX debug jittest one-proc. The test is type.wast.js, so it's obviously related to this patch, but I don't know why other test runs aren't affected.

Ah, I think we're missing more compiler detection.

The ref-types and multi-value compiler environment flags can be true even if they
are disabled by compile time flag. This mostly works for ref-types because all
new instructions are only decoded if the compile time flag is enabled. This does
not work for multi-value as it affects type validation.

Depends on D55029

Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/f0865dabd55a
Only allow function types with one result when CL is enabled. r=lth
https://hg.mozilla.org/integration/autoland/rev/774ad330f4f6
Only enable multi-value and ref-types compiler environment flag if build time flag is set. r=lth

Ugh. I compiled this locally and ran tests but must not have that warning enabled as an error.

Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/0cc1d688b917
Only allow function types with one result when CL is enabled. r=lth
https://hg.mozilla.org/integration/autoland/rev/9d52a8119f41
Only enable multi-value and ref-types compiler environment flag if build time flag is set. r=lth
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
You need to log in before you can comment on or make changes to this bug.