[Webkit-unassigned] [Bug 219848] [WASM-References] Add support for memory.fill

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=219848

Yusuke Suzuki <ysuzuki at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ysuzuki at apple.com
 Attachment #416145|review?                     |review+
              Flags|                            |

--- 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.html#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#memory-instructions.

Can you put this to JavaScriptCore/ChangeLog side?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20201215/ea3f7f64/attachment.htm>