[JSC] DFG op_call_varargs should not assume that one-previous-local of freeReg is...
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Sep 2019 02:31:45 +0000 (02:31 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Sep 2019 02:31:45 +0000 (02:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202014

Reviewed by Saam Barati.

JSTests:

* stress/call-varargs-inlining-should-not-clobber-previous-to-free-register.js: Added.
(__v0):

Source/JavaScriptCore:

Let's look into the bytecode generated by the test.

    [   0] enter
    [   1] get_scope          loc4
    [   3] mov                loc5, loc4
    [   6] check_traps
    [   7] mov                loc6, callee
    [  10] create_direct_arguments loc7
    [  12] to_this            this
    [  15] mov                loc8, loc7
    [  18] mov                loc9, loc6
    [  21] mov                loc12, Undefined(const0)
    [  24] get_by_id          loc11, loc6, 0
    [  29] jneq_ptr           loc11, ApplyFunction, 18(->47)
    [  34] mov                loc11, loc6
    [  37] call_varargs       loc11, loc11, this, loc8, loc13, 0
    [  45] jmp                17(->62)
    [  47] mov                loc16, loc6
    [  50] mov                loc15, this
    [  53] mov                loc14, loc8
    [  56] call               loc11, loc11, 3, 22
    ...

call_varargs uses loc13 as firstFreeReg (first usable bottom register in the current stack-frame to spread variadic arguments after this).
This is correct. And call_varargs uses |this| as this argument for the call_varargs. This |this| argument is not in a region starting from loc13.
And it is not in the previous place to loc13 (|this| is not loc12).

On the other hand, DFG::ByteCodeParser's inlining path is always assuming that the previous to firstFreeReg is usable and part of arguments.
But this is wrong. loc12 in the above bytecode is used for `[  56] call               loc11, loc11, 3, 22`'s argument later, and this call assumes
that loc12 is not clobbered by call_varargs. But DFG and FTL clobbers it.

The test is recursively calling the same function, and we inline the same function one-level. And stack-overflow error happens when inlined
CallForwardVarargs (from op_call_varargs) is called. FTL recovers the frames, and at this point, outer function's loc12 is recovered to garbage since
LoadVarargs clobbers it. And we eventually use it and crash.

    60:<!0:-> LoadVarargs(Check:Untyped:Kill:@30, MustGen, start = loc13, count = loc15, machineStart = loc7, machineCount = loc9, offset = 0, mandatoryMinimum = 0, limit = 2, R:World, W:Stack(-16),Stack(-14),Stack(-13),Heap, Exits, ClobbersExit, bc#37, ExitValid)

This LoadVarargs clobbers loc12, loc13, and loc15 while loc12 is used.

In all the tiers, op_call_varargs first allocates enough region to hold varargs including |this|. And we store |this| value to a correct place.
DFG should not assume that the previous register to firstFreeReg is used for |this|.

This patch fixes DFG::ByteCodeParser's stack region calculation for op_call_varargs inlining. And we rename maxNumArguments to maxArgumentCountIncludingThis to
represent that `maxArgumentCountIncludingThis` includes |this| count.

* bytecode/CallLinkInfo.cpp:
(JSC::CallLinkInfo::setMaxArgumentCountIncludingThis):
(JSC::CallLinkInfo::setMaxNumArguments): Deleted.
* bytecode/CallLinkInfo.h:
(JSC::CallLinkInfo::addressOfMaxArgumentCountIncludingThis):
(JSC::CallLinkInfo::maxArgumentCountIncludingThis):
(JSC::CallLinkInfo::addressOfMaxNumArguments): Deleted.
(JSC::CallLinkInfo::maxNumArguments): Deleted.
* bytecode/CallLinkStatus.cpp:
(JSC::CallLinkStatus::computeFor):
(JSC::CallLinkStatus::dump const):
* bytecode/CallLinkStatus.h:
(JSC::CallLinkStatus::maxArgumentCountIncludingThis const):
(JSC::CallLinkStatus::maxNumArguments const): Deleted.
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleVarargsInlining):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::emitCall):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::emitCall):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileDirectCallOrConstruct):
* jit/JITCall.cpp:
(JSC::JIT::compileSetupFrame):
* jit/JITCall32_64.cpp:
(JSC::JIT::compileSetupFrame):
* jit/JITOperations.cpp:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250116 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
JSTests/ChangeLog
JSTests/stress/call-varargs-inlining-should-not-clobber-previous-to-free-register.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CallLinkInfo.cpp
Source/JavaScriptCore/bytecode/CallLinkInfo.h
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp
Source/JavaScriptCore/bytecode/CallLinkStatus.h
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/jit/JITCall.cpp
Source/JavaScriptCore/jit/JITCall32_64.cpp
Source/JavaScriptCore/jit/JITOperations.cpp

index c99e44c..5258053 100644 (file)
@@ -1,3 +1,13 @@
+2019-09-19  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] DFG op_call_varargs should not assume that one-previous-local of freeReg is usable
+        https://bugs.webkit.org/show_bug.cgi?id=202014
+
+        Reviewed by Saam Barati.
+
+        * stress/call-varargs-inlining-should-not-clobber-previous-to-free-register.js: Added.
+        (__v0):
+
 2019-09-19  Tadeu Zagallo  <tzagallo@apple.com>
 
         Syntax checker should report duplicate __proto__ properties
diff --git a/JSTests/stress/call-varargs-inlining-should-not-clobber-previous-to-free-register.js b/JSTests/stress/call-varargs-inlining-should-not-clobber-previous-to-free-register.js
new file mode 100644 (file)
index 0000000..02a52a3
--- /dev/null
@@ -0,0 +1,9 @@
+//@ runDefault("--useConcurrentJIT=0", "--watchdog=4000", "--watchdog-exception-ok")
+(function __v0() {
+    try {
+        __v0(__v0.apply(this, arguments));
+    } catch (e) {
+        for (let i = 0; i < 10000; i++) {
+        }
+    }
+})(2);
index 34ab656..4bd72e1 100644 (file)
@@ -1,3 +1,83 @@
+2019-09-19  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] DFG op_call_varargs should not assume that one-previous-local of freeReg is usable
+        https://bugs.webkit.org/show_bug.cgi?id=202014
+
+        Reviewed by Saam Barati.
+
+        Let's look into the bytecode generated by the test.
+
+            [   0] enter
+            [   1] get_scope          loc4
+            [   3] mov                loc5, loc4
+            [   6] check_traps
+            [   7] mov                loc6, callee
+            [  10] create_direct_arguments loc7
+            [  12] to_this            this
+            [  15] mov                loc8, loc7
+            [  18] mov                loc9, loc6
+            [  21] mov                loc12, Undefined(const0)
+            [  24] get_by_id          loc11, loc6, 0
+            [  29] jneq_ptr           loc11, ApplyFunction, 18(->47)
+            [  34] mov                loc11, loc6
+            [  37] call_varargs       loc11, loc11, this, loc8, loc13, 0
+            [  45] jmp                17(->62)
+            [  47] mov                loc16, loc6
+            [  50] mov                loc15, this
+            [  53] mov                loc14, loc8
+            [  56] call               loc11, loc11, 3, 22
+            ...
+
+        call_varargs uses loc13 as firstFreeReg (first usable bottom register in the current stack-frame to spread variadic arguments after this).
+        This is correct. And call_varargs uses |this| as this argument for the call_varargs. This |this| argument is not in a region starting from loc13.
+        And it is not in the previous place to loc13 (|this| is not loc12).
+
+        On the other hand, DFG::ByteCodeParser's inlining path is always assuming that the previous to firstFreeReg is usable and part of arguments.
+        But this is wrong. loc12 in the above bytecode is used for `[  56] call               loc11, loc11, 3, 22`'s argument later, and this call assumes
+        that loc12 is not clobbered by call_varargs. But DFG and FTL clobbers it.
+
+        The test is recursively calling the same function, and we inline the same function one-level. And stack-overflow error happens when inlined
+        CallForwardVarargs (from op_call_varargs) is called. FTL recovers the frames, and at this point, outer function's loc12 is recovered to garbage since
+        LoadVarargs clobbers it. And we eventually use it and crash.
+
+            60:<!0:-> LoadVarargs(Check:Untyped:Kill:@30, MustGen, start = loc13, count = loc15, machineStart = loc7, machineCount = loc9, offset = 0, mandatoryMinimum = 0, limit = 2, R:World, W:Stack(-16),Stack(-14),Stack(-13),Heap, Exits, ClobbersExit, bc#37, ExitValid)
+
+        This LoadVarargs clobbers loc12, loc13, and loc15 while loc12 is used.
+
+        In all the tiers, op_call_varargs first allocates enough region to hold varargs including |this|. And we store |this| value to a correct place.
+        DFG should not assume that the previous register to firstFreeReg is used for |this|.
+
+        This patch fixes DFG::ByteCodeParser's stack region calculation for op_call_varargs inlining. And we rename maxNumArguments to maxArgumentCountIncludingThis to
+        represent that `maxArgumentCountIncludingThis` includes |this| count.
+
+        * bytecode/CallLinkInfo.cpp:
+        (JSC::CallLinkInfo::setMaxArgumentCountIncludingThis):
+        (JSC::CallLinkInfo::setMaxNumArguments): Deleted.
+        * bytecode/CallLinkInfo.h:
+        (JSC::CallLinkInfo::addressOfMaxArgumentCountIncludingThis):
+        (JSC::CallLinkInfo::maxArgumentCountIncludingThis):
+        (JSC::CallLinkInfo::addressOfMaxNumArguments): Deleted.
+        (JSC::CallLinkInfo::maxNumArguments): Deleted.
+        * bytecode/CallLinkStatus.cpp:
+        (JSC::CallLinkStatus::computeFor):
+        (JSC::CallLinkStatus::dump const):
+        * bytecode/CallLinkStatus.h:
+        (JSC::CallLinkStatus::maxArgumentCountIncludingThis const):
+        (JSC::CallLinkStatus::maxNumArguments const): Deleted.
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::handleVarargsInlining):
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::emitCall):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::emitCall):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileDirectCallOrConstruct):
+        * jit/JITCall.cpp:
+        (JSC::JIT::compileSetupFrame):
+        * jit/JITCall32_64.cpp:
+        (JSC::JIT::compileSetupFrame):
+        * jit/JITOperations.cpp:
+
 2019-09-19  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Canvas: show WebGPU shader pipelines
index 393f182..ce9be71 100644 (file)
@@ -190,11 +190,11 @@ ExecutableBase* CallLinkInfo::executable()
     return jsCast<ExecutableBase*>(m_lastSeenCalleeOrExecutable.get());
 }
 
-void CallLinkInfo::setMaxNumArguments(unsigned value)
+void CallLinkInfo::setMaxArgumentCountIncludingThis(unsigned value)
 {
     RELEASE_ASSERT(isDirect());
     RELEASE_ASSERT(value);
-    m_maxNumArguments = value;
+    m_maxArgumentCountIncludingThis = value;
 }
 
 void CallLinkInfo::visitWeak(VM& vm)
index 89875da..9c93d0c 100644 (file)
@@ -295,17 +295,17 @@ public:
         return static_cast<CallType>(m_callType);
     }
 
-    uint32_t* addressOfMaxNumArguments()
+    uint32_t* addressOfMaxArgumentCountIncludingThis()
     {
-        return &m_maxNumArguments;
+        return &m_maxArgumentCountIncludingThis;
     }
 
-    uint32_t maxNumArguments()
+    uint32_t maxArgumentCountIncludingThis()
     {
-        return m_maxNumArguments;
+        return m_maxArgumentCountIncludingThis;
     }
     
-    void setMaxNumArguments(unsigned);
+    void setMaxArgumentCountIncludingThis(unsigned);
 
     static ptrdiff_t offsetOfSlowPathCount()
     {
@@ -342,7 +342,7 @@ public:
     }
 
 private:
-    uint32_t m_maxNumArguments { 0 }; // For varargs: the profiled maximum number of arguments. For direct: the number of stack slots allocated for arguments.
+    uint32_t m_maxArgumentCountIncludingThis { 0 }; // For varargs: the profiled maximum number of arguments. For direct: the number of stack slots allocated for arguments.
     CodeLocationLabel<JSInternalPtrTag> m_callReturnLocationOrPatchableJump;
     CodeLocationLabel<JSInternalPtrTag> m_hotPathBeginOrSlowPathStart;
     CodeLocationNearCall<JSInternalPtrTag> m_hotPathOther;
index fe29a9b..9a000fc 100644 (file)
@@ -157,7 +157,7 @@ CallLinkStatus CallLinkStatus::computeFor(
     UNUSED_PARAM(profiledBlock);
     
     CallLinkStatus result = computeFromCallLinkInfo(locker, callLinkInfo);
-    result.m_maxNumArguments = callLinkInfo.maxNumArguments();
+    result.m_maxArgumentCountIncludingThis = callLinkInfo.maxArgumentCountIncludingThis();
     return result;
 }
 
@@ -474,8 +474,8 @@ void CallLinkStatus::dump(PrintStream& out) const
     if (!m_variants.isEmpty())
         out.print(comma, listDump(m_variants));
     
-    if (m_maxNumArguments)
-        out.print(comma, "maxNumArguments = ", m_maxNumArguments);
+    if (m_maxArgumentCountIncludingThis)
+        out.print(comma, "maxArgumentCountIncludingThis = ", m_maxArgumentCountIncludingThis);
 }
 
 } // namespace JSC
index f0ed407..0a101ca 100644 (file)
@@ -104,7 +104,7 @@ public:
 
     bool isClosureCall() const; // Returns true if any callee is a closure call.
     
-    unsigned maxNumArguments() const { return m_maxNumArguments; }
+    unsigned maxArgumentCountIncludingThis() const { return m_maxArgumentCountIncludingThis; }
     
     bool finalize(VM&);
     
@@ -129,7 +129,7 @@ private:
     bool m_couldTakeSlowPath { false };
     bool m_isProved { false };
     bool m_isBasedOnStub { false };
-    unsigned m_maxNumArguments { 0 };
+    unsigned m_maxArgumentCountIncludingThis { 0 };
 };
 
 } // namespace JSC
index f6a7d53..07f69e6 100644 (file)
@@ -1832,7 +1832,7 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
     NodeType callOp, InlineCallFrame::Kind kind)
 {
     VERBOSE_LOG("Handling inlining (Varargs)...\nStack: ", currentCodeOrigin(), "\n");
-    if (callLinkStatus.maxNumArguments() > Options::maximumVarargsForInlining()) {
+    if (callLinkStatus.maxArgumentCountIncludingThis() > Options::maximumVarargsForInlining()) {
         VERBOSE_LOG("Bailing inlining: too many arguments for varargs inlining.\n");
         return false;
     }
@@ -1850,16 +1850,16 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
         mandatoryMinimum = 0;
     
     // includes "this"
-    unsigned maxNumArguments = std::max(callLinkStatus.maxNumArguments(), mandatoryMinimum + 1);
+    unsigned maxArgumentCountIncludingThis = std::max(callLinkStatus.maxArgumentCountIncludingThis(), mandatoryMinimum + 1);
 
     CodeSpecializationKind specializationKind = InlineCallFrame::specializationKindFor(kind);
-    if (inliningCost(callVariant, maxNumArguments, kind) > getInliningBalance(callLinkStatus, specializationKind)) {
+    if (inliningCost(callVariant, maxArgumentCountIncludingThis, kind) > getInliningBalance(callLinkStatus, specializationKind)) {
         VERBOSE_LOG("Bailing inlining: inlining cost too high.\n");
         return false;
     }
     
-    int registerOffset = firstFreeReg + 1;
-    registerOffset -= maxNumArguments; // includes "this"
+    int registerOffset = firstFreeReg;
+    registerOffset -= maxArgumentCountIncludingThis;
     registerOffset -= CallFrame::headerSizeInRegisters;
     registerOffset = -WTF::roundUpToMultipleOf(stackAlignmentRegisters(), -registerOffset);
 
@@ -1878,7 +1878,7 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
         data->start = VirtualRegister(remappedArgumentStart + 1);
         data->count = VirtualRegister(remappedRegisterOffset + CallFrameSlot::argumentCount);
         data->offset = argumentsOffset;
-        data->limit = maxNumArguments;
+        data->limit = maxArgumentCountIncludingThis;
         data->mandatoryMinimum = mandatoryMinimum;
         
         if (callOp == TailCallForwardVarargs)
@@ -1907,7 +1907,7 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
         
         set(VirtualRegister(argumentStart), get(thisArgument), ImmediateNakedSet);
         unsigned numSetArguments = 0;
-        for (unsigned argument = 1; argument < maxNumArguments; ++argument) {
+        for (unsigned argument = 1; argument < maxArgumentCountIncludingThis; ++argument) {
             VariableAccessData* variable = newVariableAccessData(VirtualRegister(remappedArgumentStart + argument));
             variable->mergeShouldNeverUnbox(true); // We currently have nowhere to put the type check on the LoadVarargs. LoadVarargs is effectful, so after it finishes, we cannot exit.
             
@@ -1939,7 +1939,7 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
     // those arguments. Even worse, if the intrinsic decides to exit, it won't really have anywhere to
     // exit to: LoadVarargs is effectful and it's part of the op_call_varargs, so we can't exit without
     // calling LoadVarargs twice.
-    inlineCall(callTargetNode, result, callVariant, registerOffset, maxNumArguments, kind, nullptr, insertChecks);
+    inlineCall(callTargetNode, result, callVariant, registerOffset, maxArgumentCountIncludingThis, kind, nullptr, insertChecks);
 
 
     VERBOSE_LOG("Successful inlining (varargs, monomorphic).\nStack: ", currentCodeOrigin(), "\n");
index a6d46a5..8430570 100644 (file)
@@ -826,7 +826,7 @@ void SpeculativeJIT::emitCall(Node* node)
 
     if (isDirect) {
         info->setExecutableDuringCompilation(executable);
-        info->setMaxNumArguments(numAllocatedArgs);
+        info->setMaxArgumentCountIncludingThis(numAllocatedArgs);
 
         if (isTail) {
             RELEASE_ASSERT(node->op() == DirectTailCall);
index 68980c0..bd9c3bb 100644 (file)
@@ -776,7 +776,7 @@ void SpeculativeJIT::emitCall(Node* node)
     
     if (isDirect) {
         callLinkInfo->setExecutableDuringCompilation(executable);
-        callLinkInfo->setMaxNumArguments(numAllocatedArgs);
+        callLinkInfo->setMaxArgumentCountIncludingThis(numAllocatedArgs);
 
         if (isTail) {
             RELEASE_ASSERT(node->op() == DirectTailCall);
index b259893..f1f7dc3 100644 (file)
@@ -8311,7 +8311,7 @@ private:
                         CallLinkInfo::DirectTailCall, node->origin.semantic, InvalidGPRReg);
                     callLinkInfo->setExecutableDuringCompilation(executable);
                     if (numAllocatedArgs > numPassedArgs)
-                        callLinkInfo->setMaxNumArguments(numAllocatedArgs);
+                        callLinkInfo->setMaxArgumentCountIncludingThis(numAllocatedArgs);
                     
                     jit.addLinkTask(
                         [=] (LinkBuffer& linkBuffer) {
@@ -8345,7 +8345,7 @@ private:
                     node->origin.semantic, InvalidGPRReg);
                 callLinkInfo->setExecutableDuringCompilation(executable);
                 if (numAllocatedArgs > numPassedArgs)
-                    callLinkInfo->setMaxNumArguments(numAllocatedArgs);
+                    callLinkInfo->setMaxArgumentCountIncludingThis(numAllocatedArgs);
                 
                 params.addLatePath(
                     [=] (CCallHelpers& jit) {
index 557fd62..c10c40d 100644 (file)
@@ -111,9 +111,9 @@ JIT::compileSetupFrame(const Op& bytecode, CallLinkInfo* info)
 
     // Profile the argument count.
     load32(Address(regT1, CallFrameSlot::argumentCount * static_cast<int>(sizeof(Register)) + PayloadOffset), regT2);
-    load32(info->addressOfMaxNumArguments(), regT0);
+    load32(info->addressOfMaxArgumentCountIncludingThis(), regT0);
     Jump notBiggest = branch32(Above, regT0, regT2);
-    store32(regT2, info->addressOfMaxNumArguments());
+    store32(regT2, info->addressOfMaxArgumentCountIncludingThis());
     notBiggest.link(this);
     
     // Initialize 'this'.
index ac77a64..b67c5bf 100644 (file)
@@ -202,9 +202,9 @@ JIT::compileSetupFrame(const Op& bytecode, CallLinkInfo* info)
 
     // Profile the argument count.
     load32(Address(regT1, CallFrameSlot::argumentCount * static_cast<int>(sizeof(Register)) + PayloadOffset), regT2);
-    load32(info->addressOfMaxNumArguments(), regT0);
+    load32(info->addressOfMaxArgumentCountIncludingThis(), regT0);
     Jump notBiggest = branch32(Above, regT0, regT2);
-    store32(regT2, info->addressOfMaxNumArguments());
+    store32(regT2, info->addressOfMaxArgumentCountIncludingThis());
     notBiggest.link(this);
     
     // Initialize 'this'.
index efa254c..3c3e362 100644 (file)
@@ -1147,7 +1147,7 @@ void JIT_OPERATION operationLinkDirectCall(ExecState* exec, CallLinkInfo* callLi
         EXCEPTION_ASSERT_UNUSED(throwScope, throwScope.exception() == error);
         if (UNLIKELY(error))
             return;
-        unsigned argumentStackSlots = callLinkInfo->maxNumArguments();
+        unsigned argumentStackSlots = callLinkInfo->maxArgumentCountIncludingThis();
         if (argumentStackSlots < static_cast<size_t>(codeBlock->numParameters()))
             codePtr = functionExecutable->entrypointFor(kind, MustCheckArity);
         else