[WASM-References] Add support for Funcref in parameters and return types

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Michaud]

Add support for Funcref as a value type.

[Justin Michaud]

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

[Saam Barati]

Comment on attachment 371816 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        a funcref (née anyfunc), we first make sure it is an exported wasm function or null. 

remove non-ascii character here.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1016
> +            } else if (!value.isFunction(*instance->owner<JSObject>()->vm()))

Maybe I'm missing something, but based on the type system, aren't the only options for this value jsNull() or web assembly host function?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:606
> +        JSValue value = JSValue::decode(encValue);
> +        if (instance->table()->type() == TableElementType::Anyref)
> +            instance->table()->set(idx, value);
> +        else if (instance->table()->type() == TableElementType::Funcref) {
> +            WebAssemblyFunction* wasmFunction;
> +            WebAssemblyWrapperFunction* wasmWrapperFunction;
> +
> +            if (isWebAssemblyHostFunction(*instance->owner<JSObject>()->vm(), value, wasmFunction, wasmWrapperFunction)) {
> +                ASSERT(!!wasmFunction || !!wasmWrapperFunction);
> +                if (wasmFunction)
> +                    instance->table()->asFuncrefTable()->setFunction(idx, jsCast<JSObject*>(value), wasmFunction->importableFunction(), &wasmFunction->instance()->instance());
> +                else
> +                    instance->table()->asFuncrefTable()->setFunction(idx, jsCast<JSObject*>(value), wasmWrapperFunction->importableFunction(), &wasmWrapperFunction->instance()->instance());
> +            } else if (!value.isFunction(*instance->owner<JSObject>()->vm()))
> +                instance->table()->clear(idx);
> +            else
> +                ASSERT_NOT_REACHED();
> +        } else
> +            ASSERT_NOT_REACHED();

This is identical to WasmAIRIRGenerator. Let's make a helper for it.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:622
> +        JSValue val = instance->getFunctionWrapper(index);
> +        ASSERT(val.isFunction(*instance->owner<JSObject>()->vm()));
> +        return JSValue::encode(val);

ditto.

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:289
> +        // TODO check if used in ref.func

?

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:290
> +        if (m_exportedFunctionIndices.contains(functionIndex) || true) {

oops

> Source/JavaScriptCore/wasm/WasmInstance.cpp:47
> +    return (Checked<size_t>(module.moduleInformation().functionIndexSpaceSize()) * sizeof(WriteBarrier<Unknown>)).unsafeGet();

This is not great. We probably only ref.func a few functions, but we're allocating space for all of them. Can we have a mapping from index=>index, where ModuleInformation just has a HashMap from index in function index space to index in this buffer?

> Source/JavaScriptCore/wasm/WasmInstance.cpp:65
> +        new (&m_globals.get()[i].anyref) WriteBarrier<Unknown>(UndefinedWriteBarrierTag);

Don't we want the null value to be jsNull? Not undefined?

> Source/JavaScriptCore/wasm/WasmInstance.cpp:67
> +    for (unsigned i = 0; i < m_module->moduleInformation().functionIndexSpaceSize(); ++i)
> +        new (&m_functionWrappers.get()[i]) WriteBarrier<Unknown>(UndefinedWriteBarrierTag);

I think it's ok to just zero this. Since they should all get filled in (see above). Also, based on above, maybe we just want to use Vector?

> Source/JavaScriptCore/wasm/WasmValidate.cpp:191
> +    auto type = m_module.tableInformation.type() == TableElementType::Funcref ? Type::Anyfunc : Type::Anyref;

I suggested this helper in the past, but maybe I was wrong. It would be nice if the tableInformation.type() returned Wasm type.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:211
> +    WASM_VALIDATOR_FAIL_IF(index >= m_module.functionIndexSpaceSize(), "ref.func index ", index, " is too large");

nit: it'd be nice to get a better error message here, maybe one that contains "index" and "index space size"

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:333
> +                emitWasmToJSException(jit, GPRInfo::argumentGPR2, Wasm::ExceptionType::FuncrefNotWasm);

I vote for just going to the slow path instead of this. It makes us have to reason less about if this is a valid place to emit this exception handler.
(My guess: this might be wrong, and it might have to do with callee saves.)

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:244
> +                    return exception(createJSWebAssemblyLinkError(exec, vm, importFailMessage(import, "imported global", "must be a wasm exported function")));

nit: error message should also say "or null".

[Justin Michaud]

Created attachment 372095 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 372095 [details]
Patch

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

I found GC bug, so r-, but the code looks really nice!

> Source/JavaScriptCore/wasm/WasmInstance.cpp:98
> +void setWasmTableElement(Wasm::Instance* instance, uint32_t idx, uint64_t encValue)

Is encValue always encoded JSValue? If so, let's use EncodedJSValue type instead. And the name "encodedValue" is better.
I also like "index" instead of "idx".

> Source/JavaScriptCore/wasm/WasmInstance.cpp:123
> +    JSValue val = instance->getFunctionWrapper(index);

val => value.

> Source/JavaScriptCore/wasm/WasmInstance.h:167
> +    HashMap<uint32_t, WriteBarrier<Unknown>, IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> m_functionWrappers;

I think visitChildren handling for m_functionWrappers is missing.

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:127
> +        emitWasmToJSException(jit, GPRInfo::argumentGPR2, argumentsIncludeI64 ? ExceptionType::I64ArgumentType : ExceptionType::I64ReturnType);

I like to see the word "Throw" for such a function. So, "emitThrowWasmToJSException" would be nice.

[Justin Michaud]

Comment on attachment 372095 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmInstance.h:167
>> +    HashMap<uint32_t, WriteBarrier<Unknown>, IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> m_functionWrappers;
> 
> I think visitChildren handling for m_functionWrappers is missing.

We do it in the JS wrapper, right?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:98
> +        visitor.appendUnbarriered(thisObject->instance().getFunctionWrapper(i));

We do the function wrapper visiting here

[Justin Michaud]

Created attachment 372170 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 372170 [details]
Patch

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

r=me with several fixes.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:86
> +    return m_functionWrappers.contains(i) ? m_functionWrappers.get(i).get() : jsNull();

`contains(i)` and `get(i)` calls are duplicate basically. So we are looking up hash table twice.
Can we just use `get(i)`? I think it will return empty WriteBarrier thing.
Then, we can convert JSEmpty to JSNull, and we can do this function without using `contains`.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:98
> +        visitor.appendUnbarriered(thisObject->instance().getFunctionWrapper(i));

Ah, I missed it. Why not just iterating the hashmap of wrappers? I think # of wrappers can be much smaller than the total number of function space size. And it also avoids repeated calls of `contains()` / `get()` in getFunctionWrapper.
And I think the hashtable can be modified by the user's setFunctionWrapper. So locking is necessary.

[Justin Michaud]

Created attachment 372256 [details]
Patch

[Justin Michaud]

Created attachment 372257 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 372257 [details]
Patch

Clearing flags on attachment: 372257

Committed r246504: <https://trac.webkit.org/changeset/246504>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51814501>