Bug 198102 – [WASM-References] Support Anyref in globals

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Justin Michaud]

Support Anyref global variables, imports and exports in WASM.

[Justin Michaud]

Created attachment 370381 [details]
Patch

[Saam Barati]

Comment on attachment 370381 [details]
Patch

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

r- because concurrent GC issues in your asm. Will continue to review.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:533
> +        append(block, Move, Arg::immPtr(tagCFunctionPtr<void*>(func, B3CCallPtrTag)), callee);

👍🏼

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:566
> +    void emitWriteBarrierForJsWrapper();

"JsWrapper" => "JSWrapper"

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1075
> +    append(Move, Arg::bigImm(Instance::offsetOfOwner()), cell);

really big imm? I don't think that's actually needed. Just need to verify on arm64. You could add an assert and just use this as an imm to the load.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1083
> +    append(Branch8, Arg::relCond(MacroAssembler::Above), Arg::addr(cell, JSCell::cellStateOffset()), Arg::imm(blackThreshold));
> +    m_currentBlock->setSuccessors(continuation, slowPath);

This looks wrong since we're not guaranteed at a machine level that this load happens before the store that happened above.

See SpeculativeJIT::compileStoreBarrier in the DFG.

AssemblyHelpers::barrierBranch() is loading the barrier threshold from the VM's Heap itself. We don't have that here.

[Saam Barati]

Comment on attachment 370381 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1083
>> +    m_currentBlock->setSuccessors(continuation, slowPath);
> 
> This looks wrong since we're not guaranteed at a machine level that this load happens before the store that happened above.
> 
> See SpeculativeJIT::compileStoreBarrier in the DFG.
> 
> AssemblyHelpers::barrierBranch() is loading the barrier threshold from the VM's Heap itself. We don't have that here.

One solution is to unconditionally emit a memory fence. However, this isn't optimal. It would be nice if we could get the barrier threshold here.

[Saam Barati]

Comment on attachment 370381 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmInstance.h:162
> +    MallocPtr<bool> m_globalsShouldMark;

This is kinda yucky. A few things:
1. Maybe this should be hash map? Seems unlikely most will be things we need to mark?
2. If we do keep it like this, definitely don't want to use a mallocptr here. At the very least, it would be a Vector<>. But maybe we should just instead use a bitvector. bit vector is nice because it means we're only adding one bit for each entry in the globals, which only increases memory use by 1/64th.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:97
> +    for (unsigned i = 0; i < thisObject->instance().numGlobals(); ++i) {
> +        // FIXME: We need to box wasm Funcrefs once they are supported here.
> +        if (thisObject->instance().shouldMarkGlobal(i))
> +            visitor.appendUnbarriered(JSValue::decode(thisObject->instance().loadI64Global(i)));
> +    }

Then this can just be a walk through the bitvector to get the index of every set bit, then just grab said global.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:386
> +                // FIXME: We need to box wasm Funcrefs once they are supported here.

please link to bugzilla

[Justin Michaud]

Created attachment 370485 [details]
Patch

[Saam Barati]

Comment on attachment 370485 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1086
> +    append(Load8, Arg::addr(cell, JSCell::cellStateOffset()), cellState);

Can this go in the branch below?

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1095
> +    m_currentBlock->setSuccessors(slowPath2, continuation);

Shouldn’t we just go to slowPath3 if the mutator isn’t fenced since that proves we need to write barrier? Also, you’re currently going to continuation when mutator is fenced. That’s wrong. You need to run the code below that uses the fence

[Saam Barati]

Comment on attachment 370485 [details]
Patch

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

Would be good to try to get a test that crashes. Probably run with collectContinuously=1, and allocate and store repeatedly in a loop

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:675
> +        FrequentedBlock(continuation), FrequentedBlock(slowPath2, FrequencyClass::Rare));

Same bug

[Justin Michaud]

Created attachment 370525 [details]
Patch

[Justin Michaud]

collectContinuously=1 did expose a different bug (globals needed to be zeroed in the constructor), but I was unable to get a test case that made it crash. I think the logic should be correct this time though.

[Saam Barati]

Comment on attachment 370525 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:534
> +        append(block, Move, Arg::
> +        immPtr(tagCFunctionPtr<void*>(func, B3CCallPtrTag)), callee);

style: oops

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:686
> +        m_currentBlock->appendNew<Value>(m_proc, Above, origin(), cellState, m_currentBlock->appendNew<Const32Value>(m_proc, origin(), blackThreshold)),

This is wrong. You need to reload the cell state here.

[Saam Barati]

Comment on attachment 370525 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmInstance.cpp:59
> +    memset(m_globals.get(), 0, globalMemoryByteSize(m_module.get()));

do you have a test for this that would've caused a crash?

> Source/JavaScriptCore/wasm/WasmInstance.h:163
> +    BitVector m_globalsShouldMark;

naming nit: "global to mark" of "globals should mark"

> Source/JavaScriptCore/wasm/WasmInstance.h:169
> +    unsigned m_numGlobals { 0 };

this field is not needed.

[Saam Barati]

Comment on attachment 370525 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmInstance.cpp:78
> +void Instance::setGlobal(unsigned i, JSValue bits)

nit: bits => value

It would also be good to somehow mark these places when we do threads to ensure they're unreachable.

[Saam Barati]

Comment on attachment 370525 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmInstance.h:163
>> +    BitVector m_globalsShouldMark;
> 
> naming nit: "global to mark" of "globals should mark"

instead of*

[Justin Michaud]

Comment on attachment 370525 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmInstance.cpp:59
>> +    memset(m_globals.get(), 0, globalMemoryByteSize(m_module.get()));
> 
> do you have a test for this that would've caused a crash?

collectContinuously=1 with the current test is how I discovered it.

[Justin Michaud]

Created attachment 370552 [details]
Patch

[EWS Watchlist]

Comment on attachment 370552 [details]
Patch

Attachment 370552 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12275583

New failing tests:
js/dom/custom-constructors.html

[EWS Watchlist]

Created attachment 370562 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Justin Michaud]

Test failure seems unrelated, and seems to have been happening already.

[Justin Michaud]

Created attachment 370584 [details]
Patch

[Saam Barati]

Comment on attachment 370584 [details]
Patch

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

r=me

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1097
> +    auto* patch = addPatchpoint(B3::Void);

let's call this doFence

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1100
> +    patch->setGenerator([] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
> +        jit.memoryFence();
> +    });

nit: Let's file a follow up, but B3/Air both have Fence instructions. We should just use those instead of a patchpoint and make sure we have the proper effects.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:680
> +    B3::PatchpointValue* doBarrier = m_currentBlock->appendNew<B3::PatchpointValue>(m_proc, B3::Void, origin());

let's call this doFence

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:95
> +    VM* m_vm; // Pointer so that offsetof works.

no need for comment.

[Justin Michaud]

Created attachment 370611 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 370611 [details]
Patch

Clearing flags on attachment: 370611

Committed r245765: <https://trac.webkit.org/changeset/245765>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

It appears that after changes in r245765 there are 23 constant failures on the JSB Debug queue.

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20(Tests)?numbuilds=50

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2877/steps/jscore-test/logs/stdio

** The following JSC stress test failures have been introduced:
	wasm.yaml/wasm/function-tests/context-switch.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/memory-alignment.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/memory-import-and-grow.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/stack-trace.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/trap-after-cross-instance-call.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/trap-load-2.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/trap-store-2.js.wasm-collect-continuously
	wasm.yaml/wasm/function-tests/trap-store.js.wasm-collect-continuously
	wasm.yaml/wasm/fuzz/export-function.js.wasm-collect-continuously
	wasm.yaml/wasm/fuzz/memory.js.wasm-collect-continuously
	wasm.yaml/wasm/js-api/Instance.imports.exports.unicode.js.wasm-collect-continuously
	wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-collect-continuously
	wasm.yaml/wasm/js-api/element.js.wasm-collect-continuously
	wasm.yaml/wasm/js-api/export-arity.js.wasm-collect-continuously
	wasm.yaml/wasm/js-api/test_Instance.js.wasm-collect-continuously
	wasm.yaml/wasm/js-api/test_memory.js.wasm-collect-continuously
	wasm.yaml/wasm/js-api/wasm-to-wasm-bad-signature.js.wasm-collect-continuously
	wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.wasm-collect-continuously
	wasm.yaml/wasm/references/anyref_globals.js.wasm-collect-continuously
	wasm.yaml/wasm/self-test/test_BuilderJSON.js.wasm-collect-continuously
	wasm.yaml/wasm/spec-tests/f32_cmp.wast.js.wasm-collect-continuously