1515917 - GC disablement tests fail with --wasm-gc --wasm-compiler=ion

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Lars T Hansen [:lth]]

If we upgrade jit-test/tests/wasm/gc/directives.txt thusly (as we should):

|jit-test| test-also=--no-wasm-ion; test-also=--no-wasm-baseline; test-join=--wasm-gc; include:wasm.js

there will be new failures in tables-generalized-disabled.js and disabled.js, which will fail when run with --wasm-gc --no-wasm-baseline, a combination that has apparently not been well tested under the default tiering policy.

One cause of this is clearly what Benjamin is working on over in bug 1509441, namely, making sense of all our configuration options and how they combine, but I also think it is deeper than that because we sometimes only look at cx->options().wasmGc() without looking at the configured compilers, and that leads us astray.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1515917-gc-support-discriminator.patch

I would ask for review on this even though I think your patches on bug 1509441 should take precedence, if there's any conflict, but you've disabled review requests, so needinfo it is :)

Anyway the gist here is that at a minimum every part of the system should at least use the same predicate for determining whether GC is supported with compile-time / run-time switches.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1515917-gc-support-discriminator-v2.patch

(Fix a silly bug)

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9032930 [details] [diff] [review]
bug1515917-gc-support-discriminator-v2.patch

Review of attachment 9032930 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmJS.cpp
@@ +58,5 @@
>  
> +bool wasm::HasGcSupport(JSContext* cx) {
> +#ifdef ENABLE_WASM_GC
> +  bool isSupported = cx->options().wasmGc() && cx->options().wasmBaseline();
> +#ifdef ENABLE_WASM_CRANELIFT

style nit: Does the new style guide disallow us from having spaces for nested #ifdef? If so, should we start putting:

// ENABLE_WASM_CRANELIFT

on the equivalent closing #endif, to make things clearer?

@@ +61,5 @@
> +  bool isSupported = cx->options().wasmGc() && cx->options().wasmBaseline();
> +#ifdef ENABLE_WASM_CRANELIFT
> +  if (cx->options().wasmForceCranelift()) {
> +    isSupported = false;
> +  }

This function could be rewritten to avoid the `isSupported` flag:

if (cranelift) {
  return false;
}
return cx options wasmGc and wasmBaseline;

Comment 4

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 9032930 [details] [diff] [review]
> bug1515917-gc-support-discriminator-v2.patch
> 
> Review of attachment 9032930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/wasm/WasmJS.cpp
> @@ +58,5 @@
> >  
> > +bool wasm::HasGcSupport(JSContext* cx) {
> > +#ifdef ENABLE_WASM_GC
> > +  bool isSupported = cx->options().wasmGc() && cx->options().wasmBaseline();
> > +#ifdef ENABLE_WASM_CRANELIFT
> 
> style nit: Does the new style guide disallow us from having spaces for
> nested #ifdef? If so, should we start putting:
> 
> // ENABLE_WASM_CRANELIFT
> 
> on the equivalent closing #endif, to make things clearer?

It's not obvious that it /disallows/ it but clang-format will remove such spaces with current settings.  There's an ongoing side conversation about whether it would not be better to make clang-format indent pp directives; it has the ability to do so.

> 
> @@ +61,5 @@
> > +  bool isSupported = cx->options().wasmGc() && cx->options().wasmBaseline();
> > +#ifdef ENABLE_WASM_CRANELIFT
> > +  if (cx->options().wasmForceCranelift()) {
> > +    isSupported = false;
> > +  }
> 
> This function could be rewritten to avoid the `isSupported` flag:
> 
> if (cranelift) {
>   return false;
> }
> return cx options wasmGc and wasmBaseline;

Yes, probably would be clearer.

Comment 5

[Lars T Hansen [:lth]]

Hm, actually with --tbpl there are additional failures falling out from the combination of --wasm-gc with some of the other switches.  wasm/gc/anyref-global-prebarrier.js has three failures because stack traces do not match the expectation in these cases:

  --baseline-eager --wasm-gc
  --ion-eager --ion-offthread-compile=off --wasm-gc
  --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads --wasm-gc

(Running it manually, it also fails with --ion-eager --wasm-gc, so the last two lines above are just a special case of that.)

We see:

  Error: no plausible stacks found, observed: /!>/0,!>/!>/
  Expected one of:
  /!>/0,!>//0,!>/!>/

This suggests (maybe) that the changes to the stack walking as it affects the JS jits may need to be tweaked further.  I won't be landing this patch until I've tracked that down but I'll upload the latest version.

Comment 6

[Lars T Hansen [:lth]]

Oh btw, those test failures are on arm-simulator only.  Joy.

Comment 7

[Lars T Hansen [:lth]]

Also, an interesting tsan failure in the TypedObject code on x64-opt with --ion-eager --wasm-gc.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4abdc181f48477c92abf51af5b3d09ab9da31c99&selectedJob=219742464

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1515917-gc-support-discriminator-v3.patch

Carrying bbouvier's r+ until we know where all the problems are coming from.

Comment 9

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #5)
> Hm, actually with --tbpl there are additional failures falling out from the
> combination of --wasm-gc with some of the other switches. 
> wasm/gc/anyref-global-prebarrier.js has three failures because stack traces
> do not match the expectation in these cases:
> 
>   --baseline-eager --wasm-gc
>   --ion-eager --ion-offthread-compile=off --wasm-gc
>   --ion-eager --ion-offthread-compile=off --ion-check-range-analysis
> --ion-extra-checks --no-sse3 --no-threads --wasm-gc
> 
> (Running it manually, it also fails with --ion-eager --wasm-gc, so the last
> two lines above are just a special case of that.)
> 
> We see:
> 
>   Error: no plausible stacks found, observed: /!>/0,!>/!>/
>   Expected one of:
>   /!>/0,!>//0,!>/!>/
> 
> This suggests (maybe) that the changes to the stack walking as it affects
> the JS jits may need to be tweaked further.  I won't be landing this patch
> until I've tracked that down but I'll upload the latest version.

Appears to be caused by --ion-eager and --baseline-eager injecting frames we aren't prepared to see because we're running jitted JS, a new thing for this test.  Just disabling the test when the jits are enabled is fine.  We'll have to assume --tblp also runs with both jits disabled, though I don't know if it does...

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1515917-disable-test-if-js-jit.patch

Benjamin, what do you think of this as an additional tweak?  On the one hand, it's heavy-handed to disable the test if we have any JS jit available; on the other hand, --tbpl will test with --no-ion --no-baseline, so this will get run as expected, and we will be testing the wasm barrier, as we want to.

Comment 11

[Lars T Hansen [:lth]]

Also, using the test-join directive is probably not quite right because it means we'll never run these tests without --wasm-gc and thus won't run the disablement tests.  It's still better than what we had, though; and if we're desperate we can put the disablement tests in a different directory.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9034168 [details] [diff] [review]
bug1515917-disable-test-if-js-jit.patch

Review of attachment 9034168 [details] [diff] [review]:
-----------------------------------------------------------------

There's a way to tell the testing framework that the observed frames can be one of N supplied possible frame sequences, by adding more arrays to the array of arrays (second parameter) of assertEqPreciseStacks. So we could use this to add the observed arrays when in baseline / ion mode. But if that's too much hassle, this patch will do, agreed. Thanks for the help tweak for getJitCompilerOptions.

Comment 13

[Lars T Hansen [:lth]]

Comment on attachment 9034168 [details] [diff] [review]
bug1515917-disable-test-if-js-jit.patch

Patch moved to older, independent bug 1511429 and landed there.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7543b4c2813c
Generalize testing for wasm GC availability.  r=bbouvier

Comment 15

[Lars T Hansen [:lth]]

(In reply to Pulsebot from comment #14)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7543b4c2813c
Generalize testing for wasm GC availability. r=bbouvier

This landed the changes to the feature testing, but not the changes to the directives, because those require adjustments elsewhere.

Comment 16

[Lars T Hansen [:lth]]

Attachment: bug1515917-change-directives.patch

Remaining unlanded code: actually change the directives. Carrying r+ from benjamin.

Comment 17

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/7543b4c2813c

Comment 18

[Lars T Hansen [:lth]]

Update subject because JS shell switches have changed.

Comment 19

[Lars T Hansen [:lth]]

Attachment: Bug 1515917 - Multiple test-also flags + fix disablement tests. r?bbouvier

Allowing test-also to take multiple flags improves test coverage since
we can test eg --wasm-gc --wasm-compiler=ion.

Doing so uncovered some weaknesses in the tests that test what happens
when features are disabled either by configuration or by flag, so this
patch also fixes those problems and comments them more carefully."

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb14440dc5b
Multiple test-also flags + fix disablement tests. r=bbouvier

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8eb14440dc5b