We should have a validator that checks for places that DFGClobberize is blatantly wrong. The easiest of these is to check for VM reentry without: Read(World); Write(Heap);
<rdar://problem/60786138>
Created attachment 394286 [details] Patch
Comment on attachment 394286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394286&action=review > Source/JavaScriptCore/ChangeLog:3 > + Add Clobberize validator I feel like this needs some kind of a qualifier on it. Maybe "basic Clobberize validator" > Source/JavaScriptCore/dfg/DFGClobberize.h:137 > + if constexpr (validateDFGClobberize) > + clobberTopFunctor(); let's make this a runtime option. We can default it to true when !!ASSERT_ENABLED, and can you also make some tests run with it on? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1846 > + m_jit.store8(TrustedImm32(0), reinterpret_cast<char*>(&vm()) + OBJECT_OFFSETOF(VM, didEnterVM)); &vm.didEnterVM > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5313 > + m_jit.load8(reinterpret_cast<char*>(&vm()) + OBJECT_OFFSETOF(VM, didEnterVM), scratch); &vm().didEnterVM > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:269 > + m_out.store32As8(m_out.int32Zero, m_out.absolute(reinterpret_cast<char*>(vm) + OBJECT_OFFSETOF(VM, didEnterVM))); &vm->didEnterVM > Source/JavaScriptCore/interpreter/Interpreter.cpp:869 > + auto clobberizeValidator = makeScopeExit([&] { > + if constexpr (validateDFGClobberize) > + vm.didEnterVM = true; > + }); do this for other entry points too? > Source/JavaScriptCore/runtime/VM.h:292 > +constexpr bool validateDFGClobberize = ASSERT_ENABLED; let's do a runtime option. I think it's fine to always set "didEnterVM" inside the runtime to true unconditionally, it's unlikely to cost any perf compared to vmEntry
Comment on attachment 394286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394286&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1631 > + m_out.store(m_out.int32Zero, m_out.absolute(reinterpret_cast<char*>(&vm()) + OBJECT_OFFSETOF(VM, didEnterVM))); I think you need to set this to zero on each basic block start point
Created attachment 394525 [details] Patch
very red
Created attachment 394536 [details] Patch
Ping; still in review!
Comment on attachment 394536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394536&action=review r=me with comments. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:269 > + m_out.store32As8(m_out.int32Zero, m_out.absolute(reinterpret_cast<char*>(vm) + OBJECT_OFFSETOF(VM, didEnterVM))); Remove this when `!Options::validateDFGClobberize()`. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:514 > + Remove this line. > Source/JavaScriptCore/jit/JITCodeInlines.h:39 > + vm->didEnterVM = true; Use makeScopeExit? > Source/JavaScriptCore/llint/LLIntThunks.h:45 > + vm->didEnterVM = true; Use makeScopeExit?
Comment on attachment 394536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394536&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1863 > + auto scratch = m_jit.scratchRegister(); > + m_jit.load8(&vm().didEnterVM, scratch); > + auto ok = m_jit.branchTest32(MacroAssembler::Zero, scratch); Why not using branchTest8() with AbsoluteAddress? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1906 > + auto scratch = m_jit.scratchRegister(); > + m_jit.load8(&vm().didEnterVM, scratch); > + auto ok = m_jit.branchTest32(MacroAssembler::Zero, scratch); Why not using branchTest8 with AbsoluteAddress?
Is there a reason we're not landing this and closing out the bug?
Comment on attachment 394536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394536&action=review >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:269 >> + m_out.store32As8(m_out.int32Zero, m_out.absolute(reinterpret_cast<char*>(vm) + OBJECT_OFFSETOF(VM, didEnterVM))); > > Remove this when `!Options::validateDFGClobberize()`. Done. >> Source/JavaScriptCore/jit/JITCodeInlines.h:39 >> + vm->didEnterVM = true; > > Use makeScopeExit? I didn't because there's only one code path through here but I can change it if you prefer. >> Source/JavaScriptCore/llint/LLIntThunks.h:45 >> + vm->didEnterVM = true; > > Use makeScopeExit? Ditto.
Created attachment 398525 [details] Patch for landing
Committed r261181: <https://trac.webkit.org/changeset/261181> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398525 [details].
(In reply to Yusuke Suzuki from comment #10) > Comment on attachment 394536 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394536&action=review > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1863 > > + auto scratch = m_jit.scratchRegister(); > > + m_jit.load8(&vm().didEnterVM, scratch); > > + auto ok = m_jit.branchTest32(MacroAssembler::Zero, scratch); > > Why not using branchTest8() with AbsoluteAddress? > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1906 > > + auto scratch = m_jit.scratchRegister(); > > + m_jit.load8(&vm().didEnterVM, scratch); > > + auto ok = m_jit.branchTest32(MacroAssembler::Zero, scratch); > > Why not using branchTest8 with AbsoluteAddress? Whoops missed this comment will add in follow up.