Bug 226296 – [WASM-Function-References] Add support for (ref null? $t) type constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[Asumu Takikawa]

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

[Asumu Takikawa]

Created attachment 429803 [details]
Patch

[EWS Watchlist]

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

[Dmitry]

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

[Yusuke Suzuki]

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.

[Asumu Takikawa]

Created attachment 429937 [details]
Patch

[Asumu Takikawa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/78784066>

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Comment on attachment 429937 [details]
Patch

For now, putting r- since we need some handling in IC.

[Asumu Takikawa]

Created attachment 431350 [details]
Patch

[Asumu Takikawa]

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

[Asumu Takikawa]

Created attachment 432202 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 432202 [details]
Patch

r=me

[EWS]

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