Bug 198760 – [WASM-References] Add support for multiple tables

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Justin Michaud]

Add support for multiple tables

[Justin Michaud]

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

[Saam Barati]

Comment on attachment 372080 [details]
Patch

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

Mostly LGTM, but some questions and I think I found a bug.

> Source/JavaScriptCore/ChangeLog:9
> +        existing users to give a table index. We also add some extra checks for table.get/set.

extra checks. Why didn't we do this before? Can you explain?

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:958
> +        if (idx >= instance->table(tableIndex)->length())
> +            return 0;

why? We didn't do this before.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:96
> +    *bitwise_cast<Table**>(bitwise_cast<char*>(this) + offsetOfTablePtr(m_numImportFunctions, i)) = &table.leakRef();
> +    this->table(i)->ref();

this is wrong. this table is already +1, and you're leaking it, then ref() after. I think leakRef is all you need.

> Source/JavaScriptCore/wasm/WasmInstance.h:147
> +        return (offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions + sizeof(Table*) * numTables).unsafeGet();

I think you need to do rounding here after the number of import functions. Or add some static asserts that they have same alignment as ImportFunctionInfo

[Justin Michaud]

Comment on attachment 372080 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:958
>> +            return 0;
> 
> why? We didn't do this before.

I never wrote a test case for out of bounds access until now. It should trap according to the semantics, but instead we just hit the release assert. I am moving these checks to the funcref patch since I responded to the EncodedJSValue comments there.

[Justin Michaud]

Created attachment 372175 [details]
patch (requires funcref patch)

This is the patch with the comments above addressed, and rebased to apply after the funcref patch

[Justin Michaud]

Created attachment 372266 [details]
Patch

[Saam Barati]

Comment on attachment 372266 [details]
Patch

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

r=me with comments. I found one bug, and the rest are nits. Also, I didn't read through all testing, but I commented in areas where it would be good to have tests.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1918
> +        RELEASE_ASSERT(Arg::isValidAddrForm(Instance::offsetOfTablePtr(m_numImportFunctions, tableIndex), B3::Width64));

this is now wrong. Before, it was guaranteed, but now it's not. On arm64, this may not fit in an immediate. Please add a test and make this a branch.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1923
> +        append(Move, Arg::addr(instanceValue(), Instance::offsetOfTablePtr(m_numImportFunctions, tableIndex)), callableFunctionBufferLength);

this is where the branch should be. Basically, look for "isValidForm", and you need to branch on instruction selection based on that. Should be easy to make an arm64 test fail by having many, many, tables

> Source/JavaScriptCore/wasm/WasmInstance.cpp:113
> +    if (auto* t = this->table(i))
> +        t->deref();

t => table

> Source/JavaScriptCore/wasm/WasmInstance.h:157
> +        return (offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions + sizeof(Table*) * numTables).unsafeGet();

I think this should be:

roundUpToMultipleOf<sizeof(Table*)>((offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions) + sizeof(Table*)*numTables

Or static assert

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:219
>      WASM_PARSER_FAIL_IF(!parseVarUInt32(count), "can't get Table's count");

let's add a maximum number of tables here

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:379
> +        WASM_PARSER_FAIL_IF(m_info->tables[tableIndex].type() != TableElementType::Funcref, "Table ", tableIndex, " must have type 'anyfunc' to have an element section");

Do we have a test?

> Source/JavaScriptCore/wasm/WasmValidate.cpp:183
> +    WASM_VALIDATOR_FAIL_IF(tableIndex >= m_module.tableCount(), "table index ", tableIndex, " is invalid, limit is ", m_module.tableCount());

Do we have a test?

> Source/JavaScriptCore/wasm/WasmValidate.cpp:192
> +    WASM_VALIDATOR_FAIL_IF(tableIndex >= m_module.tableCount(), "table index ", tableIndex, " is invalid, limit is ", m_module.tableCount());

Do we have a test?

> Source/JavaScriptCore/wasm/WasmValidate.cpp:197
> +    WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Anyref
> +        && m_module.tables[tableIndex].type() != TableElementType::Funcref, "table.set expects the table to have type anyref or anyfunc");

do we have tests for this?

> Source/JavaScriptCore/wasm/WasmValidate.cpp:387
> +    WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Funcref, "Table must have type Anyfunc to call");

do we have tests for this?

> JSTests/wasm/spec-tests/imports.wast.js:199
> +// These tests assert that we can only have one table. The references spec makes this incorrect, and this should be fixed when it gets merged into the main spec.
> +//assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x02\x8d\x80\x80\x80\x00\x02\x00\x00\x01\x70\x00\x0a\x00\x00\x01\x70\x00\x0a");
>  
>  // imports.wast:297
> -assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x02\x87\x80\x80\x80\x00\x01\x00\x00\x01\x70\x00\x0a\x04\x84\x80\x80\x80\x00\x01\x70\x00\x0a");
> +//assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x02\x87\x80\x80\x80\x00\x01\x00\x00\x01\x70\x00\x0a\x04\x84\x80\x80\x80\x00\x01\x70\x00\x0a");
>  
>  // imports.wast:301
> -assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x04\x87\x80\x80\x80\x00\x02\x70\x00\x0a\x70\x00\x0a");
> +//assert_invalid("\x00\x61\x73\x6d\x01\x00\x00\x00\x04\x87\x80\x80\x80\x00\x02\x70\x00\x0a\x70\x00\x0a");

just delete these lines.

[Justin Michaud]

Comment on attachment 372266 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmInstance.h:157
>> +        return (offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions + sizeof(Table*) * numTables).unsafeGet();
> 
> I think this should be:
> 
> roundUpToMultipleOf<sizeof(Table*)>((offsetOfTail() + sizeof(ImportFunctionInfo) * numImportFunctions) + sizeof(Table*)*numTables
> 
> Or static assert

How does this differ from the static assert above?

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:219
>>      WASM_PARSER_FAIL_IF(!parseVarUInt32(count), "can't get Table's count");
> 
> let's add a maximum number of tables here

I just picked 1000000 arbitrarily, but I will file a spec bug: https://webassembly.github.io/reference-types/js-api/index.html#limits says 1 still.

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:379
>> +        WASM_PARSER_FAIL_IF(m_info->tables[tableIndex].type() != TableElementType::Funcref, "Table ", tableIndex, " must have type 'anyfunc' to have an element section");
> 
> Do we have a test?

Yes, one in the other patch as well as one in this patch for when the table index != 0

>> Source/JavaScriptCore/wasm/WasmValidate.cpp:197
>> +        && m_module.tables[tableIndex].type() != TableElementType::Funcref, "table.set expects the table to have type anyref or anyfunc");
> 
> do we have tests for this?

It is currently impossible to do so, so I will turn this into a debug assert.

>> Source/JavaScriptCore/wasm/WasmValidate.cpp:387
>> +    WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Funcref, "Table must have type Anyfunc to call");
> 
> do we have tests for this?

It is a parse error, so I will turn this into a release assert.

[Justin Michaud]

Comment on attachment 372266 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1923
>> +        append(Move, Arg::addr(instanceValue(), Instance::offsetOfTablePtr(m_numImportFunctions, tableIndex)), callableFunctionBufferLength);
> 
> this is where the branch should be. Basically, look for "isValidForm", and you need to branch on instruction selection based on that. Should be easy to make an arm64 test fail by having many, many, tables

The test case I uploaded runs on iOS, and I added a static assert here to warn anyone who tries to increase the limit in the future. Is that OK?

[Justin Michaud]

Created attachment 372313 [details]
Patch

[Saam Barati]

Comment on attachment 372266 [details]
Patch

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

>>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1923
>>> +        append(Move, Arg::addr(instanceValue(), Instance::offsetOfTablePtr(m_numImportFunctions, tableIndex)), callableFunctionBufferLength);
>> 
>> this is where the branch should be. Basically, look for "isValidForm", and you need to branch on instruction selection based on that. Should be easy to make an arm64 test fail by having many, many, tables
> 
> The test case I uploaded runs on iOS, and I added a static assert here to warn anyone who tries to increase the limit in the future. Is that OK?

Your test uses the maximum number of import functions too? What if you remove the limit on the number of tables, and use a large number for the table index?

>>> Source/JavaScriptCore/wasm/WasmValidate.cpp:197
>>> +        && m_module.tables[tableIndex].type() != TableElementType::Funcref, "table.set expects the table to have type anyref or anyfunc");
>> 
>> do we have tests for this?
> 
> It is currently impossible to do so, so I will turn this into a debug assert.

Why is it impossible? Isn’t this the job of the validator?

>>> Source/JavaScriptCore/wasm/WasmValidate.cpp:387
>>> +    WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Funcref, "Table must have type Anyfunc to call");
>> 
>> do we have tests for this?
> 
> It is a parse error, so I will turn this into a release assert.

Why? Aren’t we validating here? The whole point is we can see arbitrary things, right?

[Justin Michaud]

Created attachment 372347 [details]
Patch

[Justin Michaud]

Created attachment 372376 [details]
Patch

[Justin Michaud]

(In reply to Saam Barati from comment #11)
> >>> Source/JavaScriptCore/wasm/WasmValidate.cpp:197
> >>> +        && m_module.tables[tableIndex].type() != TableElementType::Funcref, "table.set expects the table to have type anyref or anyfunc");
> >> 
> >> do we have tests for this?
> > 
> > It is currently impossible to do so, so I will turn this into a debug assert.
> 
> Why is it impossible? Isn’t this the job of the validator?
The type makes it impossible, but I wasn't sure how to put a static_assert to warn anyone who adds a new table type.

> 
> >>> Source/JavaScriptCore/wasm/WasmValidate.cpp:387
> >>> +    WASM_VALIDATOR_FAIL_IF(m_module.tables[tableIndex].type() != TableElementType::Funcref, "Table must have type Anyfunc to call");
> >> 
> >> do we have tests for this?
> > 
> > It is a parse error, so I will turn this into a release assert.
> 
> Why? Aren’t we validating here? The whole point is we can see arbitrary
> things, right?
It is a parse error if the table index is too big / the wrong type (the check was there before this patch), and I added a test to make sure it is a parse error.

[Justin Michaud]

Created attachment 372380 [details]
Patch

[Saam Barati]

Comment on attachment 372376 [details]
Patch

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

Still LGTM, two small comments

> Source/JavaScriptCore/ChangeLog:9
> +        Support multiple wasm tables. We turn tableInformation into a tables array, and update all of the
> +        existing users to give a table index.

Sorry, I should have commented earlier, but you should have at least a sentence on the implementation details, e.g, that it hangs off the end of Instance. Feel free to add even more.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:113
> +    if (auto* table = this->table(i))
> +        table->deref();

shouldn't this be an assert that !table(i)? We have that assert in the JS code.

[Justin Michaud]

Created attachment 372381 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 372381 [details]
Patch

Clearing flags on attachment: 372381

Committed r246571: <https://trac.webkit.org/changeset/246571>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51867688>