Attachment 368744 Details for Bug 197405 – Patch

[patch] Patch

bug-197405-20190501181740.patch (text/plain), 16.12 KB, created by Yusuke Suzuki on 2019-05-01 18:17:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-05-01 18:17:40 PDT

Size: 16.12 KB

patch

obsolete

>Subversion Revision: 244848
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index c54bf38ce688b41366d8f6680277263c3c267116..164c77add52c5990b3f6fe843742a8463aa21063 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,52 @@
>+2019-05-01  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [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  Darin Adler  <darin@apple.com>
> 
>         WebKit has too much of its own UTF-8 code and should rely more on ICU's UTF-8 support
>diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>index 14c562cbba752c426f58baaebd227c7ce532ae03..06fb181d3d71755422deb55d0e67384ec4307359 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -1608,22 +1608,69 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, VirtualRegister result, Ca
>         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 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, VirtualRegister result, Ca
>         // 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 @@ void ByteCodeParser::handleGetById(
>     //    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 @@ void ByteCodeParser::handlePutById(
>             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;
>diff --git a/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp b/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp
>index d3cbba21171411d9255fb82b0f2dc94105e9c868..2f4f5d1db60d699b9d57cad46f70b5e9d79781b6 100644
>--- a/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp
>@@ -157,14 +157,11 @@ class LiveCatchVariablePreservationPhase : public Phase {
>         };
> 
>         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 @@ class LiveCatchVariablePreservationPhase : public Phase {
>             };
> 
>             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,15 +192,8 @@ class LiveCatchVariablePreservationPhase : public Phase {
>             }
> 
>             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();
>-
>-                int stackOffset = inlineCallFrame ? inlineCallFrame->stackOffset : 0;
>-                if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()])
>-                    || operand.isArgument()
>-                    || (operand.offset() == stackOffset + CallFrame::thisArgumentOffset())) {
>+                if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()]) || operand.isArgument()) {
> 
>                     ASSERT(isValidFlushLocation(block, nodeIndex, operand));
> 
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 1e0fb752998b4fcd4eb7415b3ea7d95873ee78bf..dddf2a64666e038dc836fae312742a7350cc3bb4 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-05-01  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [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.
>+
>+        * stress/getter-setter-inlining-should-emit-movhint.js: Added.
>+        (foo):
>+        (test):
>+        (i.o.get f):
>+        (i.o.set f):
>+
> 2019-05-01  Ross Kirsling  <ross.kirsling@sony.com>
> 
>         Unreviewed correction to Test262 expectations following r244828.
>diff --git a/JSTests/stress/getter-setter-inlining-should-emit-movhint.js b/JSTests/stress/getter-setter-inlining-should-emit-movhint.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..28896f6b405e3170b0224424509c3041dde7657f
>--- /dev/null
>+++ b/JSTests/stress/getter-setter-inlining-should-emit-movhint.js
>@@ -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);
>+}

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197405: 368542 | 368594 | 368596 | 368622 | 368627 | 368643 | 368644 | 368649 | 368650 | 368744