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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=226296

--- Comment #8 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 429937
  --> https://bugs.webkit.org/attachment.cgi?id=429937
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210604/1167489f/attachment-0001.htm>