1330661 - wasm: add shell tests for serialization/deserialization

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P3)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Bug 1276029 has provided new JS APIs for serializing and deserializing wasm modules. The same way we have testing builtins for asm.js caching, we would have the same thing for wasm.

Ideally, we want:
- to be able to test all the valid cases (serialization and deserialization)
- to be able to apply binary tweaks in specific portions of the cached module (the build-id header), to test corrupted content.
- intrinsics that are introduced for this latter bullet point to be fuzzing-safe.

Luke says we might need a new opaque object type that would contain the Module and compiled code files too.

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #0)
> Luke says we might need a new opaque object type that would contain the
> Module and compiled code files too.

I already added one to the JS Shell, which is named CacheEntry, and that I use for testing XDR encoding and decoding.
This is a JSObject which abstract over any possible caching implementation, independently of how it would be stored.

It was made to be fuzzing-safe, by only providing accessors to its internal buffer to other shell functions.

Comment 2

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 3

[Luke Wagner [:luke]]

I think this would be a good place to start, refactoring the interface toward what we'd want for bug 1487113.

Comment 4

[Luke Wagner [:luke]]

Attachment: remove-dead-code

Just removing some dead bits left over from bug 1469395.

Comment 5

[Luke Wagner [:luke]]

Attachment: move-code

Pure code motion to move these two functions closer to their use (since I was here and this bugs me from time to time).

Comment 6

[Luke Wagner [:luke]]

Attachment: move-assumptions

With IDB support gone, it's no longer necessary for a wasm::Module to track the Assumptions it was compiled with.  Moreover, with the way alt-data works, Module::(de)serialize don't have to store the Assumptions: the buildid gets put into the key which asks a given cache entry if it has the alt data.  This nicely simplifies the code.  The asm.js cache does still need to store Assumptions, though, so this patch moves Assumptions into AsmJS.cpp.

Comment 7

[Luke Wagner [:luke]]

Attachment: process-build-id-op

This patch makes the BuildIdOp a process-wide global (like the LargeAllocationFailureCallback) so it can be retrieved from any thread at any time.  r? lth for the js/src changes, r? mccr8 for the Gecko changes.

Comment 8

[Luke Wagner [:luke]]

Attachment: move-link-data

This patch both stops storing LinkData in the Module since it is no longer needed for serialization at arbitrary times.  Moreover, it turns out there's no need for a separate LinkDataTier: all LinkData is inherently tier-specific.  This simplifies the code and the next patches.

Comment 9

[Luke Wagner [:luke]]

Attachment: new-serialization-interface (WIP)

WIP patch that changes the JS::StreamConsumer interface to allow a host to ask to receive an "optimizing encoding" of the stream contents.  This is in anticipation of bug 1487113.  Mostly done just need to actually write some shell functions to test it.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 9005750 [details] [diff] [review]
process-build-id-op

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

r=me for the non js/src parts

Comment 11

[Lars T Hansen [:lth]]

Comment on attachment 9005749 [details] [diff] [review]
move-assumptions

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

Nice.

Comment 12

[Lars T Hansen [:lth]]

Comment on attachment 9005750 [details] [diff] [review]
process-build-id-op

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

I like it.

::: js/src/vm/Runtime.h
@@ +1191,5 @@
>  // and may be null. See comment in jsapi.h.
>  extern mozilla::Atomic<JS::LargeAllocationFailureCallback> OnLargeAllocationFailure;
>  
> +// This callback is set by JS::SetBuildIdOp and may be null. See comment in jsapi.h.
> +// and may be null. See comment in jsapi.h.

Second line of the comment should be removed.

Comment 13

[Lars T Hansen [:lth]]

Comment on attachment 9005752 [details] [diff] [review]
move-link-data

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

Seems reasonable.

::: js/src/wasm/WasmModule.h
@@ +65,5 @@
> +    // per-instance code patching.
> +
> +    mutable Atomic<bool>    debugCodeClaimed_;
> +    const UniqueConstBytes  debugUnlinkedCode_;
> +    const UniqueLinkData    debugLinkData_;

+1 for the renaming.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/186b3f9b43c1
Baldr: remove some dead wasm-IDB code (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/52fdd53134d5
Baldr: move static functions in WasmCompile.cpp to be closer to their first use (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/603f0c6003b3
Baldr: move Assumptions out of Module (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd32db49222
Baldr: make the BuildIdOp a process global (r=lth,mccr8)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9287930e6a49
Baldr: move LinkData out of Module (r=lth)

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/186b3f9b43c1
https://hg.mozilla.org/mozilla-central/rev/52fdd53134d5
https://hg.mozilla.org/mozilla-central/rev/603f0c6003b3
https://hg.mozilla.org/mozilla-central/rev/0dd32db49222
https://hg.mozilla.org/mozilla-central/rev/9287930e6a49

Comment 17

[Luke Wagner [:luke]]

Attachment: enable-baseline-on-table-gc

Completely unrelated, but I noticed these tests could be reenabled for baseline.  I can't think what extra edges baseline may be adding; perhaps all the refactorings since have removed them.

Comment 18

[Luke Wagner [:luke]]

Attachment: new-serialization-interface

Now with a full shell impl and tests!  With this patch, the whole (de)serialization pipeline gets exercised by the shell, not just via asm.js caching.  Some notes:
 - The goofy AddRef()/Release() signatures are intended so that a standard nsISupports impl in Gecko fills these in.
 - Making sure the build-id matches is now the embedding's responsibility, but it is release-asserted to be identicial by (otherwise superfluously) (de)serializing the build-id.

(Sorry for delay in reviewing your other patches; I'll commence now.)

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 9010314 [details] [diff] [review]
enable-baseline-on-table-gc

Derp, this was the *JS* baseline, not wasm baseline, and I had forgot to test with --tbpl which of course tests --baseline-eager.

Comment 20

[Lars T Hansen [:lth]]

Comment on attachment 9010317 [details] [diff] [review]
new-serialization-interface

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

No red flags.

::: js/src/builtin/TestingFunctions.cpp
@@ +637,5 @@
>  static bool
> +WasmCachingIsSupported(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    args.rval().setBoolean(wasm::HasSupport(cx) && cx->options().wasmIon());

This should be ... && cx->options().wasmIon() && wasm::IonCanCompile(), see similar code in HasAvailableCompilerTier().  For example, the code you have above will return true currently on ARM64 but probably should not.

(Thus arguably some of the other predicates in this file are actually wrong; the baseline compiler requires the SDIV/UDIV instructions on ARM for example, but WasmDebuggingIsSupported ignores that problem apparently.)

@@ +5824,5 @@
>  "  returns false also if WebAssembly is not supported"),
>  
> +    JS_FN_HELP("wasmCachingIsSupported", WasmCachingIsSupported, 0, 0,
> +"wasmCachingIsSupported()",
> +"  Returns a boolean indicating whether WebAssembly caching is enabled."),

"enabled" => "supported", since those are not the same thing.

::: js/src/shell/js.cpp
@@ +6812,5 @@
> +
> +        if (!dstBytes->resize(srcLength)) {
> +            return;
> +        }
> +        memcpy(dstBytes->begin(), srcBytes, srcLength);;

Spurious ";"

::: js/src/wasm/WasmModule.cpp
@@ +359,5 @@
> +    }
> +
> +    buildId->infallibleAppend('(');
> +    while (cpu) {
> +        buildId->infallibleAppend('0' + (cpu & 0xf));

This works, but I'm curious if you were really intending a hexadecimal encoding here.

Comment 21

[Luke Wagner [:luke]]

(In reply to Lars T Hansen [:lth] from comment #20)
> > +        buildId->infallibleAppend('0' + (cpu & 0xf));
> 
> This works, but I'm curious if you were really intending a hexadecimal
> encoding here.

Yeah, I considered doing a proper hex rendering, but it was more code and, at least from what I could tell, more temporary allocations for the various existing methods, so I just went with the bare minimum that satisfied the uniqueness requirement.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc165233051
Baldr: add JS::OptimizedEncodingListener interface, shell implementation of it and tests (r=lth)

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/bbc165233051