Bug 164724 - WebAssembly: update binary format to 0xD version
Summary: WebAssembly: update binary format to 0xD version
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on:
Blocks: 159775 165308 165345
  Show dependency treegraph
 
Reported: 2016-11-14 12:12 PST by JF Bastien
Modified: 2016-12-02 15:39 PST (History)
6 users (show)

See Also:


Attachments
patch (174.92 KB, patch)
2016-11-29 21:09 PST, JF Bastien
saam: review+
Details | Formatted Diff | Diff
Helper script to synchronize BinaryFormat.md and wasm.json (967 bytes, text/x-python-script)
2016-11-29 21:09 PST, JF Bastien
no flags Details
patch (174.97 KB, patch)
2016-11-30 16:50 PST, JF Bastien
no flags Details | Formatted Diff | Diff
patch (178.33 KB, patch)
2016-11-30 22:28 PST, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-11-14 12:12:14 PST
We're currently at 0xC.
Comment 1 JF Bastien 2016-11-29 21:09:06 PST
Created attachment 295701 [details]
patch
Comment 2 JF Bastien 2016-11-29 21:09:59 PST
Created attachment 295702 [details]
Helper script to synchronize BinaryFormat.md and wasm.json
Comment 3 WebKit Commit Bot 2016-11-29 21:11:38 PST
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".
Comment 4 Saam Barati 2016-11-30 00:21:21 PST
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"?
Comment 5 JF Bastien 2016-11-30 09:56:33 PST
(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
Comment 6 Saam Barati 2016-11-30 16:37:35 PST
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?
Comment 7 JF Bastien 2016-11-30 16:50:10 PST
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
Comment 8 JF Bastien 2016-11-30 16:50:51 PST
Created attachment 295790 [details]
patch

Add FIXME URL.
Comment 9 JF Bastien 2016-11-30 22:28:00 PST
Created attachment 295820 [details]
patch

Minor rebase+fix with my previous patch, and Yusuke's patch.
Comment 10 WebKit Commit Bot 2016-11-30 23:04:40 PST
Comment on attachment 295820 [details]
patch

Clearing flags on attachment: 295820

Committed r209175: <http://trac.webkit.org/changeset/209175>
Comment 11 WebKit Commit Bot 2016-11-30 23:04:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Keith Miller 2016-12-01 07:09:30 PST
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.
Comment 13 JF Bastien 2016-12-02 09:20:16 PST
Trying to revert here due to odd breakage: https://bugs.webkit.org/show_bug.cgi?id=165308