Changeset 243626 in webkit

Timestamp:

Mar 28, 2019 3:05:34 PM (5 years ago)

Author:

Tadeu Zagallo

Message:

CodeBlock::jettison() should disallow repatching its own calls
​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):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/call-link-info-osrexit-repatch.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/CallLinkInfo.cpp (modified) (5 diffs)
• Source/JavaScriptCore/bytecode/CallLinkInfo.h (modified) (2 diffs)
• Source/JavaScriptCore/bytecode/CodeBlock.cpp (modified) (1 diff)
• Source/JavaScriptCore/jit/Repatch.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/bytecode/CallLinkInfo.cpp

@@ -62 +62 @@
     , m_clearedByVirtual(false)
     , m_allowStubs(true)
-    , m_isLinked(false)
+    , m_clearedByJettison(false)
     , m_callType(None)
     , m_calleeGPR(255)
@@ -128 +128 @@
     MacroAssembler::repatchPointer(hotPathBegin(), callee);
     m_calleeOrCodeBlock.set(vm, owner, callee);
-    m_isLinked = true;
 }
 
@@ -136 +135 @@
     MacroAssembler::repatchPointer(hotPathBegin(), nullptr);
     m_calleeOrCodeBlock.clear();
-    m_isLinked = false;
 }
 
@@ -149 +147 @@
     RELEASE_ASSERT(isDirect());
     m_calleeOrCodeBlock.setMayBeNull(vm, owner, codeBlock);
-    m_isLinked = true;
 }
 
@@ -156 +153 @@
     RELEASE_ASSERT(isDirect());
     m_calleeOrCodeBlock.clear();
-    m_isLinked = false;
 }
 

trunk/Source/JavaScriptCore/bytecode/CallLinkInfo.h

@@ -271 +271 @@
     }
 
+    bool clearedByJettison()
+    {
+        return m_clearedByJettison;
+    }
+
     void setClearedByVirtual()
     {
         m_clearedByVirtual = true;
+    }
+
+    void setClearedByJettison()
+    {
+        m_clearedByJettison = true;
     }
     
@@ -351 +361 @@
     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;

trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -2050 +2050 @@
         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());

trunk/Source/JavaScriptCore/jit/Repatch.cpp

@@ -896 +896 @@
 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();