[webkit-reviews] review granted: [Bug 198760] [WASM-References] Add support for multiple tables : [Attachment 372266] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 198760: [WASM-References] Add support for multiple tables
https://bugs.webkit.org/show_bug.cgi?id=198760

Attachment 372266: Patch

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




--- Comment #7 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 372266
  --> https://bugs.webkit.org/attachment.cgi?id=372266
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_numImportFunct
ions, 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\x
00\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\x
70\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.