Bug 165150

Summary: WebAssembly JS API: implement start function
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Severity: Normal CC: commit-queue, jfbastien, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 161709, 165658, 165714    
Description Flags
jfbastien: commit-queue-
jfbastien: commit-queue-
sbarati: review+, sbarati: commit-queue-
commit-queue: commit-queue-
jfbastien: commit-queue+
commit-queue: commit-queue-
patch none

Description JF Bastien 2016-11-29 12:00:00 PST
Comment 1 JF Bastien 2016-12-08 22:13:22 PST
Created attachment 296641 [details]

I've started implementing this. Here are basic JSON tests.
Comment 2 JF Bastien 2016-12-08 22:43:47 PST
Created attachment 296643 [details]

Feed the JSON tests from the Builder. I still have some checking to do, then the binary side.
Comment 3 JF Bastien 2016-12-09 14:13:47 PST
Created attachment 296698 [details]

Patch for landing.
Comment 4 JF Bastien 2016-12-09 14:18:25 PST
Comment 5 Saam Barati 2016-12-09 14:26:41 PST
Comment on attachment 296698 [details]

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.
Comment 6 JF Bastien 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.
Comment 7 JF Bastien 2016-12-09 14:48:30 PST
Created attachment 296705 [details]

Address comments.
Comment 8 WebKit Commit Bot 2016-12-09 14:49:35 PST
Comment on attachment 296705 [details]

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
Comment 9 JF Bastien 2016-12-09 14:58:08 PST
Created attachment 296709 [details]

Fix merge conflict.
Comment 10 JF Bastien 2016-12-09 15:15:53 PST
Created attachment 296713 [details]

Another merge issue.
Comment 11 WebKit Commit Bot 2016-12-09 15:41:26 PST
Comment on attachment 296713 [details]

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)


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
Comment 12 JF Bastien 2016-12-09 16:06:31 PST
Created attachment 296731 [details]

Fix bot build issue which doesn't occur on local cmake build.
Comment 13 WebKit Commit Bot 2016-12-09 18:34:33 PST
Comment on attachment 296731 [details]

Clearing flags on attachment: 296731

Committed r209642: <http://trac.webkit.org/changeset/209642>
Comment 14 WebKit Commit Bot 2016-12-09 18:34:37 PST
All reviewed patches have been landed.  Closing bug.