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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 14 16:51:34 PDT 2019


Saam Barati <sbarati at apple.com> has denied 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 372080: Patch

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




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 372080
  --> https://bugs.webkit.org/attachment.cgi?id=372080
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


More information about the webkit-reviews mailing list