Bug 162870

Summary: Auto-generate WASMOps.h, share with testing JSON file
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 162706, 163267    
Attachments:
Description Flags
patch
none
patch
ggaren: review+, ggaren: commit-queue-
patch
none
patch
none
patch none

JF Bastien
Reported 2016-10-03 11:03:36 PDT
./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.
Attachments
patch (46.09 KB, patch)
2016-10-03 11:11 PDT, JF Bastien 		no flags
DetailsFormatted DiffDiff
patch (44.33 KB, patch)
2016-10-03 11:17 PDT, JF Bastien 		ggaren: review+
ggaren: commit-queue-
DetailsFormatted DiffDiff
patch (46.32 KB, patch)
2016-10-03 11:52 PDT, JF Bastien 		no flags
DetailsFormatted DiffDiff
patch (46.37 KB, patch)
2016-10-03 13:57 PDT, JF Bastien 		no flags
DetailsFormatted DiffDiff
patch (46.77 KB, patch)
2016-10-03 14:23 PDT, JF Bastien 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
JF Bastien
Comment 1 2016-10-03 11:11:31 PDT
Created attachment 290493 [details] patch
JF Bastien
Comment 2 2016-10-03 11:17:19 PDT
Created attachment 290495 [details] patch Fix think-o in integer handling.
Geoffrey Garen
Comment 3 2016-10-03 11:25:51 PDT
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 4 2016-10-03 11:34:25 PDT
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
Comment 5 2016-10-03 11:52:58 PDT
Created attachment 290500 [details] patch Fix build, haven't addressed comments yet. Will do after lunch.
JF Bastien
Comment 6 2016-10-03 13:57:02 PDT
Created attachment 290517 [details] patch Address comments. I think this is good to go.
JF Bastien
Comment 7 2016-10-03 14:23:11 PDT
Created attachment 290519 [details] patch I failed at adding one file to the previous review.
Keith Miller
Comment 8 2016-10-03 14:24:58 PDT
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 9 2016-10-03 15:46:05 PDT
Comment on attachment 290519 [details] patch Clearing flags on attachment: 290519 Committed r206756: <http://trac.webkit.org/changeset/206756>
WebKit Commit Bot
Comment 10 2016-10-03 15:46:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.