Bug 164757

Created attachment 295080 [details] patch This patch puts a bunch of the pieces in place (I think the right places?) but doesn't yet implement link and evaluate on WebAssemblyModuleRecord, and doesn't register the functions on it from constructJSWebAssemblyInstance. I'll need to address these two, but would appreciate early comments to make sure I'm heading in the right direction.

Created attachment 295116 [details] patch Add SymbolTable and JSScope. Need to populate JSScope, and pass both to the ModuleEnvironment (I think?).

Comment on attachment 295116 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295116&action=review Quick comment. > Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:87 > + JSModuleEnvironment* importedModuleEnvironment = jsCast<JSModuleRecord*>(resolution.moduleRecord)->moduleEnvironment(); This should use AbstractModuleRecord. And AbstractModuleRecord should have JSModuleEnvironment. > Source/JavaScriptCore/runtime/JSModuleRecord.h:68 > WriteBarrier<JSModuleEnvironment> m_moduleEnvironment; I think the abstract module record also should have module environment. How to instantiate it (in linking phase) depends on the type of module record. > Source/JavaScriptCore/runtime/JSScope.cpp:83 > + JSModuleRecord* importedRecord = jsCast<JSModuleRecord*>(resolution.moduleRecord); If we do that, we can use AbstractModuleRecord here. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:88 > + auto moduleRecordDeleter = [] (WebAssemblyModuleRecord* r) { > + WebAssemblyModuleRecord::destroy(r); > + }; > + std::unique_ptr<WebAssemblyModuleRecord, decltype(moduleRecordDeleter)> moduleRecord(WebAssemblyModuleRecord::create(state, vm, globalObject->webAssemblyModuleRecordStructure(), moduleKey, sourceCode, declaredVariables, lexicalVariables), moduleRecordDeleter); WebAssemblyModuleRecord and JSModuleRecord are GC managed objects. So you cannot manage it in unique_ptr. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:87 > + std::function<void(JSCell*)> symbolDeleter = [] (JSCell* s) { > + SymbolTable::destroy(s); > + }; > + plan.getModuleInformation()->exportSymbolTable = std::unique_ptr<SymbolTable, decltype(symbolDeleter)>(SymbolTable::create(vm), symbolDeleter); SymbolTable is GC managed object. So you cannot do that. You can hold it in WriteBarrier in the owner. And you need to write `visitChildren` function for the owner.

Created attachment 295241 [details] patch Address comments: - Move the JSEnvironment to the AbstractModuleRecord. - Don't use unique_ptr for GC objects. That was very silly. - Put the ModuleEnvironment on the ModuleRecord. I also moved the binary format to emitting UTF-8 by default because UTF-16 makes it a PITA to interact with. This still seems slightly buggy, will fix later. The important part is putting the field name / function onto the WebAssembly Module Record (which then gets exposed as `exports` on the Instance). I'm not totally sure this is correct, but it correctly exposes a registered function. The dubious bits I'd appreciate feedback on are in constructJSWebAssemblyInstance. From test_Instance.js: (function ExportedAnswer() { const builder = (new Builder()) .Type().End() .Function().End() .Export() .Function("answer") .End() .Code() .Function("answer", { params: [], ret: "i32" }) .I32Const(42) .Return() .End() .End(); const bin = builder.WebAssembly().get(); const module = new WebAssembly.Module(bin); const instance = new WebAssembly.Instance(module); assert.eq(instance.exports.answer(), 42); })(); This calls "answer"... and promptly crashes here: ASSERTION FAILED: isCell() ../Source/JavaScriptCore/runtime/JSCJSValue.cpp(371) : JSC::JSString *JSC::JSValue::toStringSlowCase(JSC::ExecState *, bool) const 1 0x1053a261d WTFCrash 2 0x105034967 JSC::JSValue::toStringSlowCase(JSC::ExecState*, bool) const 3 0x104042910 JSC::JSValue::toString(JSC::ExecState*) const 4 0x104fa3b2f slow_path_to_string 5 0x104dd395c llint_entry 6 0x104dd8f10 llint_entry 7 0x104dd8f10 llint_entry 8 0x104dd17de vmEntryToJavaScript 9 0x104d53ff4 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 10 0x104d05c60 JSC::Interpreter::execute(JSC::ModuleProgramExecutable*, JSC::ExecState*, JSC::JSModuleEnvironment*) 11 0x1050928dc JSC::JSModuleRecord::evaluate(JSC::ExecState*) 12 0x10508d0e5 JSC::JSModuleLoader::evaluate(JSC::ExecState*, JSC::JSValue, JSC::JSValue, JSC::JSValue) 13 0x105142d15 JSC::moduleLoaderPrototypeEvaluate(JSC::ExecState*) ... It's weird because it's calling a JSModuleRecord, and I'm pretty sure I gave it a WebAssemblyModuleRecord. I'll investigate!

Comment on attachment 295241 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295241&action=review Second round. Looks good direction! And suggestions. > Source/JavaScriptCore/runtime/JSModuleEnvironment.h:5 > * modification, are permitted provided that the following conditions JSModuleEnvironment should have AbstractModuleRecord* instead of JSModuleRecord*. > Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:128 > + JSModuleRecord* targetModule = jsCast<JSModuleRecord*>(resolution.moduleRecord); Let's change it to AbstractModuleRecord*. > Source/JavaScriptCore/runtime/JSModuleRecord.cpp:134 > + JSModuleRecord* importedModule = jsCast<JSModuleRecord*>(hostResolveImportedModule(exec, importEntry.moduleRequest)); This should be AbstractModuleRecord*. JSModuleRecord should have the ability to link itself against WebAssemblyModuleRecord. > Source/JavaScriptCore/wasm/WasmModuleParser.h:43 > + : Parser(sourceBuffer, sourceLength), m_vm(vm) Let's break line. : Parser(sourceBuffer, sourceLength) , m_vm(vm) > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:81 > + JSLexicalEnvironment* exportEnvironment = JSLexicalEnvironment::create(vm, globalObject, nullptr, exportSymbolTable, JSValue()); Instead of JSLexicalEnvironment, we should create JSModuleEnvironment. At that time, we need to pass WebAssemblyModuleRecord*. So I suggest the way to instantiate it like this. 1. First, create WebAssemblyModuleRecord*. 2. In WebAssemblyModuleRecord::link phase, we will create JSModuleEnvironment*. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:123 > + JSValue thisModule(moduleRecord); > + unsigned offset = 0; > + for (const auto& e : moduleInformation->exports) { > + JSValue exportedValue; > + PutPropertySlot slot(thisModule); > + slot.setNewProperty(moduleRecord, offset++); > + switch (e.kind) { > + case Wasm::External::Function: { > + // 1. If e is a closure c: > + // i. If there is an Exported Function Exotic Object func in funcs whose func.[[Closure]] equals c, then return func. > + // ii. (Note: At most one wrapper is created for any closure, so func is unique, even if there are multiple occurrances in the list. Moreover, if the item was an import that is already an Exported Function Exotic Object, then the original function object will be found. For imports that are regular JS functions, a new wrapper will be created.) > + // iii. Otherwise: > + // a. Let func be an Exported Function Exotic Object created from c. > + // b. Append func to funcs. > + // c. Return func. > + const Wasm::FunctionCompilation* compiledFunction = compiledFunctions->at(e.functionIndex).get(); > + const B3::Compilation* entryPoint = compiledFunction->code.get(); > + const Wasm::Signature* signature = moduleInformation->functions.at(e.functionIndex).signature; > + WebAssemblyFunction* function = WebAssemblyFunction::create(vm, globalObject, signature->arguments.size(), e.field.string(), CallableWebAssemblyFunction(entryPoint, signature)); > + exportedValue = JSValue(function); > + } break; > + case Wasm::External::Table: { > + // FIXME https://bugs.webkit.org/show_bug.cgi?id=164135 > + } break; > + case Wasm::External::Memory: { > + // FIXME https://bugs.webkit.org/show_bug.cgi?id=164134 > + } break; > + case Wasm::External::Global: { > + // FIXME https://bugs.webkit.org/show_bug.cgi?id=164133 > + // In the MVP, only immutable global variables can be exported. > + } break; > + } > + RELEASE_ASSERT(JSLexicalEnvironment::put(exportEnvironment, state, e.field, exportedValue, slot)); > + if (verbose) > + dataLogLn("Instance registering \"", e.field, "\": ", exportedValue); > + RELEASE_ASSERT(moduleRecord->putDirect(vm, e.field, exportedValue, None)); > + } These operation will be done in WebAssemblyModuleRecord::link. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:86 > + std::function<void(JSCell*)> symbolDeleter = [] (JSCell* s) { > + SymbolTable::destroy(s); > + }; Drop this. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:89 > + SymbolTable* exportSymbolTable = SymbolTable::create(vm); > + for (unsigned symbolIndex = 0; symbolIndex != plan.getModuleInformation()->exports.size(); ++symbolIndex) > + exportSymbolTable->add(plan.getModuleInformation()->exports[symbolIndex].field.impl(), SymbolTableEntry(VarOffset(ScopeOffset(symbolIndex)))); Here is idiomatic way. SymbolTable::takeNextOffset(NoLockingNecessary) & SymbolTable::set() auto offset = exportSymbolTable->takeNextOffset(NoLockingNecessary); exportSymbolTable->set(NoLockingNecessary, name.impl(), SymbolTableEntry(VarOffset(offset))); > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.h:52 > + JSLexicalEnvironment* exportEnvironment() > + { > + ASSERT(m_exportEnvironment); > + return m_exportEnvironment.get(); > + } This is not necessary. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.h:65 > + WriteBarrier<JSLexicalEnvironment> m_exportEnvironment; It is not necessary. AbstractModuleRecord holds JSModuleEnvironment*. Maybe I think this is the cause of the current crash :) JSModuleNamespaceObject will be created with AbstractModuleRecord's JSModuleEnvironment.

Comment on attachment 295241 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295241&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:119 > + RELEASE_ASSERT(JSLexicalEnvironment::put(exportEnvironment, state, e.field, exportedValue, slot)); I recommend to use bool shouldThrowReadOnlyError = false; bool ignoreReadOnlyErrors = true; bool putResult = false; symbolTablePutTouchWatchpointSet(moduleEnvironment, exec, e.field, exportedValue, shouldThrowReadOnlyError, ignoreReadOnlyErrors, putResult); > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:122 > + RELEASE_ASSERT(moduleRecord->putDirect(vm, e.field, exportedValue, None)); This is not necessary.

Created attachment 295322 [details] patch Addressed all previous comments. Changes since last patch: - Implement UTF-8 strings properly, all code points now work. - JSModuleEnvironment has a AbstractModuleRecord* instead of JSModuleRecord*. - JSModuleRecord can link itself against a WebAssemblyModuleRecord (FIXME: needs testing). - Move JSModuleEnvironment creation to WebAssemblyModuleRecord::link. - Create exportSymbolTable more idiomatically. - Remove now-useless function. - AbstractModuleRecord already contains the JSModuleEnvironment, don't duplicate it in WebAssemblyModuleRecord. - Use symbolTablePutTouchWatchpointSet to populate the JSModuleEnvironment. This is now much cleaner, thanks Yusuke! I still have the same crash, will investigate next (it's late here, signing out). IIUC the core problem is that vmEntryToWasm returns a malformed EncodedJSValue. From the JSValue::toString frame: (JSC::JSValue) $4 = { u = { asInt64 = 42 ptr = 0x000000000000002a asBits = (payload = 42, tag = 0) } } The wasm code is supposed to return an i32 with value 42. There's the 42 in there, but its type is object (I think?) so it tries to use toString on it? Or something like that, I'll have to figure out how these things are encoded, haven't looked yet.

Comment on attachment 295322 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295322&action=review OK, the module record and module environment things look good. Several nits. I'll look into WASM calling code. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:4470 > + AD4937C21DDBE60A0077C807 /* AbstractModuleRecord.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = AbstractModuleRecord.h; sourceTree = "<group>"; }; AbstractModuleRecord.h needs to be "Private" header in Xcode to be included from WebCore. > Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp:303 > + // Technically, all the JSModuleRecords have the Map<ExportName, Resolution> for caching. Change this to AbstractModuleRecords. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:365 > } break; Use, ` break; } ` > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:368 > + } break; Ditto > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:371 > + } break; Ditto > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:375 > + } break; Ditto > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:61 > - putDirectWithoutTransition(vm, Identifier::fromString(&vm, "exports"), m_moduleNamespaceObject.get(), None); > + putDirect(vm, Identifier::fromString(&vm, "exports"), JSValue(m_moduleNamespaceObject.get()->moduleRecord()->moduleEnvironment()), None); We need to expose module namespace object. So the change of this line is not necessary. > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:48 > + const Wasm::ModuleInformation* moduleInformation() const { return m_moduleInformation.get(); } > + const Wasm::CompiledFunctions* compiledFunctions() const { return &m_compiledFunctions; } Recently, we tend to use references for non-JS-heap-managed non-null pointers. Use reference instead. > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:49 > + SymbolTable* exportSymbolTable() const { return m_exportSymbolTable.get(); } So, this should be pointer since SymbolTable is JS-managed. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:85 > + JSValue arg = state->argument(argIndex); Use `uncheckedArgument()` instead of `argument()` since we already ensured that the argIndex is less than the argument count. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:99 > + case Wasm::I32: > + arg = JSValue(arg.toInt32(state)); > + break; > + case Wasm::F32: > + arg = JSValue(arg.toFloat(state)); > + break; > + case Wasm::F64: > + arg = JSValue(arg.toNumber(state)); > + break; In these cases, the exception may happen. So you need to check the exception after calling the above conversion. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:58 > +struct CallableWebAssemblyFunction { > + const B3::Compilation* code; > + const Wasm::Signature* signature; > + CallableWebAssemblyFunction(const B3::Compilation* code, const Wasm::Signature* signature) > + : code(code) > + , signature(signature) > + { > + } > + CallableWebAssemblyFunction() = delete; > + CallableWebAssemblyFunction(const CallableWebAssemblyFunction&) = delete; > + CallableWebAssemblyFunction(CallableWebAssemblyFunction&&) = default; > + CallableWebAssemblyFunction& operator=(const CallableWebAssemblyFunction&) = delete; > +}; How about storing JSWebAssemblyInstance here instead of WebAssemblyFunction? Sine the lifetime of the above pointer values depend on JSWebAssemblyModule <- JSWebAssemblyInstance link. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:85 > + for (unsigned symbolIndex = 0; symbolIndex != plan.getModuleInformation()->exports.size(); ++symbolIndex) { Let's use `for (auto& exportedName : plan.getModuleInformation()->exports)`. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:94 > + JSValue thisModule(this); It's not necessary. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:98 > + PutPropertySlot slot(thisModule); It should be just `PutPropertySlot slot(this)`. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:113 > + exportedValue = JSValue(function); You can just use `exportedValue = function;`. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:114 > + } break; Use ` break; } ` instead. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:117 > + } break; Ditto > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:120 > + } break; Ditto > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:124 > + } break; Ditto > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:140 > + (void)state; Use `UNUSED_PARAM(state);`.

Comment on attachment 295322 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295322&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:117 > + EncodedJSValue result = vmEntryToWasm(code->code().executableAddress(), &vm, &protoCallFrame); It seems that this WASM code just returns the raw representation of the int32_t value, `42`. I think the compiled WASM code does not icnlude any code that attempts to box the result value because the WASM function can be called from the WASM code, right? So, we need to box the value returned by the WASM function according to the result type of the signature. In this case, first, we receive the value as uint64_t. And next, we need to box it according to the result type of the signature: "int32_t". We need to write `result = JSValue(static_cast<int32_t>(rawRepresentation))` in the `Wasm::I32` case. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:121 > + break; I think we need to return the value for Void case. (Maybe, undefined?). > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:124 > + case Wasm::I64: > + RELEASE_ASSERT_NOT_REACHED(); > + break; Does this never happen?

Comment on attachment 295322 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295322&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:75 > + const B3::Compilation* code = callable.code; Use `jsEntryPoint`, right? > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:92 > + arg = JSValue(arg.toInt32(state)); According to the testWasm.cpp, I think the correct way to box the argument in Wasm function is, `JSValue::decode(arg.toInt32(state))`. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:95 > + arg = JSValue(arg.toFloat(state)); According to the testWasm.cpp, I think the correct way to box the argument in Wasm function is, `JSValue::decode(bitwise_cast<uint32_t>(arg.toFloat(state)))`. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:98 > + arg = JSValue(arg.toNumber(state)); According to the testWasm.cpp, I think the correct way to box the argument in Wasm function is, `JSValue::decode(bitwise_cast<uint64_t>(arg.toFloat(state)))`. >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:117 >> + EncodedJSValue result = vmEntryToWasm(code->code().executableAddress(), &vm, &protoCallFrame); > > It seems that this WASM code just returns the raw representation of the int32_t value, `42`. > I think the compiled WASM code does not icnlude any code that attempts to box the result value because the WASM function can be called from the WASM code, right? > > So, we need to box the value returned by the WASM function according to the result type of the signature. > In this case, first, we receive the value as uint64_t. > And next, we need to box it according to the result type of the signature: "int32_t". > We need to write `result = JSValue(static_cast<int32_t>(rawRepresentation))` in the `Wasm::I32` case. Ah, no. Sorry. You should call `jsEntryPoint`'s code instead, right? >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:121 >> + break; > > I think we need to return the value for Void case. (Maybe, undefined?). Ignore this. See the updated comment.

Comment on attachment 295322 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295322&action=review >>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:117 >>> + EncodedJSValue result = vmEntryToWasm(code->code().executableAddress(), &vm, &protoCallFrame); >> >> It seems that this WASM code just returns the raw representation of the int32_t value, `42`. >> I think the compiled WASM code does not icnlude any code that attempts to box the result value because the WASM function can be called from the WASM code, right? >> >> So, we need to box the value returned by the WASM function according to the result type of the signature. >> In this case, first, we receive the value as uint64_t. >> And next, we need to box it according to the result type of the signature: "int32_t". >> We need to write `result = JSValue(static_cast<int32_t>(rawRepresentation))` in the `Wasm::I32` case. > > Ah, no. Sorry. You should call `jsEntryPoint`'s code instead, right? Sorry. I've just read testWasm.cpp and WasmB3IRGenerator.cpp. You still need some boxing code. >>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:121 >>> + break; >> >> I think we need to return the value for Void case. (Maybe, undefined?). > > Ignore this. See the updated comment. Need some boxing code. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:127 > + case Wasm::F64: Need boxing code here.

Created attachment 295426 [details] patch Address all comments. This now seems pretty good! It calls into wasm and returns the right value (42 of course). There's still a bunch more to do, but I could do that in a separate patch or add to this one. A few things I left open or answer questions: >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:61 >> - putDirectWithoutTransition(vm, Identifier::fromString(&vm, "exports"), m_moduleNamespaceObject.get(), None); >> + putDirect(vm, Identifier::fromString(&vm, "exports"), JSValue(m_moduleNamespaceObject.get()->moduleRecord()->moduleEnvironment()), None); > > We need to expose module namespace object. So the change of this line is not necessary. WebAssemblyModuleRecord::link does: symbolTablePutTouchWatchpointSet(moduleEnvironment, ...); So the properties are on the module environment, no? i.e. doing instance.exports.answer looks up what that line put on the module environment. Or do I misunderstand? I do need `putDirect` from this line though. >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:58 >> +struct CallableWebAssemblyFunction { > > How about storing JSWebAssemblyInstance here instead of WebAssemblyFunction? I'm not sure I understand the distinction: WebAssemblyFunction contains a WebAssemblyFunctionCell, which itself contains the CallableWebAssemblyFunction. IIUC they're all one-to-one, so putting the JSWebAssemblyInstance on the root (WebAssemblyFunction) or the leaf (CallableWebAssemblyFunction) is kind of the same, no? Although I can't put CallableWebAssemblyFunction in a WriteBarrier<> since it's not a JS object, right? So I'd have to make it a JS object first? Either seems fine to me, I just want to understand why it's better :-) >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:117 >> + EncodedJSValue result = vmEntryToWasm(code->code().executableAddress(), &vm, &protoCallFrame); > > It seems that this WASM code just returns the raw representation of the int32_t value, `42`. > I think the compiled WASM code does not icnlude any code that attempts to box the result value because the WASM function can be called from the WASM code, right? That's what I thought was happening, but I thought EncodedJSValue shouldn't be that way (it should be encoded)? Maybe it's a slight abuse of that type. > So, we need to box the value returned by the WASM function according to the result type of the signature. > In this case, first, we receive the value as uint64_t. > And next, we need to box it according to the result type of the signature: "int32_t". > We need to write `result = JSValue(static_cast<int32_t>(rawRepresentation))` in the `Wasm::I32` case. Yeah that's what I did for now, it works fine. I also changed to call through jsEntryPoint, but that seems to do the wrong thing (as you noticed, we still need some boxing). I'll check with Keith in a follow-up. Doesn't really matter for now since it seems to work. He'd mentioned something about removing one layer later (I'm guessing this code can be folded into jsEntryPoint as you thought was possible). >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:124 >> + case Wasm::I64: >> + RELEASE_ASSERT_NOT_REACHED(); >> + break; > > Does this never happen? It shouldn't happen for now: wasm import / export can have i64 in general, but a JS embedding of wasm must reject such a module with i64 import / export for now because JS can't handle it. We've talked about allowing something, but it seems better to wait for JS to have true i64 support than splitting in two or something similarly silly.

Attachment 295426 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:126: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 295426 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295426&action=review Nice! I can set r=me once you address the comments and add the ChangeLog with the description. And let's fix the build errors in gtk, mac, and mac-wk2 before landing. > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:61 > + putDirect(vm, Identifier::fromString(&vm, "exports"), JSValue(m_moduleNamespaceObject.get()->moduleRecord()->moduleEnvironment()), None); The module environment is not intended to be exposed to users' code. The module namespace object wraps this environment and provides the way to access these values. Here, we need to expose the module namespace object instead of the module environment. So, we need `putDirect(vm, Identifier::fromString(&vm, "exports"), m_moduleNamespaceObject.get(), None);` here. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:101 > + RETURN_IF_EXCEPTION(scope, JSValue::encode(arg)); Use `RETURN_IF_EXCEPTION(scope, encodedJSValue());` > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:118 > + EncodedJSValue rawResult = vmEntryToWasm(jsEntryPoint->code().executableAddress(), &vm, &protoCallFrame); Right. When fixing this issue, you need to fix vmEntryToWasm thing and the code generated in jsEntryPoint. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:126 > + return JSValue::encode(JSValue(static_cast<int32_t>(rawResult))); Let's fix this indentation. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:57 > + CallableWebAssemblyFunction() = delete; > + CallableWebAssemblyFunction(const CallableWebAssemblyFunction&) = delete; > + CallableWebAssemblyFunction(CallableWebAssemblyFunction&&) = default; > + CallableWebAssemblyFunction& operator=(const CallableWebAssemblyFunction&) = delete; How about using `WTF_MAKE_NONCOPYABLE()`? > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:58 > +}; My concern is that there are no explicit owner of the above B3::Compilation* and Wasm::Signature*. Who ensures the lifetime of these pointers? I thought WebAssemblyFunction::m_instance can ensure it. But it seems that this is not set. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:86 > + WriteBarrier<JSWebAssemblyInstance> m_instance; This should be set in anywhere. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:84 > + RETURN_IF_EXCEPTION(scope, JSValue::encode(startResult)); Use `RETURN_IF_EXCEPTION(scope, encodedJSValue());`.

Created attachment 295436 [details] patch Address a few minor comments, I still have to look into the more significant ones and fix the other builds.

Created attachment 295453 [details] patch Rebase and fix random things (still haven't addressed bigger comments, will look on plane).

Created attachment 295538 [details] patch Fix all comments (except changelog, this isn't quite ready yet). >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:61 >> + putDirect(vm, Identifier::fromString(&vm, "exports"), JSValue(m_moduleNamespaceObject.get()->moduleRecord()->moduleEnvironment()), None); > > The module environment is not intended to be exposed to users' code. > The module namespace object wraps this environment and provides the way to access these values. > Here, we need to expose the module namespace object instead of the module environment. I think you're right, *but*: - When I expose the module environment then `instance.answer()` is a recognized property and calling it works (it was added in `link` through `symbolTablePutTouchWatchpointSet`). - When I expose the module namespace object instead, it doesn't expose anything. IIUC this means `link` isn't doing all the work it needs? i.e. the slots aren't filled properly, and the exports resolved, right? I'll play with this. >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:58 > > My concern is that there are no explicit owner of the above B3::Compilation* and Wasm::Signature*. > Who ensures the lifetime of these pointers? > > I thought WebAssemblyFunction::m_instance can ensure it. But it seems that this is not set. Oops yeah I forgot to set the instance here! Thanks for catching this. After that, B3::Compilation are in CompiledFunctions which is owned by JSWebAssemblyModule as a Vector. They're ref-counted, so if the internet doesn't use all functions we can make this more complex by propagating the refcount (so exported-but-not-imported things can be freed). Each JSWebAssemblyInstance has a WriteBarrier to JSWebAssemblyModule, and each WebAssemblyFunction has a JSWebAssemblyInstance (so does the WebAssemblyModuleRecord). JSWebAssemblyModule also has ModuleInformation, which contains the Wasm::Signature for the exports and imports.

> >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:61 > >> + putDirect(vm, Identifier::fromString(&vm, "exports"), JSValue(m_moduleNamespaceObject.get()->moduleRecord()->moduleEnvironment()), None); > > > > The module environment is not intended to be exposed to users' code. > > The module namespace object wraps this environment and provides the way to access these values. > > Here, we need to expose the module namespace object instead of the module environment. > > I think you're right, *but*: > > - When I expose the module environment then `instance.answer()` is a > recognized property and calling it works (it was added in `link` through > `symbolTablePutTouchWatchpointSet`). > - When I expose the module namespace object instead, it doesn't expose > anything. IIUC this means `link` isn't doing all the work it needs? i.e. the > slots aren't filled properly, and the exports resolved, right? I'll play > with this. I'll punt this to another bug: https://bugs.webkit.org/show_bug.cgi?id=165121 I don't think it's super hard, but it'll be good to unblock other things quickly.

Created attachment 295550 [details] patch I think this is ready to go.

Comment on attachment 295550 [details] patch Attachment 295550 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2586575 New failing tests: intersection-observer/intersection-observer-entry-interface.html

Created attachment 295577 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 295550 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295550&action=review > JSTests/ChangeLog:25 > + An Instance's `exports` property wasn't populated with exports. > + > + According to the spec [0], `exports` should present itself as a WebAssembly > + Module Record. In order to do this we need to split JSModuleRecord into > + AbstractModuleRecord (without the `link` and `evaluate` functions), and > + JSModuleRecord (which implements link and evaluate). We can then have a separate > + WebAssemblyModuleRecord which shares most of the implementation. > + > + `exports` then maps function names to WebAssemblyFunction and > + WebAssemblyFunctionCell, which call into the B3-generated WebAssembly code. > + > + A follow-up patch will do imports. > + > + A few things of note: > + > + - Use Identifier instead of String. They get uniqued, we need them for the JSModuleNamespaceObject. This is safe because JSWebAssemblyModule creation is on the main thread. > + - JSWebAssemblyInstance needs to refer to the JSWebAssemblyModule used to create it, because the module owns the code, identifiers, etc. The world would be very sad if it got GC'd. > + - Instance.exports shouldn't use putWithoutTransition because it affects all Structures, whereas here each instance needs its own exports. > + - Expose the compiled functions, and pipe them to the InstanceConstructor. Start moving things around to split JSModuleRecord out into JS and WebAssembly parts. > + - LowLevelBinary: support 3-byte integers. > + - LowLevelBinary: support proper UTF-8 2003 code points (instead of UTF-16). Style: a lot of this is repeated inside JavaScriptCore/ChangeLog. Can you just put what's relevant to the test directory inside this changelog? > JSTests/wasm/js-api/test_Instance.js:24 > + const builder = (new Builder()) > + .Type().End() > + .Function().End() > + .Export() > + .Function("answer") > + .End() > + .Code() > + .Function("answer", { params: [], ret: "i32" }) > + .I32Const(42) > + .Return() > + .End() > + .End(); I'm new to this API, do you mind explaining what all this means? > JSTests/wasm/js-api/test_Instance.js:30 > + assert.eq(result, 42); Maybe add more tests for testing the calling into wasm layer? > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:234 > + i.module = Identifier::fromString(m_vm, moduleString); Style: Can we give this a better name than "i"? > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:288 > + size_t start = 0, end = 0; I'm not 100% sure what WebKit style guide says here. I suspect it says two size_t variables declared on their own lines. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:358 > + e.field = Identifier::fromString(m_vm, fieldString); Style: Can we give this a better name than "e"? > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:368 > + break; Style: break statement should be inside the block statement. > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:62 > + // putDirect(vm, Identifier::fromString(&vm, "exports"), JSValue(m_moduleNamespaceObject.get()), None); I'm not a fan of putting code in comments w/ a FIXME. Maybe just the FIXME is enough w/ more explanation? > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:62 > + Vector<JSValue> boxedArgs; To me, it would seem less sketchy to just write this as: Vector<uint64_t> args; And then later do: JSValue* values = bitwise_cast<JSValue*>(args.data()) and maybe a static_assert(sizeof(JSValue) == sizeof(uint64_t)) > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:95 > + JSValue firstArgument; > + int argCount = 1; > + JSValue* remainingArgs = nullptr; > + if (boxedArgs.size()) { > + remainingArgs = boxedArgs.data(); > + firstArgument = *remainingArgs; > + remainingArgs++; > + argCount = boxedArgs.size(); > + } > + > + ProtoCallFrame protoCallFrame; > + protoCallFrame.init(nullptr, nullptr, firstArgument, argCount, remainingArgs); This code doesn't look correct to me. What happens when boxedArgs.size() == 0? We potentially have garbage bits for firstArgument. Also, what you're passing for firstArgument is the |this| of the function. Is that intended? Also, you're passing nullptr for the callee. I think you want to pass the WebAssemblyFunction. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:118 > + WebAssemblyFunctionCell* functionCell = WebAssemblyFunctionCell::create(vm, WTFMove(callable)); Why are WebAssemblyFunction and WebAssemblyFunctionCell two different things if they're 1:1? > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:48 > + const B3::Compilation* jsEntryPoint; why "jsEntryPoint" and not just "entryPoint"? Not sure what's "JS" about the entry point, since it's Wasm code. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:57 > + CallableWebAssemblyFunction(CallableWebAssemblyFunction&&) = default; > + WTF_MAKE_NONCOPYABLE(CallableWebAssemblyFunction); > + CallableWebAssemblyFunction() = delete; Style: I think this usually goes at the top of the struct for WebKit style. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:83 > + // The export symbol table is the same for all Instances of a Modules. "a Modules" => "a Module" > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:85 > + for (auto& e : plan.getModuleInformation()->exports) { Style: please pick a better name than "e". Maybe "export"? > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:42 > +const ClassInfo WebAssemblyModuleRecord::s_info = { "WebAssemblyModuleRecord", &Base::s_info, 0, CREATE_METHOD_TABLE(WebAssemblyModuleRecord) }; Style: nullptr instead of 0 > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:96 > + for (const auto& e : moduleInformation.exports) { Style: Lets give this a better variable name than "e". Maybe 'export'? > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:115 > + break; Style: usually the 'break' statement goes inside the block. (below as well) > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:145 > + // FIXME this should call the module's `start` function, if any. Please open a bugzilla bug for this. > Source/WTF/ChangeLog:27 > + An Instance's `exports` property wasn't populated with exports. > + > + According to the spec [0], `exports` should present itself as a WebAssembly > + Module Record. In order to do this we need to split JSModuleRecord into > + AbstractModuleRecord (without the `link` and `evaluate` functions), and > + JSModuleRecord (which implements link and evaluate). We can then have a separate > + WebAssemblyModuleRecord which shares most of the implementation. > + > + `exports` then maps function names to WebAssemblyFunction and > + WebAssemblyFunctionCell, which call into the B3-generated WebAssembly code. > + > + A follow-up patch will do imports. > + > + A few things of note: > + > + - Use Identifier instead of String. They get uniqued, we need them for the JSModuleNamespaceObject. This is safe because JSWebAssemblyModule creation is on the main thread. > + - JSWebAssemblyInstance needs to refer to the JSWebAssemblyModule used to create it, because the module owns the code, identifiers, etc. The world would be very sad if it got GC'd. > + - Instance.exports shouldn't use putWithoutTransition because it affects all Structures, whereas here each instance needs its own exports. > + - Expose the compiled functions, and pipe them to the InstanceConstructor. Start moving things around to split JSModuleRecord out into JS and WebAssembly parts. > + - LowLevelBinary: support 3-byte integers. > + - LowLevelBinary: support proper UTF-8 2003 code points (instead of UTF-16). > + > + [0]: https://github.com/WebAssembly/design/blob/master/JS.md#webassemblyinstance-constructor Each changelog should just have information in it pertinent to what's changed under that directory. So this changelog should just have the "silence a warning" comment below

Created attachment 295617 [details] patch Address Saam's comments. A few notes: > > JSTests/wasm/js-api/test_Instance.js:24 > > + const builder = (new Builder()) > > + .Type().End() > > + .Function().End() > > + .Export() > > + .Function("answer") > > + .End() > > + .Code() > > + .Function("answer", { params: [], ret: "i32" }) > > + .I32Const(42) > > + .Return() > > + .End() > > + .End(); > > I'm new to this API, do you mind explaining what all this means? I updated JSTests/wasm/README.md to better explains things. > > JSTests/wasm/js-api/test_Instance.js:30 > > + assert.eq(result, 42); > > Maybe add more tests for testing the calling into wasm layer? Which part? There's a bunch of tests elsewhere (see the README for a list). The only new thing is the `exports` object. > > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:62 > > + Vector<JSValue> boxedArgs; > > To me, it would seem less sketchy to just write this as: > Vector<uint64_t> args; > And then later do: > JSValue* values = bitwise_cast<JSValue*>(args.data()) > and maybe a static_assert(sizeof(JSValue) == sizeof(uint64_t)) This would need to be a reinterpret_cast to be correct. I'm not sure this is the right thing though: we want to to coercions as I'm doing now, not just reinterpreting the bits. Or am I misunderstanding your suggestion? > > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:95 > > + JSValue firstArgument; > > + int argCount = 1; > > + JSValue* remainingArgs = nullptr; > > + if (boxedArgs.size()) { > > + remainingArgs = boxedArgs.data(); > > + firstArgument = *remainingArgs; > > + remainingArgs++; > > + argCount = boxedArgs.size(); > > + } > > + > > + ProtoCallFrame protoCallFrame; > > + protoCallFrame.init(nullptr, nullptr, firstArgument, argCount, remainingArgs); > > This code doesn't look correct to me. > What happens when boxedArgs.size() == 0? We potentially have garbage bits > for firstArgument. Also, what you're passing for firstArgument is the |this| > of the function. Is that intended? > Also, you're passing nullptr for the callee. I think you want to pass the > WebAssemblyFunction. That's imported from jsc.cpp:callWasmFunction. IIUC garbage in firstArgument can't be reached anyways (because the wasm signature enforces this), but I initialized it to empty to be safe. I asked Keith, |this| was easier at the time but he says we could change it. I changed the callee. > > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:118 > > + WebAssemblyFunctionCell* functionCell = WebAssemblyFunctionCell::create(vm, WTFMove(callable)); > > Why are WebAssemblyFunction and WebAssemblyFunctionCell two different things > if they're 1:1? Happy to change it, I though that was the idiomatic thing based on JSNativeStdFunction. You'd just put the CallableWebAssemblyFunction directly in the WebAssemblyFunction instead of having the indirection? > > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.h:48 > > + const B3::Compilation* jsEntryPoint; > > why "jsEntryPoint" and not just "entryPoint"? Not sure what's "JS" about the > entry point, since it's Wasm code. It's to keep the same naming as WasmFormat.h:FunctionCompilation. I think Keith's intent was "entry from JS" as opposed to "code" which is the entry from wasm. > > Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:85 > > + for (auto& e : plan.getModuleInformation()->exports) { > > Style: please pick a better name than "e". Maybe "export"? Can't be "export", it's a keyword in C++ :-) "exp" it is.

Comment on attachment 295617 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295617&action=review r=me with comments. > JSTests/wasm/Builder.js:385 > + // FIXME What's the FIXME? > JSTests/wasm/Builder.js:404 > + // FIXME ditto. > JSTests/wasm/Builder.js:423 > + // FIXME ditto. > JSTests/wasm/README.md:7 > +JavaScriptCore opcodes). I think you want to say "JavaScriptCore B3 opcodes" > JSTests/wasm/README.md:50 > + // Declare a Type section, which the builder will auto-fill as functions are defined. > + .Type().End() > + // Declare a Function section, which the builder will auto-fill as functions are defined. > + .Function().End() > + .Export() > + // Export the "answer" function (defined below) to JavaScript. > + .Function("answer") > + .End() > .Code() > - .Function() > - .Nop() > - .Nop() > + // The "answer" function takes no parameters, and returns an i32. > + .Function("answer", { params: [], ret: "i32" }) > + .I32Const(42) > + .Return() I would include an example with the control flow scope lambda. e.g. .Block(b => b.I32Const(42).Return) > Source/JavaScriptCore/ChangeLog:23 > + - Use Identifier instead of String. They get uniqued, we need them for the JSModuleNamespaceObject. This is safe because JSWebAssemblyModule creation is on the main thread. I don't think this always going to be the case, since if a user calls the promise api we probably want to return ASAP. Although, I'm not opposed to the change anyway. > Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp:101 > +auto AbstractModuleRecord::tryGetImportEntry(UniquedStringImpl* localName) -> std::optional<ImportEntry> http://www.reactiongifs.com/r/but-why.gif

Looks like you also have a CMake build issue.

Created attachment 295705 [details] patch >> Source/JavaScriptCore/ChangeLog:23 >> + - Use Identifier instead of String. They get uniqued, we need them for the JSModuleNamespaceObject. This is safe because JSWebAssemblyModule creation is on the main thread. > > I don't think this always going to be the case, since if a user calls the promise api we probably want to return ASAP. Although, I'm not opposed to the change anyway. Hmm... We only create Identifier at parse time, not compile time, so we could return the promise only after parse (and dispatching to compilation threads) or immediately. We could delay that if you think it's useful. Let's do it the right way when we get there. >> Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp:101 >> +auto AbstractModuleRecord::tryGetImportEntry(UniquedStringImpl* localName) -> std::optional<ImportEntry> > > http://www.reactiongifs.com/r/but-why.gif 😭 Yusuke likes fancy C++ 😭 > Looks like you also have a CMake build issue. Oh weird. It's complaining about not having a file that's totally there in the diff (it's not missing from CMakeLists, but rather from the filesystem). I pretty much only test with the cmake build and mine's happy! Let's try that again on the bots...

Comment on attachment 295617 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295617&action=review >> Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp:101 >> +auto AbstractModuleRecord::tryGetImportEntry(UniquedStringImpl* localName) -> std::optional<ImportEntry> > > http://www.reactiongifs.com/r/but-why.gif Because I don't want to write `std::optional<AbstractModuleRecord::ImportEntry>` here :)

Comment on attachment 295705 [details] patch Clearing flags on attachment: 295705 Committed r209123: <http://trac.webkit.org/changeset/209123>

All reviewed patches have been landed. Closing bug.