Bug 229710 - [WASM-Function-References] Add call_ref spec tests
Summary: [WASM-Function-References] Add call_ref spec tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry
URL:
Keywords: InRadar
Depends on:
Blocks: 247393
  Show dependency treegraph
 
Reported: 2021-08-31 05:30 PDT by Dmitry
Modified: 2022-11-02 17:08 PDT (History)
8 users (show)

See Also:


Attachments
Patch (41.14 KB, patch)
2021-08-31 05:41 PDT, Dmitry
no flags Details | Formatted Diff | Diff
Patch (54.84 KB, patch)
2021-09-07 06:08 PDT, Dmitry
no flags Details | Formatted Diff | Diff
Patch (72.85 KB, patch)
2021-09-30 04:08 PDT, Dmitry
no flags Details | Formatted Diff | Diff
Patch (82.18 KB, patch)
2021-10-24 03:55 PDT, Dmitry
no flags Details | Formatted Diff | Diff
Patch (78.11 KB, patch)
2021-10-25 03:27 PDT, Dmitry
no flags Details | Formatted Diff | Diff
Patch (78.02 KB, patch)
2021-10-26 23:20 PDT, Dmitry
no flags Details | Formatted Diff | Diff
Patch (78.07 KB, patch)
2021-10-27 02:04 PDT, Dmitry
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry 2021-08-31 05:30:44 PDT
[WASM-Function-References] Add call_ref spec tests
Comment 1 Dmitry 2021-08-31 05:41:29 PDT
Created attachment 436878 [details]
Patch
Comment 2 EWS Watchlist 2021-08-31 05:42:27 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 Yusuke Suzuki 2021-08-31 13:10:37 PDT
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?
Comment 4 Radar WebKit Bug Importer 2021-09-07 05:31:18 PDT
<rdar://problem/82817483>
Comment 5 Dmitry 2021-09-07 06:08:18 PDT
Created attachment 437493 [details]
Patch
Comment 6 Dmitry 2021-09-07 06:14:46 PDT
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
Comment 7 Yusuke Suzuki 2021-09-15 00:44:47 PDT
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?
Comment 8 Yusuke Suzuki 2021-09-15 00:55:03 PDT
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()?
Comment 9 Yusuke Suzuki 2021-09-15 00:58:19 PDT
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?
Comment 10 Yusuke Suzuki 2021-09-15 01:00:55 PDT
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?
Comment 11 Dmitry 2021-09-30 04:08:47 PDT
Created attachment 439723 [details]
Patch
Comment 12 Dmitry 2021-09-30 04:13:08 PDT
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.
Comment 13 Yusuke Suzuki 2021-10-05 04:51:53 PDT
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.
Comment 14 Dmitry 2021-10-24 03:55:15 PDT
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
Comment 15 Dmitry 2021-10-24 03:55:59 PDT
Created attachment 442301 [details]
Patch
Comment 16 Dmitry 2021-10-25 03:27:45 PDT
Created attachment 442354 [details]
Patch
Comment 17 Dmitry 2021-10-26 23:20:26 PDT
Created attachment 442571 [details]
Patch
Comment 18 Dmitry 2021-10-27 02:04:53 PDT
Created attachment 442573 [details]
Patch
Comment 19 Yusuke Suzuki 2021-10-27 11:58:15 PDT
Comment on attachment 442573 [details]
Patch

r=me
Comment 20 EWS 2021-10-27 12:29:05 PDT
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].