[webkit-reviews] review granted: [Bug 198761] [WASM-References] Add support for Table.size, grow and fill instructions : [Attachment 372156] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 198761: [WASM-References] Add support for Table.size, grow and fill
instructions
https://bugs.webkit.org/show_bug.cgi?id=198761

Attachment 372156: Patch

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




--- Comment #8 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 372156
  --> https://bugs.webkit.org/attachment.cgi?id=372156
Patch

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

r=me

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:307
> +	   switch ((ExtTableOpType) extOp) {

Use `static_cast<ExtTableOpType>(extOp)`.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:90
> +    if (!newSize || *newSize == oldSize)
> +	   return -1;

delta = 0, returning -1, I think this is a spec bug. So, can we have a FIXME
comment here with an url pointing bugzilla issue about the spec bug?

> Source/JavaScriptCore/wasm/WasmInstance.cpp:107
> +    unsigned i = offset;
> +    unsigned n = count;

Can we use the names `offset` and `count` for the local variables instead?
Using `offset` and `count` is better than `i` and `n`.
Like, putting `unsafeOffset` and `unsafeCount` as parameters, validate with the
above if-statement, and copy them to the `unsigned` offset and count.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:110
> +    if (i >= instance->table()->length() || i + n >
instance->table()->length())
> +	   return false;

I think only `i + n > instance->table()->length()` is OK. The maximum value of
i and n is INT32_MAX. INT32_MAX + INT32_MAX does not exceed UINT32_MAX, so we
do not need to consider about overflow too.

> Source/JavaScriptCore/wasm/WasmTable.cpp:130
> +    for (uint32_t i = m_length; i < allocatedLength(newLength); ++i)
> +	   m_jsValues.get()[i].setWithoutWriteBarrier(jsNull());
> +

This part is already fixed in https://trac.webkit.org/changeset/246487/webkit,
so it is not necessary.