Closed Bug 1362084 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: yury, Assigned: yury)

References

Details

Attachments

(1 file)

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.
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: nobody → ydelendik
Blocks: 1364535
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 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 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+
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f25a37d7822e
Add binary source and JS source maps support. r=luke
https://hg.mozilla.org/mozilla-central/rev/f25a37d7822e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.