Bug 148373

Created attachment 259748 [details] Patch

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

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.

Created attachment 260007 [details] WIP

Created attachment 260021 [details] Patch

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

(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 on attachment 260021 [details] Patch Clearing flags on attachment: 260021 Committed r189079: <http://trac.webkit.org/changeset/189079>

All reviewed patches have been landed. Closing bug.

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)

Re-opened since this is blocked by bug 148555

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.

Created attachment 260117 [details] Patch

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 on attachment 260117 [details] Patch r=me

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

Created attachment 260130 [details] Patch

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

Created attachment 260171 [details] Patch

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.

(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 on attachment 260171 [details] Patch Clearing flags on attachment: 260171 Committed r189123: <http://trac.webkit.org/changeset/189123>

All reviewed patches have been landed. Closing bug.