1523993 - Enhance wasm debugging capabilities

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

• with the possibility to control whether fast paths are enabled at run time, through env variables or other ways (see bug 1522458 comment 6).
• allow to print all the values of arguments / return values at wasm call sites and on wasm imports.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.patch

This does the first part of this bug; see commit message.

I haven't gone as far as adding the javascript.options.environment about:config pref yet, because it would require parsing flags etc., and I'd like Spidermonkey to have a unified way to do so before doing it for wasm flags.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 9040438 [details] [diff] [review]
1.patch

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

I like the idea of having WASM_CODEGEN_DEBUG defined in debug builds (so that it doesn't bitrot) but I don't see where that happens in this patch.

::: js/src/jit/JitOptions.h
@@ +77,5 @@
>  #endif
> +#ifdef WASM_CODEGEN_DEBUG
> +  bool enableWasmJitExit;
> +  bool enableWasmJitEntry;
> +  bool enableWasmIonInlining;

nit: "inlining" makes me think of something we're not doing; how about "enableWasmFastJitEntry" or something?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.patch

Oops, forgot to fold the moz.configure patch.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 9040479 [details] [diff] [review]
1.patch

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

(Note also the "enableWasmFastJitEntry" suggestion in comment 2.)

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #2)

Comment on attachment 9040438 [details] [diff] [review]
1.patch

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

I like the idea of having WASM_CODEGEN_DEBUG defined in debug builds (so
that it doesn't bitrot) but I don't see where that happens in this patch.

::: js/src/jit/JitOptions.h
@@ +77,5 @@

#endif
+#ifdef WASM_CODEGEN_DEBUG

• bool enableWasmJitExit;
• bool enableWasmJitEntry;
• bool enableWasmIonInlining;

nit: "inlining" makes me think of something we're not doing; how about
"enableWasmFastJitEntry" or something?

I can understand this is misstating that it's inlining wasm into ion, which is not the case indeed. Technically, we're inlining the entry into the Ion caller, so fast jit entry sounds awkward to describe this. Maybe enableWasmIonFastCalls? (the few trials I've done with inline in the name make it sound confusing)

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.spew.patch

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1523993: Add JitOptions to disable wasm fast paths; r=luke

This adds a new configure option --enable-wasm-codegen-debug, which defaults
to true in debug builds (disable otherwise).

It's a first type so as to be able to use this in both the shell and browser,
using the env variables JIT_OPTION_{JitOption field name}.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1523993: Spew debug information for wasm calls; r=luke

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/454d1b05007a
Add JitOptions to disable wasm fast paths; r=luke
https://hg.mozilla.org/integration/autoland/rev/e8a0e28d1443
Spew debug information for wasm calls; r=luke

Comment 10

[Cristian Brindusan [:cbrindusan]]

Backed out 2 changesets (Bug 1523993) for build bustages on WasmStubs.cpp.

Backout: https://hg.mozilla.org/integration/autoland/rev/cb8a8117064b6bc7d142604948e3c8a20f74f612

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=e8a0e28d1443a7f423418b7aea637ce4cc8f4818&selectedJob=226542134

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226542134&repo=autoland&lineNumber=10440

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a66a76627b3
Add JitOptions to disable wasm fast paths; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/de9bc20a8ed5
Spew debug information for wasm calls; r=luke

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the backout; Maybe<> arguments have to be const&.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d7ef0c0e5d
Expose AddressOf in WasmBuiltins.h; r=bustage

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1986c84c8302
Add js/Printf.h to WasmStubs.cpp; r=bustage

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/6a66a76627b3
https://hg.mozilla.org/mozilla-central/rev/de9bc20a8ed5
https://hg.mozilla.org/mozilla-central/rev/b9d7ef0c0e5d
https://hg.mozilla.org/mozilla-central/rev/1986c84c8302