Bug 226296 - [WASM-Function-References] Add support for (ref null? $t) type constructor
Summary: [WASM-Function-References] Add support for (ref null? $t) type constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-26 14:51 PDT by Asumu Takikawa
Modified: 2021-06-24 19:23 PDT (History)
9 users (show)

See Also:


Attachments
Patch (56.46 KB, patch)
2021-05-26 15:23 PDT, Asumu Takikawa
no flags Details | Formatted Diff | Diff
Patch (51.33 KB, patch)
2021-05-27 15:12 PDT, Asumu Takikawa
no flags Details | Formatted Diff | Diff
Patch (54.96 KB, patch)
2021-06-14 12:01 PDT, Asumu Takikawa
no flags Details | Formatted Diff | Diff
Patch (55.16 KB, patch)
2021-06-24 12:25 PDT, Asumu Takikawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Asumu Takikawa 2021-05-26 14:51:15 PDT
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).
Comment 1 Asumu Takikawa 2021-05-26 15:23:47 PDT
Created attachment 429803 [details]
Patch
Comment 2 EWS Watchlist 2021-05-26 15:24:46 PDT
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 3 Dmitry 2021-05-27 00:30:16 PDT
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 4 Yusuke Suzuki 2021-05-27 03:07:15 PDT
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.
Comment 5 Asumu Takikawa 2021-05-27 15:12:06 PDT
Created attachment 429937 [details]
Patch
Comment 6 Asumu Takikawa 2021-05-27 15:17:10 PDT
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.
Comment 7 Radar WebKit Bug Importer 2021-06-02 14:52:57 PDT
<rdar://problem/78784066>
Comment 8 Yusuke Suzuki 2021-06-04 01:16:56 PDT
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 9 Yusuke Suzuki 2021-06-07 20:28:47 PDT
Comment on attachment 429937 [details]
Patch

For now, putting r- since we need some handling in IC.
Comment 10 Asumu Takikawa 2021-06-14 12:01:14 PDT
Created attachment 431350 [details]
Patch
Comment 11 Asumu Takikawa 2021-06-14 12:06:35 PDT
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()`.
Comment 12 Asumu Takikawa 2021-06-24 12:25:03 PDT
Created attachment 432202 [details]
Patch
Comment 13 Yusuke Suzuki 2021-06-24 18:56:59 PDT
Comment on attachment 432202 [details]
Patch

r=me
Comment 14 EWS 2021-06-24 19:23:36 PDT
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].