I like the principles of never enabling anything that was explicitly disabled by a flag and having sufficient wasmXEnabled() for all interesting X to use in "|jit-test| skip-if:" and inside tests to deal with the consequences.

Also, building on what we already have and trying to remove the "should"/"may"/"use" distinction, how about:

• "XSupported" means "supported by the build/hardware; impossible to use if not supported" (iiuc, the current meaning)
• "XFlag" reflects the (default or explicit) bool flag value of an about:config pref or shell flag.
• "XEnabled" === "XSupported && XFlag" (is this the current meaning?)

Considering the outliers:

• Maybe we can simply kill the GC opt-in header, even before we have Ion+GC support by saying that, until GC works in Ion, GC is only enabled when WasmBaselineEnabled && !WasmIonEnabled && !WasmCraneliftEnabled. Then all the GC tests have skip-if: !wasmGCEnabled().
• --test-wasm-await-tier2 and wasm.delay-tier2 have no effect if tiering is not enabled due to other flags. Tests that want to assert something about tiers should skip-if tiering is not enabled (tested by some new wasmTieringIsEnabled()).

Would that work?

(In reply to Luke Wagner [:luke] from comment #15)

I like the principles of never enabling anything that was explicitly disabled by a flag and having sufficient wasmXEnabled() for all interesting X to use in "|jit-test| skip-if:" and inside tests to deal with the consequences.

Also, building on what we already have and trying to remove the "should"/"may"/"use" distinction, how about:

• "XSupported" means "supported by the build/hardware; impossible to use if not supported" (iiuc, the current meaning)
• "XFlag" reflects the (default or explicit) bool flag value of an about:config pref or shell flag.
• "XEnabled" === "XSupported && XFlag" (is this the current meaning?)

This seems reasonable, and I'm also generally in favor of fail-fast.

Considering the outliers:

• Maybe we can simply kill the GC opt-in header, even before we have Ion+GC support by saying that, until GC works in Ion, GC is only enabled when WasmBaselineEnabled && !WasmIonEnabled && !WasmCraneliftEnabled. Then all the GC tests have skip-if: !wasmGCEnabled().

The opt-in header serves another main purpose, namely, to make sure compiled wasm code is compatible with the prototype encodings used by the engine running that code. Without this you'd get all sorts of mysterious breakage. I want to not require the header for reftypes asap, but it will IMO need to be required for other gc feature things that are not stable.

It is possible that --wasm-gc could just (explicitly) be an alias for --no-wasm-ion --no-wasm-cranelift, now that the suppressGC hacks have been removed, although I don't think I'm in favor of it. At the moment, we get to test that the gc bits that are currently only supported by the baseline compiler interact properly with ion-compiled code, in multi-module situations. That seems useful.

• --test-wasm-await-tier2 and wasm.delay-tier2 have no effect if tiering is not enabled due to other flags. Tests that want to assert something about tiers should skip-if tiering is not enabled (tested by some new wasmTieringIsEnabled()).

My preference would be for an early exit if a flag is set that can't be honored, ie, not "no effect" but "abort". I guess this is harder in the browser though, so "no effect" is a workable compromise.

(In reply to Benjamin Bouvier [:bbouvier] from comment #14)

(In reply to Lars T Hansen [:lth] from comment #10)

Comment on attachment 9028935 [details] [diff] [review]
1.rename-compileargs.patch

I really think the source of confusion is 2), because it does silently
overrides what we chose to use.

I agree.

A suggested alternative would be a
combination of explicitly throwing if there's a contradiction, and using
more testing functions to know if we can do things, which would be more
explicit and avoid bugs in general.

I agree with that too, although "throwing" is hard sometimes in the context of the browser.

(For instance: if a test declares
--force-wasm-tiering and --no-wasm-baseline, then it would throw because the
latter flag contradicts the former; but a canWasmTier() function is
introduced and called early, exiting the test since there's a contradiction
in the testing flags.)

So I guess the "throwing" would be happen on the first-tier compilation, "new WebAssembly.Module" et al. This seems reasonable to me.

This solution would be better for the long time,
since it doesn't introduce silent overrides. In the initial context of this
bug, it means using Cranelift on x86 32 bits would throw, which is
explicitly nice, as opposed to the final state after my patches (Cranelift
is silently disabled, or disabled with a console message if we finally
decide to take the fourth path).

Right.

Luke, Lars, what do you think? Should we go on with the current patches now,
and move on to explicit throwing later? Or do explicit throwing now?

IMO let's leap into the future and implement sanity checks that throw, when we can. (Also we need to do better when we test, see bug 1515917 for ugly details.)

Thanks for your input! I like the flag/supported/enabled naming (it's clearer than may, etc).

That being said (and sorry in advance for the long post), going back to this, I have a few questions.

1. It seems there's a confusion related to what should --wasm-force-cranelift do, since it's used in two contexts.

A. Does it mean "use Cranelift when Ion would be used"? Which is the current behavior, and so we do not really "force" usage of Cranelift, since the baseline can be used in practice. This has led me to confusion in the past, where I thought I was testing Cranelift, although Spidermonkey would sometimes run baseline.
B. Does it mean "we want to use Cranelift unconditionally, and not tier"?

It's actually even a bit deeper than this: --no-wasm-ion and --no-wasm-baseline worked fine as long as there were only 2 compilers. In the future if/when cranelift is enabled by default, --no-wasm-ion can mean "force baseline all the time" or "baseline as a first tier, cranelift as a second one if it's available" (depending on whether we enable the Cranelift flag).

It sounds tempting to replace the --no-wasm-* switches too, so that we have:

• --wasm-unique-backend={ion,cranelift,baseline} (or --wasm-always-backend) to mean we want to test only one backend. Then --no-wasm-ion (resp baseline) is equivalent to --wasm-unique-backend=baseline (resp ion) in current terms, and we can also request testing only Cranelift.
• --wasm-tier-backend={ion,cranelift} to tell which backend can be used as the second tier backend.

It would be an error to use both flags at the same time.

Using GC or debug would require --wasm-unique-backend and throw otherwise.

Forcing tiering would require --wasm-tier-backend (and per the first rule, would be an error to use with --wasm-unique-backend).

2. In addition to this --wasm-force-cranelift issue, I think the proper way to deal with this is to construct CompileArgs immutably, in a factory style (CompileArgs::make(JSContext*, ScriptedCaller&&)). Then, one can't reset the fields arbitrarily, and the structure is immutably built (with factory variants for other threads where we don't have a JSContext and guess what's actually possible, and then check at instantiation). I tried this in the past, but this couldn't work without explicit throwing if two flags were contradicting each other. Now that we've agreed upon explicitly throwing, this should be possible to do.

3. Also, asked Lars on IRC if we had some benefits from using HasGcTypes::{True/False} and not just a plain bool, and we both agreed it could just be a boolean.

How do these proposals sound to you?

I agree the time has come for the --no-wasm-whatever switches to go.

Could we roll the compiler selection switches into one?

--wasm-compiler=baseline+ion
--wasm-compiler=baseline
--wasm-compiler=baseline+cranelift
--wasm-compiler=none

I ask because this might simplify testing somewhat and it would make it clear what is being asked, and it matches (or could match) what wasmCompilationPolicy() is returning. The keyword can be none, baseline, ion, cranelift, baseline+ion or baseline+cranelift. It can't be nothing. The default is baseline+ion except on ARM64, where it is baseline.

Now as you say, --wasm-gc would (temporarily) require also --wasm-compiler=baseline, or it could imply it and it could error out if another value was provided. And forcing tiering would require a baseline+something pair.

See commit message. The code is much easier to read this way in my opinion.

Using the flag/enabled terms; it feels clearer. There's a need for another patch for the peculiar case of gcTypesEnabled, which is a mix of checking the flag and support.

(not to be checked in, but helped to find things i broke)

Sorry for the patch ordering, spotted nits in patch 3.

So after this series of patches, the --no-wasm, --no-wasm-baseline, --no-wasm-ion, --wasm-force-cranelift switches have been updated to a single one, as suggested by Lars: --wasm-compiler={none,baseline,baseline+ion,ion,cranelift,baseline+cranelift}.

The last patch implements consistency checking between the --wasm-compiler switch and other flags. The default value hasn't changed: wasm is enabled, as well as baseline and ion. See examples below, where a.js is a script compiling a single wasm module (no GC opt-int) and gc.js is the same with a GC feature opt-in:

➜ d64 ./dist/bin/js /tmp/a.js
one shot compilation with ion
➜ d64 ./dist/bin/js --wasm-compiler=baseline /tmp/a.js
one shot compilation with baseline
➜ d64 ./dist/bin/js --wasm-compiler=ion /tmp/a.js
one shot compilation with ion
➜ d64 ./dist/bin/js --wasm-compiler=cranelift /tmp/a.js
one shot compilation with cranelift
➜ d64 ./dist/bin/js --wasm-compiler=baseline+cranelift /tmp/a.js
one shot compilation with cranelift
➜ d64 ./dist/bin/js --wasm-compiler=baseline+ion /tmp/a.js
one shot compilation with ion
➜ d64 ./dist/bin/js --wasm-compiler=baseline+ion --test-wasm-await-tier2 /tmp/a.js
two-tiered compilation starting with baseline
➜ d64 ./dist/bin/js --wasm-compiler=baseline --test-wasm-await-tier2 /tmp/a.js
/tmp/a.js:1:1 Error: can't force tiering without both baseline and a second tier
Stack:
@/tmp/a.js:1:1
➜ d64 ./dist/bin/js --wasm-compiler=baseline+cranelift --test-wasm-await-tier2 /tmp/a.js
two-tiered compilation starting with baseline

➜ d64 ./dist/bin/js --wasm-compiler=baseline --wasm-gc /tmp/gc.js
one shot compilation with baseline
➜ d64 ./dist/bin/js --wasm-compiler=ion --wasm-gc /tmp/gc.js
/tmp/gc.js:1:1 Error: can't use wasm debug/gc without baseline
Stack:
@/tmp/gc.js:1:1
➜ d64 ./dist/bin/js --wasm-compiler=baseline+ion --wasm-gc /tmp/gc.js
one shot compilation with baseline
➜ d64 ./dist/bin/js --wasm-compiler=baseline+ion --wasm-gc --test-wasm-await-tier2 /tmp/gc.js
one shot compilation with baseline
// In this case, test wasm await tier 2 is silently overriden, because wasm-gc already forces the baseline compiler.
➜ d64 ./dist/bin/js --wasm-compiler=cranelift --wasm-gc /tmp/gc.js
/tmp/gc.js:1:1 Error: can't use wasm debug/gc without baseline
Stack:
@/tmp/gc.js:1:1

I am building the whole browser now to make sure this will (actually!) solve the initial issue, that is, that we raise an error when Cranelift isn't natively supported.

I could confirm that disabling baseline, enabling wasm-gc and cranelift leads to an alert() with the Spidermonkey error on the WebAssembly.org demo.

A small change in functionality, because there was one more workflow I didn't test and wanted to support: jit_tests.py --args=--wasm-compiler=ion wasm, for instance.

Some tests hard fail under debug in this case, others because of --test-wasm-await-tier2. For the debug tests, we can just add a skip-if directive in the test; for the tier2 testing, unfortunately there's not much better than silently ignoring the switch, if it's given (we could alternatively have it be the equivalent of --wasm-compiler=baseline+ion, but it would still interfere badly with --wasm-gc). Updated the patch to reflect this.

(In reply to Lars T Hansen [:lth] from comment #36)

Comment on attachment 9038572 [details] [diff] [review]
5.return-errors-and-allow-cranelift.patch

Review of attachment 9038572 [details] [diff] [review]:

Nice work.

Thanks for the quick reviews!

(style nits in the first review not addressed since mach clang format will take care of this; I learnt yesterday it's run once a week by release managers)

::: js/src/builtin/TestingFunctions.cpp
@@ +687,5 @@

}

+static bool WasmDebugSupport(JSContext* cx, unsigned argc, Value* vp) {

• CallArgs args = CallArgsFromVp(argc, vp);
• args.rval().setBoolean(cx->options().wasmBaseline());

For this predicate, I think we want && BaselineCanCompile() since the
baseline compiler sometimes requires hardware features that are not on every
platform. But I'm not 100% sure what will happen then. Mostly this change
would be a no-op but ARMv7 systems without IDIV would return false for
wasmDebugSupport, for example. I think that would just cause some debug
tests not to run, since wasmDebugSupport is not used for anything else.

I've added this. We might need to review all the places where we use the options() flags and consider this to be the ultimate truth of whether the actual backend is enabled or not, because we've had a few issues with this in the past.

@@ +105,5 @@

• if (forceTiering && (!baseline || (!cranelift && !ion))) {
• // This can happen only in testing, and in this case we don't have a
• // proper way to signal the error, so just silently override the default,
• // instead of adding a skip-if directive to every test using debug/gc.

Printing a warning would just be annoying, I guess?

We could do it, but warnings are disabled by default, and they would not show up locally or in automation, which is why I decided to just silently ignore the option in this case. So keeping no warning, as you agreed with this on IRC.

(In reply to Benjamin Bouvier [:bbouvier] from comment #37)

(In reply to Lars T Hansen [:lth] from comment #36)

+static bool WasmDebugSupport(JSContext* cx, unsigned argc, Value* vp) {

• CallArgs args = CallArgsFromVp(argc, vp);
• args.rval().setBoolean(cx->options().wasmBaseline());

For this predicate, I think we want && BaselineCanCompile() since the
baseline compiler sometimes requires hardware features that are not on every
platform. But I'm not 100% sure what will happen then. Mostly this change
would be a no-op but ARMv7 systems without IDIV would return false for
wasmDebugSupport, for example. I think that would just cause some debug
tests not to run, since wasmDebugSupport is not used for anything else.

I've added this. We might need to review all the places where we use the options() flags and consider this to be the ultimate truth of whether the actual backend is enabled or not, because we've had a few issues with this in the past.

This is how HasReftypesSupport got introduced: so that we could always just use that instead of checking the options(). There could be HasWasmBaselineCompiler() similarly.

Decoder, Gary: note this updates shell flags (as reflected in shell/fuzz-flags.txt):

• --no-wasm, --no-wasm-baseline and --no-wasm-ion have been removed
• everything has been replaced with --wasm-compiler, which can take any of the following values: none, baseline, ion+baseline, ion, cranelift, ion+cranelift. All of them but the last two would be nice to test.

Thanks!

Drive-by pedantry:

Actually, "baseline+ion" and "baseline+cranelift", not "ion+baseline" and "ion+cranelift".

https://hg.mozilla.org/mozilla-central/rev/e9ffcdc09fdd
https://hg.mozilla.org/mozilla-central/rev/85c33286abbf
https://hg.mozilla.org/mozilla-central/rev/48dc14f79fb0
https://hg.mozilla.org/mozilla-central/rev/677eda6bdaf9
https://hg.mozilla.org/mozilla-central/rev/cb20dcd8ea7e
https://hg.mozilla.org/mozilla-central/rev/7df604faea71

(In reply to Benjamin Bouvier [:bbouvier] from comment #41)

• --no-wasm, --no-wasm-baseline and --no-wasm-ion have been removed
• everything has been replaced with --wasm-compiler, which can take any of the following values: none, baseline, ion+baseline, ion, cranelift, ion+cranelift. All of them but the last two would be nice to test.

Thanks for the needinfo! I quickly added this to funfuzz as things were breaking all round...

https://github.com/MozillaSecurity/funfuzz/commit/e0afd61b4b6201558de5578e5c5cb07e6dce8c30

(Clearing ni? since we have the fuzz-flags file now.)