| Summary: | [JSC] ASSERT failed in stress/for-in-tests.js (32bit) | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Xan Lopez <xan.lopez> | ||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | cgarcia, ews-watchlist, keith_miller, mark.lam, mikhail, msaboff, saam, tzagallo, webkit-bug-importer, xan.lopez, ysuzuki | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Xan Lopez
2021-08-26 01:26:51 PDT
Created attachment 437283 [details]
v1
Comment on attachment 437283 [details] v1 View in context: https://bugs.webkit.org/attachment.cgi?id=437283&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13659 > + // Zero out modeGPR, it's not used anymore and we might want to reuse it. > + m_jit.xorPtr(modeGPR, modeGPR); I don't think this is necessary. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13699 > + // We can reuse modeGPR, since it's not used anymore. This reduces a bit the register pressure in some architectures. > + storageGPR = modeGPR; I think this is not correct. modeGPR comes from SpeculateStrictInt32Operand. This means that some other node can expect that this register still contains the value for mode. If we change it, then we will break that. Created attachment 437864 [details]
WIP
WIP. Implement a generic call op for enumeratorGetByVal that just calls the slow path, and use it on 32bit. There's a ugly hack (passing the metadata pointer to the slow path method) to be able to reuse easily the code, since this is just for testing. Seems to get rid of a lot of failures without introducing a ton of new regressions (like the previous attempt).
Comment on attachment 437864 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=437864&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4486 > + JSValueRegsFlushedCallResult result(this); > + JSValueRegs resultRegs = result.regs(); This should be done after calling flushRegisters() > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4499 > + SpeculateStrictInt32Operand index(this, m_graph.varArgChild(node, 3)); > + SpeculateStrictInt32Operand mode(this, m_graph.varArgChild(node, 4)); > + SpeculateCellOperand enumerator(this, m_graph.varArgChild(node, 5)); We should retrieve GPRReg for each ones before calling flushRegisters(). > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1014 > + JSValue propertyName = GET_C(bytecode.m_propertyName).jsValue(); It was GET() in the previous code (not GET_C). Created attachment 437947 [details]
v3
v3, addressing comments, added a pointer to a follow-up to reenable the optimizations, passed the arrayProfile and enumerator metadata directly to the slow path op so I don't need to include the bytecode header.
Created attachment 437948 [details]
v4
v4, minor nit
Comment on attachment 437948 [details]
v4
Can you set r? if this is ready?
(In reply to Yusuke Suzuki from comment #8) > Comment on attachment 437948 [details] > v4 > > Can you set r? if this is ready? Yeah, I think this is good to go but I want to double check it's not causing any other issues. Will set r? then! In the meantime bug #230150 is another related (easier) issue. Yeah, so I think there's definitely at least one mistake in the current patch. I get this in JSTests/stress/for-in-primitive-index-on-prototype.js:
DFG ASSERTION FAILED: Edge verification error: D@71->Check:Cell:D@67 was expected to have type Cell but has type Int32 (12884901888)
../../Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h(173) : void JSC::DFG::AbstractInterpreter<AbstractStateType>::verifyEdge(JSC::DFG::Node*, JSC::DFG::Edge) [with AbstractStateType = JSC::DFG::InPlaceAbstractState]
Thread 1 "jsc" received signal SIGABRT, Aborted.
__libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
47 ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S: No such file or directory.
(gdb) bt
#0 __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
#1 0xf5e6dea0 in __libc_signal_restore_set (set=0xfffeb51c) at ../sysdeps/unix/sysv/linux/internal-signals.h:86
#2 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:48
#3 0xf5e5e7a2 in __GI_abort () at abort.c:79
#4 0xf621a210 in CRASH_WITH_INFO(...) () at WTF/Headers/wtf/Assertions.h:750
#5 0xf65b4456 in JSC::DFG::AbstractInterpreter<JSC::DFG::InPlaceAbstractState>::verifyEdge (this=0xf22ffec0, node=0xf369ba80, edge=...)
at ../../Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:173
#6 0xf65ad73c in JSC::DFG::AbstractInterpreter<JSC::DFG::InPlaceAbstractState>::verifyEdges (this=0xf22ffec0, node=0xf369ba80)
at ../../Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:179
#7 0xf6591140 in JSC::DFG::AbstractInterpreter<JSC::DFG::InPlaceAbstractState>::executeEffects (this=0xf22ffec0, clobberLimit=8, node=0xf369ba80)
at ../../Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:348
#8 0xf6851e5e in JSC::DFG::AbstractInterpreter<JSC::DFG::InPlaceAbstractState>::executeEffects (this=0xf22ffec0, indexInBlock=8)
at ../../Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4708
#9 0xf67f9944 in JSC::DFG::SpeculativeJIT::compileCurrentBlock (this=0xf22ffb00) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2309
#10 0xf67f9e7a in JSC::DFG::SpeculativeJIT::compile (this=0xf22ffb00) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2401
#11 0xf66b6f6e in JSC::DFG::JITCompiler::compileBody (this=0xfffed9c0) at ../../Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:135
#12 0xf66b895c in JSC::DFG::JITCompiler::compileFunction (this=0xfffed9c0) at ../../Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:437
#13 0xf67343f8 in JSC::DFG::Plan::compileInThreadImpl (this=0xf369f000) at ../../Source/JavaScriptCore/dfg/DFGPlan.cpp:343
#14 0xf6c65bcc in JSC::JITPlan::compileInThread (this=0xf369f000, thread=0x0) at ../../Source/JavaScriptCore/jit/JITPlan.cpp:165
#15 0xf6ca8214 in JSC::JITWorklist::enqueue (this=0xf36c5288, plan=...) at ../../Source/JavaScriptCore/jit/JITWorklist.cpp:83
#16 0xf665cb8c in JSC::DFG::compileImpl (vm=..., codeBlock=0xf1eac1c0, profiledDFGCodeBlock=0x0, mode=JSC::JITCompilationMode::DFG, osrEntryBytecodeIndex=...,
mustHandleValues=..., callback=...) at ../../Source/JavaScriptCore/dfg/DFGDriver.cpp:90
#17 0xf665cc38 in JSC::DFG::compile (vm=..., codeBlock=0xf1eac1c0, profiledDFGCodeBlock=0x0, mode=JSC::JITCompilationMode::DFG, osrEntryBytecodeIndex=...,
mustHandleValues=..., callback=...) at ../../Source/JavaScriptCore/dfg/DFGDriver.cpp:106
#18 0xf6c5e78e in JSC::operationOptimize (vmPointer=0xf24f5000, bytecodeIndexBits=0) at ../../Source/JavaScriptCore/jit/JITOperations.cpp:2095
#19 0xf25ff804 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
quit)
Seems the speculation in the new method is wrong, or something is out of sync with what is happening now.
(In reply to Xan Lopez from comment #10) > > Seems the speculation in the new method is wrong, or something is out of > sync with what is happening now. For 32bit the Fixup phase has base as a cell, so we should either change that or make the DFG call operator for 32bit just do that. New patch will follow. Created attachment 438037 [details]
v5
Speculate cell for base operand on 32bit.
Comment on attachment 438037 [details]
v5
r=me
Comment on attachment 438037 [details] v5 View in context: https://bugs.webkit.org/attachment.cgi?id=438037&action=review Ah, I found a bug. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4486 > + SpeculateCellOperand baseOperand(this, m_graph.varArgChild(node, 0), ManualOperandSpeculation); If you want to use SpeculateCellOperand, then you need to use CellUse / KnownCellUse for base edge. But we do not have that edge, and no speculate(). You need to attach CellUse / KnownCellUse in fixup phase in 32bit. (And you also need to check the other EnumeratorGetByVal use to ensure that edge UseKind is handled. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4488 > + JSValueOperand property(this, m_graph.varArgChild(node, 1), ManualOperandSpeculation); This ManualOperandSpeculation is wrong: if you use ManualOperandSpeculation, then you need to call `speculate(...)`. > Source/JavaScriptCore/runtime/CommonSlowPaths.h:131 > + // If propertyName is not a cell then we are in index+named mode, so do what RecoverNameAndGetVal does. > + JSString* string = enumerator->propertyNameAtIndex(index); > + auto propertyName = string->toIdentifier(globalObject); > + RETURN_IF_EXCEPTION(scope, { }); > + RELEASE_AND_RETURN(scope, baseValue.get(globalObject, propertyName)); Why do we need this new code? Can you add a test which stress this code path? Created attachment 438060 [details]
v6
- the FixUp phase was already setting CellUse for the base, so no changes there.
- get rid of the ManualOperandSpeculation args.
- add a comment clarifying what's the reason for the extra branch in the CommonSlowPaths method, with a reference to the bug to get rid of it again.
(In reply to Xan Lopez from comment #15) > Created attachment 438060 [details] > v6 > > - add a comment clarifying what's the reason for the extra branch in the > CommonSlowPaths method, with a reference to the bug to get rid of it again. oh, and this is already tested by several tests, right. the same one in the bug title will fail without this. Comment on attachment 438060 [details] v6 View in context: https://bugs.webkit.org/attachment.cgi?id=438060&action=review r=me with nits. > Source/JavaScriptCore/dfg/DFGOperations.cpp:2550 > + JSValue base = JSValue(baseCell); > + RETURN_IF_EXCEPTION(scope, { }); > + This is not necessary. You can just pass JSCell* baseCell to CommonSlowPaths::opEnumeratorGetByVal > Source/JavaScriptCore/dfg/DFGOperations.cpp:2552 > + RETURN_IF_EXCEPTION(scope, { }); This is not necessary since JSValue::decode never throws an error. Created attachment 438080 [details]
v7
- Change the explicit JSCell*->JSValue conversion for an implicit one when calling the slow path
- Make the new code in the CommonSlowPaths method just available for 32bit
Comment on attachment 438080 [details] v7 View in context: https://bugs.webkit.org/attachment.cgi?id=438080&action=review > Source/JavaScriptCore/dfg/DFGOperations.cpp:2549 > + RETURN_IF_EXCEPTION(scope, { }); This is not necessary. Please remove this. Created attachment 438104 [details]
v8
Remove unnecessary RETURN_IF_EXCEPTION()
Comment on attachment 438104 [details]
v8
r=me
Found 1 new test failure: imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html Committed r282385 (241646@main): <https://commits.webkit.org/241646@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 438104 [details]. |