Added: trunk/JSTests/stress/getter-setter-inlining-should-emit-movhint.js (0 => 244864)
--- trunk/JSTests/stress/getter-setter-inlining-should-emit-movhint.js (rev 0)
+++ trunk/JSTests/stress/getter-setter-inlining-should-emit-movhint.js 2019-05-02 02:40:44 UTC (rev 244864)
@@ -0,0 +1,30 @@
+//@ runDefault("--useRandomizingFuzzerAgent=1", "--usePolymorphicCallInliningForNonStubStatus=1", "--seedOfRandomizingFuzzerAgent=2896922505", "--useLLInt=0", "--useConcurrentJIT=0")
+function foo(o) {
+ o.f = 0;
+ return o.f;
+}
+noInline(foo);
+
+let counter = 0;
+
+function test(o, value) {
+ var result = foo(o);
+ if (result < value)
+ throw new Error(result);
+ if (counter < value)
+ throw new Error(counter);
+ Array.of(arguments);
+}
+
+for (var i = 0; i < 100000; ++i) {
+ var o = {
+ get f() {
+ return o
+ },
+ set f(v) {
+ counter++;
+ this.z = 0;
+ }
+ };
+ test(o, i, i);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (244863 => 244864)
--- trunk/Source/_javascript_Core/ChangeLog 2019-05-02 02:20:51 UTC (rev 244863)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-05-02 02:40:44 UTC (rev 244864)
@@ -1,3 +1,52 @@
+2019-05-01 Yusuke Suzuki <[email protected]>
+
+ [JSC] Inlining Getter/Setter should care availability of ad-hocly constructed frame
+ https://bugs.webkit.org/show_bug.cgi?id=197405
+
+ Reviewed by Saam Barati.
+
+ When inlining getter and setter calls, we setup a stack frame which does not appear in the bytecode.
+ Because Inlining can switch on executable, we could have a graph like this.
+
+ BB#0
+ ...
+ 30: GetSetter
+ 31: MovHint(loc10)
+ 32: SetLocal(loc10)
+ 33: MovHint(loc9)
+ 34: SetLocal(loc9)
+ ...
+ 37: GetExecutable(@30)
+ ...
+ 41: Switch(@37)
+
+ BB#2
+ 42: GetLocal(loc12, bc#7 of caller)
+ ...
+ --> callee: loc9 and loc10 are arguments of callee.
+ ...
+ <HERE, exit to callee, loc9 and loc10 are required in the bytecode>
+
+ When we prune OSR availability at the beginning of BB#2 (bc#7 in the caller), we prune loc9 and loc10's liveness because the caller does not actually have loc9 and loc10.
+ However, when we begin executing the callee, we need OSR exit to be aware of where it can recover the arguments to the setter, loc9 and loc10.
+
+ This patch inserts MovHint at the beginning of callee for a getter / setter stack frame to make arguments (loc9 and loc10 in the above example) recoverable from OSR exit.
+ We also move arity fixup DFG nodes from the caller to the callee, since moved arguments are not live in the caller too.
+
+ Interestingly, this fix also reveals the existing issue in LiveCatchVariablePreservationPhase. We emitted Flush for |this| of InlineCallFrame blindly if we saw InlineCallFrame
+ inside a block which is covered by catch handler. But this is wrong because inlined function can finish its execution within the block, and |this| is completely unrelated to
+ the catch handler if the catch handler is in the outer callee. We already collect all the live locals at the catch handler. And this locals must include arguments too if the
+ catch handler is in inlined function. So, we should not emit Flush for each |this| of seen InlineCallFrame. This emitted Flush may connect unrelated locals in the catch handler
+ to the locals that is only defined and used in the inlined function, and it leads to the results like DFG says the local is live while the bytecode says the local is dead.
+ This results in reading and using garbage in OSR entry because DFG OSR entry needs to fill live DFG values from the stack.
+
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::inlineCall):
+ (JSC::DFG::ByteCodeParser::handleGetById):
+ (JSC::DFG::ByteCodeParser::handlePutById):
+ * dfg/DFGLiveCatchVariablePreservationPhase.cpp:
+ (JSC::DFG::LiveCatchVariablePreservationPhase::handleBlockForTryCatch):
+
2019-05-01 Michael Saboff <[email protected]>
ASSERTION FAILED: !m_needExceptionCheck with --validateExceptionChecks=1; ProxyObject.getOwnPropertySlotCommon/JSFunction.callerGetter
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (244863 => 244864)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2019-05-02 02:20:51 UTC (rev 244863)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2019-05-02 02:40:44 UTC (rev 244864)
@@ -1608,22 +1608,69 @@
calleeVariable->mergeShouldNeverUnbox(true);
}
+ InlineStackEntry* callerStackTop = m_inlineStackTop;
+ InlineStackEntry inlineStackEntry(this, codeBlock, codeBlock, callee.function(), result,
+ (VirtualRegister)inlineCallFrameStart, argumentCountIncludingThis, kind, continuationBlock);
+
+ // This is where the actual inlining really happens.
+ unsigned oldIndex = m_currentIndex;
+ m_currentIndex = 0;
+
+ switch (kind) {
+ case InlineCallFrame::GetterCall:
+ case InlineCallFrame::SetterCall: {
+ // When inlining getter and setter calls, we setup a stack frame which does not appear in the bytecode.
+ // Because Inlining can switch on executable, we could have a graph like this.
+ //
+ // BB#0
+ // ...
+ // 30: GetSetter
+ // 31: MovHint(loc10)
+ // 32: SetLocal(loc10)
+ // 33: MovHint(loc9)
+ // 34: SetLocal(loc9)
+ // ...
+ // 37: GetExecutable(@30)
+ // ...
+ // 41: Switch(@37)
+ //
+ // BB#2
+ // 42: GetLocal(loc12, bc#7 of caller)
+ // ...
+ // --> callee: loc9 and loc10 are arguments of callee.
+ // ...
+ // <HERE, exit to callee, loc9 and loc10 are required in the bytecode>
+ //
+ // When we prune OSR availability at the beginning of BB#2 (bc#7 in the caller), we prune loc9 and loc10's liveness because the caller does not actually have loc9 and loc10.
+ // However, when we begin executing the callee, we need OSR exit to be aware of where it can recover the arguments to the setter, loc9 and loc10. The MovHints in the inlined
+ // callee make it so that if we exit at <HERE>, we can recover loc9 and loc10.
+ for (int index = 0; index < argumentCountIncludingThis; ++index) {
+ VirtualRegister argumentToGet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffset));
+ addToGraph(MovHint, OpInfo(argumentToGet.offset()), getDirect(argumentToGet));
+ }
+ break;
+ }
+ default:
+ break;
+ }
+
if (arityFixupCount) {
// Note: we do arity fixup in two phases:
// 1. We get all the values we need and MovHint them to the expected locals.
- // 2. We SetLocal them inside the callee's CodeOrigin. This way, if we exit, the callee's
+ // 2. We SetLocal them after that. This way, if we exit, the callee's
// frame is already set up. If any SetLocal exits, we have a valid exit state.
// This is required because if we didn't do this in two phases, we may exit in
- // the middle of arity fixup from the caller's CodeOrigin. This is unsound because if
- // we did the SetLocals in the caller's frame, the memcpy may clobber needed parts
- // of the frame right before exiting. For example, consider if we need to pad two args:
+ // the middle of arity fixup from the callee's CodeOrigin. This is unsound because exited
+ // code does not have arity fixup so that remaining necessary fixups are not executed.
+ // For example, consider if we need to pad two args:
// [arg3][arg2][arg1][arg0]
// [fix ][fix ][arg3][arg2][arg1][arg0]
// We memcpy starting from arg0 in the direction of arg3. If we were to exit at a type check
- // for arg3's SetLocal in the caller's CodeOrigin, we'd exit with a frame like so:
+ // for arg3's SetLocal in the callee's CodeOrigin, we'd exit with a frame like so:
// [arg3][arg2][arg1][arg2][arg1][arg0]
- // And the caller would then just end up thinking its argument are:
- // [arg3][arg2][arg1][arg2]
+ // Since we do not perform arity fixup in the callee, this is the frame used by the callee.
+ // And the callee would then just end up thinking its argument are:
+ // [fix ][fix ][arg3][arg2][arg1][arg0]
// which is incorrect.
Node* undefined = addToGraph(JSConstant, OpInfo(m_constantUndefined));
@@ -1642,29 +1689,23 @@
// In such cases, we do not need to move frames.
if (registerOffsetAfterFixup != registerOffset) {
for (int index = 0; index < argumentCountIncludingThis; ++index) {
- Node* value = get(virtualRegisterForArgument(index, registerOffset));
- VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(index, registerOffsetAfterFixup));
+ VirtualRegister argumentToGet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffset));
+ Node* value = getDirect(argumentToGet);
+ VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(index));
addToGraph(MovHint, OpInfo(argumentToSet.offset()), value);
m_setLocalQueue.append(DelayedSetLocal { currentCodeOrigin(), argumentToSet, value, ImmediateNakedSet });
}
}
for (int index = 0; index < arityFixupCount; ++index) {
- VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(argumentCountIncludingThis + index, registerOffsetAfterFixup));
+ VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(argumentCountIncludingThis + index));
addToGraph(MovHint, OpInfo(argumentToSet.offset()), undefined);
m_setLocalQueue.append(DelayedSetLocal { currentCodeOrigin(), argumentToSet, undefined, ImmediateNakedSet });
}
// At this point, it's OK to OSR exit because we finished setting up
- // our callee's frame. We emit an ExitOK below from the callee's CodeOrigin.
+ // our callee's frame. We emit an ExitOK below.
}
- InlineStackEntry inlineStackEntry(this, codeBlock, codeBlock, callee.function(), result,
- (VirtualRegister)inlineCallFrameStart, argumentCountIncludingThis, kind, continuationBlock);
-
- // This is where the actual inlining really happens.
- unsigned oldIndex = m_currentIndex;
- m_currentIndex = 0;
-
// At this point, it's again OK to OSR exit.
m_exitOK = true;
addToGraph(ExitOK);
@@ -4371,8 +4412,7 @@
// the dreaded arguments object on the getter, the right things happen. Well, sort of -
// since we only really care about 'this' in this case. But we're not going to take that
// shortcut.
- int nextRegister = registerOffset + CallFrame::headerSizeInRegisters;
- set(VirtualRegister(nextRegister++), base, ImmediateNakedSet);
+ set(virtualRegisterForArgument(0, registerOffset), base, ImmediateNakedSet);
// We've set some locals, but they are not user-visible. It's still OK to exit from here.
m_exitOK = true;
@@ -4555,9 +4595,8 @@
m_inlineStackTop->remapOperand(
VirtualRegister(registerOffset)).toLocal());
- int nextRegister = registerOffset + CallFrame::headerSizeInRegisters;
- set(VirtualRegister(nextRegister++), base, ImmediateNakedSet);
- set(VirtualRegister(nextRegister++), value, ImmediateNakedSet);
+ set(virtualRegisterForArgument(0, registerOffset), base, ImmediateNakedSet);
+ set(virtualRegisterForArgument(1, registerOffset), value, ImmediateNakedSet);
// We've set some locals, but they are not user-visible. It's still OK to exit from here.
m_exitOK = true;
Modified: trunk/Source/_javascript_Core/dfg/DFGLiveCatchVariablePreservationPhase.cpp (244863 => 244864)
--- trunk/Source/_javascript_Core/dfg/DFGLiveCatchVariablePreservationPhase.cpp 2019-05-02 02:20:51 UTC (rev 244863)
+++ trunk/Source/_javascript_Core/dfg/DFGLiveCatchVariablePreservationPhase.cpp 2019-05-02 02:40:44 UTC (rev 244864)
@@ -157,14 +157,11 @@
};
Operands<VariableAccessData*> currentBlockAccessData(block->variablesAtTail.numberOfArguments(), block->variablesAtTail.numberOfLocals(), nullptr);
- HashSet<InlineCallFrame*> seenInlineCallFrames;
auto flushEverything = [&] (NodeOrigin origin, unsigned index) {
RELEASE_ASSERT(currentExceptionHandler);
- auto flush = [&] (VirtualRegister operand, bool alwaysInsert) {
- if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()])
- || operand.isArgument()
- || alwaysInsert) {
+ auto flush = [&] (VirtualRegister operand) {
+ if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()]) || operand.isArgument()) {
ASSERT(isValidFlushLocation(block, index, operand));
@@ -180,12 +177,8 @@
};
for (unsigned local = 0; local < block->variablesAtTail.numberOfLocals(); local++)
- flush(virtualRegisterForLocal(local), false);
- for (InlineCallFrame* inlineCallFrame : seenInlineCallFrames)
- flush(VirtualRegister(inlineCallFrame->stackOffset + CallFrame::thisArgumentOffset()), true);
- flush(VirtualRegister(CallFrame::thisArgumentOffset()), true);
-
- seenInlineCallFrames.clear();
+ flush(virtualRegisterForLocal(local));
+ flush(VirtualRegister(CallFrame::thisArgumentOffset()));
};
for (unsigned nodeIndex = 0; nodeIndex < block->size(); nodeIndex++) {
@@ -199,16 +192,9 @@
}
if (currentExceptionHandler && (node->op() == SetLocal || node->op() == SetArgumentDefinitely || node->op() == SetArgumentMaybe)) {
- InlineCallFrame* inlineCallFrame = node->origin.semantic.inlineCallFrame();
- if (inlineCallFrame)
- seenInlineCallFrames.add(inlineCallFrame);
VirtualRegister operand = node->local();
+ if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()]) || operand.isArgument()) {
- int stackOffset = inlineCallFrame ? inlineCallFrame->stackOffset : 0;
- if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()])
- || operand.isArgument()
- || (operand.offset() == stackOffset + CallFrame::thisArgumentOffset())) {
-
ASSERT(isValidFlushLocation(block, nodeIndex, operand));
VariableAccessData* variableAccessData = currentBlockAccessData.operand(operand);
