Bug 162870 – Auto-generate WASMOps.h, share with testing JSON file

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

[JF Bastien]

./Source/JavaScriptCore/wasm/WASMOps.h can be auto-generated from the same JSON file that I intend to use for testing as part of 162706. That allows us to follow wasm format updates more easily, and reduces the number of places where we enter the same data.

I propose we just auto-generate it manually for now, and if we find it useful we can add it to the build process. I'm usually not super thrilled about adding pre-build steps to multiple build systems because this tends to break in subtle manners, and I don't expect us to change this auto-generated file very often.

[JF Bastien]

Created attachment 290493 [details]
patch

[JF Bastien]

Created attachment 290495 [details]
patch

Fix think-o in integer handling.

[Geoffrey Garen]

Comment on attachment 290495 [details]
patch

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

r=me - with some minor refinements below.

> JSTests/stress/wasm/to-c++.js:1
> +// Use the JSON description of WebAssembly to generate the JavaScriptCore header file.

Let's say WASMOps.h in this comment.

Can we call this file generate-wasmops-header.js?

> JSTests/stress/wasm/to-c++.js:3
> +const json_file = 'wasm.json';

WebKit style is camel case: jsonFile. This comment applies to lots of stuff in this file.

> JSTests/stress/wasm/to-c++.js:23
> +const cpp_macro = (wasm_opcode, value, b3_opcode) => " \\\n    macro(" + to_cpp(wasm_opcode) + ", 0x" + parseInt(value).toString(16) + ", " + b3_opcode + ")";

Since we generating code with a script, should we nix the macros and just generate the full code, or do we think the macros aid readability?

> JSTests/stress/wasm/wasm.json:1
> +{

Let's add a note to this file that reminds the author to run the generation script after changing this file.

[Keith Miller]

Comment on attachment 290495 [details]
patch

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

I think you are going to have a problem building this since a lot of the WASM C++ code blindly assumes there is a corresponding B3 opcode for most of the operations.

>> JSTests/stress/wasm/to-c++.js:23
>> +const cpp_macro = (wasm_opcode, value, b3_opcode) => " \\\n    macro(" + to_cpp(wasm_opcode) + ", 0x" + parseInt(value).toString(16) + ", " + b3_opcode + ")";
> 
> Since we generating code with a script, should we nix the macros and just generate the full code, or do we think the macros aid readability?

I think we want these macros. They are nice for handling entire classes of WASM ops at once. If you want to see examples of how I have been using them, take a look at WASMFunctionParser.h.

>> JSTests/stress/wasm/wasm.json:1
>> +{
> 
> Let's add a note to this file that reminds the author to run the generation script after changing this file.

Better yet, add it to the style checker script!

[JF Bastien]

Created attachment 290500 [details]
patch

Fix build, haven't addressed comments yet. Will do after lunch.

[JF Bastien]

Created attachment 290517 [details]
patch

Address comments. I think this is good to go.

[JF Bastien]

Created attachment 290519 [details]
patch

I failed at adding one file to the previous review.

[Keith Miller]

Comment on attachment 290519 [details]
patch

r=me, although I still think we should the build comment to the style checker. We can do that if failing to update the C++ file becomes a problem though.

[WebKit Commit Bot]

Comment on attachment 290519 [details]
patch

Clearing flags on attachment: 290519

Committed r206756: <http://trac.webkit.org/changeset/206756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.