Bug 198102 - [WASM-References] Support Anyref in globals
Summary: [WASM-References] Support Anyref in globals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-21 21:58 PDT by Justin Michaud
Modified: 2019-05-28 09:05 PDT (History)
9 users (show)

See Also:


Attachments
Patch (22.77 KB, patch)
2019-05-21 22:11 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (27.51 KB, patch)
2019-05-22 21:57 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (27.68 KB, patch)
2019-05-23 15:13 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (27.30 KB, patch)
2019-05-23 19:36 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (13.71 MB, application/zip)
2019-05-23 22:13 PDT, Build Bot
no flags Details
Patch (29.43 KB, patch)
2019-05-24 13:29 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (29.39 KB, patch)
2019-05-24 17:11 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2019-05-21 21:58:02 PDT
Support Anyref global variables, imports and exports in WASM.
Comment 1 Justin Michaud 2019-05-21 22:11:36 PDT
Created attachment 370381 [details]
Patch
Comment 2 Saam Barati 2019-05-22 16:59:11 PDT
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.
Comment 3 Saam Barati 2019-05-22 17:00:21 PDT
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.
Comment 4 Saam Barati 2019-05-22 17:05:17 PDT
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
Comment 5 Justin Michaud 2019-05-22 21:57:47 PDT
Created attachment 370485 [details]
Patch
Comment 6 Saam Barati 2019-05-22 22:30:59 PDT
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
Comment 7 Saam Barati 2019-05-22 22:37:00 PDT
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
Comment 8 Justin Michaud 2019-05-23 15:13:04 PDT
Created attachment 370525 [details]
Patch
Comment 9 Justin Michaud 2019-05-23 15:16:57 PDT
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.
Comment 10 Saam Barati 2019-05-23 16:47:31 PDT
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.
Comment 11 Saam Barati 2019-05-23 16:50:08 PDT
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.
Comment 12 Saam Barati 2019-05-23 16:53:51 PDT
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.
Comment 13 Saam Barati 2019-05-23 16:55:05 PDT
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*
Comment 14 Justin Michaud 2019-05-23 19:27:51 PDT
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.
Comment 15 Justin Michaud 2019-05-23 19:36:25 PDT
Created attachment 370552 [details]
Patch
Comment 16 Build Bot 2019-05-23 22:13:40 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2019-05-23 22:13:43 PDT Comment hidden (obsolete)
Comment 18 Justin Michaud 2019-05-24 10:25:17 PDT
Test failure seems unrelated, and seems to have been happening already.
Comment 19 Justin Michaud 2019-05-24 13:29:32 PDT
Created attachment 370584 [details]
Patch
Comment 20 Saam Barati 2019-05-24 15:10:45 PDT
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.
Comment 21 Justin Michaud 2019-05-24 17:11:55 PDT
Created attachment 370611 [details]
Patch
Comment 22 WebKit Commit Bot 2019-05-24 17:51:27 PDT
Comment on attachment 370611 [details]
Patch

Clearing flags on attachment: 370611

Committed r245765: <https://trac.webkit.org/changeset/245765>
Comment 23 WebKit Commit Bot 2019-05-24 17:51:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Shawn Roberts 2019-05-28 09:05:49 PDT
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