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

• Component: WebAssembly
• Status: RESOLVED FIXED

[Dmitry]

[WASM-References] Add support for table.copy

[Dmitry]

Created attachment 415201 [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".

[Keith Miller]

Comment on attachment 415201 [details]
Patch

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

Looks good. Can you change the operation to use Checked<> though?

> Source/JavaScriptCore/wasm/WasmOperations.cpp:729
> +static bool isSumOverflow(uint32_t lhs, uint32_t rhs)
> +{
> +    const uint64_t sum = static_cast<uint64_t>(lhs) + static_cast<uint64_t>(rhs);
> +    return sum > static_cast<uint64_t>(std::numeric_limits<int32_t>::max());
> +}

I think you can use Checked<uint32_t> for this.

[Saam Barati]

Comment on attachment 415201 [details]
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

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

lenght => length

> Source/JavaScriptCore/wasm/WasmInstance.cpp:134
> +            for (uint32_t idx = len; idx > 0; --idx)
> +                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)

>> 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

> Source/JavaScriptCore/wasm/WasmOperations.cpp:739
> +    if (dstTable->isExternrefTable() != srcTable->isExternrefTable())
> +        return false;

the error message you throw in the JIT code seems different than this type of error

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:424
> +            // We don't have the table.init instruction to use passive element segments,
> +            // 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?

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:501
> +            // We don't have the table.init instruction to use passive element segments,
> +            // but we need to parse them for the table.copy spec test, so we just skip them.

ditto

[Dmitry]

Comment on attachment 415201 [details]
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.

[Dmitry]

Created attachment 415276 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 415276 [details]
Patch

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

r=me with nits.

> Source/JavaScriptCore/bytecode/BytecodeList.rb:1618
> +        dstTableIdx: unsigned,
> +        srcTableIdx: unsigned,

Let's use Index instead of Idx since the other bytecodes use Index / index.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1180
> +auto AirIRGenerator::addTableCopy(unsigned dstTableIdx, unsigned srcTableIdx, ExpressionType dstOffset, ExpressionType srcOffset, ExpressionType length) -> PartialResult
> +{
> +    ASSERT(dstOffset.tmp());
> +    ASSERT(dstOffset.type() == Type::I32);
> +
> +    ASSERT(srcOffset.tmp());
> +    ASSERT(srcOffset.type() == Type::I32);
> +
> +    ASSERT(length.tmp());
> +    ASSERT(length.type() == Type::I32);
> +
> +    auto result = tmpForType(Type::I32);
> +    emitCCall(
> +        &operationWasmTableCopy, result, instanceValue(),
> +        addConstant(Type::I32, dstTableIdx),
> +        addConstant(Type::I32, srcTableIdx),

Let's use Index instead of Idx.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:792
>  
> +auto B3IRGenerator::addTableCopy(unsigned dstTableIdx, unsigned srcTableIdx, ExpressionType dstOffset, ExpressionType srcOffset, ExpressionType length) -> PartialResult
> +{
> +    auto result = m_currentBlock->appendNew<CCallValue>(
> +        m_proc, toB3Type(I32), origin(),
> +        m_currentBlock->appendNew<ConstPtrValue>(m_proc, origin(), tagCFunction<OperationPtrTag>(operationWasmTableCopy)),
> +        instanceValue(),
> +        m_currentBlock->appendNew<Const32Value>(m_proc, origin(), dstTableIdx),
> +        m_currentBlock->appendNew<Const32Value>(m_proc, origin(), srcTableIdx),
> +        dstOffset, srcOffset, length);

Let's use Index instead of Idx.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:683
> +            unsigned dstTableIdx;
> +            WASM_PARSER_FAIL_IF(!parseVarUInt32(dstTableIdx), "can't parse destination table index");
> +            WASM_VALIDATOR_FAIL_IF(dstTableIdx >= m_info.tableCount(), "table index ", dstTableIdx, " is invalid, limit is ", m_info.tableCount());
> +
> +            unsigned srcTableIdx;
> +            WASM_PARSER_FAIL_IF(!parseVarUInt32(srcTableIdx), "can't parse source table index");
> +            WASM_VALIDATOR_FAIL_IF(srcTableIdx >= m_info.tableCount(), "table index ", srcTableIdx, " is invalid, limit is ", m_info.tableCount());
> +
> +            const auto srcType = m_info.table(srcTableIdx).wasmType();
> +            const auto dstType = m_info.table(dstTableIdx).wasmType();
> +            WASM_VALIDATOR_FAIL_IF(srcType != dstType, "type mismatch at table.copy. got ", srcType, " and ", dstType);
> +
> +            TypedExpression dstOffset;
> +            TypedExpression srcOffset;
> +            TypedExpression length;
> +            WASM_TRY_POP_EXPRESSION_STACK_INTO(length, "table.copy");
> +            WASM_TRY_POP_EXPRESSION_STACK_INTO(srcOffset, "table.copy");
> +            WASM_TRY_POP_EXPRESSION_STACK_INTO(dstOffset, "table.copy");
> +
> +            WASM_VALIDATOR_FAIL_IF(I32 != dstOffset.type(), "table.copy dst_offset to type ", dstOffset.type(), " expected ", I32);
> +            WASM_VALIDATOR_FAIL_IF(I32 != srcOffset.type(), "table.copy src_offset to type ", srcOffset.type(), " expected ", I32);
> +            WASM_VALIDATOR_FAIL_IF(I32 != length.type(), "table.copy length to type ", length.type(), " expected ", I32);
> +
> +            WASM_TRY_ADD_TO_CONTEXT(addTableCopy(dstTableIdx, srcTableIdx, dstOffset, srcOffset, length));

Ditto.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:123
> +void Instance::tableCopy(uint32_t dstOffset, uint32_t srcOffset, uint32_t len, uint32_t dstTableIdx, uint32_t srcTableIdx)

Let's use `length` instead of `len`.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:132
> +    auto forTableElement = [&] (auto fn) {

Don't put space between `[]` and  `(...)` in new code.
And `forEachTableElement` would be better name for this function.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:134
> +            for (uint32_t idx = len; idx--;)

Let's use index.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:139
> +            for (uint32_t idx = 0; idx < len; ++idx)

Ditto.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:145
> +        forTableElement([] (Table* dstTable, Table* srcTable, uint32_t dstIdx, uint32_t srcIdx) {

Don't put space between `[]` and  `(Table* dstTable, Table* srcTable, uint32_t dstIdx, uint32_t srcIdx)` in new code.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:151
> +    forTableElement([] (Table* dstTable, Table* srcTable, uint32_t dstIdx, uint32_t srcIdx) {

Ditto.

> Source/JavaScriptCore/wasm/WasmInstance.h:80
> +    void tableCopy(uint32_t dstOffset, uint32_t srcOffset, uint32_t len, uint32_t dstTableIdx, uint32_t srcTableIdx);

Ditto for Idx and len.

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:1149
> +auto LLIntGenerator::addTableCopy(unsigned dstTableIdx, unsigned srcTableIdx, ExpressionType dstOffset, ExpressionType srcOffset, ExpressionType length) -> PartialResult
> +{
> +    WasmTableCopy::emit(this, dstOffset, srcOffset, length, dstTableIdx, srcTableIdx);
> +    return { };
> +}

Ditto.

> Source/JavaScriptCore/wasm/WasmModuleInformation.h:70
> +    const TableInformation& table(unsigned idx) const { return tables[idx]; }

Ditto.

> Source/JavaScriptCore/wasm/WasmOperations.cpp:759
> +JSC_DEFINE_JIT_OPERATION(operationWasmTableCopy, bool, (Instance* instance, unsigned dstTableIdx, unsigned srcTableIdx, int32_t dstOffset, int32_t srcOffset, int32_t length))
> +{
> +    ASSERT(dstTableIdx < instance->module().moduleInformation().tableCount());
> +    ASSERT(srcTableIdx < instance->module().moduleInformation().tableCount());
> +    const Table* dstTable = instance->table(dstTableIdx);
> +    const Table* srcTable = instance->table(srcTableIdx);
> +    ASSERT(dstTable->type() == srcTable->type());
> +
> +    if ((srcOffset < 0) || (dstOffset < 0) || (length < 0))
> +        return false;
> +
> +    Checked<uint32_t, RecordOverflow> lastDstElementIdxChecked = static_cast<uint32_t>(dstOffset);
> +    lastDstElementIdxChecked += static_cast<uint32_t>(length);
> +
> +    uint32_t lastDstElementIdx;
> +    if (lastDstElementIdxChecked.safeGet(lastDstElementIdx) == CheckedState::DidOverflow)
> +        return false;
> +
> +    if (lastDstElementIdx > dstTable->length())
> +        return false;
> +
> +    Checked<uint32_t, RecordOverflow> lastSrcElementIdxChecked = static_cast<uint32_t>(srcOffset);
> +    lastSrcElementIdxChecked += static_cast<uint32_t>(length);
> +
> +    uint32_t lastSrcElementIdx;
> +    if (lastSrcElementIdxChecked.safeGet(lastSrcElementIdx) == CheckedState::DidOverflow)
> +        return false;
> +
> +    if (lastSrcElementIdx > srcTable->length())
> +        return false;
> +
> +    instance->tableCopy(dstOffset, srcOffset, length, dstTableIdx, srcTableIdx);
> +    return true;
> +}

Ditto.

> Source/JavaScriptCore/wasm/WasmTable.cpp:139
> +void Table::copy(const Table* srcTable, uint32_t dstIdx, uint32_t srcIdx)
> +{
> +    RELEASE_ASSERT(isExternrefTable());
> +    RELEASE_ASSERT(srcTable->isExternrefTable());
> +
> +    set(dstIdx, srcTable->get(srcIdx));
> +}

Ditto.

> Source/JavaScriptCore/wasm/WasmTable.cpp:226
> +void FuncRefTable::copyFunction(const FuncRefTable* srcTable, uint32_t dstIdx, uint32_t srcIdx)
> +{
> +    if (srcTable->get(srcIdx).isNull()) {
> +        clear(dstIdx);
> +        return;
> +    }
> +
> +    setFunction(dstIdx, jsCast<JSObject*>(srcTable->get(srcIdx)), srcTable->function(srcIdx), srcTable->instance(srcIdx));
> +}

Ditto.

> Source/JavaScriptCore/wasm/WasmTable.h:79
> +    void copy(const Table* srcTable, uint32_t dstIdx, uint32_t srcIdx);

Ditto.

> Source/JavaScriptCore/wasm/WasmTable.h:105
> +    void copyFunction(const FuncRefTable* srcTable, uint32_t dstIdx, uint32_t srcIdx);

Ditto.

[Dmitry]

Created attachment 415507 [details]
Patch

[Dmitry]

Fixed all nits, thanks Yusuke.

[Yusuke Suzuki]

Comment on attachment 415507 [details]
Patch

r=me

[EWS]

Committed r270524: <https://trac.webkit.org/changeset/270524>

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

[Radar WebKit Bug Importer]

<rdar://problem/72064181>