Bug 148373 – Create WebAssembly functions

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

[Sukolsak Sakshuwong]

Create functions from WebAssembly files generated by pack-asmjs <https://github.com/WebAssembly/polyfill-prototype-1>.

[Sukolsak Sakshuwong]

Created attachment 259748 [details]
Patch

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Sukolsak Sakshuwong]

Created attachment 260007 [details]
WIP

[Sukolsak Sakshuwong]

Created attachment 260021 [details]
Patch

[Sukolsak Sakshuwong]

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.

[Geoffrey Garen]

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.

[Sukolsak Sakshuwong]

(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.

[WebKit Commit Bot]

Comment on attachment 260021 [details]
Patch

Clearing flags on attachment: 260021

Committed r189079: <http://trac.webkit.org/changeset/189079>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

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)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148555

[Geoffrey Garen]

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.

[Sukolsak Sakshuwong]

Created attachment 260117 [details]
Patch

[Sukolsak Sakshuwong]

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"

[Geoffrey Garen]

Comment on attachment 260117 [details]
Patch

r=me

[WebKit Commit Bot]

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

[Sukolsak Sakshuwong]

Created attachment 260130 [details]
Patch

[Sukolsak Sakshuwong]

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.

[Yusuke Suzuki]

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.)

[Sukolsak Sakshuwong]

Created attachment 260171 [details]
Patch

[Filip Pizlo]

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.

[Sukolsak Sakshuwong]

(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.

[WebKit Commit Bot]

Comment on attachment 260171 [details]
Patch

Clearing flags on attachment: 260171

Committed r189123: <http://trac.webkit.org/changeset/189123>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.