Closed Bug 1274618 Opened 4 years ago Closed 4 years ago

[wasm] Replace WebAssembly text source view with pilot format

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: yury, Assigned: yury)

References

Details

Attachments

(4 files)

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 on attachment 8755047 [details]
MozReview Request: Bug 1274618 - Fixes wasmBinaryToText for call expressions. r?sunfish

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

Looks good!
Attachment #8755047 - Flags: review?(sunfish) → review+
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.
Attachment #8755048 - Flags: review?(sunfish) → review+
Comment on attachment 8755049 [details]
MozReview Request: Bug 1274618 - Generate labels for branch statements. r?sunfish

https://reviewboard.mozilla.org/r/54362/#review51068
Attachment #8755049 - Flags: review?(sunfish) → review+
See Also: → 1254984
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.
Attachment #8755050 - Flags: review?(sunfish) → review+
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 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 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 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/
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 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/
Returning back prefixes: we cannot infer types if values are dropped. See https://reviewboard.mozilla.org/r/54364/diff/1-3/ for better delta.
Keywords: checkin-needed
Blocks: 1277063
Blocks: 1280471
See Also: → 1285976
You need to log in before you can comment on or make changes to this bug.