RESOLVED FIXED 165150
WebAssembly JS API: implement start function
https://bugs.webkit.org/show_bug.cgi?id=165150
Summary WebAssembly JS API: implement start function
JF Bastien
Reported 2016-11-29 12:00:00 PST
.
Attachments
WIP (2.15 KB, patch)
2016-12-08 22:13 PST, JF Bastien
jfbastien: commit-queue-
patch (4.88 KB, patch)
2016-12-08 22:43 PST, JF Bastien
jfbastien: commit-queue-
patch (18.92 KB, patch)
2016-12-09 14:13 PST, JF Bastien
saam: review+
saam: commit-queue-
patch (20.10 KB, patch)
2016-12-09 14:48 PST, JF Bastien
commit-queue: commit-queue-
patch (20.83 KB, patch)
2016-12-09 14:58 PST, JF Bastien
jfbastien: commit-queue+
patch (21.43 KB, patch)
2016-12-09 15:15 PST, JF Bastien
commit-queue: commit-queue-
patch (21.45 KB, patch)
2016-12-09 16:06 PST, JF Bastien
no flags
JF Bastien
Comment 1 2016-12-08 22:13:22 PST
Created attachment 296641 [details] WIP I've started implementing this. Here are basic JSON tests.
JF Bastien
Comment 2 2016-12-08 22:43:47 PST
Created attachment 296643 [details] patch Feed the JSON tests from the Builder. I still have some checking to do, then the binary side.
JF Bastien
Comment 3 2016-12-09 14:13:47 PST
Created attachment 296698 [details] patch Patch for landing.
JF Bastien
Comment 4 2016-12-09 14:18:25 PST
Saam Barati
Comment 5 2016-12-09 14:26:41 PST
Comment on attachment 296698 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=296698&action=review > JSTests/wasm/js-api/test_Start.js:14 > + const instance = new WebAssembly.Instance(module); Please add a test that proves that this ran. Maybe by writing to memory. > Source/JavaScriptCore/wasm/WasmFormat.h:128 > + std::optional<uint32_t> startFunctionIndexSpace; Why not just call this "startFunctionIndex"? > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:397 > + || startFunctionIndex >= m_functionIndexSpace.size()) Please add a test for this. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:401 > + if (signature->arguments.size() != 0 > + || signature->returnType != Void) Please add a test for this.
JF Bastien
Comment 6 2016-12-09 14:39:20 PST
> > Source/JavaScriptCore/wasm/WasmFormat.h:128 > > + std::optional<uint32_t> startFunctionIndexSpace; > > Why not just call this "startFunctionIndex"? Because the value we receive in the function index space, so it can include imports for now. Once we resolve the spec issue we'll have to change things, but it's still important to note that it isn't just the internal function index.
JF Bastien
Comment 7 2016-12-09 14:48:30 PST
Created attachment 296705 [details] patch Address comments.
WebKit Commit Bot
Comment 8 2016-12-09 14:49:35 PST
Comment on attachment 296705 [details] patch Rejecting attachment 296705 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 296705, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: unk #4 succeeded at 461 (offset 25 lines). patching file Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp Hunk #1 succeeded at 102 with fuzz 1 (offset 12 lines). patching file Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h patching file Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp patching file Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.h Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/2681933
JF Bastien
Comment 9 2016-12-09 14:58:08 PST
Created attachment 296709 [details] patch Fix merge conflict.
JF Bastien
Comment 10 2016-12-09 15:15:53 PST
Created attachment 296713 [details] patch Another merge issue.
WebKit Commit Bot
Comment 11 2016-12-09 15:41:26 PST
Comment on attachment 296713 [details] patch Rejecting attachment 296713 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: t -o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/jsc Undefined symbols for architecture x86_64: "JSC::Wasm::ModuleInformation::~ModuleInformation()", referenced from: functionTestWasmModuleFunctions(JSC::ExecState*) in jsc.o ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) ** BUILD FAILED ** The following build commands failed: Ld /Volumes/Data/EWS/WebKit/WebKitBuild/Release/jsc normal x86_64 (1 failure) Full output: http://webkit-queues.webkit.org/results/2682613
JF Bastien
Comment 12 2016-12-09 16:06:31 PST
Created attachment 296731 [details] patch Fix bot build issue which doesn't occur on local cmake build.
WebKit Commit Bot
Comment 13 2016-12-09 18:34:33 PST
Comment on attachment 296731 [details] patch Clearing flags on attachment: 296731 Committed r209642: <http://trac.webkit.org/changeset/209642>
WebKit Commit Bot
Comment 14 2016-12-09 18:34:37 PST
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.