1311994 - Baldr: update binary format to 0xd

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Luke Wagner [:luke]]

Attachment: type-renumber

Comment 2

[Luke Wagner [:luke]]

Attachment: op-renumber

I also took the opportunity to further purge Expr::CallImport.

Comment 3

[Luke Wagner [:luke]]

Attachment: add-imm

Unfortunately, current_memory/grow_memory are no longer plain-vanilla nullary/binary nodes so I had to add separate AST nodes which is what most of the patch is about.

Also, the baseline was being trixie and using the call decoding path to decode these two ops so I had to look at what the call path was doing and inline the relevant parts so baseline could use the same ExprIter::read(CurrentMemory|GrowMemory) which knows to decode the immediate.

Comment 4

[Dan Gohman [:sunfish]]

Comment on attachment 8803479 [details] [diff] [review]
type-renumber

Review of attachment 8803479 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmBinary.h
@@ +889,5 @@
> +        static_assert(size_t(TypeCode::Max) <= INT8_MAX, "fits");
> +        uint8_t u8;
> +        if (!readFixedU8(&u8))
> +            return false;
> +        *type = (ExprType)u8;

This cast has undefined behavior in C++ if the u8 value is not one of the defined ExprType values. A practical consequence could be that a subsequent switch on the enum value may assume the enum value is valid and omit checks for invalid values.

It's pre-existing, but the cast to ValType in readValType above also has this problem.

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8803480 [details] [diff] [review]
op-renumber

Review of attachment 8803480 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 8803482 [details] [diff] [review]
add-imm

Review of attachment 8803482 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp
@@ +1350,5 @@
>  static bool
> +PrintCurrentMemory(WasmPrintContext& c, AstCurrentMemory& cm)
> +{
> +    const char* opStr = "current_memory";
> +    return c.buffer.append(opStr, strlen(opStr));

This could be simplified to just `c.buffer.append("grow_memory"), since WasmPrintBuffer has an append which handles string literals.
 
(As an aside, at a glance, that WasmPrintBuffer::append function should probably assert that the array is nul-terminated.)

@@ +1357,5 @@
> +static bool
> +PrintGrowMemory(WasmPrintContext& c, AstGrowMemory& gm)
> +{
> +    const char* opStr = "grow_memory";
> +    if (!c.buffer.append(opStr, strlen(opStr)))

ditto

::: js/src/asmjs/WasmBinaryToText.cpp
@@ +483,5 @@
>        return false;
>  
> +    const char* opStr = "current_memory";
> +    if (!c.buffer.append(opStr, strlen(opStr)))
> +        return false;

ditto

@@ +503,1 @@
>      if (!c.buffer.append(opStr, strlen(opStr)))

ditto

Comment 7

[Luke Wagner [:luke]]

Attachment: type-renumber

This patch uses an explicit representation type so that the casting is legal and we don't have to propagate the non-enum out to all callers.

Comment 8

[Luke Wagner [:luke]]

Attachment: rm-wasm

Also, when the demo is updated to not use Wasm.experimentalVersion, but instead detect WebAssembly and then use WebAssembly.validate to feature-detect, this patch removes Wasm.

Comment 9

[Dan Gohman [:sunfish]]

Comment on attachment 8803559 [details] [diff] [review]
type-renumber

Review of attachment 8803559 [details] [diff] [review]:
-----------------------------------------------------------------

Per discussion, there's an idea to use fixed-type and plain enums to use the type system to differentiate between values that are known to be a valid ValType etc. and values that aren't, but that can be done in a later change.

Comment 10

[Luke Wagner [:luke]]

Attachment: rm-warning

And this change removes the current warning we generate for when validation fails because otherwise we end up printing spurious console warnings for feature detection like we're doing in the 0xD demo.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8803956 [details] [diff] [review]
rm-warning

Review of attachment 8803956 [details] [diff] [review]:
-----------------------------------------------------------------

(sorry for the drive by review) It's a bit unfortunate to do so. Could we hide this behind a flag instead?

Comment 12

[Luke Wagner [:luke]]

Well I think the default needs to not print anything because failure doesn't mean anything is wrong.

Comment 13

[Dan Gohman [:sunfish]]

Comment on attachment 8803583 [details] [diff] [review]
rm-wasm

Review of attachment 8803583 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63994a896664
Baldr: update type codes to match 0xd (r=sunfish)
https://hg.mozilla.org/integration/mozilla-inbound/rev/25937c3220ee
Baldr: update op codes to match 0xd (r=sunfish)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd8ec5750d3
Baldr: add flags immediates to current_memory/grow_memory/call_indirect (r=sunfish)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e3bb5d0a8a9
Baldr: remove Wasm object (r=sunfish)

Comment 15

[Luke Wagner [:luke]]

Landing the necessary bits, happy to continue discussing what to do about the warning message.  I'm not sure a pref that's default off will be useful to many developers.  I think devs would have an easier time getting a failure message with `new WebAssembly.Module(m)`.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63994a896664
https://hg.mozilla.org/mozilla-central/rev/25937c3220ee
https://hg.mozilla.org/mozilla-central/rev/7fd8ec5750d3
https://hg.mozilla.org/mozilla-central/rev/3e3bb5d0a8a9

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

For feature testing, I agree that printing a message will be confusing; that being said, not having a useful hint in the general case makes the goal of WebAssembly.Validate a bit unclear. It doesn't help much that it returns false without saying anything; then, reverting to using WebAssembly.Module ctor to get the precise error message defeats the purpose of having WebAssembly.Validate in the first place.

Another random idea: at the spec level, WebAssembly.Validate could return a simple object with two fields:
- validated: boolean
- hint: string message
Then, the behavior is entirely controlled by the users; if they want only the validation, they can look at the `validated` boolean. If they want an explanation, they can print the `hint` string. The format of this hint would be non-deterministic, and implementations could, but wouldn't have to, implement it.

Maybe we could keep the warning, and have Emscripten generate a console.log() message before the feature testing, expliciting that warnings in the console are not meaningful at this point; then having another console.log() message after the feature testing?

Comment 18

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> For feature testing, I agree that printing a message will be confusing; that
> being said, not having a useful hint in the general case makes the goal of
> WebAssembly.Validate a bit unclear.

The purpose of WebAssembly.validate is twofold: it allows more efficient feature detection than "try to compile a module" (which generates a garbage module in the success case) and also it requires eager validation (wasm currently allows validation errors to be reported at runtime).  Thus the intention is *programmatic* use, where only the resulting boolean is required, not interactive or debugging use by developers.  In contrast, validation failure inside `new Module` is considered exceptional and thus a detailed error report is a good idea.

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 8803956 [details] [diff] [review]
rm-warning

Flipping over to Benjamin, based on previous discussion.

Comment 20

[Luke Wagner [:luke]]

Attachment: rm-warning

Oops, forgot to run tests.

Comment 21

[Luke Wagner [:luke]]

Attachment: rm-lingering-wasm-bits

Found and removed a few more stragglers.

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8804335 [details] [diff] [review]
rm-warning

Review of attachment 8804335 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation, I do agree now (especially considering that my proposal would have been most useful to wasm code generators implementers, and they can just use the WebAssembly.Module ctor temporarily instead, which gives them a more precise error).

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8804337 [details] [diff] [review]
rm-lingering-wasm-bits

Review of attachment 8804337 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/574e9598c471
Baldr: remove WebAssembly.validate warning (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb02e92724f5
Baldr: remove WebAssembly.experimentalVersion (r=bbouvier)

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/574e9598c471
https://hg.mozilla.org/mozilla-central/rev/eb02e92724f5