1271375 - [wasm] Parse binary format into AST tree

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Yury Delendik (:yury)]

This will also restore wasmBinaryToText functionality  (bug 1259295 disabled it) to complete the use case and provide testing.

The WasmTextToBinary.cpp contains AST data structures that can be refactored into separate file/header and used for both processes: text-to-binary and binary-to-text. The latter will evolve in the future and will require to have in-memory representation (in comparison with the current implementation, Bug 1254984)

Comment 1

[Yury Delendik (:yury)]

Attachment: MozReview Request: Bug 1271375 - Extracts WASM AST and fixes wasmBinaryToText. r?luke

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

Comment 2

[Yury Delendik (:yury)]

Comment on attachment 8750405 [details]
MozReview Request: Bug 1271375 - Extracts WASM AST and fixes wasmBinaryToText. r?luke

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

Comment 3

[Yury Delendik (:yury)]

Comment on attachment 8750405 [details]
MozReview Request: Bug 1271375 - Extracts WASM AST and fixes wasmBinaryToText. r?luke

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

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8750405 [details]
MozReview Request: Bug 1271375 - Extracts WASM AST and fixes wasmBinaryToText. r?luke

https://reviewboard.mozilla.org/r/51397/#review49159

Fantastic!  Thanks for doing this.

::: js/src/asmjs/WasmAST.h:45
(Diff revision 3)
> +using WasmAstVector = mozilla::Vector<T, 0, LifoAllocPolicy<Fallible>>;
> +
> +template <class K, class V, class HP>
> +using WasmAstHashMap = HashMap<K, V, HP, LifoAllocPolicy<Fallible>>;
> +
> +class WasmName

The motivation for prefixing everything with Wasm- was to avoid name clashes in unified builds.  However, instead, we could use Ast- as the prefix which you've already started doing in WasmBinaryToAST.  One character shorter, and it avoids the logical redundancy of wasm::Wasm-.  Since you're touching all this code anyways, could you just fold in this rename from Wasm- to Ast-?  (Also, some types like WasmAstVector would simply become AstVector.)

::: js/src/asmjs/WasmBinaryToAST.cpp:60
(Diff revision 3)
> +    typedef AstDecodeStackItem Value;
> +};
> +
> +typedef ExprIter<AstDecodePolicy> AstDecodeExprIter;
> +
> +class AstDecodeContext {

{ on new line

::: js/src/asmjs/WasmBinaryToText.cpp:176
(Diff revision 3)
>  {
>      return RenderExprType(c, ToExprType(type));
>  }
>  
>  static bool
> -RenderExpr(WasmRenderContext& c);
> +RenderName(WasmRenderContext& c, const WasmName& name) {

{ on new line (here and below)

::: js/src/asmjs/WasmBinaryToText.cpp:1343
(Diff revision 3)
> +    if (!BinaryToAst(cx, bytes, length, lifo, &module))
>          return false;
>  
> +    WasmRenderContext c(cx, module, buffer);
> +
>      // FIXME: Implement binary-to-text and re-enable this.

You can remove this comment now.

Comment 5

[Yury Delendik (:yury)]

Comment on attachment 8750405 [details]
MozReview Request: Bug 1271375 - Extracts WASM AST and fixes wasmBinaryToText. r?luke

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

Comment 6

[Yury Delendik (:yury)]

https://reviewboard.mozilla.org/r/51397/#review49159

> The motivation for prefixing everything with Wasm- was to avoid name clashes in unified builds.  However, instead, we could use Ast- as the prefix which you've already started doing in WasmBinaryToAST.  One character shorter, and it avoids the logical redundancy of wasm::Wasm-.  Since you're touching all this code anyways, could you just fold in this rename from Wasm- to Ast-?  (Also, some types like WasmAstVector would simply become AstVector.)

Made such replacements in the following files: WasmAST.h, WasmBinaryToAST.*, WasmBinaryTextToBinary.cpp and WasmBinaryToText.cpp

Comment 7

[Luke Wagner [:luke]]

I can rebase and land

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad3cdb2bd05

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1ad3cdb2bd05

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

this reduced the number of constructors from build metrics :)

https://treeherder.mozilla.org/perf.html#/alerts?id=1219