[Webkit-unassigned] [Bug 219427] [WASM-References] Add support for table.copy

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Dmitry <dbezhetskov at igalia.com> ---
Comment on attachment 415201
  --> https://bugs.webkit.org/attachment.cgi?id=415201
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Add support for table.copy from reference types proposal:
> 
> might be worth describing what it does

Ok, added more info

>> Source/JavaScriptCore/wasm/WasmFunctionParser.h:670
>> +            TypedExpression lenght;
> 
> lenght => length

Done.

>> Source/JavaScriptCore/wasm/WasmInstance.cpp:134
>> +                fn(dstTable, srcTable, dstOffset + (idx - 1), srcOffset + (idx - 1));
> 
> I prefer to write this loop as:
> 
> ```
> for (uint32_t idx = len; idx--; )
> ```
> Then inside the body of the loop, you can just refer to idx instead of (idx - 1)

Wise choice, thanks! Fixed.

>>> Source/JavaScriptCore/wasm/WasmOperations.cpp:729
>>> +}
>> 
>> I think you can use Checked<uint32_t> for this.
> 
> just make sure to not use CrashOnOverflow and use RecordOverflow instead

Ah, thanks! Remove my bicycle.

>> Source/JavaScriptCore/wasm/WasmOperations.cpp:739
>> +        return false;
> 
> the error message you throw in the JIT code seems different than this type of error

ah, this should be a validation error, move this from here.

>> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:424
>> +            // but we need to parse them for the table.copy spec test, so we just skip them.
> 
> This seems very fishy. We're parsing something we don't support just for a test?

I want to test table.copy with big and good spec tests but don't want to support all table instructions in one PR.
The spec test is weird, they declare passive element segments for table.copy's tests but don't use them at all... 

Without table.init instruction passive element segments are just syntax for nothing.

BTW I already have table.init patch, so once we finish with that I'll upload next one and remove this comments.

-- 
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/20201203/678b6cb8/attachment.htm>