1447591 - wasm:: Investigate removing wasm::BinaryToText

Status: RESOLVED FIXED

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

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: firezemissiles.patch

Pros:
- less code to maintain
- duplication of intent with debugger.html: debugger.html embeds its own wasm->text converter, which is faster than ours (iirc), and it's now used by default (?) in the devtools.
- most test cases found by binary fuzzers could be translated with binaryen or wabt.

Cons:
- our binary-to-text translator is more resilient when there are invalid sections in binary fuzzers test cases. This kind of feature could be added to binaryen/wabt instead.
- Spidermonkey embedders should now also embed debugger.html for debugging wasm, instead of using the plain Debugger API. I don't know how difficult this is. Embedding has never quite been a priority, and debugging wasm in embedders might be an extremely rare use case, if existent at all.

Let's try a simple try build and see if things break when manually using the devtools.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=516520f6c428d533ceaa35d9903462c8e80aef3d

Comment 1

[Nicolas B. Pierron [:nbp]]

Jason can you confirm the affirmation from Benjmamin?

Comment 2

[Jason Laster [:jlast]]

This is a better question for yury

Comment 3

[Yury Delendik (:yury)]

We need to make this branch unreachable when createText is calld. I can find two e.g. at https://dxr.mozilla.org/mozilla-central/rev/ce7072c02389db590c487ca5863b7f00ce22336a/js/src/vm/Debugger.cpp#7020 and one at ensureSourceMap. All of then rely on maybeBytecode_ exist.

Let us make maybeBytecode null before https://dxr.mozilla.org/mozilla-central/rev/ce7072c02389db590c487ca5863b7f00ce22336a/js/src/wasm/WasmModule.cpp#1300 when binarySource is not set, in addition to MOZ_CRASH.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.stub-debugger-apis.patch

This patch stubs out a few Debugger API functions for wasm, emitting a warning when they're used suggesting to use debugger.html instead.

I'll try-run this patch and make sure that none of these functions are actually used in a regular build with MOZ_CRASH stubbing.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.fire-ze-missilez.patch

This patch removes wasm::BinaryToText and friends.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Two patches combined:
24 files changed, 148 insertions(+), 5642 deletions(-)

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8986492 [details] [diff] [review]
2.fire-ze-missilez.patch

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

You rock, thanks!

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8986492 [details] [diff] [review]
2.fire-ze-missilez.patch

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

::: js/src/wasm/WasmBinaryToAST.cpp
@@ -1938,5 @@
> -
> -    if (c.d.currentPosition() != bodyEnd)
> -        return c.d.fail("function body length mismatch");
> -
> -    size_t sigIndex = c.env().funcIndexToSigIndex(funcIndex);

I think you can remove ModuleEnvironment::funcIndexToSigIndex() now.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8986491 [details] [diff] [review]
1.stub-debugger-apis.patch

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

Yeah, this patch is wrong.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.debugger-api.patch

So maybe this patch is more correct: it keeps all the existing behavior when binarySource_ is true, and just returns early when it's false. I don't clearly understand what binarySource_ means, but your comment on the other bug seems to suggest this is the right thing.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

The try build for this last patch still shows test errors I don't comprehend. Yury, would you be so kind to investigate these, since you've written all this code in the first place, and there seems to be many implicit knowledge that I don't have, please?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7484656175c6c5b1d915b46fd46cbc89611c42da

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.debugger-api.patch

Comment 13

[Yury Delendik (:yury)]

Comment on attachment 8987814 [details] [diff] [review]
1.debugger-api.patch

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

We probably need to remove wasm-05.js -- it is no longer useful.

Looks good with the changes.

::: js/src/jit-test/tests/debug/wasm-07.js
@@ -30,5 @@
> -      assertEq(offsets.length, 5);
> -      offsets.forEach(offset => {
> -          var loc = wasmScript.getOffsetLocation(offset);
> -          assertEq(loc.isEntryPoint, true);
> -          assertEq(loc.lineNumber > 0, true);

we probable want to keep two asserts above, in binary mode lineNumber == offset.

::: js/src/jit-test/tests/debug/wasm-getAllColumnOffsets.js
@@ -28,5 @@
>  )');
>  
> -// There shall be total 8 lines with single and unique offset per line.
> -var usedOffsets = Object.create(null), usedLines = Object.create(null);
> -assertEq(offsets1.length, 8);

We probably want to turn binary mode to test getAllColumnOffsets()

::: js/src/wasm/WasmDebug.cpp
@@ +52,5 @@
>      "Restart with developer tools open to view WebAssembly source";
>  
> +const char useDebuggerHtml[] =
> +    "The Debugger API doesn't generate the wasm text format representation; "
> +    "use Debugger.html for this.";

People might not know what is debugger.html. Something like "configure the debugger to display WebAssembly bytecode"?

::: js/src/wasm/WasmTextToBinary.cpp
@@ +5558,5 @@
>      if (!EncodeLocalEntries(e, varTypes))
>          return false;
>  
>      for (AstExpr* expr : func.body()) {
> +        fprintf(stderr, "offset=%zu\n", e.currentOffset());

Unneeded logging

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5347dee1f2
Stub out a few Debugger APIs for wasm; r=yury
https://hg.mozilla.org/integration/mozilla-inbound/rev/0800fdb509d2
Remove wasm::BinaryToText; r=luke

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks Yury for the help debugging tests and for the review! I ended up enabling binary source mode in all the wasm tests by putting it into wasmRunWithDebugger (and tests still passed!).

Comment 16

[Cristina Coroiu [:ccoroiu]]

Backed out 2 changesets (bug 1447591) for mochitest failures at dom/base/test/test_postMessages.html

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f36433f9bd35fca2ab2041d265b717652fc8ca2

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0800fdb509d2025d309ab23e67c305705d52282f

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185111530&repo=mozilla-inbound&lineNumber=7413

[task 2018-06-27T09:52:48.899Z] 09:52:48     INFO - TEST-PASS | dom/base/test/test_postMessages.html | WebAssembly.compile function should exist 
[task 2018-06-27T09:52:48.900Z] 09:52:48     INFO - Buffered messages finished
[task 2018-06-27T09:52:48.901Z] 09:52:48     INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_postMessages.html | Test timed out. 
[task 2018-06-27T09:52:48.901Z] 09:52:48     INFO -     reportError@SimpleTest/TestRunner.js:121:7
[task 2018-06-27T09:52:48.902Z] 09:52:48     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
[task 2018-06-27T09:52:48.902Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.903Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.903Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.904Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.904Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.904Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.904Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.905Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.905Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.905Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.906Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.906Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.906Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.906Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.907Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.907Z] 09:52:48     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-27T09:52:48.907Z] 09:52:48     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:380:5
[task 2018-06-27T09:52:48.907Z] 09:52:48     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
[task 2018-06-27T09:52:48.907Z] 09:52:48     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
[task 2018-06-27T09:52:48.908Z] 09:52:48     INFO -     hookupTests@SimpleTest/setup.js:266:5
[task 2018-06-27T09:52:48.908Z] 09:52:48     INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
[task 2018-06-27T09:52:48.909Z] 09:52:48     INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
[task 2018-06-27T09:52:48.909Z] 09:52:48     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Ha crap, I changed the second parameter of wasm::TextToBinary so it must be a boolean (indicating whether we want to generate binary offsets, to test the debugger's behavior in binary mode), and one mochitest was passing the string "new-format" to this function. Let's see if this fixes it...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d97ee6a627e66a4904361bb440cdc1bdf2ee616c

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

That was it! Opened bug 1471839 for the fact that the error materialized as a timeout while it was a JS error.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38d2f921a918
Stub out a few Debugger APIs for wasm; r=yury
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a694ff1004
Remove wasm::BinaryToText; r=luke

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/38d2f921a918
https://hg.mozilla.org/mozilla-central/rev/e7a694ff1004