Bug 219297 – [WASM-References] Add table.init

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

[Dmitry]

[WASM-References] Add table.init

[Dmitry]

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

[Dmitry]

Created attachment 414915 [details]
Patch

[Saam Barati]

Comment on attachment 414915 [details]
Patch

This patch seems to have two patches combined together. Can you upload table.init on its own?

[Dmitry]

(In reply to Saam Barati from comment #4)
> Comment on attachment 414915 [details]
> Patch
> 
> This patch seems to have two patches combined together. Can you upload
> table.init on its own?

Ah, sorry for this, still don't understand how to push two dependent patches.
BTW, I want to land table.copy with spec-tests first and then land table.init because table.init ref-types spec depends on table.copy (https://github.com/WebAssembly/reference-types/blob/master/test/core/table_init.wast#L156).

[Radar WebKit Bug Importer]

<rdar://problem/71931757>

[Dmitry]

Created attachment 415626 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 415626 [details]
Patch

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

The direction looks good, but I found several issues.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1163
> +    auto result = tmpForType(Type::I32);
> +    emitCCall(
> +        &operationWasmElemDrop, result, instanceValue(),
> +        addConstant(Type::I32, elementIndex));
> +
> +    emitCheck([&] {
> +        return Inst(BranchTest32, nullptr, Arg::resCond(MacroAssembler::Zero), result, result);
> +    }, [=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
> +        this->emitThrowException(jit, ExceptionType::OutOfBoundsTableAccess);
> +    });
> +
> +    return { };

Do we need exception check here? Looks like elem_drop instruction never returns false.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:787
> +    auto result = m_currentBlock->appendNew<CCallValue>(
> +        m_proc, toB3Type(I32), origin(),
> +        m_currentBlock->appendNew<ConstPtrValue>(m_proc, origin(), tagCFunction<OperationPtrTag>(operationWasmElemDrop)),
> +        instanceValue(),
> +        m_currentBlock->appendNew<Const32Value>(m_proc, origin(), elementIndex));
> +
> +    {
> +        CheckValue* check = m_currentBlock->appendNew<CheckValue>(m_proc, Check, origin(),
> +            m_currentBlock->appendNew<Value>(m_proc, Equal, origin(), result, m_currentBlock->appendNew<Const32Value>(m_proc, origin(), 0)));
> +
> +        check->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
> +            this->emitExceptionCheck(jit, ExceptionType::OutOfBoundsTableAccess);
> +        });
> +    }
> +

Ditto.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:168
> +const Element* Instance::elem(unsigned i) const

Let's name it `elementAt`.

> Source/JavaScriptCore/wasm/wasm.json:72
> +        "table.init":          { "category": "exttable",   "value":  252, "return": ["i32"],                         "parameter": ["i32", "i32", "i32"],          "immediate": [{"name": "element_index",  "type": "varuint32"}, {"name": "table_index","type": "varuint32"}],"description": "initialize table from a given element", "extendedOp": 12 },

Does `table.init` return "i32" value? Can you point at the spec what value should be returned?

> Source/JavaScriptCore/wasm/wasm.json:73
> +        "elem.drop":           { "category": "exttable",   "value":  252, "return": ["i32"],                         "parameter": [],                             "immediate": [{"name": "element_index",  "type": "varuint32"}],                                             "description": "prevents further use of a passive element segment", "extendedOp": 13 },

Ditto for elem.drop. And can you check the other table instructions too?

[Yusuke Suzuki]

Comment on attachment 415626 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmOperations.cpp:724
> +JSC_DEFINE_JIT_OPERATION(operationWasmElemDrop, bool, (Instance* instance, unsigned elementIndex))

Let's make return type "void" since it always returns "true".

[Dmitry]

Comment on attachment 415626 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1163
>> +    return { };
> 
> Do we need exception check here? Looks like elem_drop instruction never returns false.

Ah, thanks, fixed!

>> Source/JavaScriptCore/wasm/WasmInstance.cpp:168
>> +const Element* Instance::elem(unsigned i) const
> 
> Let's name it `elementAt`.

renamed

>> Source/JavaScriptCore/wasm/WasmOperations.cpp:724
>> +JSC_DEFINE_JIT_OPERATION(operationWasmElemDrop, bool, (Instance* instance, unsigned elementIndex))
> 
> Let's make return type "void" since it always returns "true".

Done

>> Source/JavaScriptCore/wasm/wasm.json:73
>> +        "elem.drop":           { "category": "exttable",   "value":  252, "return": ["i32"],                         "parameter": [],                             "immediate": [{"name": "element_index",  "type": "varuint32"}],                                             "description": "prevents further use of a passive element segment", "extendedOp": 13 },
> 
> Ditto for elem.drop. And can you check the other table instructions too?

Yeah, I noticed that too when I looked at table.fill, copied code and forgot about it :)
Fixed table.copy, elem.drop and table.fill.

[Dmitry]

Created attachment 415733 [details]
Patch

[Dmitry]

Created attachment 415747 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 415747 [details]
Patch

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

Nice. I'm putting r- since I found several bugs.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:621
> +            WASM_PARSER_FAIL_IF(!parseVarUInt32(elementIndex), "can't parse element index");
> +            WASM_VALIDATOR_FAIL_IF(elementIndex >= m_info.elementCount(), "element index ", elementIndex, " is invalid, limit is ", m_info.elementCount());
> +
> +            unsigned tableIndex;
> +            WASM_PARSER_FAIL_IF(!parseVarUInt32(tableIndex), "can't parse element index");
> +            WASM_VALIDATOR_FAIL_IF(tableIndex >= m_info.tableCount(), "table index ", tableIndex, " is invalid, limit is ", m_info.tableCount());

This parsing behavior is different from the other table ext operations. This means that we need to have special handling in `FunctionParser<Context>::parseUnreachableExpression()` side too.
The purpose of `FunctionParser<Context>::parseUnreachableExpression()` is skip all wasm stack pushing/poping, but validates imm etc. for unreachable wasm opcodes.
Can you add them?
I think we need TableCopy one too.
And, can you add tests for these cases? I think we can just emit these opcodes after return, and ensure we are stressing table opcodes in parseUnreachableExpression.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:73
> +    for (unsigned elementIdx = 0; elementIdx < m_passiveElements.size(); ++elementIdx) {

Rename idx to index.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:183
> +    for (uint32_t idx = 0; idx < length; ++idx) {

Rename it to index.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:187
> +        JSWebAssemblyInstance* jsInstance = owner<JSWebAssemblyInstance>();
> +        JSWebAssemblyTable* jsTable = jsInstance->table(tableIndex);

We should put them out of the loop.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:199
> +        JSGlobalObject* globalObject = jsInstance->globalObject();
> +        VM& vm = globalObject->vm();

Ditto. Let's put them out of the loop.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:246
> +void Instance::tableInit(uint32_t dstOffset, uint32_t srcOffset, uint32_t len, uint32_t elementIndex, uint32_t tableIndex)

len => length.

[Dmitry]

Created attachment 415850 [details]
Patch

[Dmitry]

Added test in .wasm binary format because our wabt harness can't emit invalid wasm files.

[Yusuke Suzuki]

Comment on attachment 415850 [details]
Patch

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

r=me with one important optimization.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:77
> +    m_passiveElements.resize(m_module->moduleInformation().elementCount());
> +    for (unsigned elementIndex = 0; elementIndex < m_passiveElements.size(); ++elementIndex) {
> +        const auto& element = m_module->moduleInformation().elements[elementIndex];
> +        if (element.isPassive())
> +            m_passiveElements[elementIndex] = element;
> +    }

I think m_passiveElements can be a BitVector, and this is better in terms of memory.
m_module is held by WasmInstance. And we can get corresponding element by m_module->moduleInformation().elements[index].
So, only thing we would like to know is that the given passive element is dropped or not.
Can you change it to save memory? WasmInstance is kept alive so long as wasm code is running. So saving memory is very important.

> Source/JavaScriptCore/wasm/WasmInstance.h:233
> +    Vector<Optional<Element>> m_passiveElements;

Let's make it BitVector.

[Yusuke Suzuki]

Comment on attachment 415850 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:610
> +            RELEASE_ASSERT(element.isActive());

I don't think this is necessary or making it to ASSERT. forEachActiveElement is explicitly saying this.

[Dmitry]

Created attachment 415968 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 415968 [details]
Patch

r=me

[EWS]

Committed r270689: <https://trac.webkit.org/changeset/270689>

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