(I'm filing this as a followup so as not to delay tiering further, but this is not a large amount of work.) In existing code, one can tag test cases with "test-also-wasm-baseline", which causes the test case to be run both with and without --wasm-always-baseline. When tiering (bug 1277562) lands, the situation changes so that one can tag test cases with "test-also-no-wasm-baseline" instead; doing so causes the test case to be run both with and without --no-wasm-baseline. Unfortunately that change leads to different test coverage than before. In the past, the test would be run both baseline-compiled and ion-compiled. With tiering, without the flag, the test is run baseline-compiled until the ion code is ready and is patchd in, and then ion-compiled; with the flag, it is run ion-compiled always. There's no way to truly guarantee that only baseline-compiled code is run. Tiering introduces a new shell flag, --no-wasm-ion, which can be used to work around that, and the testing system should be expanded to make use of it. For example, we could have "test-also-no-wasm-ion" as an additional flag, and/or we could have the abbreviation "test-wasm-tiering" imply the combination of the two, for a total of three runs: without any flags, with --no-wasm-ion, and with --no-wasm-baseline.

Furthermore: In bug 1277562 there are some patches (designated 16c, 16d, and 16e) that are not landing in the initial wave because there's no agreement on them yet: they implement command line switches to the JS shell, environment variables (useful with the browser), and various ways of manipulating tiering from the command line and from JS. This functionality has been helpful in finding many bugs in the tiering code and should land in some form or other.

Patch 16c from bug 1277652. For Luke's comments to this patch, see https://bugzilla.mozilla.org/show_bug.cgi?id=1277562#c122. Note that the disableTier2 flag tests different code than disabling the ion compiler. When the ion compiler is disabled, the compilation mode is determined early to be baseline-only. When tier2 is disabled through this switch, the ion compilation is forced to fail after having been scheduled (at least that's the theory).

Patch 16d from bug 1277562: implement JS testing functions for this functionality, and some test cases.

Patch 16e from bug 1277562: implement some environment variables that can be used from the command line to affect behavior also in the browser.

See also discussion starting at bug 1404683 comment 16. We *must* have "test-also-no-wasm-ion" or "test-wasm-tiering" as discussed in comment 0 above, otherwise rabaldr is likely not to be tested at all since the tiering rules will take us directly to ion for almost every program. This should happen soon -- right now we're flying blind. In general we should also consider whether the standard test switches (the combinations generated by --tbpl) are desirable for wasm compilation; I expect they probably are not and that it's a waste of time to run wasm tests with all these combinations. (Probably depends on how much JS there is in the test.) For one thing, standard Ion / Baseline switches do not affect Baldr / Rabaldr, and we have our own rules for background compilation and so on.

This introduces a directive, test-also-no-wasm-ion, and adds that to the directives.txt files so that we get some test coverage of rabaldr, which now has few operative regression tests. It is quite possible that this directive will disappear and that we'll have some tiering directive instead; I just don't want to go without test coverage until then.

Comment on attachment 8916940 [details] [diff] [review] bug1388785-force-wasm-baseline.patch Review of attachment 8916940 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa6adeac346cc0aea40f440bf725353834416621 Bug 1388785 - also force rabaldr to be tested. r=bbouvier

So thinking about better testing that would've caught bug 1406041, how about this: 1. add a new shell flag --test-wasm-tiering that: a. forces tiering except where disabled by debugging or !BaselineCanCompile() b. blocks on tier2 completion upon instantiation of a Module 2. add a new jit-test directive test-also-wasm-tiering that sets this flag

Unrelated, but I noticed the default ContextOptions disable wasm/wasm-ion/wasm-baseline. This patch changes the default to match the shipping default.

I also noticed this testing mode which has been around a year. I'm open to hear other opinions, but it seems to me it's not providing much value: - it simply controls whether we run an optimization pass in Ion (no effect on Baseline nor later in Cretonne) - I haven't seen it catch anything - I expect we get more thorough OOB testing via ASan/Valgrind

(Oops, forgot to qref.)

Comment on attachment 8919939 [details] [diff] [review] change-wasm-defaults Review of attachment 8919939 [details] [diff] [review]: ----------------------------------------------------------------- Seems fine, and entails no functional change anywhere as far as I can tell, but removes a footgun.

Comment on attachment 8919940 [details] [diff] [review] rm-wasm-always-bce Review of attachment 8919940 [details] [diff] [review]: ----------------------------------------------------------------- It seems that this switch may have had a different function before, wherein it affected code generation? Anyway I agree this provides very little value now, if any.

Comment on attachment 8919942 [details] [diff] [review] test-wasm-tiering Review of attachment 8919942 [details] [diff] [review]: ----------------------------------------------------------------- This is a good testing mode but I think we'll need more modes, so the '--test-wasm-tiering' name is too broad imo. How about '--test-wasm-await-tier2' (yeah, clunky; open to other suggestions) instead?

wasm-always-bce was indeed more useful before: it was unconditionally generating bounds checks for all the memory accesses, potentially revealing errors with: - signal handlers handling accesses they shouldn't have handled on WASM_HUGE platforms (asm.js). Not sure it ever happened in practice... - removed bounds checks that shouldn't have been, with wasm BCE on !WASM_HUGE platforms. There have been a few instances of this, and that was why the test directive had been introduced. It seems it has been tested enough, and since this code hasn't changed for a while, it makes sense to remove it to lower resource consumption on pushes. If we were to get back to WasmBCE.cpp, it'd be nice to have it again, though.

(In reply to Lars T Hansen [:lth] from comment #16) > Comment on attachment 8919942 [details] [diff] [review] > test-wasm-tiering > > Review of attachment 8919942 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is a good testing mode but I think we'll need more modes, so the > '--test-wasm-tiering' name is too broad imo. How about > '--test-wasm-await-tier2' (yeah, clunky; open to other suggestions) instead? BTW the 'test-also-wasm-tiering' directive seems like the right thing with the right name, and once we have more testing switches / modes the directive can expand into these multiple modes. It is just the propagation of that name into the shell and engine I think ought to have a narrower name.

Ok, great points. I'll keep test-also-wasm-tiering and change shell flag to --test-wasm-await-tier2.

Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b84fae5dfa32 Change wasm flags' default states in ContextOptions (r=lth) https://hg.mozilla.org/integration/mozilla-inbound/rev/c637d9a5317a Remove --wasm-check-bce flag (r=lth) https://hg.mozilla.org/integration/mozilla-inbound/rev/b1dc87a94262 Add --test-wasm-await-tier2 and use it in jit-tests (r=lth)

Re-adding leave-open, I think we need more here.

Ah, my bad, I see you assigned this to yourself. Your call, then.

Ah, I didn't know any of us had any imminent plans to add further testing here so I thought best to close out this bug and file new bugs for future work. One can definitely imagine bugs that only happened when the tier2 transition happens after or during some amount of tier1 execution, but I couldn't think of a way to test this in an orthogonal manner like --test-wasm-await-tier2 does.

I agree it's difficult to say what more we want. The WASM_DELAY_TIER2=n switch to just hold up the start of tier2 compilation for n ms did find some problems, as I remember it, but I'm not sure what its value would be now. In the past we've discussed various chaos modes. One could imagine a switch that forces the patching to patch only every other entry or similar. This would then (probabilistically) test that patched and unpatched code intercall properly. Not clear to me what the value is, once we've established this works. We can revisit if/when we get around to patching imports and exports :)

https://hg.mozilla.org/mozilla-central/rev/b84fae5dfa32 https://hg.mozilla.org/mozilla-central/rev/c637d9a5317a https://hg.mozilla.org/mozilla-central/rev/b1dc87a94262

Comment on attachment 8919942 [details] [diff] [review] test-wasm-tiering (Clearing r? because patch with requested fixes landed already.)

Oh my, I had thought that patch already had r+; I must have confused that with another r+. Sorry about that, didn't mean to land preemptively.