wasm: add shell tests for serialization/deserialization

RESOLVED FIXED in Firefox 64



3 years ago
10 months ago


(Reporter: bbouvier, Assigned: luke)


(Blocks 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)



(6 attachments, 2 obsolete attachments)

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.
Priority: -- → P3
(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.
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.
Closed: Last year
Resolution: --- → INACTIVE
Resolution: INACTIVE → ---
Component: JavaScript Engine: JIT → Javascript: Web Assembly
I think this would be a good place to start, refactoring the interface toward what we'd want for bug 1487113.
Just removing some dead bits left over from bug 1469395.
Assignee: nobody → luke
Attachment #9005747 - Flags: review?(lhansen)
Posted patch move-codeSplinter Review
Pure code motion to move these two functions closer to their use (since I was here and this bugs me from time to time).
Attachment #9005748 - Flags: review?(lhansen)
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.
Attachment #9005749 - Flags: review?(lhansen)
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.
Attachment #9005750 - Flags: review?(lhansen)
Attachment #9005750 - Flags: review?(continuation)
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.
Attachment #9005752 - Flags: review?(lhansen)
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 on attachment 9005750 [details] [diff] [review]

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

r=me for the non js/src parts
Attachment #9005750 - Flags: review?(continuation) → review+
Blocks: 1487113
Attachment #9005747 - Flags: review?(lhansen) → review+
Attachment #9005748 - Flags: review?(lhansen) → review+
Comment on attachment 9005749 [details] [diff] [review]

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

Attachment #9005749 - Flags: review?(lhansen) → review+
Comment on attachment 9005750 [details] [diff] [review]

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.
Attachment #9005750 - Flags: review?(lhansen) → review+
Comment on attachment 9005752 [details] [diff] [review]

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.
Attachment #9005752 - Flags: review?(lhansen) → review+
Keywords: leave-open
Pushed by lwagner@mozilla.com:
Baldr: remove some dead wasm-IDB code (r=lth)
Baldr: move static functions in WasmCompile.cpp to be closer to their first use (r=lth)
Baldr: move Assumptions out of Module (r=lth)
Baldr: make the BuildIdOp a process global (r=lth,mccr8)
Pushed by lwagner@mozilla.com:
Baldr: move LinkData out of Module (r=lth)
No longer blocks: 1276029
Posted patch enable-baseline-on-table-gc (obsolete) — Splinter Review
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.
Attachment #9010314 - Flags: review?(lhansen)
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.)
Attachment #9005755 - Attachment is obsolete: true
Attachment #9010317 - Flags: review?(lhansen)
Comment on attachment 9010314 [details] [diff] [review]

Derp, this was the *JS* baseline, not wasm baseline, and I had forgot to test with --tbpl which of course tests --baseline-eager.
Attachment #9010314 - Attachment is obsolete: true
Attachment #9010314 - Flags: review?(lhansen)
Comment on attachment 9010317 [details] [diff] [review]

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.
Attachment #9010317 - Flags: review?(lhansen) → review+
(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.
Pushed by lwagner@mozilla.com:
Baldr: add JS::OptimizedEncodingListener interface, shell implementation of it and tests (r=lth)
Keywords: leave-open
Closed: Last year10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.