Bug 229681 – Implement the WebAssembly exception handling proposal

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

https://github.com/WebAssembly/exception-handling/blob/master/proposals/exception-handling/Exceptions.md

[Tadeu Zagallo]

<rdar://81603387>

[Tadeu Zagallo]

Created attachment 438359 [details]
WIP

[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".

[Tadeu Zagallo]

Created attachment 440106 [details]
WIP

[Keith Miller]

Comment on attachment 440106 [details]
WIP

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

drive by comments

> Source/JavaScriptCore/ChangeLog:9
> +        WIP Spec: https://github.com/WebAssembly/exception-handling

Please add a change log covering:

The new instructions
Why callee is on VM.
Air is missing
B3 uses Variables everywhere now.

> Source/JavaScriptCore/llint/LLIntCommon.h:32
> -#define LLINT_TRACING 0
> +#define LLINT_TRACING 1

Revert.

> Source/JavaScriptCore/llint/WebAssembly.asm:259
> +    # TODO: is this correct? or do we need to store in the slow path

Should this TODO still exist? Why do we need this?

> Source/JavaScriptCore/llint/WebAssembly.asm:2866
> +    subp 8, t2
> +    addp 8, t1

Seems like we could special case ARM64 here with the store-add/sub instructions. Maybe add a FIXME?

> Source/JavaScriptCore/runtime/VM.h:1323
> +#if ENABLE(WEBASSEMBLY)
> +    Vector<Wasm::Tag> m_wasmTags;
> +#endif

Why is this here? Aren't tags ref counted? I assume you meant to delete this.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3006
> +auto AirIRGenerator::addTry(BlockSignature, Stack& , ControlType& , Stack& ) -> PartialResult

Can you add a link to a bug to either implement exceptions here or rip the Air generator out?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:275
> +        Vector<Value*> phis;

Can you file a bug to compress this structure slightly. I think a lot of these could be union members.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:681
> +            getInstance->effects.writesPinned = false;
> +            getInstance->effects.readsPinned = true;
> +            getInstance->resultConstraints = { ValueRep::reg(m_wasmContextInstanceGPR) };

Wait, why does this say that it only reads pinned when it actually only writes pinned?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:857
> +            auto preciseAllocationCase = jit.branchTestPtr(CCallHelpers::NonZero, GPRInfo::regT0, CCallHelpers::TrustedImm32(PreciseAllocation::halfAlignment));

How do we get a mix of precise and non-precise callees for wasm? Is there inline data appended to the cell? If we don't know can you file a bug and put a FIXME here.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:897
> +    m_currentBlock = m_topLevelBlock;
> +    m_currentBlock->appendNew<Value>(m_proc, EntrySwitch, Origin());
> +    for (BasicBlock* block : m_rootBlocks)
> +        m_currentBlock->appendSuccessor(FrequentedBlock(block));

I thought B3 had a side entry mechanism? Is there a reason we're not using that?

[Tadeu Zagallo]

Comment on attachment 440106 [details]
WIP

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        WIP Spec: https://github.com/WebAssembly/exception-handling
> 
> Please add a change log covering:
> 
> The new instructions
> Why callee is on VM.
> Air is missing
> B3 uses Variables everywhere now.

Yes, absolutely, I'll update the ChangeLog before the final patch.

>> Source/JavaScriptCore/llint/LLIntCommon.h:32
>> +#define LLINT_TRACING 1
> 
> Revert.

Oops

>> Source/JavaScriptCore/llint/WebAssembly.asm:259
>> +    # TODO: is this correct? or do we need to store in the slow path
> 
> Should this TODO still exist? Why do we need this?

The TODO should be gone. This is how we now where the thrown from when unwinding.

>> Source/JavaScriptCore/llint/WebAssembly.asm:2866
>> +    addp 8, t1
> 
> Seems like we could special case ARM64 here with the store-add/sub instructions. Maybe add a FIXME?

sure

>> Source/JavaScriptCore/runtime/VM.h:1323
>> +#endif
> 
> Why is this here? Aren't tags ref counted? I assume you meant to delete this.

deleted

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3006
>> +auto AirIRGenerator::addTry(BlockSignature, Stack& , ControlType& , Stack& ) -> PartialResult
> 
> Can you add a link to a bug to either implement exceptions here or rip the Air generator out?

Will do

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:275
>> +        Vector<Value*> phis;
> 
> Can you file a bug to compress this structure slightly. I think a lot of these could be union members.

I think the only things that could overlap are the special and all the exception specific ones, which one save one word. I'll file the bug.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:681
>> +            getInstance->resultConstraints = { ValueRep::reg(m_wasmContextInstanceGPR) };
> 
> Wait, why does this say that it only reads pinned when it actually only writes pinned?

That's a good point. I copied the code from the prologue and didn't realize.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:857
>> +            auto preciseAllocationCase = jit.branchTestPtr(CCallHelpers::NonZero, GPRInfo::regT0, CCallHelpers::TrustedImm32(PreciseAllocation::halfAlignment));
> 
> How do we get a mix of precise and non-precise callees for wasm? Is there inline data appended to the cell? If we don't know can you file a bug and put a FIXME here.

I'm not actually sure, that's just how we do it everywhere else. I'll add the FIXME.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:897
>> +        m_currentBlock->appendSuccessor(FrequentedBlock(block));
> 
> I thought B3 had a side entry mechanism? Is there a reason we're not using that?

I'm not sure what you are referring to. This is the only way I saw how to do it.

[Tadeu Zagallo]

Created attachment 440322 [details]
Patch

[Tadeu Zagallo]

Created attachment 440337 [details]
Patch

Fix build

[Tadeu Zagallo]

Created attachment 440368 [details]
Patch

More build fixes

[Tadeu Zagallo]

Created attachment 440370 [details]
Patch

More build fixes (2)

[Tadeu Zagallo]

Created attachment 440373 [details]
Patch

More build fixes (3)

[Tadeu Zagallo]

Created attachment 440392 [details]
Patch

Fix llint when fast TLS is disabled

[Tadeu Zagallo]

Created attachment 440424 [details]
Patch

Fix clobbered patchpoint arguments on throw and rethrow

[Yusuke Suzuki]

Comment on attachment 440424 [details]
Patch

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

Quick comment about runtime.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:303
> +void Instance::addTag(const Tag& tag)

Let's take `Ref<Tag>&& tag` instead.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyException.h:34
> +class JSWebAssemblyException : public JSDestructibleObject {

Put `final`.
And we should not use JSDestructibleObject anymore except for JSC shell testing classes etc.
Use JSNonFinalObject for Base. And define IsoSubspace with destructible CellType.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyException.h:36
> +    using Base = JSDestructibleObject;

Use JSNonFinalObject.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTag.h:36
> +    using Base = JSNonFinalObject;

Need `static const bool needsDestruction = true`.
And need to define destroy function since this is destructible.

> Source/JavaScriptCore/wasm/js/WebAssemblyExceptionConstructor.h:39
> +    typedef InternalFunction Base;

Use `using` for newer code.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:405
> +            UNUSED_VARIABLE(expectedSignatureIndex);

This is not necessary.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:411
> +            RETURN_IF_EXCEPTION(scope, void());

addTag will never throw. So let's remove it.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:443
> +        m_instance->instance().addTag(Wasm::Tag::create(Wasm::SignatureInformation::get(signatureIndex)).leakRef());

leakRef() looks not great. Let's just pass Ref&& and define addTag(Ref&&) function.

> Source/JavaScriptCore/wasm/js/WebAssemblyTagConstructor.cpp:109
> +JSWebAssemblyTag* WebAssemblyTagConstructor::createTag(JSGlobalObject* globalObject, CallFrame* callFrame, const Wasm::Tag& tag)
> +{
> +    VM& vm = globalObject->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +
> +    JSObject* newTarget = asObject(callFrame->newTarget());
> +    Structure* structure = JSC_GET_DERIVED_STRUCTURE(vm, webAssemblyTagStructure, newTarget, callFrame->jsCallee());
> +    RETURN_IF_EXCEPTION(scope, nullptr);
> +
> +    RELEASE_AND_RETURN(scope, JSWebAssemblyTag::create(vm, globalObject, structure, tag));
> +}

Let's remove this since nobody is using that.

> Source/JavaScriptCore/wasm/js/WebAssemblyTagConstructor.h:44
> +    typedef InternalFunction Base;

Use `using` for the new code.

[Tadeu Zagallo]

Created attachment 440463 [details]
Patch

[Tadeu Zagallo]

(In reply to Yusuke Suzuki from comment #14)
> 
> Quick comment about runtime.

Thanks Yusuke. I just uploaded a version of the patch with all the fixes you suggested.

[Keith Miller]

Comment on attachment 440463 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:94
> +    unsigned m_callSiteIndex {s_invalidCallSiteIndex };

Style: add space after {

> Source/JavaScriptCore/wasm/WasmTag.cpp:36
> +}} // namespace JSC::Wasm

Style add space between }s

> Source/JavaScriptCore/wasm/WasmTag.h:55
> +    static uint32_t s_id;

This needs to be atomic.

> Source/JavaScriptCore/wasm/generateWasmB3IRGeneratorInlinesHeader.py:197
> -""" + generateB3Code(opcode, b3op) + """;
> +""" + generateB3Code(opcode, b3op) + """

Why make this change?

> Source/JavaScriptCore/wasm/wasm.json:45
> +        "Exception": { "type": "varuint7", "value": 13, "description": "Exception declarations" }

I think we should keep this consistent with the one in JSTests. I moved this line up so the builder had the proper expected section ordering.

[Tadeu Zagallo]

Comment on attachment 440463 [details]
Patch

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

Thanks, I'll update with the other suggestions.

>> Source/JavaScriptCore/wasm/generateWasmB3IRGeneratorInlinesHeader.py:197
>> +""" + generateB3Code(opcode, b3op) + """
> 
> Why make this change?

I changed the code generated to use variables, and I ended up moving around how the semicolons were emitted.

[Tadeu Zagallo]

Created attachment 440683 [details]
Patch for landing

[EWS]

Committed r283852 (242731@main): <https://commits.webkit.org/242731@main>

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

[Saam Barati]

Comment on attachment 440683 [details]
Patch for landing

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

First round of comments. Will continue to review tomorrow.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:595
> +

nit: no new line here

> Source/JavaScriptCore/interpreter/Interpreter.cpp:655
> +    const Wasm::Tag* m_wasmTag { nullptr };

to be safe, should this be a RefPtr?

> Source/JavaScriptCore/interpreter/Interpreter.h:94
> +            : m_valid(false)

nit: can be a field initializer and this be "= default"

> Source/JavaScriptCore/jit/JSInterfaceJIT.h:60
> +        void convertCalleeToVM(RegisterID callee);

this isn't used

> Source/JavaScriptCore/llint/WebAssembly.asm:259
> +    storei PC, ArgumentCountIncludingThis + TagOffset[cfr]

Do we need to also do this once we've left the catch region?

> Source/JavaScriptCore/llint/WebAssembly.asm:2825
> +wasmOp(rethrow, WasmRethrow, macro(ctx)

Why don't we need to copyCalleeSavesToEntryFrameCalleeSavesBuffer here? Looks like it might be wrong at first glance.

> Source/JavaScriptCore/llint/WebAssembly.asm:2888
> +macro catchAllImpl(ctx, storeWasmInstance)

This looks almost identical to the beginning of catch macro. Can we share code instead of copy-pasting?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:134
> +            UNUSED_PARAM(proc);
> +            UNUSED_PARAM(origin);

Why? They're both used below.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:136
> +            for (unsigned i = 0; i < signature->returnCount(); ++i)
> +                phis.append(proc.add<Value>(Phi, toB3Type(signature->returnType(i)), origin));

In the spirit of making stuff variables, should these just be variables?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:466
> +    void connectControlEntry(unsigned& indexInBuffer, Value* pointer, ControlData&, Stack& expressionStack, ControlData& currentData, bool fillLoopPhis = false);

nit: I think a better name is connectControlAtEntrypoint

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:590
> +    jit.store32(CCallHelpers::TrustedImm32(m_callSiteIndex), CCallHelpers::tagFor(CallFrameSlot::argumentCountIncludingThis));

It worries we don't do this when calling out to random C runtime calls from 2 perspectives:
1. Are there really no places we can catch exceptions from the runtime calls?
2. Will those throw and see a stale call site index, leading us to catch an exception with a stackmap that doesn't actually represent the current program machine state.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:874
> +    Ref<B3::Air::PrologueGenerator> catchPrologueGenerator = createSharedTask<B3::Air::PrologueGeneratorFunction>([] (CCallHelpers& jit, B3::Air::Code& code) {

nit: there is no reason afaict to generate this code more than once ever. Can this just be a shared between all the things in a process?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:878
> +        {

I commented on that helper JIT function being unused, but maybe you meant to use it here?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2455
> +void B3IRGenerator::connectControlEntry(unsigned& indexInBuffer, Value* pointer, ControlData& data, Stack& expressionStack, ControlData& currentData, bool fillLoopPhis)

these names are a bit confusing. What's "data" vs "currentData"?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2457
> +    // For each stack entry enclosed by this loop we need to replace the value with a phi so we can fill it on OSR entry.

this comment isn't accurate anymore.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2462
> +        if (fillLoopPhis)
> +            m_currentBlock->appendNew<UpsilonValue>(m_proc, origin(), load, data.phis[i]);

Can we move to a world where everything is a variable? This kinda confuses me tbh. Not sure why it's sometimes ok. I know why it's sometimes not ok.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2586
> +PatchpointExceptionHandle B3IRGenerator::preparePatchpointForExceptions(BasicBlock* block, PatchpointValue* patch)

nice

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2592
> +    Vector<Value*> stackmap;

nit: this name isn't quite accurate. I'd call this "liveValues"

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2709
> +    m_exceptionHandlers.append({ HandlerType::Delegate, data.tryStart(), ++m_callSiteIndex, 0, m_tryDepth, targetDepth });

what's up with using m_callSiteIndex here?

How did you implement delegates? I don't quite understand how that code works.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2725
> +    RegisterSet clobber = RegisterSet::macroScratchRegisters();
> +    clobber.add(GPRInfo::argumentGPR2);
> +    clobber.add(GPRInfo::argumentGPR3);
> +    patch->clobber(clobber);
> +    patch->clobberLate(RegisterSet::volatileRegistersForJSCall());

I think this can just be summarized as patch->clobberLate(RegisterSet::volatileRegistersForJSCall()) for how you use things below.

You can also just use nonArgumentGPR0 as your scratch register, so B3 doesn't potentially allocate a callee save just for your scratch.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2734
> +        {

ditto about that helper

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2763
> +    PatchpointValue* patch = m_proc.add<PatchpointValue>(B3::Void, origin());
> +    patch->clobber(RegisterSet::macroScratchRegisters());

similar comments about patchpoint clobbering and what not as catch

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2893
> +        --m_tryDepth;

this is > 0 even while in a catch? Is that intended? Seems like a weird name if so.

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:202
> +    else if (!m_moduleInformation->m_functionDoesNotUseExceptions.quickGet(functionIndex))
> +        forceUsingB3 = true;

small nit: I would've avoided the double negative here, and just made "m_functionsThatUseExceptions" and made the check "if (m_functionsThatUseExceptions.quickGet(functionIndex)", and then, of course, invert the bit set.

> Source/JavaScriptCore/wasm/WasmCallee.h:77
> +    virtual FunctionCodeBlock* functionCodeBlock() const { return 0; }

- This isn't a great name, and is even more confusing where it's used in the unwinder. This should be named something like "llintFunctionCodeBlock"

- Also, "return nullptr", not "return 0"

> Source/JavaScriptCore/wasm/js/JSWebAssemblyException.h:34
> +class JSWebAssemblyException final : public JSNonFinalObject {

this sin't supposed to be an ErrorInstance or something like that spec wise?

[Saam Barati]

Comment on attachment 440683 [details]
Patch for landing

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

> Source/JavaScriptCore/wasm/WasmInstance.h:244
> +    Vector<RefPtr<const Tag>> m_tags;

are these hash-consed?

[Saam Barati]

Comment on attachment 440683 [details]
Patch for landing

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

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2779
> +        {

similar comments about that unused helper here. Maybe it can be used?

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2893
>> +        --m_tryDepth;
> 
> this is > 0 even while in a catch? Is that intended? Seems like a weird name if so.

Reading this more, we only increment for Try. Should we really decrement for Catch?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:209
> +            if ((VirtualRegister)src != (VirtualRegister)dst)

Nit: static_cast.

Also, is the cast needed?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:503
> +    Checked<unsigned> m_maxTryDepth { 0 };

this seems unused?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:551
> +template<typename Opcode>
> +void LLIntGenerator::repatch(const CatchRewriteInfo& info)

Is this used?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:672
> +    const unsigned base = stackOffset - CallFrame::headerSizeInRegisters - 1;

you add 1 above, and subtract 1 here. Is it actually needed?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:1069
> +    Ref<Label> continuation = newLabel();

how does this label get emitted?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:1199
> +    walkExpressionStack(args, [&](VirtualRegister& arg, VirtualRegister slot) {
> +        if (arg == slot)
> +            return;
> +        WasmMov::emit(this, slot, arg);
> +        arg = slot;
> +    });

why is this needed?

> Source/JavaScriptCore/wasm/WasmOperations.cpp:913
> +JSC_DEFINE_JIT_OPERATION(operationWasmThrow, void*, (Instance* instance, CallFrame* callFrame, unsigned exceptionIndex, EncodedJSValue* arguments))

nit: uint64_t* instead of EncodedJSValue* as the parameter here, since they're not JSValues

> Source/JavaScriptCore/wasm/js/WebAssemblyTagPrototype.cpp:115
> +    RELEASE_AND_RETURN(throwScope, JSValue::encode(type));

nit: this can just be "return JSValue::encode(type)" since we exception check the line above.

[Tadeu Zagallo]

Comment on attachment 440683 [details]
Patch for landing

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

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:655
>> +    const Wasm::Tag* m_wasmTag { nullptr };
> 
> to be safe, should this be a RefPtr?

I don't think it's necessary, the exception has to be live while unwinding, but I can change it if you think it's safer.

>> Source/JavaScriptCore/interpreter/Interpreter.h:94
>> +            : m_valid(false)
> 
> nit: can be a field initializer and this be "= default"

oh, that's a good point, forgot about that.

>> Source/JavaScriptCore/jit/JSInterfaceJIT.h:60
>> +        void convertCalleeToVM(RegisterID callee);
> 
> this isn't used

Yeah, I meant to use it where you pointed below, but it didn't work out

>> Source/JavaScriptCore/llint/WebAssembly.asm:259
>> +    storei PC, ArgumentCountIncludingThis + TagOffset[cfr]
> 
> Do we need to also do this once we've left the catch region?

I don't think so? we can only throw again if you call a slow path again, so the it'd have to be updated by then

>> Source/JavaScriptCore/llint/WebAssembly.asm:2825
>> +wasmOp(rethrow, WasmRethrow, macro(ctx)
> 
> Why don't we need to copyCalleeSavesToEntryFrameCalleeSavesBuffer here? Looks like it might be wrong at first glance.

Keith wrote this part, I assumed it was because if we rethrow we must have caught, therefore the VM must have the most up-to-date, but as I write I don't think that always holds, e.g. if you throw, catch, call, rethrow. I think we also need to do it here.

>> Source/JavaScriptCore/llint/WebAssembly.asm:2888
>> +macro catchAllImpl(ctx, storeWasmInstance)
> 
> This looks almost identical to the beginning of catch macro. Can we share code instead of copy-pasting?

yeah, we could share some code here, I'll update

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:134
>> +            UNUSED_PARAM(origin);
> 
> Why? They're both used below.

This must be left over from when I was testing, I'll remove.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:136
>> +                phis.append(proc.add<Value>(Phi, toB3Type(signature->returnType(i)), origin));
> 
> In the spirit of making stuff variables, should these just be variables?

I like the idea of keeping them Phis to distinguish between where we merge control flow from the stack variables, but I don't feel too strong about it.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:466
>> +    void connectControlEntry(unsigned& indexInBuffer, Value* pointer, ControlData&, Stack& expressionStack, ControlData& currentData, bool fillLoopPhis = false);
> 
> nit: I think a better name is connectControlAtEntrypoint

I reused the previous name, it kinda operates per ControlEntry, but I agree that is better.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:590
>> +    jit.store32(CCallHelpers::TrustedImm32(m_callSiteIndex), CCallHelpers::tagFor(CallFrameSlot::argumentCountIncludingThis));
> 
> It worries we don't do this when calling out to random C runtime calls from 2 perspectives:
> 1. Are there really no places we can catch exceptions from the runtime calls?
> 2. Will those throw and see a stale call site index, leading us to catch an exception with a stackmap that doesn't actually represent the current program machine state.

I inspected the code and couldn't find any other C calls that could throw exceptions that would be catchable from Wasm. The other exceptions that are thrown from the runtime are explicitly marked as not catchable from Wasm, so the stale call site index is irrelevant.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:874
>> +    Ref<B3::Air::PrologueGenerator> catchPrologueGenerator = createSharedTask<B3::Air::PrologueGeneratorFunction>([] (CCallHelpers& jit, B3::Air::Code& code) {
> 
> nit: there is no reason afaict to generate this code more than once ever. Can this just be a shared between all the things in a process?

I was concerned with preserving the register state, but I guess we could move everything up to line 902 into a thunk, then just make this function call the thunk, call the probe and adjust the stack?

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:878
>> +        {
> 
> I commented on that helper JIT function being unused, but maybe you meant to use it here?

Yeah, I meant to, but it's not accessible unfortunately, I will remove the function

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2455
>> +void B3IRGenerator::connectControlEntry(unsigned& indexInBuffer, Value* pointer, ControlData& data, Stack& expressionStack, ControlData& currentData, bool fillLoopPhis)
> 
> these names are a bit confusing. What's "data" vs "currentData"?

yeah, the naming is not good. "data" is the ControlEntry we're connecting, "currentData" is the ControlEntry that will become an entrypoint.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2457
>> +    // For each stack entry enclosed by this loop we need to replace the value with a phi so we can fill it on OSR entry.
> 
> this comment isn't accurate anymore.

I'll remove

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2462
>> +            m_currentBlock->appendNew<UpsilonValue>(m_proc, origin(), load, data.phis[i]);
> 
> Can we move to a world where everything is a variable? This kinda confuses me tbh. Not sure why it's sometimes ok. I know why it's sometimes not ok.

Not sure what do you mean by "it's ok sometimes". I'm still not sure whether I prefer Phis or variables, but I agree in this case it'd make things simpler.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2592
>> +    Vector<Value*> stackmap;
> 
> nit: this name isn't quite accurate. I'd call this "liveValues"

fair

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2709
>> +    m_exceptionHandlers.append({ HandlerType::Delegate, data.tryStart(), ++m_callSiteIndex, 0, m_tryDepth, targetDepth });
> 
> what's up with using m_callSiteIndex here?
> 
> How did you implement delegates? I don't quite understand how that code works.

We discussed this offline, but m_callSiteIndex here is used to capture the range for the try/catch/delegate. We increment it here too just to match how we select the handler when we have the bytecode index, since the delegate try/catch/delegate would have a new index in bytecode.

Delegate is implemented in WasmHandlerInfo.cpp by skipping certain catch candidates until we get to the target depth.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2725
>> +    patch->clobberLate(RegisterSet::volatileRegistersForJSCall());
> 
> I think this can just be summarized as patch->clobberLate(RegisterSet::volatileRegistersForJSCall()) for how you use things below.
> 
> You can also just use nonArgumentGPR0 as your scratch register, so B3 doesn't potentially allocate a callee save just for your scratch.

oh, that's a good point, since I'm not reading from params in registers. I'll update it.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2763
>> +    patch->clobber(RegisterSet::macroScratchRegisters());
> 
> similar comments about patchpoint clobbering and what not as catch

You mean as throw? We still need to clobber macroScratchRegisters though, right? It also seems that I don't actually use `clobberLate` below....

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2893
>> +        --m_tryDepth;
> 
> this is > 0 even while in a catch? Is that intended? Seems like a weird name if so.

Yeah, I guess it could be named tryCatchDepth?

>> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:202
>> +        forceUsingB3 = true;
> 
> small nit: I would've avoided the double negative here, and just made "m_functionsThatUseExceptions" and made the check "if (m_functionsThatUseExceptions.quickGet(functionIndex)", and then, of course, invert the bit set.

The only reason it is this way is so it defaults to false, but I guess I could set everything to true early on, but it just seemed unnecessary.

>> Source/JavaScriptCore/wasm/WasmCallee.h:77
>> +    virtual FunctionCodeBlock* functionCodeBlock() const { return 0; }
> 
> - This isn't a great name, and is even more confusing where it's used in the unwinder. This should be named something like "llintFunctionCodeBlock"
> 
> - Also, "return nullptr", not "return 0"

that makes sense. No idea why I wrote return 0....

>> Source/JavaScriptCore/wasm/WasmInstance.h:244
>> +    Vector<RefPtr<const Tag>> m_tags;
> 
> are these hash-consed?

no, they shouldn't be as per the spec

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyException.h:34
>> +class JSWebAssemblyException final : public JSNonFinalObject {
> 
> this sin't supposed to be an ErrorInstance or something like that spec wise?

I looked into how other exceptions worked and they didn't extend ErrorInstance. We do use use the ErrorInstanceType blow.

[Tadeu Zagallo]

Comment on attachment 440683 [details]
Patch for landing

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

>>>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2893
>>>> +        --m_tryDepth;
>>> 
>>> this is > 0 even while in a catch? Is that intended? Seems like a weird name if so.
>> 
>> Reading this more, we only increment for Try. Should we really decrement for Catch?
> 
> Yeah, I guess it could be named tryCatchDepth?

Sorry, your second comment hadn't loaded before. We increment at beginning of a try/catch block, and we have to decrement at the end. We may or may not have had a catch block.

>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:209
>> +            if ((VirtualRegister)src != (VirtualRegister)dst)
> 
> Nit: static_cast.
> 
> Also, is the cast needed?

This cast is needed because this might either be a VirtualRegister or a TypedExpression (which has the appropriate operation for casting into VirtualRegister).

>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:503
>> +    Checked<unsigned> m_maxTryDepth { 0 };
> 
> this seems unused?

I think it might be, it was used before when I used to emit the exceptions at the end.

>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:551
>> +void LLIntGenerator::repatch(const CatchRewriteInfo& info)
> 
> Is this used?

No, also left over with m_maxTryDepth.

>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:672
>> +    const unsigned base = stackOffset - CallFrame::headerSizeInRegisters - 1;
> 
> you add 1 above, and subtract 1 here. Is it actually needed?

yes, because of the alignment

>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:1069
>> +    Ref<Label> continuation = newLabel();
> 
> how does this label get emitted?

At addEndToUnreachable

>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:1199
>> +    });
> 
> why is this needed?

because we might have constants or elided moves in the stack, but we rely on all values being contiguous in the stack for the llint, so we have to normalize it

[Tadeu Zagallo]

Thanks, I think I addressed all your comments minus a few nits. I'll make a follow up patch to implement your suggestions.

[Saam Barati]

Comment on attachment 440683 [details]
Patch for landing

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

>>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:590
>>> +    jit.store32(CCallHelpers::TrustedImm32(m_callSiteIndex), CCallHelpers::tagFor(CallFrameSlot::argumentCountIncludingThis));
>> 
>> It worries we don't do this when calling out to random C runtime calls from 2 perspectives:
>> 1. Are there really no places we can catch exceptions from the runtime calls?
>> 2. Will those throw and see a stale call site index, leading us to catch an exception with a stackmap that doesn't actually represent the current program machine state.
> 
> I inspected the code and couldn't find any other C calls that could throw exceptions that would be catchable from Wasm. The other exceptions that are thrown from the runtime are explicitly marked as not catchable from Wasm, so the stale call site index is irrelevant.

we spoke offline after I wrote this. Makes sense.

>>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:874
>>> +    Ref<B3::Air::PrologueGenerator> catchPrologueGenerator = createSharedTask<B3::Air::PrologueGeneratorFunction>([] (CCallHelpers& jit, B3::Air::Code& code) {
>> 
>> nit: there is no reason afaict to generate this code more than once ever. Can this just be a shared between all the things in a process?
> 
> I was concerned with preserving the register state, but I guess we could move everything up to line 902 into a thunk, then just make this function call the thunk, call the probe and adjust the stack?

oh, I totally missed that. That's a good point. I'd keep it as is and ignore my original suggestion.

>>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2763
>>> +    patch->clobber(RegisterSet::macroScratchRegisters());
>> 
>> similar comments about patchpoint clobbering and what not as catch
> 
> You mean as throw? We still need to clobber macroScratchRegisters though, right? It also seems that I don't actually use `clobberLate` below....

yeah, I meant as throw above. I guess for full correctness it should probably also clobber all volatile registers for call. That said, I wonder if it'll ever matter in practice since this is terminal. But better to be safe.

>>>>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2893
>>>>> +        --m_tryDepth;
>>>> 
>>>> this is > 0 even while in a catch? Is that intended? Seems like a weird name if so.
>>> 
>>> Reading this more, we only increment for Try. Should we really decrement for Catch?
>> 
>> Yeah, I guess it could be named tryCatchDepth?
> 
> Sorry, your second comment hadn't loaded before. We increment at beginning of a try/catch block, and we have to decrement at the end. We may or may not have had a catch block.

I'm still a bit confused on this code, since we use m_tryDepth to see if we're in a try, right? But if you're in a catch, I don't think it dictates if you're in a try. For example, a top-level catch.

>>> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:202
>>> +        forceUsingB3 = true;
>> 
>> small nit: I would've avoided the double negative here, and just made "m_functionsThatUseExceptions" and made the check "if (m_functionsThatUseExceptions.quickGet(functionIndex)", and then, of course, invert the bit set.
> 
> The only reason it is this way is so it defaults to false, but I guess I could set everything to true early on, but it just seemed unnecessary.

gotcha. I think it's ok, since the code isn't complex.

>>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:1199
>>> +    });
>> 
>> why is this needed?
> 
> because we might have constants or elided moves in the stack, but we rely on all values being contiguous in the stack for the llint, so we have to normalize it

gotcha. Maybe worth a comment in this scenario?

[Michael Catanzaro]

Reopening to attach new patch.

[Michael Catanzaro]

Created attachment 441877 [details]
Patch for landing

[EWS]

Committed r284531 (243273@main): <https://commits.webkit.org/243273@main>

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