Bug 148373 - Create WebAssembly functions
Summary: Create WebAssembly functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 148555
Blocks: 146064
  Show dependency treegraph
 
Reported: 2015-08-23 21:26 PDT by Sukolsak Sakshuwong
Modified: 2015-08-28 14:07 PDT (History)
10 users (show)

See Also:


Attachments
Patch (21.94 KB, patch)
2015-08-23 22:06 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
WIP (61.81 KB, patch)
2015-08-26 17:33 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (62.00 KB, patch)
2015-08-26 19:25 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (62.96 KB, patch)
2015-08-27 21:03 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (64.57 KB, patch)
2015-08-27 23:39 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (65.23 KB, patch)
2015-08-28 13:14 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sukolsak Sakshuwong 2015-08-23 21:26:06 PDT
Create functions from WebAssembly files generated by pack-asmjs <https://github.com/WebAssembly/polyfill-prototype-1>.
Comment 1 Sukolsak Sakshuwong 2015-08-23 22:06:36 PDT
Created attachment 259748 [details]
Patch
Comment 2 Geoffrey Garen 2015-08-24 10:51:08 PDT
Comment on attachment 259748 [details]
Patch

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

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:68
> +        Vector<UnlinkedInstruction, 0, UnsafeVectorOverflow> instructions;
> +        instructions.append(op_enter);
> +        result->setInstructions(std::make_unique<UnlinkedInstructionStream>(instructions));

This seems wrong. What does op_enter mean in the web assembly context? What does it mean to have bytecode at all, when we don't have web assembly bytecodes?

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:185
> +#if ENABLE(WEBASSEMBLY)
> +    unsigned m_isWebAssemblyFunction : 1;
> +    WriteBarrier<JSWASMModule> m_webAssemblyModule;
> +    unsigned m_webAssemblyFunctionIndex;
> +#endif

I think it would be clearer -- and better for memory use -- to add a webassembly subclass rather than building this into all function executables.
Comment 3 Geoffrey Garen 2015-08-24 14:29:24 PDT
Comment on attachment 259748 [details]
Patch

I think WASMExecutable should be a subclass of ExecutableBase, just like NativeExecutable. JSFunction already knows, a little bit, how to multiplex between JavaScript code and foreign code at the executable level, so this should be a natural fit.
Comment 4 Geoffrey Garen 2015-08-24 14:29:57 PDT
Also, there's no need for an unlinked WASM executable. We use unlinked for caching the result of parsing, but WASM is already in that short-hand format, and not in a text format.
Comment 5 Sukolsak Sakshuwong 2015-08-26 17:33:01 PDT
Created attachment 260007 [details]
WIP
Comment 6 Sukolsak Sakshuwong 2015-08-26 19:25:54 PDT
Created attachment 260021 [details]
Patch
Comment 7 Sukolsak Sakshuwong 2015-08-26 20:02:15 PDT
Thanks for the review.

(In reply to comment #2)
> Comment on attachment 259748 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259748&action=review
> 
> > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:68
> > +        Vector<UnlinkedInstruction, 0, UnsafeVectorOverflow> instructions;
> > +        instructions.append(op_enter);
> > +        result->setInstructions(std::make_unique<UnlinkedInstructionStream>(instructions));
> 
> This seems wrong. What does op_enter mean in the web assembly context? What
> does it mean to have bytecode at all, when we don't have web assembly
> bytecodes?

It is a hacky way to circumvent assertions that assert that instructions.size() > 0. Those assertions shouldn't have been called in the first place. I have removed it.

(In reply to comment #3)
> Comment on attachment 259748 [details]
> Patch
> 
> I think WASMExecutable should be a subclass of ExecutableBase, just like
> NativeExecutable. JSFunction already knows, a little bit, how to multiplex
> between JavaScript code and foreign code at the executable level, so this
> should be a natural fit.

Done. I create WebAssemblyExecutable as a subclass of ExecutableBase.

(In reply to comment #4)
> Also, there's no need for an unlinked WASM executable. We use unlinked for
> caching the result of parsing, but WASM is already in that short-hand
> format, and not in a text format.

Done.
Comment 8 Geoffrey Garen 2015-08-27 15:04:22 PDT
Comment on attachment 260021 [details]
Patch

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

r=me

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:110
> +        m_hash = CodeBlockHash(ownerScriptExecutable()->source(), specializationKind());

In future, it is best to separate a renaming change like this from the functionality change in the rest of this patch, and put the two in two separate patches.

> Source/JavaScriptCore/runtime/Executable.cpp:614
> +    m_jitCodeForCallWithArityCheck = MacroAssemblerCodePtr();
> +    m_jitCodeForCallWithArityCheckAndPreserveRegs = MacroAssemblerCodePtr();

Are you going to fill these in in a separate patch? We're going to need them, I think.
Comment 9 Sukolsak Sakshuwong 2015-08-27 15:32:04 PDT
(In reply to comment #8)
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:110
> > +        m_hash = CodeBlockHash(ownerScriptExecutable()->source(), specializationKind());
> 
> In future, it is best to separate a renaming change like this from the
> functionality change in the rest of this patch, and put the two in two
> separate patches.

Thanks. Will do.

> > Source/JavaScriptCore/runtime/Executable.cpp:614
> > +    m_jitCodeForCallWithArityCheck = MacroAssemblerCodePtr();
> > +    m_jitCodeForCallWithArityCheckAndPreserveRegs = MacroAssemblerCodePtr();
> 
> Are you going to fill these in in a separate patch? We're going to need
> them, I think.

I am going to implement the arity check in a separate patch. But these two lines will stay the same, just like what we currently have in ScriptExecutable::installCode(). These two variables are just cached results, I think. ExecutableBase::entrypointFor() will fill them.
Comment 10 WebKit Commit Bot 2015-08-27 18:13:56 PDT
Comment on attachment 260021 [details]
Patch

Clearing flags on attachment: 260021

Committed r189079: <http://trac.webkit.org/changeset/189079>
Comment 11 WebKit Commit Bot 2015-08-27 18:14:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Jessie Berlin 2015-08-27 19:15:00 PDT
This broke the build:

https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20%28Build%29/builds/8266/steps/compile-webkit/logs/stdio

Ld /Volumes/Data/slave/yosemite-debug/build/WebKitBuild/Debug/libWebCoreTestSupport.dylib normal x86_64
    cd /Volumes/Data/slave/yosemite-debug/build/Source/WebCore
    export MACOSX_DEPLOYMENT_TARGET=10.10
    /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -arch x86_64 -dynamiclib -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk -L/Volumes/Data/slave/yosemite-debug/build/WebKitBuild/Debug -F/Volumes/Data/slave/yosemite-debug/build/WebKitBuild/Debug -F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/PrivateFrameworks -filelist /Volumes/Data/slave/yosemite-debug/build/WebKitBuild/WebCore.build/Debug/WebCoreTestSupport.build/Objects-normal/x86_64/WebCoreTestSupport.LinkFileList -Xlinker --no-demangle -unexported_symbols_list Configurations/WebCore.unexp -install_name @rpath/libWebCoreTestSupport.dylib -mmacosx-version-min=10.10 -stdlib=libc++ -framework CoreFoundation -framework JavaScriptCore -framework WebCore -single_module -compatibility_version 1 -current_version 602.1.1 -Xlinker -dependency_info -Xlinker /Volumes/Data/slave/yosemite-debug/build/WebKitBuild/WebCore.build/Debug/WebCoreTestSupport.build/Objects-normal/x86_64/WebCoreTestSupport_dependency_info.dat -o /Volumes/Data/slave/yosemite-debug/build/WebKitBuild/Debug/libWebCoreTestSupport.dylib
Undefined symbols for architecture x86_64:
  "__ZN3JSC16ScriptExecutable6s_infoE", referenced from:
      __ZN3JSC16ScriptExecutable4infoEv in Internals.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Comment 13 WebKit Commit Bot 2015-08-27 19:17:54 PDT
Re-opened since this is blocked by bug 148555
Comment 14 Geoffrey Garen 2015-08-27 20:20:30 PDT
I'm not sure why Internals ends up referencing this value, but the easy fix is to use DECLARE_EXPORT_INFO instead of DECLARE_INFO.
Comment 15 Sukolsak Sakshuwong 2015-08-27 21:03:20 PDT
Created attachment 260117 [details]
Patch
Comment 16 Sukolsak Sakshuwong 2015-08-27 21:04:02 PDT
Comment on attachment 260117 [details]
Patch

Change DECLARE_INFO in the header of ScriptExecutable to DECLARE_EXPORT_INFO and add these two paragraphs to ChangeLog:

"This patch introduces WebAssemblyExecutable, a new subclass of
ExecutableBase for WebAssembly functions. CodeBlocks can now have
an owner that is not a ScriptExecutable. This patch changes the
return type of CodeBlock::ownerExecutable() from ScriptExecutable*
to ExecutableBase*. It also changes code that calls ScriptExecutable's
methods on CodeBlock::ownerExecutable() to use
CodeBlock::ownerScriptExecutable(), which does jsCast<ScriptExecutable*>.

Since ownerScriptExecutable() is called from WebCore and it uses
jsCast<ScriptExecutable*>, this patch needs to export
ScriptExecutable::info(). This should fix the build error in
https://bugs.webkit.org/show_bug.cgi?id=148555"
Comment 17 Geoffrey Garen 2015-08-27 22:28:18 PDT
Comment on attachment 260117 [details]
Patch

r=me
Comment 18 WebKit Commit Bot 2015-08-27 23:15:31 PDT
Comment on attachment 260117 [details]
Patch

Rejecting attachment 260117 [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-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 260117, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
    -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 189085 = e420ab2048e32c262d778affbb12442fa5bb8588
r189086 = 49e71b9cccf4025b44563d4a6384c2bb5ec86c73
r189087 = f73c3a458395e3820429806fc4696689947aaae7
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/115468
Comment 19 Sukolsak Sakshuwong 2015-08-27 23:39:32 PDT
Created attachment 260130 [details]
Patch
Comment 20 Sukolsak Sakshuwong 2015-08-27 23:48:29 PDT
svn-apply failed because this patch <https://bugs.webkit.org/show_bug.cgi?id=148559>, which landed about an hour ago, introduced 5 new uses of CodeBlock::ownerExecutable(). I have fixed them accordingly.
Comment 21 Yusuke Suzuki 2015-08-28 11:36:35 PDT
Comment on attachment 260130 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:1469
>          executable = funcObj->jsExecutable();

FYI, during implementing ModuleProgramExecutable in my module patch, I found the following code in WebCore Internals.cpp

if (executable->isFunctionExecutable()) {
    ...
} else if (executable->isEvalExecutable())
    result.appendLiteral("eval");
else {
    ASSERT(executable->isProgramExecutable());
    ...
}

So we need to insert executable->isWebAssemblyExecutable() case here (of course, Internals is just used for the tests. So this assertion never occurs in the production I think.)
Comment 22 Sukolsak Sakshuwong 2015-08-28 13:14:42 PDT
Created attachment 260171 [details]
Patch
Comment 23 Filip Pizlo 2015-08-28 13:19:14 PDT
Comment on attachment 260171 [details]
Patch

R=me based on ggaren's earlier review and my review of what I think is the new parts.
Comment 24 Sukolsak Sakshuwong 2015-08-28 13:24:10 PDT
(In reply to comment #21)
> Comment on attachment 260130 [details]
>
> So we need to insert executable->isWebAssemblyExecutable() case here (of
> course, Internals is just used for the tests. So this assertion never occurs
> in the production I think.)

Thanks. I have added the following diff to WebCore/testing/Internals.cpp

@@ -1485,10 +1485,14 @@ String Internals::parserMetaData(Deprecated::ScriptValue value)
         result.append('"');
     } else if (executable->isEvalExecutable())
         result.appendLiteral("eval");
-    else {
-        ASSERT(executable->isProgramExecutable());
+    else if (executable->isProgramExecutable())
         result.appendLiteral("program");
-    }
+#if ENABLE(WEBASSEMBLY)
+    else if (executable->isWebAssemblyExecutable())
+        result.appendLiteral("WebAssembly");
+#endif
+    else
+        ASSERT_NOT_REACHED();
 
     result.appendLiteral(" { ");
     result.appendNumber(startLine);

As you wrote, Internals is just used for the tests. The code that I added ('result.appendLiteral("WebAssembly");') will never be executed, because we are currently not testing WebAssembly. But even if we test it, I'm wondering whether this code will ever be reached, given how WebAssembly is loaded.
Comment 25 WebKit Commit Bot 2015-08-28 14:07:41 PDT
Comment on attachment 260171 [details]
Patch

Clearing flags on attachment: 260171

Committed r189123: <http://trac.webkit.org/changeset/189123>
Comment 26 WebKit Commit Bot 2015-08-28 14:07:49 PDT
All reviewed patches have been landed.  Closing bug.