Summary: | [WASM-References] Support Anyref in globals | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Michaud <justin_michaud> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | Justin Michaud <justin_michaud> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, sroberts, webkit-bug-importer, ysuzuki | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Justin Michaud
2019-05-21 21:58:02 PDT
Created attachment 370381 [details]
Patch
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 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 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 Created attachment 370485 [details]
Patch
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 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 Created attachment 370525 [details]
Patch
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 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 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 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 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 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. Created attachment 370552 [details]
Patch
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 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
Test failure seems unrelated, and seems to have been happening already. Created attachment 370584 [details]
Patch
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. Created attachment 370611 [details]
Patch
Comment on attachment 370611 [details] Patch Clearing flags on attachment: 370611 Committed r245765: <https://trac.webkit.org/changeset/245765> All reviewed patches have been landed. Closing bug. 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 |