1285976 - [wasm] Add view source map to binary offsets for generated WebAssembly text format

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Yury Delendik (:yury)]

Currently we can view generated WebAssembly module source. For debugger we need to add way to resolve line numbers of the generate source to the offset (e.g. in the binary wasm format). This offset can be further used to set/resolve the breakpoint location or navigate to the generate source using binary file offset.

Comment 1

[Yury Delendik (:yury)]

Attachment: Bug 1285976 - Adds map of generated WebAssembly source to its binary format.

Review commit: https://reviewboard.mozilla.org/r/63470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63470/

Comment 2

[Yury Delendik (:yury)]

It will be nice if we will have offset for end-of-functions (see https://github.com/WebAssembly/design/pull/666#issuecomment-231748825) - this could help us to use binary file offsets for function epilogues.

Comment 3

[Yury Delendik (:yury)]

Comment on attachment 8769719 [details]
Bug 1285976 - Adds map of generated WebAssembly source to its binary format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63470/diff/1-2/

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8769719 [details]
Bug 1285976 - Adds map of generated WebAssembly source to its binary format.

https://reviewboard.mozilla.org/r/63470/#review60816

Looks good, just nits from me, but could you get Shu to review the Debugger.cpp changes?

::: js/src/asmjs/WasmBinaryToAST.cpp:223
(Diff revision 2)
>  
>  static bool
>  AstDecodeExpr(AstDecodeContext& c);
>  
>  static bool
> -AstDecodeCall(AstDecodeContext& c)
> +AstDecodeCall(AstDecodeContext& c, uint32_t exprOffset)

Instead of pushing all the exprOffsets and the setOffset() calls into each of these functions, could you have AstDecodeExpr() collect the AstNode*s from each of the cases and then have one setOffset() call at the end of the switch?

::: js/src/asmjs/WasmBinaryToExperimentalText.h:49
(Diff revision 2)
>      {}
>  };
>  
> +// The generated source location for the AST node/expression. The offset field refers
> +// an offset in an binary format file.
> +struct WasmExprLoc

For these three structs, could you avoid the Wasm prefix here inside namespace wasm?  If that leads to unified-build conflicts, then perhaps we could find different meaningful names.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:63
(Diff revision 2)
>      GroupPrecedence = 16,
>  };
>  
> +// StringBuffer wrapper to track the position (line and column) within the generated
> +// source.
> +class WasmPrintBuffer {

{ on new line

::: js/src/asmjs/WasmInstance.h:152
(Diff revision 2)
>  
>      void deoptimizeImportExit(uint32_t importIndex);
>  
> +    // Text format support:
> +
> +    bool getLineOffsets(size_t lineno, Vector<uint32_t, 0, SystemAllocPolicy>& offsets);

Uint32Vector.  Also, could you put this function right below createText() and update the comment to start with "Text format support functions."?

::: js/src/asmjs/WasmInstance.cpp:842
(Diff revision 2)
>      ImportExit& exit = importToExit(import);
>      exit.code = codeSegment_->code() + import.interpExitCodeOffset();
>      exit.baselineScript = nullptr;
>  }
>  
> +struct WasmSourceMapLinenoComparator

No need for Wasm prefix in local file.  Perhaps put this in an unnamed namespace, though.

::: js/src/asmjs/WasmInstance.cpp:852
(Diff revision 2)
> +    explicit WasmSourceMapLinenoComparator(uint32_t lineno_) : lineno(lineno_) {}
> +    const uint32_t lineno;
> +};
> +
> +bool
> +Instance::getLineOffsets(size_t lineno, Vector<uint32_t, 0, SystemAllocPolicy>& offsets)

Can you move this definition to be right under Instance::createText()?

::: js/src/asmjs/WasmInstance.cpp:860
(Diff revision 2)
> +    if (!maybeSourceMap_)
> +        return false;
> +
> +    if (lineno < experimentalWarningLinesCount)
> +        return true;
> +    lineno -= experimentalWarningLinesCount;

lol.... still worth it though :)

::: js/src/asmjs/WasmInstance.cpp:866
(Diff revision 2)
> +
> +    // Binary search for the expression with the specified line number and
> +    // rewind to the first expression, if more than one expression on the same line.
> +    size_t match;
> +    if (!BinarySearchIf(maybeSourceMap_->exprlocs(),
> +                        0, maybeSourceMap_->exprlocs().length(),

Can you hoist a single 'exprlocs' reference that can be used here and below?  That should hopefully make these statements fit on one line.

Comment 5

[Yury Delendik (:yury)]

Comment on attachment 8769719 [details]
Bug 1285976 - Adds map of generated WebAssembly source to its binary format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63470/diff/2-3/

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8769719 [details]
Bug 1285976 - Adds map of generated WebAssembly source to its binary format.

https://reviewboard.mozilla.org/r/63470/#review62256

Debugger parts lgtm.

::: js/src/vm/Debugger.cpp:5996
(Diff revision 3)
>  
>      args.rval().setObject(*result);
>      return true;
>  }
>  
> -static bool
> +class DebuggerScriptGetLineOffsetsMatcher {

Nit: { on new line

::: js/src/vm/Debugger.cpp:6041
(Diff revision 3)
>  
> +    ReturnType match(Handle<WasmInstanceObject*> instance) {
> +        if (!result_)
> +            return false;
> +
> +        Vector<uint32_t, 0, SystemAllocPolicy> offsets;

You have a JSContext, so you can just use Vector<uint32_t> instead of SystemAllocPolicy.

Comment 7

[Yury Delendik (:yury)]

Comment on attachment 8769719 [details]
Bug 1285976 - Adds map of generated WebAssembly source to its binary format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63470/diff/3-4/

Comment 8

[Yury Delendik (:yury)]

Comment on attachment 8769719 [details]
Bug 1285976 - Adds map of generated WebAssembly source to its binary format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63470/diff/4-5/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37cf5b8d779f
Add a map of generated WebAssembly source to its binary format. r=luke, r=shu

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37cf5b8d779f