Closed
Bug 1447591
Opened 7 years ago
Closed 6 years ago
wasm:: Investigate removing wasm::BinaryToText
Categories
(Core :: JavaScript: WebAssembly, defect, P3)
Core
JavaScript: WebAssembly
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(2 files, 3 obsolete files)
160.33 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
61.25 KB,
patch
|
yury
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Jason can you confirm the affirmation from Benjmamin?
Flags: needinfo?(jlaster)
Priority: -- → P3
Comment 2•7 years ago
|
||
This is a better question for yury
Flags: needinfo?(jlaster) → needinfo?(ydelendik)
Comment 3•6 years ago
|
||
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.
Flags: needinfo?(ydelendik)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Component: JavaScript Engine: JIT → Javascript: Web Assembly
Assignee | ||
Comment 4•6 years ago
|
||
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.
Attachment #8960900 -
Attachment is obsolete: true
Attachment #8986491 -
Flags: review?(ydelendik)
Assignee | ||
Comment 5•6 years ago
|
||
This patch removes wasm::BinaryToText and friends.
Attachment #8986492 -
Flags: review?(luke)
Assignee | ||
Comment 6•6 years ago
|
||
Two patches combined: 24 files changed, 148 insertions(+), 5642 deletions(-)
![]() |
||
Comment 7•6 years ago
|
||
Comment on attachment 8986492 [details] [diff] [review] 2.fire-ze-missilez.patch Review of attachment 8986492 [details] [diff] [review]: ----------------------------------------------------------------- You rock, thanks!
Attachment #8986492 -
Flags: review?(luke) → review+
![]() |
||
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 8986491 [details] [diff] [review] 1.stub-debugger-apis.patch Review of attachment 8986491 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this patch is wrong.
Attachment #8986491 -
Flags: review?(ydelendik)
Assignee | ||
Comment 10•6 years ago
|
||
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.
Attachment #8986491 -
Attachment is obsolete: true
Attachment #8987121 -
Flags: review?(ydelendik)
Assignee | ||
Comment 11•6 years ago
|
||
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
Flags: needinfo?(ydelendik)
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8987121 -
Attachment is obsolete: true
Attachment #8987121 -
Flags: review?(ydelendik)
Attachment #8987814 -
Flags: review?(ydelendik)
Comment 13•6 years ago
|
||
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
Attachment #8987814 -
Flags: review?(ydelendik) → review+
Comment 14•6 years ago
|
||
Pushed by bbouvier@mozilla.com: 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
Assignee | ||
Comment 15•6 years ago
|
||
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!).
Flags: needinfo?(ydelendik)
Comment 16•6 years ago
|
||
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
Flags: needinfo?(bbouvier)
Assignee | ||
Comment 17•6 years ago
|
||
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
Flags: needinfo?(bbouvier)
Assignee | ||
Comment 18•6 years ago
|
||
That was it! Opened bug 1471839 for the fact that the error materialized as a timeout while it was a JS error.
Comment 19•6 years ago
|
||
Pushed by bbouvier@mozilla.com: 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/38d2f921a918 https://hg.mozilla.org/mozilla-central/rev/e7a694ff1004
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
Updated•6 years ago
|
status-firefox62:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•