Bug 222903 – [WASM-Function-References] Add call_ref instruction

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

[Dmitry]

[WASM-Function-References] Add call_ref instruction

[Dmitry]

Created attachment 422554 [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 422898 [details]
Patch

[Dmitry]

Created attachment 422902 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 422902 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        Implementation is similar to the call_indirect.

Can you describe what is the difference? We need detailed information about what this does in ChangeLog.

[Dmitry]

Created attachment 423144 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75426211>

[Yusuke Suzuki]

Comment on attachment 423144 [details]
Patch

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

The code looks good, but I think we should dedupe the code.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3509
> +    // Do a context switch if needed.
> +    {
> +        auto targetInstance = g64();
> +        append(Move, Arg::addr(calleeFunction, WebAssemblyFunctionBase::offsetOfInstance()), targetInstance);
> +
> +        BasicBlock* doContextSwitch = m_code.addBlock();
> +        BasicBlock* continuation = m_code.addBlock();
> +
> +        append(Branch64, Arg::relCond(MacroAssembler::Equal), targetInstance, instanceValue());
> +        m_currentBlock->setSuccessors(continuation, doContextSwitch);
> +
> +        auto* patchpoint = addPatchpoint(B3::Void);
> +        patchpoint->effects.writesPinned = true;
> +        // We pessimistically assume we're calling something with BoundsChecking memory.
> +        // FIXME: We shouldn't have to do this: https://bugs.webkit.org/show_bug.cgi?id=172181
> +        patchpoint->clobber(PinnedRegisterInfo::get().toSave(MemoryMode::BoundsChecking));
> +        patchpoint->clobber(RegisterSet::macroScratchRegisters());
> +        patchpoint->numGPScratchRegisters = Gigacage::isEnabled(Gigacage::Primitive) ? 1 : 0;
> +
> +        patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
> +            AllowMacroScratchRegisterUsage allowScratch(jit);
> +            GPRReg targetInstance = params[0].gpr();
> +            GPRReg oldContextInstance = params[1].gpr();
> +            const PinnedRegisterInfo& pinnedRegs = PinnedRegisterInfo::get();
> +            GPRReg baseMemory = pinnedRegs.baseMemoryPointer;
> +            ASSERT(targetInstance != baseMemory);
> +            jit.loadPtr(CCallHelpers::Address(oldContextInstance, Instance::offsetOfCachedStackLimit()), baseMemory);
> +            jit.storePtr(baseMemory, CCallHelpers::Address(targetInstance, Instance::offsetOfCachedStackLimit()));
> +            jit.storeWasmContextInstance(targetInstance);
> +            // FIXME: We should support more than one memory size register
> +            //   see: https://bugs.webkit.org/show_bug.cgi?id=162952
> +            ASSERT(pinnedRegs.boundsCheckingSizeRegister != targetInstance);
> +            GPRReg scratchOrBoundsCheckingSize = Gigacage::isEnabled(Gigacage::Primitive) ? params.gpScratch(0) : pinnedRegs.boundsCheckingSizeRegister;
> +
> +            jit.loadPtr(CCallHelpers::Address(targetInstance, Instance::offsetOfCachedBoundsCheckingSize()), pinnedRegs.boundsCheckingSizeRegister); // Bound checking size.
> +            jit.loadPtr(CCallHelpers::Address(targetInstance, Instance::offsetOfCachedMemory()), baseMemory); // Memory::void*.
> +
> +            jit.cageConditionally(Gigacage::Primitive, baseMemory, pinnedRegs.boundsCheckingSizeRegister, scratchOrBoundsCheckingSize);
> +        });
> +
> +        emitPatchpoint(doContextSwitch, patchpoint, Tmp(), targetInstance, instanceValue());
> +        append(doContextSwitch, Jump);
> +        doContextSwitch->setSuccessors(continuation);
> +
> +        m_currentBlock = continuation;
> +    }
> +
> +    append(Move, Arg::addr(calleeCode), calleeCode);
> +
> +    Vector<ConstrainedTmp> extraArgs;
> +    extraArgs.append(calleeCode);
> +
> +    for (unsigned i = 0; i < signature.returnCount(); ++i)
> +        results.append(tmpForType(signature.returnType(i)));
> +
> +    auto* patchpoint = emitCallPatchpoint(m_currentBlock, signature, results, args, WTFMove(extraArgs));
> +
> +    // We need to clobber all potential pinned registers since we might be leaving the instance.
> +    // We pessimistically assume we're always calling something that is bounds checking so
> +    // because the wasm->wasm thunk unconditionally overrides the size registers.
> +    // FIXME: We should not have to do this, but the wasm->wasm stub assumes it can
> +    // use all the pinned registers as scratch: https://bugs.webkit.org/show_bug.cgi?id=172181
> +
> +    patchpoint->clobberLate(PinnedRegisterInfo::get().toSave(MemoryMode::BoundsChecking));
> +
> +    patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
> +        AllowMacroScratchRegisterUsage allowScratch(jit);
> +        jit.call(params[params.proc().resultCount(params.value()->type())].gpr(), WasmEntryPtrTag);
> +    });
> +
> +    // The call could have been to another WebAssembly instance, and / or could have modified our Memory.
> +    restoreWebAssemblyGlobalState(RestoreCachedStackLimit::Yes, m_info.memory, currentInstance, m_currentBlock);
> +
> +    return { };

I think large part of this code is the same to addCallIndirect one, can you dedupe them by extracting this part?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2722
> +    // Do a context switch if needed.
> +    {
> +        Value* offset = constant(pointerType(), safeCast<int32_t>(WebAssemblyFunctionBase::offsetOfInstance()));
> +        Value* targetInstance = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(),
> +            m_currentBlock->appendNew<Value>(m_proc, Add, origin(), callableFunction, offset));
> +        BasicBlock* continuation = m_proc.addBlock();
> +        BasicBlock* doContextSwitch = m_proc.addBlock();
> +
> +        Value* isSameContextInstance = m_currentBlock->appendNew<Value>(m_proc, Equal, origin(),
> +            targetInstance, instanceValue());
> +        m_currentBlock->appendNewControlValue(m_proc, B3::Branch, origin(),
> +            isSameContextInstance, FrequentedBlock(continuation), FrequentedBlock(doContextSwitch));
> +
> +        PatchpointValue* patchpoint = doContextSwitch->appendNew<PatchpointValue>(m_proc, B3::Void, origin());
> +        patchpoint->effects.writesPinned = true;
> +        // We pessimistically assume we're calling something with BoundsChecking memory.
> +        // FIXME: We shouldn't have to do this: https://bugs.webkit.org/show_bug.cgi?id=172181
> +        patchpoint->clobber(PinnedRegisterInfo::get().toSave(MemoryMode::BoundsChecking));
> +        patchpoint->clobber(RegisterSet::macroScratchRegisters());
> +        patchpoint->append(targetInstance, ValueRep::SomeRegister);
> +        patchpoint->append(instanceValue(), ValueRep::SomeRegister);
> +        patchpoint->numGPScratchRegisters = Gigacage::isEnabled(Gigacage::Primitive) ? 1 : 0;
> +
> +        patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
> +            AllowMacroScratchRegisterUsage allowScratch(jit);
> +            GPRReg targetInstance = params[0].gpr();
> +            GPRReg oldContextInstance = params[1].gpr();
> +            const PinnedRegisterInfo& pinnedRegs = PinnedRegisterInfo::get();
> +            GPRReg baseMemory = pinnedRegs.baseMemoryPointer;
> +            ASSERT(targetInstance != baseMemory);
> +            jit.loadPtr(CCallHelpers::Address(oldContextInstance, Instance::offsetOfCachedStackLimit()), baseMemory);
> +            jit.storePtr(baseMemory, CCallHelpers::Address(targetInstance, Instance::offsetOfCachedStackLimit()));
> +            jit.storeWasmContextInstance(targetInstance);
> +            ASSERT(pinnedRegs.boundsCheckingSizeRegister != baseMemory);
> +            // FIXME: We should support more than one memory size register
> +            //   see: https://bugs.webkit.org/show_bug.cgi?id=162952
> +            ASSERT(pinnedRegs.boundsCheckingSizeRegister != targetInstance);
> +            GPRReg scratchOrBoundsCheckingSize = Gigacage::isEnabled(Gigacage::Primitive) ? params.gpScratch(0) : pinnedRegs.boundsCheckingSizeRegister;
> +
> +            jit.loadPtr(CCallHelpers::Address(targetInstance, Instance::offsetOfCachedBoundsCheckingSize()), pinnedRegs.boundsCheckingSizeRegister); // Memory size.
> +            jit.loadPtr(CCallHelpers::Address(targetInstance, Instance::offsetOfCachedMemory()), baseMemory); // Memory::void*.
> +
> +            jit.cageConditionally(Gigacage::Primitive, baseMemory, pinnedRegs.boundsCheckingSizeRegister, scratchOrBoundsCheckingSize);
> +        });
> +        doContextSwitch->appendNewControlValue(m_proc, Jump, origin(), continuation);
> +
> +        m_currentBlock = continuation;
> +    }
> +
> +    ExpressionType calleeCode = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(),
> +        m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(), callableFunction,
> +            safeCast<int32_t>(WebAssemblyFunction::offsetOfEntrypointLoadLocation())));
> +
> +    B3::Type returnType = toB3ResultType(&signature);
> +    ExpressionType callResult = createCallPatchpoint(m_currentBlock, origin(), signature, args,
> +        scopedLambdaRef<void(PatchpointValue*)>([=] (PatchpointValue* patchpoint) -> void {
> +            patchpoint->effects.writesPinned = true;
> +            patchpoint->effects.readsPinned = true;
> +            // We need to clobber all potential pinned registers since we might be leaving the instance.
> +            // We pessimistically assume we're always calling something that is bounds checking so
> +            // because the wasm->wasm thunk unconditionally overrides the size registers.
> +            // FIXME: We should not have to do this, but the wasm->wasm stub assumes it can
> +            // use all the pinned registers as scratch: https://bugs.webkit.org/show_bug.cgi?id=172181
> +            patchpoint->clobberLate(PinnedRegisterInfo::get().toSave(MemoryMode::BoundsChecking));
> +
> +            patchpoint->append(calleeCode, ValueRep::SomeRegister);
> +            patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
> +                AllowMacroScratchRegisterUsage allowScratch(jit);
> +                jit.call(params[params.proc().resultCount(returnType)].gpr(), WasmEntryPtrTag);
> +            });
> +        }));
> +
> +    switch (returnType.kind()) {
> +    case B3::Void: {
> +        break;
> +    }
> +    case B3::Tuple: {
> +        const Vector<B3::Type>& tuple = m_proc.tupleForType(returnType);
> +        for (unsigned i = 0; i < signature.returnCount(); ++i)
> +            results.append(m_currentBlock->appendNew<ExtractValue>(m_proc, origin(), tuple[i], callResult, i));
> +        break;
> +    }
> +    default: {
> +        results.append(callResult);
> +        break;
> +    }
> +    }
> +
> +    // The call could have been to another WebAssembly instance, and / or could have modified our Memory.
> +    restoreWebAssemblyGlobalState(RestoreCachedStackLimit::Yes, m_info.memory, instanceValue(), m_proc, m_currentBlock);
> +
> +    return { };
> +}

I think large part of this is duplicate to addCallIndirect. Can you extract this and dedupe the code?

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:1255
> +        const SignatureIndex calleeSignatureIndex = reinterpret_cast<SignatureIndex>(m_expressionStack.last().type().index);

We do not need to have reinterpret_cast since Type::index is SignatureIndex

> Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:518
> +    WebAssemblyFunction* wasmFunction = jsCast<WebAssemblyFunction*>(referenceAsObject);
> +    Wasm::Instance* targetInstance = &wasmFunction->instance()->instance();
> +
> +    if (targetInstance != instance)
> +        targetInstance->setCachedStackLimit(instance->cachedStackLimit());
> +
> +    const Wasm::WasmToWasmImportableFunction& function = wasmFunction->importableFunction();
> +    WASM_CALL_RETURN(targetInstance, function.entrypointLoadLocation->executableAddress(), WasmEntryPtrTag);

Why don't we need to perform signature check here? If it is not necessary, can you insert ASSERT that ensures the signature is correct?

[Dmitry]

Created attachment 425008 [details]
Patch

[Dmitry]

Created attachment 425016 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 425016 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3351
> +    // Note: call indirect can call either WebAssemblyFunction or WebAssemblyWrapperFunction. Because

call-indirect => call-ref.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3358
> +    append(Move, Arg::addr(calleeFunction, WebAssemblyFunction::offsetOfEntrypointLoadLocation()), calleeCode); // Pointer to callee code.

WebAssemblyFunction::offsetOfEntrypointLoadLocation is only valid for WebAssemblyFunction. But the above comment is mentioning to WebAssemblyWrapperFunction.
If this is WebAssemblyWrapperFunction, then WebAssemblyFunction::offsetOfEntrypointLoadLocation() is not correct? Can you check?
And can you add a test for both cases?

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3405
>              jit.cageConditionallyAndUntag(Gigacage::Primitive, baseMemory, pinnedRegs.boundsCheckingSizeRegister, scratch);

Let's rename newContextInstance => calleeInstance

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:912
> +            AllowMacroScratchRegisterUsage allowScratch(jit);
> +            GPRReg newContextInstance = params[0].gpr();
> +            GPRReg oldContextInstance = params[1].gpr();
> +            const PinnedRegisterInfo& pinnedRegs = PinnedRegisterInfo::get();
> +            GPRReg baseMemory = pinnedRegs.baseMemoryPointer;
> +            ASSERT(newContextInstance != baseMemory);
> +            jit.loadPtr(CCallHelpers::Address(oldContextInstance, Instance::offsetOfCachedStackLimit()), baseMemory);
> +            jit.storePtr(baseMemory, CCallHelpers::Address(newContextInstance, Instance::offsetOfCachedStackLimit()));
> +            jit.storeWasmContextInstance(newContextInstance);
> +            ASSERT(pinnedRegs.boundsCheckingSizeRegister != baseMemory);
> +            // FIXME: We should support more than one memory size register
> +            //   see: https://bugs.webkit.org/show_bug.cgi?id=162952
> +            ASSERT(pinnedRegs.boundsCheckingSizeRegister != newContextInstance);
> +            GPRReg scratch = params.gpScratch(0);
> +
> +            jit.loadPtr(CCallHelpers::Address(newContextInstance, Instance::offsetOfCachedBoundsCheckingSize()), pinnedRegs.boundsCheckingSizeRegister); // Memory size.
> +            jit.loadPtr(CCallHelpers::Address(newContextInstance, Instance::offsetOfCachedMemory()), baseMemory); // Memory::void*.
> +
> +            jit.cageConditionallyAndUntag(Gigacage::Primitive, baseMemory, pinnedRegs.boundsCheckingSizeRegister, scratch);

Rename newContextInstance => calleeInstance.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2641
> +            safeCast<int32_t>(WebAssemblyFunction::offsetOfEntrypointLoadLocation())));

Ditto.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:1252
> +    case CallRef: {

We need feature flag (`false` currently) for this. Like,

WASM_PARSER_FAIL_IF(!Options::useWebAssemblyFunctionReferences(), "function references are not enabled");

[Yusuke Suzuki]

Comment on attachment 425016 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2631
> +    // WebAssemblyWrapperFunction is like calling into the embedder, we conservatively assume all call indirects

call indirect => call ref.

[Yusuke Suzuki]

Comment on attachment 425016 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:511
> +    WebAssemblyFunction* wasmFunction = jsCast<WebAssemblyFunction*>(referenceAsObject);

Isn't it possible that we get WebAssemblyWrapperFunction?

[Yusuke Suzuki]

Comment on attachment 425016 [details]
Patch

Setting r- since it is reviewed and several issues exist.

[Dmitry]

Created attachment 427337 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 427337 [details]
Patch

r=me

[EWS]

Committed r276896 (237242@main): <https://commits.webkit.org/237242@main>

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