WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
229543
[JSC] ASSERT failed in stress/for-in-tests.js (32bit)
https://bugs.webkit.org/show_bug.cgi?id=229543
Summary
[JSC] ASSERT failed in stress/for-in-tests.js (32bit)
Xan Lopez
Reported
2021-08-26 01:26:51 PDT
Very likely introduced by
r280760
, very likely making other tests fail. Stack trace: ASSERTION FAILED: currentLowest != NUM_REGS && currentSpillOrder != SpillHintInvalid ../../Source/JavaScriptCore/dfg/DFGRegisterBank.h(137) : JSC::DFG::RegisterBank<BankInfo>::RegID JSC::DFG::RegisterBank<BankInfo>::allocate(JSC::VirtualRegister&) [with BankInfo = JSC::GPRInfo; JSC::DFG::RegisterBank<BankInfo>::RegID = JSC::ARMRegisters::RegisterID] 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 0xf5eeeea0 in __libc_signal_restore_set (set=0xfffebcac) 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 0xf5edf7a2 in __GI_abort () at abort.c:79 #4 0xf629a090 in CRASH_WITH_INFO(...) () at WTF/Headers/wtf/Assertions.h:750 #5 0xf68d1e9e in JSC::DFG::RegisterBank<JSC::GPRInfo>::allocate (this=0xf23f6224, spillMe=...) at ../../Source/JavaScriptCore/dfg/DFGRegisterBank.h:137 #6 0xf68cc656 in JSC::DFG::SpeculativeJIT::allocate (this=0xf23f6000) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:237 #7 0xf68788c2 in JSC::DFG::GPRTemporary::GPRTemporary (this=0xfffebfec, jit=0xf23f6000) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1600 #8 0xf68789e0 in JSC::DFG::JSValueRegsTemporary::JSValueRegsTemporary (this=0xfffebfe4, jit=0xf23f6000) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1635 #9 0xf68a6e80 in operator() (__closure=0xfffec73c) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13597 #10 0xf68c5010 in WTF::ScopedLambdaFunctor<std::tuple<JSC::JSValueRegs, JSC::DataFormat>(JSC::DataFormat), JSC::DFG::SpeculativeJIT::compileEnumeratorGetByVal(JSC::DFG::Node*)::<lambda(JSC::GPRReg)>::<lambda(JSC::DataFormat)> >::implFunction(void *, JSC::DataFormat) (argument=0xfffec734, arguments#0=JSC::DataFormatJS) at WTF/Headers/wtf/ScopedLambda.h:106 #11 0xf68d4ca0 in WTF::ScopedLambda<std::tuple<JSC::JSValueRegs, JSC::DataFormat> (JSC::DataFormat)>::operator()<JSC::DataFormat>(JSC::DataFormat&&) const (this=0xfffec734) at WTF/Headers/wtf/ScopedLambda.h:58 #12 0xf6a417be in JSC::DFG::SpeculativeJIT::compileGetByVal(JSC::DFG::Node*, WTF::ScopedLambda<std::tuple<JSC::JSValueRegs, JSC::DataFormat> (JSC::DataFormat)> const&) ( this=0xf23f6000, node=0xf371aa20, prefix=...) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1881 #13 0xf68a7592 in operator() (__closure=0xfffec7ec, baseCellGPR=JSC::ARMRegisters::r2) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13584 #14 0xf68a776c in JSC::DFG::SpeculativeJIT::compileEnumeratorGetByVal (this=0xf23f6000, node=0xf371aa20) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13674 #15 0xf6a4b548 in JSC::DFG::SpeculativeJIT::compile (this=0xf23f6000, node=0xf371aa20) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4232 #16 0xf687b9e0 in JSC::DFG::SpeculativeJIT::compileCurrentBlock (this=0xf23f6000) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2291 #17 0xf687c042 in JSC::DFG::SpeculativeJIT::compile (this=0xf23f6000) at ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2401 #18 0xf673885e in JSC::DFG::JITCompiler::compileBody (this=0xfffed940) at ../../Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:135 #19 0xf673a250 in JSC::DFG::JITCompiler::compileFunction (this=0xfffed940) at ../../Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:437 #20 0xf67b59cc in JSC::DFG::Plan::compileInThreadImpl (this=0xf3799bc0) at ../../Source/JavaScriptCore/dfg/DFGPlan.cpp:343 #21 0xf6cb6f64 in JSC::JITPlan::compileInThread (this=0xf3799bc0, thread=0x0) at ../../Source/JavaScriptCore/jit/JITPlan.cpp:165 #22 0xf6cfe4e0 in JSC::JITWorklist::enqueue (this=0xf37c7360, plan=...) at ../../Source/JavaScriptCore/jit/JITWorklist.cpp:83 #23 0xf66de744 in JSC::DFG::compileImpl (vm=..., codeBlock=0xf1fad180, profiledDFGCodeBlock=0x0, mode=JSC::JITCompilationMode::DFG, osrEntryBytecodeIndex=..., mustHandleValues=..., callback=...) at ../../Source/JavaScriptCore/dfg/DFGDriver.cpp:90 #24 0xf66de7f0 in JSC::DFG::compile (vm=..., codeBlock=0xf1fad180, profiledDFGCodeBlock=0x0, mode=JSC::JITCompilationMode::DFG, osrEntryBytecodeIndex=..., mustHandleValues=..., callback=...) at ../../Source/JavaScriptCore/dfg/DFGDriver.cpp:106 #25 0xf6cafb26 in JSC::operationOptimize (vmPointer=0xf25f5000, bytecodeIndexBits=0) at ../../Source/JavaScriptCore/jit/JITOperations.cpp:2088 #26 0xf270d6bc in ?? ()
Attachments
v1
(6.56 KB, patch)
2021-09-03 10:40 PDT
,
Xan Lopez
no flags
Details
Formatted Diff
Diff
WIP
(23.03 KB, patch)
2021-09-10 06:45 PDT
,
Xan Lopez
no flags
Details
Formatted Diff
Diff
v3
(25.82 KB, patch)
2021-09-11 04:20 PDT
,
Xan Lopez
no flags
Details
Formatted Diff
Diff
v4
(25.61 KB, patch)
2021-09-11 04:21 PDT
,
Xan Lopez
no flags
Details
Formatted Diff
Diff
v5
(25.59 KB, patch)
2021-09-13 07:17 PDT
,
Xan Lopez
ysuzuki
: review-
Details
Formatted Diff
Diff
v6
(25.85 KB, patch)
2021-09-13 11:44 PDT
,
Xan Lopez
ysuzuki
: review+
ysuzuki
: commit-queue-
Details
Formatted Diff
Diff
v7
(25.78 KB, patch)
2021-09-13 14:35 PDT
,
Xan Lopez
ysuzuki
: review+
ysuzuki
: commit-queue-
Details
Formatted Diff
Diff
v8
(25.74 KB, patch)
2021-09-13 23:48 PDT
,
Xan Lopez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-09-02 01:28:12 PDT
<
rdar://problem/82664792
>
Xan Lopez
Comment 2
2021-09-03 10:40:38 PDT
Created
attachment 437283
[details]
v1
Yusuke Suzuki
Comment 3
2021-09-03 12:58:25 PDT
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.
Xan Lopez
Comment 4
2021-09-10 06:45:35 PDT
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).
Yusuke Suzuki
Comment 5
2021-09-10 10:00:29 PDT
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).
Xan Lopez
Comment 6
2021-09-11 04:20:07 PDT
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.
Xan Lopez
Comment 7
2021-09-11 04:21:52 PDT
Created
attachment 437948
[details]
v4 v4, minor nit
Yusuke Suzuki
Comment 8
2021-09-13 02:20:48 PDT
Comment on
attachment 437948
[details]
v4 Can you set r? if this is ready?
Xan Lopez
Comment 9
2021-09-13 03:20:42 PDT
(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.
Xan Lopez
Comment 10
2021-09-13 03:36:57 PDT
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.
Xan Lopez
Comment 11
2021-09-13 05:58:05 PDT
(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.
Xan Lopez
Comment 12
2021-09-13 07:17:06 PDT
Created
attachment 438037
[details]
v5 Speculate cell for base operand on 32bit.
Yusuke Suzuki
Comment 13
2021-09-13 10:24:47 PDT
Comment on
attachment 438037
[details]
v5 r=me
Yusuke Suzuki
Comment 14
2021-09-13 10:38:59 PDT
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?
Xan Lopez
Comment 15
2021-09-13 11:44:22 PDT
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.
Xan Lopez
Comment 16
2021-09-13 11:45:39 PDT
(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.
Yusuke Suzuki
Comment 17
2021-09-13 11:47:48 PDT
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.
Xan Lopez
Comment 18
2021-09-13 14:35:12 PDT
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
Yusuke Suzuki
Comment 19
2021-09-13 15:22:27 PDT
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.
Xan Lopez
Comment 20
2021-09-13 23:48:11 PDT
Created
attachment 438104
[details]
v8 Remove unnecessary RETURN_IF_EXCEPTION()
Yusuke Suzuki
Comment 21
2021-09-14 01:28:17 PDT
Comment on
attachment 438104
[details]
v8 r=me
EWS
Comment 22
2021-09-14 03:04:16 PDT
Found 1 new test failure: imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html
EWS
Comment 23
2021-09-14 03:13:30 PDT
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug