CodeBlock::jettison() should disallow repatching its own calls
authortzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Mar 2019 22:05:34 +0000 (22:05 +0000)
committertzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Mar 2019 22:05:34 +0000 (22:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196359
<rdar://problem/48973663>

Reviewed by Saam Barati.

JSTests:

* stress/call-link-info-osrexit-repatch.js: Added.
(foo):

Source/JavaScriptCore:

CodeBlock::jettison() calls CommonData::invalidate, which replaces the `hlt`
instruction with the jump to OSR exit. However, if the `hlt` was immediately
followed by a call to the CodeBlock being jettisoned, we would write over the
OSR exit address while unlinking all the incoming CallLinkInfos later in
CodeBlock::jettison().

Change it so that we set a flag, `clearedByJettison`, in all the CallLinkInfos
owned by the CodeBlock being jettisoned. If the flag is set, we will avoid
repatching the call during unlinking. This is safe because this call will never
be reachable again after the CodeBlock is jettisoned.

* bytecode/CallLinkInfo.cpp:
(JSC::CallLinkInfo::CallLinkInfo):
(JSC::CallLinkInfo::setCallee):
(JSC::CallLinkInfo::clearCallee):
(JSC::CallLinkInfo::setCodeBlock):
(JSC::CallLinkInfo::clearCodeBlock):
* bytecode/CallLinkInfo.h:
(JSC::CallLinkInfo::clearedByJettison):
(JSC::CallLinkInfo::setClearedByJettison):
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::jettison):
* jit/Repatch.cpp:
(JSC::revertCall):

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

JSTests/ChangeLog
JSTests/stress/call-link-info-osrexit-repatch.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CallLinkInfo.cpp
Source/JavaScriptCore/bytecode/CallLinkInfo.h
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/jit/Repatch.cpp

index 37cc90d..552fbb9 100644 (file)
@@ -1,3 +1,14 @@
+2019-03-28  Tadeu Zagallo  <tzagallo@apple.com>
+
+        CodeBlock::jettison() should disallow repatching its own calls
+        https://bugs.webkit.org/show_bug.cgi?id=196359
+        <rdar://problem/48973663>
+
+        Reviewed by Saam Barati.
+
+        * stress/call-link-info-osrexit-repatch.js: Added.
+        (foo):
+
 2019-03-28  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] imports-oom.js intermittently fails
diff --git a/JSTests/stress/call-link-info-osrexit-repatch.js b/JSTests/stress/call-link-info-osrexit-repatch.js
new file mode 100644 (file)
index 0000000..c247b18
--- /dev/null
@@ -0,0 +1,18 @@
+//@ runFTLEager("--watchdog=1000", "--watchdog-exception-ok")
+
+function foo(a, b) {
+    'use strict';
+    if (a === 0) {
+        return;
+    }
+    if (a === 0) {
+        return foo(a + 0);
+    }
+    if (a === 0) {
+        return foo(+a, 0);
+    }
+    return foo(b / 1, a, 0);
+    0 === 0
+}
+
+foo(1, 5);
index 7ff5daa..96de91f 100644 (file)
@@ -1,3 +1,36 @@
+2019-03-28  Tadeu Zagallo  <tzagallo@apple.com>
+
+        CodeBlock::jettison() should disallow repatching its own calls
+        https://bugs.webkit.org/show_bug.cgi?id=196359
+        <rdar://problem/48973663>
+
+        Reviewed by Saam Barati.
+
+        CodeBlock::jettison() calls CommonData::invalidate, which replaces the `hlt`
+        instruction with the jump to OSR exit. However, if the `hlt` was immediately
+        followed by a call to the CodeBlock being jettisoned, we would write over the
+        OSR exit address while unlinking all the incoming CallLinkInfos later in
+        CodeBlock::jettison().
+
+        Change it so that we set a flag, `clearedByJettison`, in all the CallLinkInfos
+        owned by the CodeBlock being jettisoned. If the flag is set, we will avoid
+        repatching the call during unlinking. This is safe because this call will never
+        be reachable again after the CodeBlock is jettisoned.
+
+        * bytecode/CallLinkInfo.cpp:
+        (JSC::CallLinkInfo::CallLinkInfo):
+        (JSC::CallLinkInfo::setCallee):
+        (JSC::CallLinkInfo::clearCallee):
+        (JSC::CallLinkInfo::setCodeBlock):
+        (JSC::CallLinkInfo::clearCodeBlock):
+        * bytecode/CallLinkInfo.h:
+        (JSC::CallLinkInfo::clearedByJettison):
+        (JSC::CallLinkInfo::setClearedByJettison):
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::jettison):
+        * jit/Repatch.cpp:
+        (JSC::revertCall):
+
 2019-03-27  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] Drop VM and Context cache map in JavaScriptCore.framework
index deae383..c4605c1 100644 (file)
@@ -61,7 +61,7 @@ CallLinkInfo::CallLinkInfo()
     , m_clearedByGC(false)
     , m_clearedByVirtual(false)
     , m_allowStubs(true)
-    , m_isLinked(false)
+    , m_clearedByJettison(false)
     , m_callType(None)
     , m_calleeGPR(255)
     , m_maxNumArguments(0)
@@ -127,7 +127,6 @@ void CallLinkInfo::setCallee(VM& vm, JSCell* owner, JSObject* callee)
     RELEASE_ASSERT(!isDirect());
     MacroAssembler::repatchPointer(hotPathBegin(), callee);
     m_calleeOrCodeBlock.set(vm, owner, callee);
-    m_isLinked = true;
 }
 
 void CallLinkInfo::clearCallee()
@@ -135,7 +134,6 @@ void CallLinkInfo::clearCallee()
     RELEASE_ASSERT(!isDirect());
     MacroAssembler::repatchPointer(hotPathBegin(), nullptr);
     m_calleeOrCodeBlock.clear();
-    m_isLinked = false;
 }
 
 JSObject* CallLinkInfo::callee()
@@ -148,14 +146,12 @@ void CallLinkInfo::setCodeBlock(VM& vm, JSCell* owner, FunctionCodeBlock* codeBl
 {
     RELEASE_ASSERT(isDirect());
     m_calleeOrCodeBlock.setMayBeNull(vm, owner, codeBlock);
-    m_isLinked = true;
 }
 
 void CallLinkInfo::clearCodeBlock()
 {
     RELEASE_ASSERT(isDirect());
     m_calleeOrCodeBlock.clear();
-    m_isLinked = false;
 }
 
 FunctionCodeBlock* CallLinkInfo::codeBlock()
index 8018955..a0b207f 100644 (file)
@@ -270,10 +270,20 @@ public:
         return m_clearedByVirtual;
     }
 
+    bool clearedByJettison()
+    {
+        return m_clearedByJettison;
+    }
+
     void setClearedByVirtual()
     {
         m_clearedByVirtual = true;
     }
+
+    void setClearedByJettison()
+    {
+        m_clearedByJettison = true;
+    }
     
     void setCallType(CallType callType)
     {
@@ -350,7 +360,7 @@ private:
     bool m_clearedByGC : 1;
     bool m_clearedByVirtual : 1;
     bool m_allowStubs : 1;
-    bool m_isLinked : 1;
+    bool m_clearedByJettison : 1;
     unsigned m_callType : 4; // CallType
     unsigned m_calleeGPR : 8;
     uint32_t m_maxNumArguments; // For varargs: the profiled maximum number of arguments. For direct: the number of stack slots allocated for arguments.
index b5949b1..0a9ecf1 100644 (file)
@@ -2049,6 +2049,16 @@ void CodeBlock::jettison(Profiler::JettisonReason reason, ReoptimizationMode mod
     if (vm.heap.isCurrentThreadBusy() && !vm.heap.isMarked(ownerExecutable()))
         return;
 
+#if ENABLE(JIT)
+    {
+        ConcurrentJSLocker locker(m_lock);
+        if (JITData* jitData = m_jitData.get()) {
+            for (CallLinkInfo* callLinkInfo : jitData->m_callLinkInfos)
+                callLinkInfo->setClearedByJettison();
+        }
+    }
+#endif
+
     // This accomplishes (2).
     ownerExecutable()->installCode(vm, alternative(), codeType(), specializationKind());
 
index 3028e36..48af689 100644 (file)
@@ -895,18 +895,20 @@ void linkSlowFor(
 
 static void revertCall(VM* vm, CallLinkInfo& callLinkInfo, MacroAssemblerCodeRef<JITStubRoutinePtrTag> codeRef)
 {
-    if (callLinkInfo.isDirect()) {
-        callLinkInfo.clearCodeBlock();
-        if (callLinkInfo.callType() == CallLinkInfo::DirectTailCall)
-            MacroAssembler::repatchJump(callLinkInfo.patchableJump(), callLinkInfo.slowPathStart());
-        else
-            MacroAssembler::repatchNearCall(callLinkInfo.hotPathOther(), callLinkInfo.slowPathStart());
-    } else {
-        MacroAssembler::revertJumpReplacementToBranchPtrWithPatch(
-            MacroAssembler::startOfBranchPtrWithPatchOnRegister(callLinkInfo.hotPathBegin()),
-            static_cast<MacroAssembler::RegisterID>(callLinkInfo.calleeGPR()), 0);
-        linkSlowFor(vm, callLinkInfo, codeRef);
-        callLinkInfo.clearCallee();
+    if (!callLinkInfo.clearedByJettison()) {
+        if (callLinkInfo.isDirect()) {
+            callLinkInfo.clearCodeBlock();
+            if (callLinkInfo.callType() == CallLinkInfo::DirectTailCall)
+                MacroAssembler::repatchJump(callLinkInfo.patchableJump(), callLinkInfo.slowPathStart());
+            else
+                MacroAssembler::repatchNearCall(callLinkInfo.hotPathOther(), callLinkInfo.slowPathStart());
+        } else {
+            MacroAssembler::revertJumpReplacementToBranchPtrWithPatch(
+                MacroAssembler::startOfBranchPtrWithPatchOnRegister(callLinkInfo.hotPathBegin()),
+                static_cast<MacroAssembler::RegisterID>(callLinkInfo.calleeGPR()), 0);
+            linkSlowFor(vm, callLinkInfo, codeRef);
+            callLinkInfo.clearCallee();
+        }
     }
     callLinkInfo.clearSeen();
     callLinkInfo.clearStub();