Bug 219943 – [WASM-References] Add support for memory.copy, memory.init and data.drop

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[Dmitry]

[WASM-References] Add support for memory.copy, memory.init and data.drop

[Dmitry]

Created attachment 416329 [details]
Patch

[EWS Watchlist]

This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".

[Yusuke Suzuki]

Comment on attachment 416329 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        Add support for memory.copy [dstAddress, srcAddress, length] -> [],
> +        memory.init data_index [dstAddress, srcAddress, length] -> []
> +        and for data.drop data_index.
> +        https://webassembly.github.io/reference-types/core/binary/instructions.html#memory-instructions.
> +        Overview: https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md.

Can you describe what each opcode does in ChangeLog?
ChangeLog is very important to know what is implemented, and what is supposed to be done in the patch. While URL can be broken, ChangeLog exists in the tree. And we can know the rationale behind the patch :).

> Source/JavaScriptCore/wasm/WasmInstance.cpp:186
> +    const uint32_t segmentSizeInBytes = m_passiveDataSegments.quickGet(dataSegmentIndex) ? segment->sizeInBytes : 0u;

We use `0U`.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:654
> +    if (offset + length > m_handle->size())

We should use sumOverflows

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:640
> +auto SectionParser::parseI32InitExpr(Optional<I32InitExpr>& initExpr, const char* failMessage) -> PartialResult

Let's make `const char*` to ASCIILiteral. And use ASCIILiteral to pass string to this function by using `"STRING"_s`.

> Source/JavaScriptCore/wasm/WasmSections.h:92
> +    const auto previousKnownSectionId = static_cast<uint8_t>(previousKnown);
> +    const auto nextSectionId = static_cast<uint8_t>(next);

This is not necessary.

> Source/JavaScriptCore/wasm/WasmSections.h:95
> +        && previousKnownSectionId == static_cast<uint8_t>(Section::DataCount)

It should be `previousKnown == Section::DataCount`

> Source/JavaScriptCore/wasm/WasmSections.h:96
> +        && nextSectionId == static_cast<uint8_t>(Section::Code)) {

It should be `next == Section::Code`

[Yusuke Suzuki]

Comment on attachment 416329 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:722
> +auto SectionParser::parseDataSegmentCoreSpec(uint32_t segmentNumber) -> PartialResult

I think having two functions for this is not so good in terms of test coverage etc. Can you merge it into one parseDataSegment function?

[Yusuke Suzuki]

Comment on attachment 416329 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmSections.h:94
> +    if (Options::useWebAssemblyReferences()

I think we don't need to have this check here. And instead, let's having this check in `parseDataCount`. While `parseDataCount` is called only once, validateOrder can be called more times.

[Dmitry]

Created attachment 416397 [details]
Patch

[Dmitry]

Created attachment 416402 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416402 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:25
> +

Nice. It is also nice if we have DataCount section information (like, what it is and why it is needed).

> Source/JavaScriptCore/wasm/WasmFormat.cpp:37
> +Segment* Segment::create(Optional<I32InitExpr> offset, uint32_t sizeInBytes, Kind kind)

Oh, while this is not the changed part in this patch, I think we should return `Segment::Ptr` from this function instead of calling `Segment::adoptPtr()` later.
Then, Segment's memory is managed well by unique_ptr.

> Source/JavaScriptCore/wasm/WasmFormat.cpp:52
>      return segment;

Let's return `return Segment::adoptPtr(segment)`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:759
> +            Segment* segment = Segment::create(*initExpr, dataByteLength, Segment::Kind::Active);

Let's receive this as `auto segment = Segment::create(*initExpr, dataByteLength, Segment::Kind::Active);`.
And let's keep Segment* pointer by doing like, `Segment* segmentPtr = segment.get();`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:761
> +            m_info->data.uncheckedAppend(Segment::adoptPtr(segment));

Let's pass it with `WTFMove(segment)`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:765
> +                segment->byte(dataByte) = byte;

Use segmentPtr to access since segment is already moved.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:768
> +        }

We can insert `ASSERT(Options::useWebAssemblyReferences())`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:775
> +            Segment* segment = Segment::create(WTF::nullopt, dataByteLength, Segment::Kind::Passive);

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:777
> +            m_info->data.uncheckedAppend(Segment::adoptPtr(segment));

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:781
> +                segment->byte(dataByte) = byte;

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:788
> +            WASM_PARSER_FAIL_IF(!Options::useWebAssemblyReferences(), "references are not enabled");

Let's remove this line since if `!Options::useWebAssemblyReferences()`, then execution never reaches here since we have `if (!Options::useWebAssemblyReferences() || !dataFlag) {`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:800
> +            Segment* segment = Segment::create(*initExpr, dataByteLength, Segment::Kind::Active);

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:802
> +            m_info->data.uncheckedAppend(Segment::adoptPtr(segment));

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:806
> +                segment->byte(dataByte) = byte;

Ditto.

[Dmitry]

Comment on attachment 416402 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:759
>> +            Segment* segment = Segment::create(*initExpr, dataByteLength, Segment::Kind::Active);
> 
> Let's receive this as `auto segment = Segment::create(*initExpr, dataByteLength, Segment::Kind::Active);`.
> And let's keep Segment* pointer by doing like, `Segment* segmentPtr = segment.get();`.

Actually we can just move m_info->data.uncheckedAppend(Segment::adoptPtr(segment)); after the loop

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:761
>> +            m_info->data.uncheckedAppend(Segment::adoptPtr(segment));
> 
> Let's pass it with `WTFMove(segment)`.

Thanks.

[Dmitry]

Created attachment 416407 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416407 [details]
Patch

r=me

[EWS]

Committed r270948: <https://trac.webkit.org/changeset/270948>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416407 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72441116>