[webkit-reviews] review denied: [Bug 219297] [WASM-References] Add table.init : [Attachment 415626] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Dmitry <dbezhetskov at igalia.com>'s
request for review:
Bug 219297: [WASM-References] Add table.init
https://bugs.webkit.org/show_bug.cgi?id=219297

Attachment 415626: Patch

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




--- Comment #8 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 415626
  --> https://bugs.webkit.org/attachment.cgi?id=415626
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?