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

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine: JIT
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: yury, Assigned: yury)

Tracking

(Blocks: 1 bug)

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8769719 [details]
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/
(Assignee)

Comment 2

2 years ago
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.
Blocks: 1188259
(Assignee)

Comment 3

2 years ago
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/
Attachment #8769719 - Flags: review?(luke)

Comment 4

2 years ago
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.
Attachment #8769719 - Flags: review?(luke) → review+
(Assignee)

Comment 5

2 years ago
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/
Attachment #8769719 - Flags: review?(shu)
(Assignee)

Updated

2 years ago
See Also: → bug 1286948

Updated

2 years ago
Priority: -- → P2

Comment 6

2 years ago
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.
Attachment #8769719 - Flags: review?(shu) → review+
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Comment 8

2 years ago
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/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37cf5b8d779f
Add a map of generated WebAssembly source to its binary format. r=luke, r=shu
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37cf5b8d779f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Updated

2 years ago
See Also: → bug 1308510
You need to log in before you can comment on or make changes to this bug.