1274618 - [wasm] Replace WebAssembly text source view with pilot format

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Yury Delendik (:yury)]

We would like to introduce new WebAssembly text format that will replace test s-expr format. As a baseline let's choose proposal at https://github.com/sunfishcode/design/blob/text-syntax-strawman-proposal/TextFormat.md . The intention is to replace "View source" text with this format to receive users' feedback.

We will not have parsing equivalents to it yet and we will keep current wasmTextToBinary and wasmBinaryToText test functions without change for some time (we will keep supporting both text formats).

Comment 1

[Yury Delendik (:yury)]

Attachment: MozReview Request: Bug 1274618 - Fixes wasmBinaryToText for call expressions. r?sunfish

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

Comment 2

[Yury Delendik (:yury)]

Attachment: MozReview Request: Bug 1274618 - Fixes max memory limit check and removes used fields. r?sunfish

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

Comment 3

[Yury Delendik (:yury)]

Attachment: MozReview Request: Bug 1274618 - Generate labels for branch statements. r?sunfish

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

Comment 4

[Yury Delendik (:yury)]

Attachment: MozReview Request: Bug 1274618 - Experimental WASM text format. r?sunfish

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

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8755047 [details]
MozReview Request: Bug 1274618 - Fixes wasmBinaryToText for call expressions. r?sunfish

https://reviewboard.mozilla.org/r/54358/#review51060

Looks good!

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 8755048 [details]
MozReview Request: Bug 1274618 - Fixes max memory limit check and removes used fields. r?sunfish

https://reviewboard.mozilla.org/r/54360/#review51062

The memory size parts were fixed in a different bug, so that part isn't needed anymore, but the rest of the patch looks good.

Comment 7

[Dan Gohman [:sunfish]]

Comment on attachment 8755049 [details]
MozReview Request: Bug 1274618 - Generate labels for branch statements. r?sunfish

https://reviewboard.mozilla.org/r/54362/#review51068

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8755050 [details]
MozReview Request: Bug 1274618 - Experimental WASM text format. r?sunfish

https://reviewboard.mozilla.org/r/54364/#review51224

Overall, looks good. We'll surely be iterating on the syntax more, but this looks like a good start :).

::: js/src/asmjs/WasmBinaryToExperimentalText.h:41
(Diff revision 1)
> +    bool reduceParentesis:1;
> +    bool groupBlocks:1;
> +
> +    ExperimentalTextFormatting()
> +     : allowAsciiOperators(true),
> +       reduceParentesis(true),

I suggest spelling this `reduceParentheses` or `reduceParens`.

::: js/src/asmjs/WasmBinaryToExperimentalText.h:50
(Diff revision 1)
> +
> +// Translate the given binary representation of a wasm module into the module's textual
> +// representation.
> +
> +MOZ_MUST_USE bool
> +BinaryToExperimentalText(JSContext* cx, const uint8_t* bytes, size_t length, StringBuffer& buffer, const ExperimentalTextFormatting& formatting);

SpiderMonkey code is generally kept under 100 columns; it'd be good to break this one up.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:113
(Diff revision 1)
> +    if (num < 0 && !c.buffer.append("-"))
> +        return false;
> +    if (!num)
> +        return c.buffer.append("0");
> +
> +    int64_t abs = mozilla::Abs(num);

mozilla::Abs returns an unsigned type, which is important in the case of INT64_MIN. So abs and n should be uint64_t.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:124
(Diff revision 1)
> +    }
> +    pow /= 10;
> +
> +    n = abs;
> +    while (pow) {
> +        if (!c.buffer.append("0123456789"[n / pow]))

A simpler way to convert to a digit char is to do `'0' + n / pow`.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:196
(Diff revision 1)
> +                    return false;
> +            } else {
> +                char digit1 = byte / 16, digit2 = byte % 16;
> +                if (!c.buffer.append("\\"))
> +                    return false;
> +                if (!c.buffer.append((char)(digit1 < 10 ? digit1 + '0' : digit1 + 'a' - 10)))

Technically the >= 10 case should be `digit1 - 10 + 'a'` so that you subtract 10 first and avoid potential overflow. (It doesn't matter in ASCII of course, but it's more conventional to write the code this way).

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:210
(Diff revision 1)
> +}
> +
> +static bool
> +PrintExprType(WasmPrintContext& c, ExprType type)
> +{
> +    switch (type) {

Instead of having custom code here, this function could use WasmTypes.h' ToCString(ExprType) function, with just a special case for ExprType::Void.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:249
(Diff revision 1)
> +PrintExpr(WasmPrintContext& c, AstExpr& expr);
> +
> +static bool
> +PrintFullLine(WasmPrintContext& c, AstExpr& expr)
> +{
> +  if (!PrintIndent(c))

Inconsistent indentation.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:268
(Diff revision 1)
> +}
> +
> +static bool
> +PrintUnreachable(WasmPrintContext& c, AstUnreachable& unreachable)
> +{
> +    return c.buffer.append("trap");

I know that existing S-expression formatter calls it `trap`, but in the spec this instruction is named `unreachable` so we should call it that too :).

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:352
(Diff revision 1)
> +        break;
> +      case ExprType::I64:
> +        if (!PrintInt64(c, (uint32_t)cst.val().i64()))
> +            return false;
> +        if (!c.buffer.append("LL"))
> +            return false;

I'd like to propose we omit the "LL" suffix here. "LL" is a C-ism, and I think we can make parsers infer the type of the constant from context.

(Or if we can't, maybe we'll use "i64" as the suffix, to be more wasm-oriented :-)).

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:357
(Diff revision 1)
> +            return false;
> +        break;
> +      case ExprType::F32:
> +        if (!PrintDouble(c, (double)cst.val().f32()))
> +            return false;
> +        if (!c.buffer.append("f"))

Same here with the "f" suffix.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:365
(Diff revision 1)
> +      case ExprType::F64:
> +        if (!PrintDouble(c, cst.val().f64()))
> +            return false;
> +        break;
> +      default:
> +        return false;

This should MOZ_CRASH or so, rather than just returning false.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:419
(Diff revision 1)
> +    }
> +    return true;
> +}
> +
> +static bool
> +PrintGrouppedBlock(WasmPrintContext& c, AstBlock& block)

"Groupped" should be "Grouped"

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp:892
(Diff revision 1)
> +        return false;
> +
> +    if (lsa.offset() != 0) {
> +      if (!c.buffer.append(","))
> +          return false;
> +      if (!PrintInt32(c, lsa.offset()))

My proposal has an explicit leading '+' for non-negative offsets, which is technically redundant, but it's a slight visual reminder for people reading the code what.

Comment 9

[Yury Delendik (:yury)]

Comment on attachment 8755047 [details]
MozReview Request: Bug 1274618 - Fixes wasmBinaryToText for call expressions. r?sunfish

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

Comment 10

[Yury Delendik (:yury)]

Comment on attachment 8755048 [details]
MozReview Request: Bug 1274618 - Fixes max memory limit check and removes used fields. r?sunfish

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

Comment 11

[Yury Delendik (:yury)]

Comment on attachment 8755049 [details]
MozReview Request: Bug 1274618 - Generate labels for branch statements. r?sunfish

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

Comment 12

[Yury Delendik (:yury)]

Comment on attachment 8755050 [details]
MozReview Request: Bug 1274618 - Experimental WASM text format. r?sunfish

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

Comment 13

[Yury Delendik (:yury)]

https://reviewboard.mozilla.org/r/54364/#review51224

> I suggest spelling this `reduceParentheses` or `reduceParens`.

Renamed to `reduceParens`.

> SpiderMonkey code is generally kept under 100 columns; it'd be good to break this one up.

Fixed

> mozilla::Abs returns an unsigned type, which is important in the case of INT64_MIN. So abs and n should be uint64_t.

Fixed

> A simpler way to convert to a digit char is to do `'0' + n / pow`.

Fixed

> Technically the >= 10 case should be `digit1 - 10 + 'a'` so that you subtract 10 first and avoid potential overflow. (It doesn't matter in ASCII of course, but it's more conventional to write the code this way).

Fixed

> Instead of having custom code here, this function could use WasmTypes.h' ToCString(ExprType) function, with just a special case for ExprType::Void.

We need UTF16 chars (ToCString returns char pointer). I'm keeping it as is.

> Inconsistent indentation.

Fixed

> I know that existing S-expression formatter calls it `trap`, but in the spec this instruction is named `unreachable` so we should call it that too :).

Changed to `unreachable`.

> I'd like to propose we omit the "LL" suffix here. "LL" is a C-ism, and I think we can make parsers infer the type of the constant from context.
> 
> (Or if we can't, maybe we'll use "i64" as the suffix, to be more wasm-oriented :-)).

Removed suffixes (will check if we cannot infer types later)

> This should MOZ_CRASH or so, rather than just returning false.

Added MOZ_CRASH for default statement.

> "Groupped" should be "Grouped"

Fixed

> My proposal has an explicit leading '+' for non-negative offsets, which is technically redundant, but it's a slight visual reminder for people reading the code what.

Added third argument for the PintInt32 to force sign.

Comment 14

[Yury Delendik (:yury)]

Comment on attachment 8755050 [details]
MozReview Request: Bug 1274618 - Experimental WASM text format. r?sunfish

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

Comment 15

[Yury Delendik (:yury)]

Returning back prefixes: we cannot infer types if values are dropped. See https://reviewboard.mozilla.org/r/54364/diff/1-3/ for better delta.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afb397c1fd17
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb2075e607e
https://hg.mozilla.org/integration/mozilla-inbound/rev/02e1f4b56cfc
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe20266c00e

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afb397c1fd17
https://hg.mozilla.org/mozilla-central/rev/5fb2075e607e
https://hg.mozilla.org/mozilla-central/rev/02e1f4b56cfc
https://hg.mozilla.org/mozilla-central/rev/ffe20266c00e