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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 17 15:01:02 PDT 2019

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

Attachment 372266: Patch


--- Comment #7 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 372266
  --> https://bugs.webkit.org/attachment.cgi?id=372266

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1918
> +	  
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)),

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() !=
> +	   && 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.
>  // imports.wast:297
>  // imports.wast:301

just delete these lines.

More information about the webkit-reviews mailing list