Bug 219192 – [WASM-References] Add support for active mods in element section

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

[Dmitry]

[WASM-References] Add support for active mods in element section

[Dmitry]

Created attachment 414651 [details]
Patch

[Dmitry]

Created attachment 414655 [details]
Patch

[Dmitry]

Haven't supported passive & declarative segments because there is no table.init instruction for it in JSC yet.

I'm going to add table.init with passive segments in next patches.
Then support declarative segments and add all tests from the ref-types spec repo.

[Dmitry]

Created attachment 414666 [details]
Patch

[Dmitry]

Created attachment 414886 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71758493>

[Keith Miller]

Comment on attachment 414886 [details]
Patch

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

overall looks good, but I think we should address the Optional vs sentinel value issue.

> Source/JavaScriptCore/ChangeLog:7
> +        Adjust wasm parser to parse new form of element section.

Nit: We usually put a new line here.

> Source/JavaScriptCore/wasm/WasmFormat.h:250
> +    Vector<Optional<uint32_t>> functionIndices;

Since this is long lived, and could be very large (many modules I've seen have 10k+ functions), we should probably use a sentinel value that's not realistic like UINT_MAX. Right now I think this is doubling the memory of this vector.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:611
> +            if (!element.functionIndices[i].hasValue()) {

Nit: I think you can just do `if (!element.functionIndices[I])`

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:-624
> -                    ++elementIndex;

lol we have quality code...

[Saam Barati]

Comment on attachment 414886 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        Add support for vec(exp) in element section:

can you explain what it used t be and how this is different?

> Source/JavaScriptCore/ChangeLog:12
> +        Just transformed (ref.func idx) -> idx and (ref.null func) -> null.

I don't know what this means.

> Source/JavaScriptCore/ChangeLog:13
> +        For null values call table->clear to zero value.

I don't understand the significance here. Can you explain it more

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:386
> +            const uint32_t tableIndex = 0;

nit: can be constexpr

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:412
> +        case 0x01: {
> +            WASM_PARSER_FAIL_IF(!Options::useWebAssemblyReferences(), "references are not enabled");
> +            WASM_PARSER_FAIL_IF(true, "unsupported ", elementNum, "th Element reserved byte");
> +            break;
> +        }

What do we need to do to support this? Maybe we should have a bug?

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:432
> +                Optional<uint32_t> funcIdx;

Why optional here? Seems like a weird choice since we fail unless we parse a real uint32_t

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:443
> +        case 0x03: {
> +            WASM_PARSER_FAIL_IF(!Options::useWebAssemblyReferences(), "references are not enabled");
> +            WASM_PARSER_FAIL_IF(true, "unsupported ", elementNum, "th Element reserved byte");
> +            break;

Should we have a bug on implementing this?

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:472
> +            WASM_PARSER_FAIL_IF(true, "unsupported ", elementNum, "th Element reserved byte");

What do we need to do to support this? Maybe we should have a bug?

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:487
> +            WASM_PARSER_FAIL_IF(refType == Externref, "reftype in element section should be funcref");

I think the error message and the condition here are a bit worrisome if anyone adds another enum value. Why can't we just directly check if != Funcref?

[Dmitry]

Created attachment 415097 [details]
Patch

[Dmitry]

Comment on attachment 414886 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:7
>> +        Adjust wasm parser to parse new form of element section.
> 
> Nit: We usually put a new line here.

Fixed.

>> Source/JavaScriptCore/ChangeLog:10
>> +        Add support for vec(exp) in element section:
> 
> can you explain what it used t be and how this is different?

Removed implementation details from high-level description.

>> Source/JavaScriptCore/wasm/WasmFormat.h:250
>> +    Vector<Optional<uint32_t>> functionIndices;
> 
> Since this is long lived, and could be very large (many modules I've seen have 10k+ functions), we should probably use a sentinel value that's not realistic like UINT_MAX. Right now I think this is doubling the memory of this vector.

Fixed this and all other issues. Now design of this is just one-one copy from SpiderMonkey :)

[Yusuke Suzuki]

Comment on attachment 415097 [details]
Patch

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

r=me with suggestions.

> Source/JavaScriptCore/wasm/WasmFormat.h:233
> +    constexpr static uint32_t NullFuncIndex = UINT32_MAX;

Let's define it as `nullFuncIndex`, which is WebKit's naming convention for constants.

> Source/JavaScriptCore/wasm/WasmFormat.h:235
> +    enum class Kind {

Let's make it `enum class Kind : uint8_t`.

> Source/JavaScriptCore/wasm/WasmFormat.h:248
> +    bool active() const { return kind == Kind::Active; }

isActive() would be better.

> Source/JavaScriptCore/wasm/WasmFormat.h:253
> +    TableElementType elemType;

Let's name it elementType.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:417
> +            // https://bugs.webkit.org/show_bug.cgi?id=219297

Add FIXME with comment about what is missing.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:428
> +            uint8_t elemKind;

=> elementKind.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:450
> +            // https://bugs.webkit.org/show_bug.cgi?id=219385

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:480
> +            // https://bugs.webkit.org/show_bug.cgi?id=219297

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:516
> +            // https://bugs.webkit.org/show_bug.cgi?id=219385

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.h:75
> +    PartialResult WARN_UNUSED_RETURN parseElemKind(uint8_t& elemKind);
> +    PartialResult WARN_UNUSED_RETURN parseIndexCountForElemSection(uint32_t&, const unsigned);
> +    PartialResult WARN_UNUSED_RETURN parseFuncIdxFromRefExpForElemSection(uint32_t&, const unsigned, const unsigned);
> +    PartialResult WARN_UNUSED_RETURN parseFuncIdxForElemSection(uint32_t&, const unsigned, const unsigned);

Let's rename Elem to Element. WebKit prefers non-abbreviated names :)
And since we have parseElement, using "Element" is good for consistency.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:558
> +            if (element.active()) {

if (!element.isActive())
    continue;

can be cleaner.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:619
>              // for the import.

Let's move `uint32_t functionIndex = element.functionIndices[i];` to the prologue of this loop body and use `functionIndex` instead of `element.functionIndices[i]`.

[Dmitry]

Created attachment 415193 [details]
Patch

[Dmitry]

Created attachment 415195 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 415195 [details]
Patch

r=me

[EWS]

Committed r270344: <https://trac.webkit.org/changeset/270344>

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