[wasm] wasm binary code is available from Debugger.Source

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: yury, Assigned: yury)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
Currently, we need to generate wast to display content of the wasm binary. The devtools can be changed to generate wast more efficiently on their side if we will give them wasm bytecode: less data to store at engine side or transmit to the devtools, a viewer/editor does not need to re-parse generated wast to highlight, easier to switch between wasm text formats (wast or experimental), access to binary offsets (rather than generated lines numbers), etc.

We already introduced ability to return generated text by default with Debugger.Source and devtools might take time to modify. I recommend adding Debugger::allowWasmBinarySource property to disable text generation at Debugger.Source and just return binary array. Also there is a "generated line number to bytecode offset"-mechanism at wasm::Code/DebugState that needs to be modified to directly map line number value to offset. The allowWasmBinarySource property will also help with devtools migration.

The side effect is that JavaScript source maps will just start working in binary source mode.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 3

2 years ago
PR for source maps spec at https://github.com/WebAssembly/design/pull/1051.

TODO check how easy it will be to map between columns to lines -- the spec above selects columns as bytecode offset, but Debugger API selects line. Shall we change that in this patch too?
Assignee

Updated

2 years ago
Assignee: nobody → ydelendik
Assignee

Updated

2 years ago
Blocks: 1364535

Comment 4

2 years ago
mozreview-review
Comment on attachment 8864563 [details]
Bug 1362084 - Add binary source and JS source maps support.

https://reviewboard.mozilla.org/r/136228/#review142208

Makes sense.

::: js/src/vm/Debugger.cpp:7341
(Diff revision 2)
>          return ss->substring(cx_, 0, ss->length());
>      }
>  
>      ReturnType match(Handle<WasmInstanceObject*> wasmInstance) {
> +        if (wasmInstance->instance().debug().maybeBytecode() &&
> +            wasmInstance->instance().debug().binarySource()) {

nit: { on new line

::: js/src/wasm/WasmCode.h:322
(Diff revision 2)
>      MemoryAccessVector    memoryAccesses;
>      CodeRangeVector       codeRanges;
>      CallSiteVector        callSites;
>      NameInBytecodeVector  funcNames;
>      CustomSectionVector   customSections;
> +    UniqueTwoByteChars    sourceMappingURL;

I think sourceMappingURL should probably be serialized/deserialized.

::: js/src/wasm/WasmValidate.cpp:1664
(Diff revision 2)
> +
> +    UniqueTwoByteChars name(js_pod_malloc<char16_t>(nchars + 1));
> +    if (!name)
> +        return DecodeSectionResult::Failed;
> +    name.get()[nchars] = 0;
> +    CopyAndInflateChars(name.get(), chars, nchars);

Probably this URL is utf8 encoded, not latin1, so can you use UTF8CharsToNewTwoByteCharsZ instead?

::: js/src/wasm/WasmValidate.cpp:1682
(Diff revision 2)
>  
>      if (!DecodeNameSection(d, env))
>          return false;
>  
>      while (!d.done()) {
> +        switch (MaybeDecodeSourceMappingURLSection(d, env)) {

Could decoding of the source mapping custom section be structured the same as the name section?  So that means a DecodeSourceMappingURLSection, outside the loop, DecodeSourceMappingURLSection calling a MaybeDecodeSourceMappingURLSectionBody, etc.
Comment hidden (mozreview-request)
Assignee

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8864563 [details]
Bug 1362084 - Add binary source and JS source maps support.

https://reviewboard.mozilla.org/r/136228/#review142208

> I think sourceMappingURL should probably be serialized/deserialized.

sourceMappingURL was removed in favor of wasm::DebugState::getSourceMappingURL

> Probably this URL is utf8 encoded, not latin1, so can you use UTF8CharsToNewTwoByteCharsZ instead?

Using JS_NewStringCopyUTF8N in the wasm::DebugState::getSourceMappingURL

> Could decoding of the source mapping custom section be structured the same as the name section?  So that means a DecodeSourceMappingURLSection, outside the loop, DecodeSourceMappingURLSection calling a MaybeDecodeSourceMappingURLSectionBody, etc.

This code was removed in favor of wasm::DebugState::getSourceMappingURL

Comment 7

2 years ago
mozreview-review
Comment on attachment 8864563 [details]
Bug 1362084 - Add binary source and JS source maps support.

https://reviewboard.mozilla.org/r/136228/#review143532

Nice change!

::: js/src/wasm/WasmDebug.cpp:590
(Diff revision 3)
> +DebugState::getSourceMappingURL(JSContext* cx, MutableHandleString result) const
> +{
> +    result.set(nullptr);
> +    if (!maybeBytecode_) {
> +        return true;
> +    }

nit: no { } around single-statement then branch
Attachment #8864563 - Flags: review?(luke) → review+
Comment hidden (mozreview-request)

Comment 9

2 years ago
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f25a37d7822e
Add binary source and JS source maps support. r=luke

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f25a37d7822e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.