[JSC] ASSERT failed in stress/for-in-tests.js (32bit)

[Xan Lopez]

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 ?? ()

[Radar WebKit Bug Importer]

<rdar://problem/82664792>

[Xan Lopez]

Created attachment 437283 [details]
v1

[Yusuke Suzuki]

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]

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 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]

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]

Created attachment 437948 [details]
v4

v4, minor nit

[Yusuke Suzuki]

Comment on attachment 437948 [details]
v4

Can you set r? if this is ready?

[Xan Lopez]

(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]

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]

(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]

Created attachment 438037 [details]
v5

Speculate cell for base operand on 32bit.

[Yusuke Suzuki]

Comment on attachment 438037 [details]
v5

r=me

[Yusuke Suzuki]

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]

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]

(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 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]

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 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]

Created attachment 438104 [details]
v8

Remove unnecessary RETURN_IF_EXCEPTION()

[Yusuke Suzuki]

Comment on attachment 438104 [details]
v8

r=me

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html

[EWS]

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].