Bug 229710 – [WASM-Function-References] Add call_ref spec tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dmitry]

[WASM-Function-References] Add call_ref spec tests

[Dmitry]

Created attachment 436878 [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".

[Yusuke Suzuki]

Comment on attachment 436878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436878&action=review

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:431
>              return gTypeIdx(type);

Let's rename it to gRef(type).

> Source/JavaScriptCore/wasm/WasmFormat.h:88
> -    if (sub.isTypeIdx() && parent.isFuncref())
> +    if (sub.isRef() && parent.isRefNull() && sub.index == parent.index)
> +        return true;
> +
> +    if (sub.isRef() && parent.isFuncref())

Can you explain why this is the same to the old code?
1. Why don't we need to consider about `sub.isRefNull()`?
2. Why don't we need to consider about `parent.isRef()`?

> Source/JavaScriptCore/wasm/WasmFormat.h:102
> +inline bool isFunctionRefType(Type type)
> +{
> +    return type.isRef() || type.isRefNull();
>  }

Why doesn't it contain Funcref type? And if it it correct, can you rename this function since `isFunctionRefType` sounds like `isFuncref()`.

> Source/JavaScriptCore/wasm/WasmGlobal.cpp:51
>      case TypeKind::Externref:
>      case TypeKind::Funcref:
> -    case TypeKind::TypeIdx:
>          return m_value.m_externref.get();

It removed `case TypeKind::TypeIdx:`, but it does not have `TypeKind::Ref` etc. Can you explain why?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:-550
> -        case TypeKind::TypeIdx:

It removed `case TypeKind::TypeIdx:`, but it does not have `TypeKind::Ref` etc. Can you explain why?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:-608
> -        case TypeKind::TypeIdx:

It removed `case TypeKind::TypeIdx:`, but Ref/RefNull is RELEASE_ASSERT_NOT_REACHED. Can you explain why?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:-638
> -        case TypeKind::TypeIdx:

It removed `case TypeKind::TypeIdx:`, but Ref/RefNull is RELEASE_ASSERT_NOT_REACHED. Can you explain why?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:-110
> -            case TypeKind::TypeIdx:

It removed `case TypeKind::TypeIdx:`, but Ref/RefNull is RELEASE_ASSERT_NOT_REACHED. Can you explain why?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:-185
> -            case TypeKind::TypeIdx:

It removed `case TypeKind::TypeIdx:`, but Ref/RefNull is RELEASE_ASSERT_NOT_REACHED. Can you explain why?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:-329
> -        case TypeKind::TypeIdx:

It removed `case TypeKind::TypeIdx:`, but Ref/RefNull is RELEASE_ASSERT_NOT_REACHED. Can you explain why?

[Radar WebKit Bug Importer]

<rdar://problem/82817483>

[Dmitry]

Created attachment 437493 [details]
Patch

[Dmitry]

Comment on attachment 436878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436878&action=review

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:431
>>              return gTypeIdx(type);
> 
> Let's rename it to gRef(type).

fixed

>> Source/JavaScriptCore/wasm/WasmFormat.h:88
>> +    if (sub.isRef() && parent.isFuncref())
> 
> Can you explain why this is the same to the old code?
> 1. Why don't we need to consider about `sub.isRefNull()`?
> 2. Why don't we need to consider about `parent.isRef()`?

if TypedFunctionReferences are enabled then Externref and FunctionRef are represented as nullable references with corresponding indices - https://github.com/WebAssembly/function-references/blob/master/proposals/function-references/Overview.md#reference-types.
I refactored code so that previous rule was always true.

About Subtyping:
* all references in TFR have supertype - Funcref
* non-nullable references are subtype of nullable-references
* two references are equal if corresponding heaptypes (in our case indices) are equal

[Yusuke Suzuki]

Comment on attachment 437493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437493&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        Removed redundand TypeKind::TypeIdx because new Ref and RefNull opcodes cover all
> +        the same cases.

I have several questions. So can you answer to these questions? And can you describe the changelog to answer these questions?

1. "if TypedFunctionReferences are enabled then Externref and FunctionRef are represented as nullable references with corresponding indices". But I saw Wasm::Types::Funcref and Wasm::Types::Externref use in wasm/js/WebAssemblyGlobalConstructor.cpp, Table::wasmType, and TableInformation::wasmType. Can you explain why it is correct?
2. I saw several places are not having TypeKind::TypeIdx / TypeKind::Ref / TypeKind::RefNull. For example, BytecodeDumper::formatConstant has TypeKind::Externref and TypeKind::Funcref clauses, but it does not have Ref / RefNull etc. Can you explain why this is correct?

Also, can you add tests covering the above cases?

[Yusuke Suzuki]

Comment on attachment 437493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437493&action=review

> Source/JavaScriptCore/wasm/WasmGlobal.cpp:99
> -    case TypeKind::TypeIdx:
> +    case TypeKind::Ref:
> +    case TypeKind::RefNull:

Why is it correct? TypeKind::Externref has different code from TypeKind::Funcref.
If Externref is represented as RefNull with particular index, I think this code is not doing the same thing to the previous code.
Can you add a test to ensure that TypeKind::Externref code is working after enabling Options::useWebAssemblyTypedFunctionReferences()?

[Yusuke Suzuki]

Comment on attachment 437493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437493&action=review

> Source/JavaScriptCore/wasm/WasmParser.h:329
>              sigIndex = SignatureInformation::get(info.usedSignatures[heapType].get());

Isn't it possible that sigIndex becomes the same to TypeKind::Funcref / TypeKind::Externref?

[Yusuke Suzuki]

Comment on attachment 437493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437493&action=review

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:-110
> -            case TypeKind::TypeIdx:

It is just removed. And Externref and Funcref has different code. Why is it correct?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:-185
> -            case TypeKind::TypeIdx:

It is just removed. And Externref and Funcref has different code. Why is it correct?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:-329
> -        case TypeKind::TypeIdx:

It is just removed. And Externref and Funcref has different code. Why is it correct?

[Dmitry]

Created attachment 439723 [details]
Patch

[Dmitry]

Now all places when we use TypeKind::Funcref or TypeKind::Externref are patched to accept both old and new version of this type. By new I mean (ref null funcref) and (ref null externref).

I also added a few tests to be sure that everything works as expected but it is not too much because we sooner or later will add tests from the spec repo and so, I don't want to duplicate the efforts to write more tests, but I'm open for contr arguments.

[Yusuke Suzuki]

Comment on attachment 439723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439723&action=review

Looks much better! But r- since I found several bugs.

> Source/JavaScriptCore/wasm/WasmFormat.h:101
> +inline Type FuncrefType()

Function should not be title-case.
Rename it to funcrefType()

> Source/JavaScriptCore/wasm/WasmFormat.h:104
> +        return Wasm::Type {Wasm::TypeKind::RefNull, Wasm::Nullable::Yes, static_cast<Wasm::SignatureIndex>(Wasm::TypeKind::Funcref)};

Add space around {.

> Source/JavaScriptCore/wasm/WasmFormat.h:108
> +inline Type ExternrefType()

Function should not be title-case.
Rename it to externrefType()

> Source/JavaScriptCore/wasm/WasmFormat.h:111
> +        return Wasm::Type {Wasm::TypeKind::RefNull, Wasm::Nullable::Yes, static_cast<Wasm::SignatureIndex>(Wasm::TypeKind::Externref)};

Add space around {.

> Source/JavaScriptCore/wasm/WasmGlobal.cpp:97
> +        } else if (m_type.kind == TypeKind::Ref || m_type.kind == TypeKind::RefNull || m_type.kind == TypeKind::Funcref) {

Why isn't it `isFuncref(m_type)`?

> Source/JavaScriptCore/wasm/WasmOperations.cpp:568
> +                if (!isExternref(returnType) && !value.isCallable(vm)) {

Use `isFuncref(returnType)` instead of `!isExternref(returnType)`

> Source/JavaScriptCore/wasm/WasmParser.h:311
> +        sigIndex = static_cast<SignatureIndex>(typeKind);
> +        typeKind = TypeKind::RefNull;

We are using `TypeKind::Funcref` / `TypeKind::Externref`.
Why is it OK? Doesn't normal signature conflict with these numbers?

> Source/JavaScriptCore/wasm/WasmParser.h:329
>              sigIndex = SignatureInformation::get(info.usedSignatures[heapType].get());

Isn't it possible that sigIndex becomes the same to TypeKind::Funcref / TypeKind::Externref?

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:82
> +        const auto& argumentType = signature.argument(argIndex);

I think keeping switch statement and putting Wasm::isExternref checks in default clause is cleaner.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:286
> +        case Wasm::TypeKind::Ref:
> +        case Wasm::TypeKind::RefNull:

I think Externref is represented as a part of RefNull. Why is RefNull separated from `case Wasm::TypeKind::Externref`? This is handling RefNull-based Externref as Funcref, correct?

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:-249
> -                    switch (moduleInformation.globals[import.kindIndex].type.kind) {

Keep using switch since it is cleaner.
Add Wasm::isExternref etc. handling in default clause instead.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:-304
> -                    switch (globalType.kind) {

Ditto. Keep using switch.

[Dmitry]

Comment on attachment 439723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439723&action=review

>> Source/JavaScriptCore/wasm/WasmFormat.h:101
>> +inline Type FuncrefType()
> 
> Function should not be title-case.
> Rename it to funcrefType()

done

>> Source/JavaScriptCore/wasm/WasmFormat.h:104
>> +        return Wasm::Type {Wasm::TypeKind::RefNull, Wasm::Nullable::Yes, static_cast<Wasm::SignatureIndex>(Wasm::TypeKind::Funcref)};
> 
> Add space around {.

done

>> Source/JavaScriptCore/wasm/WasmFormat.h:108
>> +inline Type ExternrefType()
> 
> Function should not be title-case.
> Rename it to externrefType()

done

>> Source/JavaScriptCore/wasm/WasmFormat.h:111
>> +        return Wasm::Type {Wasm::TypeKind::RefNull, Wasm::Nullable::Yes, static_cast<Wasm::SignatureIndex>(Wasm::TypeKind::Externref)};
> 
> Add space around {.

done

>> Source/JavaScriptCore/wasm/WasmGlobal.cpp:97
>> +        } else if (m_type.kind == TypeKind::Ref || m_type.kind == TypeKind::RefNull || m_type.kind == TypeKind::Funcref) {
> 
> Why isn't it `isFuncref(m_type)`?

`isFuncref(type)` means that type has kind Funcref (it doesn't matter how it is represented).
Here I handled two cases - `Funcref || any ref || any refnull` because it has the same code.

But I made it cleaner and split all three cases - Funcref, Externref and Ref|RefNull with type idx.

>> Source/JavaScriptCore/wasm/WasmOperations.cpp:568
>> +                if (!isExternref(returnType) && !value.isCallable(vm)) {
> 
> Use `isFuncref(returnType)` instead of `!isExternref(returnType)`

done

>> Source/JavaScriptCore/wasm/WasmParser.h:311
>> +        typeKind = TypeKind::RefNull;
> 
> We are using `TypeKind::Funcref` / `TypeKind::Externref`.
> Why is it OK? Doesn't normal signature conflict with these numbers?

I think it is OK, since `funcref and externref are reinterpreted as abbreviations (in both binary and text format) for (ref null func) and (ref null extern), respectively` (https://github.com/WebAssembly/function-references/blob/master/proposals/function-references/Overview.md#reference-types)
So here we meet typeKind == TypeKind::Funcref and typed function references is enabled so we transform these old representations to `RefNull`.

what is normal signature?

>> Source/JavaScriptCore/wasm/WasmParser.h:329
>>              sigIndex = SignatureInformation::get(info.usedSignatures[heapType].get());
> 
> Isn't it possible that sigIndex becomes the same to TypeKind::Funcref / TypeKind::Externref?

I think no because TypeKind::Funcref/TypeKind::Externref represented as negative numbers and we shouldn't allow such big indices for heap types to be equal to TypeKind::Funcref/TypeKind::Externref.
See the maximum for usedSignatures here: https://webkit-search.igalia.com/webkit/source/Source/JavaScriptCore/wasm/WasmLimits.h#40
And we check it here: https://webkit-search.igalia.com/webkit/source/Source/JavaScriptCore/wasm/WasmSectionParser.cpp#46

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:82
>> +        const auto& argumentType = signature.argument(argIndex);
> 
> I think keeping switch statement and putting Wasm::isExternref checks in default clause is cleaner.

done because most of the logic moved to `fromJSValue`

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:286
>> +        case Wasm::TypeKind::RefNull:
> 
> I think Externref is represented as a part of RefNull. Why is RefNull separated from `case Wasm::TypeKind::Externref`? This is handling RefNull-based Externref as Funcref, correct?

Sorry, this is mess. I've merged all cases together to avoid `FALLTHROUGH`, now it should be clear.

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:-249
>> -                    switch (moduleInformation.globals[import.kindIndex].type.kind) {
> 
> Keep using switch since it is cleaner.
> Add Wasm::isExternref etc. handling in default clause instead.

done

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:-304
>> -                    switch (globalType.kind) {
> 
> Ditto. Keep using switch.

done

[Dmitry]

Created attachment 442301 [details]
Patch

[Dmitry]

Created attachment 442354 [details]
Patch

[Dmitry]

Created attachment 442571 [details]
Patch

[Dmitry]

Created attachment 442573 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 442573 [details]
Patch

r=me

[EWS]

Committed r284935 (243605@main): <https://commits.webkit.org/243605@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442573 [details].