This is a bug for adding the `(ref null? $t)` type constructor, which is part of the typed function references proposal (https://github.com/WebAssembly/function-references) and also needed for GC and type imports proposals. The internal representation for such types was mostly already added in https://bugs.webkit.org/show_bug.cgi?id=222779. What's needed in addition is the ability to parse such types from Wasm binaries, and to represent nullability in the type. Adding nullability requires some additional checks in the Wasm/JS boundary in case the type is marked non-null (before typed function references, all reference types are nullable).
Created attachment 429803 [details] Patch
This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".
Comment on attachment 429803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429803&action=review > Source/JavaScriptCore/wasm/WasmParser.h:280 > + Type type = {static_cast<TypeKind>(typeKind), true, 0}; I would rather use `enum class Nullable = {Yes, No}` instead of `bool` so we can use Nullable::Yes here, but I don't mind `bool` too
Comment on attachment 429803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429803&action=review Nice. Quick comment on ChangeLog files. > Source/JavaScriptCore/ChangeLog:85 > + JSTests: > + * wasm/function-references/ref_types.js: Added. > + (module): > + (async testRefTypeLocal): > + (async testNonNullRefTypeLocal): > + (async testRefTypeInSignature): > + (async testRefTypeParamCheck): > + (async testRefGlobalCheck): > + (async testExternFuncrefNonNullCheck): > + (async testExternrefCompatibility): > + (async testNonNullExternrefIncompatible): > + (async testFuncrefCompatibility): > + (async testNonNullFuncrefIncompatible): > + * wasm/wasm.json: > + > + Source/JavaScriptCore: > + * wasm/WasmAirIRGenerator.cpp: > + (JSC::Wasm::AirIRGenerator::gTypeIdx): > + (JSC::Wasm::AirIRGenerator::tmpForType): > + (JSC::Wasm::AirIRGenerator::emitCCall): > + (JSC::Wasm::AirIRGenerator::moveOpForValueType): > + (JSC::Wasm::AirIRGenerator::AirIRGenerator): > + (JSC::Wasm::AirIRGenerator::addLocal): > + (JSC::Wasm::AirIRGenerator::addConstant): > + (JSC::Wasm::AirIRGenerator::addRefFunc): > + * wasm/WasmCallingConvention.h: > + (JSC::Wasm::WasmCallingConvention::marshallLocation const): > + (JSC::Wasm::JSCallingConvention::marshallLocation const): > + * wasm/WasmFormat.h: > + (JSC::Wasm::isSubtype): > + (JSC::Wasm::isValidHeapTypeKind): > + (JSC::Wasm::isDefaultableType): > + * wasm/WasmFunctionParser.h: > + (JSC::Wasm::FunctionParser<Context>::parse): > + (JSC::Wasm::FunctionParser<Context>::parseAnnotatedSelectImmediates): > + (JSC::Wasm::FunctionParser<Context>::checkBranchTarget): > + (JSC::Wasm::FunctionParser<Context>::parseExpression): > + * wasm/WasmGlobal.cpp: > + (JSC::Wasm::Global::get const): > + (JSC::Wasm::Global::set): > + * wasm/WasmLLIntGenerator.cpp: > + (JSC::Wasm::LLIntGenerator::callInformationForCaller): > + (JSC::Wasm::LLIntGenerator::callInformationForCallee): > + (JSC::Wasm::LLIntGenerator::addArguments): > + * wasm/WasmParser.h: > + (JSC::Wasm::Parser<SuccessType>::parseBlockSignature): > + (JSC::Wasm::Parser<SuccessType>::parseValueType): > + (JSC::Wasm::Parser<SuccessType>::parseRefType): > + * wasm/WasmSectionParser.cpp: > + (JSC::Wasm::SectionParser::parseType): > + (JSC::Wasm::SectionParser::parseElement): > + (JSC::Wasm::SectionParser::parseInitExpr): > + (JSC::Wasm::SectionParser::parseElementSegmentVectorOfExpressions): > + (JSC::Wasm::SectionParser::parseGlobalType): > + * wasm/WasmSignature.cpp: > + (JSC::Wasm::computeHash): > + * wasm/generateWasmOpsHeader.py: > + * wasm/js/WasmToJS.cpp: > + (JSC::Wasm::wasmToJS): > + * wasm/js/WebAssemblyFunction.cpp: > + (JSC::JSC_DEFINE_HOST_FUNCTION): > + * wasm/js/WebAssemblyModuleRecord.cpp: > + (JSC::WebAssemblyModuleRecord::linkImpl): > + * wasm/wasm.json: Please remove this duplicate entry. And these entries are including files that are not in Source/JavaScriptCore. > JSTests/ChangeLog:75 > + Source/JavaScriptCore: > + * wasm/WasmAirIRGenerator.cpp: > + (JSC::Wasm::AirIRGenerator::gTypeIdx): > + (JSC::Wasm::AirIRGenerator::tmpForType): > + (JSC::Wasm::AirIRGenerator::emitCCall): > + (JSC::Wasm::AirIRGenerator::moveOpForValueType): > + (JSC::Wasm::AirIRGenerator::AirIRGenerator): > + (JSC::Wasm::AirIRGenerator::addLocal): > + (JSC::Wasm::AirIRGenerator::addConstant): > + (JSC::Wasm::AirIRGenerator::addRefFunc): > + * wasm/WasmCallingConvention.h: > + (JSC::Wasm::WasmCallingConvention::marshallLocation const): > + (JSC::Wasm::JSCallingConvention::marshallLocation const): > + * wasm/WasmFormat.h: > + (JSC::Wasm::isSubtype): > + (JSC::Wasm::isValidHeapTypeKind): > + (JSC::Wasm::isDefaultableType): > + * wasm/WasmFunctionParser.h: > + (JSC::Wasm::FunctionParser<Context>::parse): > + (JSC::Wasm::FunctionParser<Context>::parseAnnotatedSelectImmediates): > + (JSC::Wasm::FunctionParser<Context>::checkBranchTarget): > + (JSC::Wasm::FunctionParser<Context>::parseExpression): > + * wasm/WasmGlobal.cpp: > + (JSC::Wasm::Global::get const): > + (JSC::Wasm::Global::set): > + * wasm/WasmLLIntGenerator.cpp: > + (JSC::Wasm::LLIntGenerator::callInformationForCaller): > + (JSC::Wasm::LLIntGenerator::callInformationForCallee): > + (JSC::Wasm::LLIntGenerator::addArguments): > + * wasm/WasmParser.h: > + (JSC::Wasm::Parser<SuccessType>::parseBlockSignature): > + (JSC::Wasm::Parser<SuccessType>::parseValueType): > + (JSC::Wasm::Parser<SuccessType>::parseRefType): > + * wasm/WasmSectionParser.cpp: > + (JSC::Wasm::SectionParser::parseType): > + (JSC::Wasm::SectionParser::parseElement): > + (JSC::Wasm::SectionParser::parseInitExpr): > + (JSC::Wasm::SectionParser::parseElementSegmentVectorOfExpressions): > + (JSC::Wasm::SectionParser::parseGlobalType): > + * wasm/WasmSignature.cpp: > + (JSC::Wasm::computeHash): > + * wasm/generateWasmOpsHeader.py: > + * wasm/js/WasmToJS.cpp: > + (JSC::Wasm::wasmToJS): > + * wasm/js/WebAssemblyFunction.cpp: > + (JSC::JSC_DEFINE_HOST_FUNCTION): > + * wasm/js/WebAssemblyModuleRecord.cpp: > + (JSC::WebAssemblyModuleRecord::linkImpl): > + * wasm/wasm.json: > + They are JSC's entries. Please do not include them in JSTests/ChangeLog. Entries should be auto-generated by webkit-patch upload --update-changelogs.
Created attachment 429937 [details] Patch
Thanks for the feedback! I changed the patch to use an enum field so that the constructor uses are clearer (and added a `isNullable` method so that checking it is nearly the same as before). I believe I've also fixed up the ChangeLogs now.
<rdar://problem/78784066>
Comment on attachment 429937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429937&action=review The direction looks good! I left several comments. The main ones are related to IC and Wasm's global set. > Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1101 > + result = tmpForType(Type {TypeKind::TypeIdx, Nullable::No, signatureIndex}); Insert space around { and }. > Source/JavaScriptCore/wasm/WasmGlobal.cpp:118 > - if (!isWebAssemblyHostFunction(vm, argument) && !argument.isNull()) { > + bool isNullable = m_type.isNullable(); > + WebAssemblyFunction* wasmFunction = nullptr; > + WebAssemblyWrapperFunction* wasmWrapperFunction = nullptr; > + if (!isWebAssemblyHostFunction(vm, argument, wasmFunction, wasmWrapperFunction) && (!isNullable || !argument.isNull())) { > throwException(globalObject, throwScope, createJSWebAssemblyRuntimeError(globalObject, vm, "Funcref must be an exported wasm function")); > return; > } > + if (m_type.kind == TypeKind::TypeIdx && (wasmFunction || wasmWrapperFunction)) { > + Wasm::SignatureIndex paramIndex = m_type.index; > + Wasm::SignatureIndex argIndex; > + if (wasmFunction) > + argIndex = wasmFunction->signatureIndex(); > + else > + argIndex = wasmWrapperFunction->signatureIndex(); > + if (paramIndex != argIndex) { > + throwException(globalObject, throwScope, createJSWebAssemblyRuntimeError(globalObject, vm, "Argument function did not match the reference type")); > + return; > + } > + } Does this nullable check happen in Wasm's global set? Can we have a test that ensures this exception happens in wasm too? > Source/JavaScriptCore/wasm/WasmParser.h:332 > + Type type = {typeKind, static_cast<Nullable>(isNullable), sigIndex}; Ditto. Insert spaces around { and }. > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:622 > + resultType = {TypeKind::TypeIdx, Nullable::No, signatureIndex}; Insert spaces around { and }. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:103 > break; We need to have the same kind of handling in jsCallEntrypointSlow. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:107 > + if (!signature.argument(argIndex).isNullable() && arg.isNull()) > + return JSValue::encode(throwException(globalObject, scope, createJSWebAssemblyRuntimeError(globalObject, vm, "Non-null Externref cannot be null"))); Ditto.
Comment on attachment 429937 [details] Patch For now, putting r- since we need some handling in IC.
Created attachment 431350 [details] Patch
Thanks for pointing out these issues! I've pushed a new patch that adds the IC paths in `WebAssemblyFunction.cpp`, and also tests them (by looping for the relevant test cases). > Does this nullable check happen in Wasm's global set? Can we have a test that ensures this exception happens in wasm too? This check should not be needed at runtime for global set, because Wasm validation should prevent it from compiling if a null argument is passed. I have added a test case that makes sure this validation happens to the end of `testRefGlobalCheck()`.
Created attachment 432202 [details] Patch
Comment on attachment 432202 [details] Patch r=me
Committed r279265 (239144@main): <https://commits.webkit.org/239144@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432202 [details].