1415224 - Kill wasm test mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

It doesn't make sense that we'd optimize the testing of i64 and NaN with custom payloads for bug 1319203; there's already a lot of instrumentation that we could spare by just having wasm do assertions in wasm directly.

This bug investigates this and see if we can remove wasm testing mode at all here.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.remove-nan.patch

WIP (removes NaNs).

6 files changed, 129 insertions(+), 238 deletions(-)

Comment 2

[Lars T Hansen [:lth]]

Looks plausible.  May need more types of assertions beyond equality?  Not sure yet.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.int64-globals-throw.patch

Something I spotted while working on this bug: the spec says importing/exporting i64 globals will cause throwing at link time, not compile time. (also filed https://github.com/WebAssembly/design/issues/1152 for asking precisely when do we throw when calling into wasm and there's an i64 in the signature)

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.remove-nan.patch

This adds infrastructure to have NaN payload assertions generated as wasm functions (that's the same approach taken by the wasm spec->JS test converter). Tests get more verbose here, but they remove the need for the wasm testing mode for NaNs, and a lot of c++ code is removed (and it makes the jit->wasm patch simpler).

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.remove-int64.patch

Same testing principle (assertions generated as wasm functions) but for int64 this time.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.remove-int64-cpp-testingmode.patch

Removes int64 testing mode support in C++.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.remove-wasm-test-mode.patch

This finishes removing wasm testing mode: it was used in one more place, so that :decoder could auto generate export calls from the non-standard "descriptors" we pass as part of WebAssembly.Module.exports() calls. I chose to make these MORE_DETERMINISTIC, since fuzzers always build shells in this mode.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Overall:  29 files changed, 796 insertions(+), 1178 deletions(-)

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8926945 [details] [diff] [review]
1.remove-nan.patch

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

Makes sense; nice to see that code go.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8926944 [details] [diff] [review]
1.int64-globals-throw.patch

Moving to its own bug.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.remove-int64-cpp.patch

(rebased)

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8926946 [details] [diff] [review]
2.remove-int64.patch

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

::: js/src/jit-test/lib/wasm.js
@@ +90,4 @@
>      }
>      newSrc += ')';
> +    // TODO remove
> +    print(newSrc)

nit: TODO

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8926948 [details] [diff] [review]
4.remove-wasm-test-mode.patch

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

Yay!  r+ with one requested change; feel free to propose alternatives, though.

::: js/src/wasm/WasmJS.cpp
@@ +544,5 @@
>          if (!kindStr)
>              return false;
>          props.infallibleAppend(IdValuePair(NameToId(names.kind), StringValue(kindStr)));
>  
> +#ifdef JS_MORE_DETERMINISTIC

I'm a bit reticent to use JS_MORE_DETERMINSTIC since it changes semantics and it's not clearly testing-only (and might morph meaning over time).  There is a MOZ_FUZZING_SAFE envvar which seems like a better signal.  We wouldn't want to probe getenv() all the time, but it looks to be cached in a fuzzingSafe global in TestingFunctions.cpp; could we use an 'extern' to grab that?

Comment 14

[Luke Wagner [:luke]]

Thanks!

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

> could we use an 'extern' to grab that?

Yes, by making fuzzingSafe non-static in the TestingFunctions.cpp file. Thanks for the reviews!

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

A note to our dear fuzzer people: note this set of patches will remove the --wasm-test-mode shell runtime flag, and annotated wasm imports/exports (with signatures) will now need the MOZ_FUZZING_SAFE environment variable set.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff37d9f812be
Remove wasm testing mode for custom NaN payloads; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcae00340c54
Have wasm assertions auto-generated in wasm; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/15613c4e6af3
Remove jit test mode support for int64 testing; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6091ddbf6b
Remove wasm test mode entirely; r=luke

Comment 18

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ff37d9f812be
https://hg.mozilla.org/mozilla-central/rev/bcae00340c54
https://hg.mozilla.org/mozilla-central/rev/15613c4e6af3
https://hg.mozilla.org/mozilla-central/rev/fe6091ddbf6b

Comment 19

[Gerry Chang [:gchang]]

Won't fix for 58, let it ride the train.