Bug 198760 - [WASM-References] Add support for multiple tables
Summary: [WASM-References] Add support for multiple tables
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-11 11:10 PDT by Justin Michaud
Modified: 2019-06-18 15:04 PDT (History)
9 users (show)

See Also:


Attachments
Patch (83.42 KB, patch)
2019-06-13 16:00 PDT, Justin Michaud
sbarati: review-
Details | Formatted Diff | Diff
patch (requires funcref patch) (85.13 KB, patch)
2019-06-14 21:21 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (88.14 KB, patch)
2019-06-17 13:15 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (99.33 KB, patch)
2019-06-17 19:00 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (99.77 KB, patch)
2019-06-18 10:29 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (100.36 KB, patch)
2019-06-18 13:51 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (100.34 KB, patch)
2019-06-18 13:59 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (97.94 KB, patch)
2019-06-18 14:13 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2019-06-11 11:10:14 PDT
Add support for multiple tables
Comment 1 Justin Michaud 2019-06-13 16:00:11 PDT
Created attachment 372080 [details]
Patch
Comment 2 Build Bot 2019-06-13 16:02:14 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 Saam Barati 2019-06-14 16:51:34 PDT
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
Comment 4 Justin Michaud 2019-06-14 19:31:20 PDT
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.
Comment 5 Justin Michaud 2019-06-14 21:21:18 PDT
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
Comment 6 Justin Michaud 2019-06-17 13:15:48 PDT
Created attachment 372266 [details]
Patch
Comment 7 Saam Barati 2019-06-17 15:01:02 PDT
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.
Comment 8 Justin Michaud 2019-06-17 16:27:54 PDT
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.
Comment 9 Justin Michaud 2019-06-17 18:57:39 PDT
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?
Comment 10 Justin Michaud 2019-06-17 19:00:22 PDT
Created attachment 372313 [details]
Patch
Comment 11 Saam Barati 2019-06-18 08:59:36 PDT
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?
Comment 12 Justin Michaud 2019-06-18 10:29:50 PDT
Created attachment 372347 [details]
Patch
Comment 13 Justin Michaud 2019-06-18 13:51:50 PDT
Created attachment 372376 [details]
Patch
Comment 14 Justin Michaud 2019-06-18 13:56:14 PDT
(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.
Comment 15 Justin Michaud 2019-06-18 13:59:14 PDT
Created attachment 372380 [details]
Patch
Comment 16 Saam Barati 2019-06-18 14:04:08 PDT
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.
Comment 17 Justin Michaud 2019-06-18 14:13:08 PDT
Created attachment 372381 [details]
Patch
Comment 18 WebKit Commit Bot 2019-06-18 15:01:09 PDT
Comment on attachment 372381 [details]
Patch

Clearing flags on attachment: 372381

Committed r246571: <https://trac.webkit.org/changeset/246571>
Comment 19 WebKit Commit Bot 2019-06-18 15:01:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2019-06-18 15:04:57 PDT
<rdar://problem/51867688>