Bug 164724 – WebAssembly: update binary format to 0xD version

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

We're currently at 0xC.

[JF Bastien]

Created attachment 295701 [details]
patch

[JF Bastien]

Created attachment 295702 [details]
Helper script to synchronize BinaryFormat.md and wasm.json

[WebKit Commit Bot]

This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".

[Saam Barati]

Comment on attachment 295701 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295701&action=review

> Source/JavaScriptCore/wasm/wasm.json:10
> +        "i32":     { "type": "varint7", "value":  -1, "b3type": "B3::Int32" },

What's "varint7"?

[JF Bastien]

(In reply to comment #4)
> Comment on attachment 295701 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295701&action=review
> 
> > Source/JavaScriptCore/wasm/wasm.json:10
> > +        "i32":     { "type": "varint7", "value":  -1, "b3type": "B3::Int32" },
> 
> What's "varint7"?

It's a signed LEB128 variable length integer, encoded in at most a byte (so 7 useful bits of encoding):

  https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#varintn

[Saam Barati]

Comment on attachment 295701 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295701&action=review

r=me

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:468
> +    case Wasm::Void:
> +    case Wasm::Func:
> +    case Wasm::Anyfunc:
> +        break;

Would this be a FIXME? If not, maybe just put RELEASE_ASSERT_NOT_REACHED here?

> Source/JavaScriptCore/wasm/WasmFormat.h:59
> +        // FIXME auto-generate this.

Please link a bugzilla here

> Source/JavaScriptCore/wasm/wasm.json:93
> +        "i32.add":             { "category": "arithmetic", "value": 106, "return": ["i32"],      "parameter": ["i32", "i32"],           "immediate": [], "b3op": "Add"          },

Where do these numbers come from?

[JF Bastien]

Comment on attachment 295701 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295701&action=review

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:468
>> +        break;
> 
> Would this be a FIXME? If not, maybe just put RELEASE_ASSERT_NOT_REACHED here?

That would just be a bug in the calling code.

I thought having the assert outside a switch was the idiomatic thing, because (IIRC) MSVC can't figure out that all paths return if the switch covers all cases and has no return after:

int foo(enum Type t)
{
  switch (t) {
  case Foo: // ... cover all of Type's values
  }
  // No return here makes some MSVCs sad.
}

>> Source/JavaScriptCore/wasm/wasm.json:93
>> +        "i32.add":             { "category": "arithmetic", "value": 106, "return": ["i32"],      "parameter": ["i32", "i32"],           "immediate": [], "b3op": "Add"          },
> 
> Where do these numbers come from?

Discussed offline, comes from https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md

[JF Bastien]

Created attachment 295790 [details]
patch

Add FIXME URL.

[JF Bastien]

Created attachment 295820 [details]
patch

Minor rebase+fix with my previous patch, and Yusuke's patch.

[WebKit Commit Bot]

Comment on attachment 295820 [details]
patch

Clearing flags on attachment: 295820

Committed r209175: <http://trac.webkit.org/changeset/209175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Keith Miller]

Comment on attachment 295820 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295820&action=review

> JSTests/wasm/Builder.js:54
> +    assert.truthy(WASM.isValidBlockType(ret), `Type return ${ret} must be valid block type`);

I think isValidBlockType is called isValidReturnType in the C++ code. We should probably use the same name for consistency. I'm indifferent to which one we choose though.

[JF Bastien]

Trying to revert here due to odd breakage: https://bugs.webkit.org/show_bug.cgi?id=165308