[webkit-reviews] review granted: [Bug 219848] [WASM-References] Add support for memory.fill : [Attachment 416145] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Dmitry <dbezhetskov at igalia.com>'s
request for review:
Bug 219848: [WASM-References] Add support for memory.fill
https://bugs.webkit.org/show_bug.cgi?id=219848

Attachment 416145: Patch

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




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

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

Looks good. Some comments.

> Source/JavaScriptCore/ChangeLog:8
> +	   Added spec tests and unreachable tests for memory.fill.

Can you put this to JSTests/ChangeLog side?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:624
> +	   auto locker = holdLock(m_handle->lock());

I don't think we need this locking. When size is updated, mprotect is already
done (because mprotect has memory barrier).
So, if `if (offset + count > m_handle->size())` pass, then we can just perform
this without taking a lock.
So, I think we can just remove `doMemoryFill` function, and merge it into
`Memory::fill`.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:633
> +    if ((unsafeOffset < 0) || (unsafeCount < 0))
> +	   return false;

Is it correct? I'm looking into
https://webassembly.github.io/bulk-memory-operations/core/exec/instructions.htm
l#exec-memory-fill, but I cannot find the corresponding thing.
Should we just handle unsafeOffset and unsafeCount as uint32_t instead?

> Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:387
> +    int32_t dstAddress = READ(instruction.m_dstAddress).unboxedInt32();
> +    int32_t targetValue = READ(instruction.m_targetValue).unboxedInt32();
> +    int32_t count = READ(instruction.m_count).unboxedInt32();

If they should be handled as uint32, let's add unboxedUInt32 and use it (this
is just `static_cast<uint32_t>(unboxedInt32())`).

> JSTests/ChangeLog:10
> +	   Add support for memory.fill from ref-types spec.
> +	   memory.fill sets all bytes in a memory region to a given byte:
> +	  
https://webassembly.github.io/reference-types/core/syntax/instructions.html#mem
ory-instructions.

Can you put this to JavaScriptCore/ChangeLog side?