Cherry-pick 272448.633@safari-7618-branch (f25738c). https://bugs.web…

…kit.org/show_bug.cgi?id=268221 [JSC] AccessCase should not hold CallLinkInfo* https://bugs.webkit.org/show_bug.cgi?id=268221 rdar://121733122 Reviewed by Justin Michaud. AccessCase holds CallLinkInfo*. But when the underlying JITStubRoutine gets destroyed, this becomes invalid. Previously, it does not matter since we always destroy CodeBlock first (synchronously), and then we clean up JITStubRoutine. So there were strict ordering. But now CodeBlock destruction can get delayed. But fundamentally speaking, having CallLinkInfo* in AccessCase is not right. This is compiled code's data structure and AccessCase should be just a data for IC feedback. In this patch we decouple CallLinkInfo* from AccessCase. CallLinkInfo's lifetime should be correctly managed by visitWeak, so, we add visitWeak iteration in MarkingGCAwareJITStubRoutine. Then we can remove CallLinkInfo from AccessCase. * JSTests/stress/decouple-calllinkinfo-from-access-case.js: Added. (F7): (f25): (f33): (C20.prototype.valueOf): (C20): (f27): * Source/JavaScriptCore/bytecode/AccessCase.cpp: (JSC::AccessCase::forEachDependentCell const): (JSC::AccessCase::doesCalls const): (JSC::AccessCase::visitWeak const): (JSC::AccessCase::collectDependentCells const): * Source/JavaScriptCore/bytecode/AccessCase.h: * Source/JavaScriptCore/bytecode/GetByStatus.cpp: (JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback): * Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp: (JSC::GetterSetterAccessCase::dumpImpl const): * Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h: * Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp: (JSC::InlineCacheCompiler::generateWithGuard): (JSC::InlineCacheCompiler::generate): (JSC::InlineCacheCompiler::generateImpl): (JSC::InlineCacheCompiler::emitProxyObjectAccess): (JSC::InlineCacheCompiler::regenerate): (JSC::InlineCacheHandler::callLinkInfoAt): (JSC::InlineCacheHandler::visitWeak const): * Source/JavaScriptCore/bytecode/InlineCacheCompiler.h: * Source/JavaScriptCore/bytecode/ProxyObjectAccessCase.cpp: (JSC::ProxyObjectAccessCase::dumpImpl const): * Source/JavaScriptCore/bytecode/ProxyObjectAccessCase.h: * Source/JavaScriptCore/bytecode/PutByStatus.cpp: (JSC::PutByStatus::computeForStubInfo): * Source/JavaScriptCore/bytecode/StructureStubInfo.cpp: (JSC::StructureStubInfo::callLinkInfoAt): * Source/JavaScriptCore/bytecode/StructureStubInfo.h: * Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp: (JSC::MarkingGCAwareJITStubRoutine::MarkingGCAwareJITStubRoutine): (JSC::MarkingGCAwareJITStubRoutine::visitWeakImpl): (JSC::MarkingGCAwareJITStubRoutine::callLinkInfoAtImpl): (JSC::GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler): (JSC::createICJITStubRoutine): * Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h: * Source/JavaScriptCore/jit/JITStubRoutine.cpp: (JSC::JITStubRoutine::callLinkInfoAt): * Source/JavaScriptCore/jit/JITStubRoutine.h: (JSC::JITStubRoutine::callLinkInfoAtImpl): Canonical link: https://commits.webkit.org/272448.633@safari-7618-branch Canonical link: https://commits.webkit.org/274313.218@webkitglib/2.44
WebKit · May 13, 2024 · cac405a · cac405a
1 parent 5574e48
commit cac405a
Show file tree

Hide file tree

Showing 17 changed files with 213 additions and 86 deletions.
diff --git a/JSTests/stress/decouple-calllinkinfo-from-access-case.js b/JSTests/stress/decouple-calllinkinfo-from-access-case.js
@@ -0,0 +1,96 @@
+// runDefault("--validateOptions=true", "--thresholdForJITSoon=10", "--thresholdForJITAfterWarmUp=10", "--thresholdForOptimizeAfterWarmUp=100", "--thresholdForOptimizeAfterLongWarmUp=100", "--thresholdForOptimizeSoon=100", "--thresholdForFTLOptimizeAfterWarmUp=1000", "--thresholdForFTLOptimizeSoon=1000", "--validateBCE=true")
+
+const ProxyConstructor = Proxy;
+const getPrototypeOf = Object.getPrototypeOf;
+const ReflectGet = Reflect.get;
+const ReflectSet = Reflect.set;
+const ReflectHas = Reflect.has;
+const setPrototypeOf = Object.setPrototypeOf;
+
+function probe(id, value) {
+    let originalPrototype, newPrototype;
+    let handler = {
+        get(target, key, receiver) {
+            if (key === '__proto__' && receiver === value) return originalPrototype;
+            if (receiver === newPrototype) return ReflectGet(target, key);
+            return ReflectGet(target, key, receiver);
+        },
+        set(target, key, value, receiver) {
+            if (receiver === newPrototype) return ReflectSet(target, key, value);
+            return ReflectSet(target, key, value, receiver);
+        },
+        has(target, key) {
+            return ReflectHas(target, key);
+        },
+    };
+
+    try {
+        originalPrototype = getPrototypeOf(value);
+        newPrototype = new ProxyConstructor(originalPrototype, handler);
+        setPrototypeOf(value, newPrototype);
+    } catch (e) {}
+}
+
+probe("v1", "2003629588");
+let v4 = 9150;
+v4--;
+probe("v6", 51828);
+function F7(a9, a10, a11) {
+    if (!new.target) { throw 'must be called with new'; }
+    const v12 = this?.constructor;
+    try { new v12(this, "object", 447824390); } catch (e) {}
+    a11 % a11;
+    this.b = a9;
+    this.g = a10;
+}
+const v15 = new F7("2003629588", "object", 447824390);
+const v16 = new F7(v15, v4, v4);
+const v17 = new F7("2003629588", 51828, 51828);
+probe("v17", v17);
+const v18 = v17?.constructor;
+probe("v18", v18);
+let v19;
+try { v19 = new v18("r", v17, "r"); } catch (e) {}
+probe("v19", v19);
+const v20 = [v17,v17];
+probe("v20", v20);
+const v21 = [F7,v15,v20,v15,v4];
+const v22 = [v4,"object",51828];
+probe("v22", v22);
+let v23;
+try { v23 = v22.reduce(v15); } catch (e) {}
+const v24 = [2,-354747782,-16,10251,-1485280459,5,6,536870888,-47153,-193790246];
+probe("v24", v24);
+function f25(a26, a27) {
+    const o28 = {
+        [a27]: a26,
+        "d": v21,
+    };
+    return o28;
+}
+f25(v16, v22);
+f25(v23, v16);
+f25(v15, v22);
+v24[4];
+function f33(a34, a35, a36, a37) {
+    probe("v36", a36);
+    ~a35;
+    v22.length = 1;
+    a36?.[v21];
+}
+v24.flatMap(f33);
+gc();
+class C20 {
+    valueOf(a22, a23) {
+        return ("n")[1204] - this;
+    }
+}
+const v26 = new C20();
+function f27(a28, a29) {
+    new BigInt64Array(3603);
+    return v26 * v26;
+}
+try {
+v26[Symbol.toPrimitive] = f27;
+v26.valueOf();
+} catch { }
diff --git a/Source/JavaScriptCore/bytecode/AccessCase.cpp b/Source/JavaScriptCore/bytecode/AccessCase.cpp
@@ -729,9 +729,6 @@ void AccessCase::forEachDependentCell(VM&, const Functor& functor) const
     switch (type()) {
     case Getter:
     case Setter: {
-        auto& accessor = this->as<GetterSetterAccessCase>();
-        if (accessor.callLinkInfo())
-            accessor.callLinkInfo()->forEachDependentCell(functor);
         break;
     }
     case CustomValueGetter:
@@ -759,9 +756,6 @@ void AccessCase::forEachDependentCell(VM&, const Functor& functor) const
     case ProxyObjectLoad:
     case ProxyObjectStore:
     case IndexedProxyObjectLoad: {
-        auto& accessor = this->as<ProxyObjectAccessCase>();
-        if (accessor.callLinkInfo())
-            accessor.callLinkInfo()->forEachDependentCell(functor);
         break;
     }
     case InstanceOfHit:
@@ -870,7 +864,7 @@ void AccessCase::forEachDependentCell(VM&, const Functor& functor) const
     }
 }
 
-bool AccessCase::doesCalls(VM& vm, Vector<JSCell*>* cellsToMarkIfDoesCalls) const
+bool AccessCase::doesCalls(VM&) const
 {
     bool doesCalls = false;
     switch (type()) {
@@ -995,12 +989,6 @@ bool AccessCase::doesCalls(VM& vm, Vector<JSCell*>* cellsToMarkIfDoesCalls) cons
         doesCalls = viaGlobalProxy();
         break;
     }
-
-    if (doesCalls && cellsToMarkIfDoesCalls) {
-        forEachDependentCell(vm, [&](JSCell* cell) {
-            cellsToMarkIfDoesCalls->append(cell);
-        });
-    }
     return doesCalls;
 }
 
@@ -1237,19 +1225,20 @@ void AccessCase::dump(PrintStream& out) const
 
 bool AccessCase::visitWeak(VM& vm) const
 {
-    if (isAccessor()) {
-        auto& accessor = this->as<GetterSetterAccessCase>();
-        if (accessor.callLinkInfo())
-            accessor.callLinkInfo()->visitWeak(vm);
-    }
-
     bool isValid = true;
     forEachDependentCell(vm, [&](JSCell* cell) {
         isValid &= vm.heap.isMarked(cell);
     });
     return isValid;
 }
 
+void AccessCase::collectDependentCells(VM& vm, Vector<JSCell*>& cells) const
+{
+    forEachDependentCell(vm, [&](JSCell* cell) {
+        cells.append(cell);
+    });
+}
+
 template<typename Visitor>
 void AccessCase::propagateTransitions(Visitor& visitor) const
 {

diff --git a/Source/JavaScriptCore/bytecode/AccessCase.h b/Source/JavaScriptCore/bytecode/AccessCase.h
@@ -269,9 +269,7 @@ class AccessCase : public ThreadSafeRefCounted<AccessCase> {
     WatchpointSet* additionalSet() const;
     bool viaGlobalProxy() const { return m_viaGlobalProxy; }
 
-    // If you supply the optional vector, this will append the set of cells that this will need to keep alive
-    // past the call.
-    bool doesCalls(VM&, Vector<JSCell*>* cellsToMark = nullptr) const;
+    bool doesCalls(VM&) const;
 
     bool isCustom() const
     {
@@ -338,6 +336,8 @@ class AccessCase : public ThreadSafeRefCounted<AccessCase> {
 
     static bool canBeShared(const AccessCase&, const AccessCase&);
 
+    void collectDependentCells(VM&, Vector<JSCell*>&) const;
+
     template<typename Func>
     void runWithDowncast(const Func&);
 

diff --git a/Source/JavaScriptCore/bytecode/GetByStatus.cpp b/Source/JavaScriptCore/bytecode/GetByStatus.cpp
@@ -263,7 +263,7 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(const Concurr
                 auto& accessCase = access.as<ProxyObjectAccessCase>();
                 auto status = GetByStatus(accessCase);
                 auto callLinkStatus = makeUnique<CallLinkStatus>();
-                if (CallLinkInfo* callLinkInfo = accessCase.callLinkInfo())
+                if (CallLinkInfo* callLinkInfo = stubInfo->callLinkInfoAt(locker, 0))
                     *callLinkStatus = CallLinkStatus::computeFor(locker, profiledBlock, *callLinkInfo, callExitSiteData);
                 status.appendVariant(GetByVariant(accessCase.identifier(), { }, invalidOffset, { }, WTFMove(callLinkStatus)));
                 return status;
@@ -375,10 +375,8 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(const Concurr
                     }
                     case AccessCase::Getter: {
                         callLinkStatus = makeUnique<CallLinkStatus>();
-                        if (CallLinkInfo* callLinkInfo = access.as<GetterSetterAccessCase>().callLinkInfo()) {
-                            *callLinkStatus = CallLinkStatus::computeFor(
-                                locker, profiledBlock, *callLinkInfo, callExitSiteData);
-                        }
+                        if (CallLinkInfo* callLinkInfo = stubInfo->callLinkInfoAt(locker, listIndex))
+                            *callLinkStatus = CallLinkStatus::computeFor(locker, profiledBlock, *callLinkInfo, callExitSiteData);
                         break;
                     }
                     default: {

diff --git a/Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp b/Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp
@@ -95,8 +95,6 @@ void GetterSetterAccessCase::dumpImpl(PrintStream& out, CommaPrinter& comma, Ind
 {
     Base::dumpImpl(out, comma, indent);
     out.print(comma, "customSlotBase = ", RawPointer(customSlotBase()));
-    if (callLinkInfo())
-        out.print(comma, "callLinkInfo = ", RawPointer(callLinkInfo()));
     out.print(comma, "customAccessor = ", RawPointer(m_customAccessor.taggedPtr()));
 }
 

diff --git a/Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h b/Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h
@@ -38,14 +38,6 @@ class GetterSetterAccessCase final : public ProxyableAccessCase {
     friend class AccessCase;
     friend class InlineCacheCompiler;
 
-    // This can return null if it hasn't been generated yet. That's
-    // actually somewhat likely because of how we do buffering of new cases.
-    // CallLinkInfo's ownership is held both by generated code via GCAwareJITStubRoutine and PolymorphicAccess.
-    // The ownership relation is PolymorphicAccess -> GCAwareJITStubRoutine -> CallLinkInfo.
-    // PolymorphicAccess can be destroyed while GCAwareJITStubRoutine is alive if we are destroying PolymorphicAccess
-    // while we are executing GCAwareJITStubRoutine. It is not possible that GetterSetterAccessCase is alive while
-    // GCAwareJITStubRoutine is destroyed.
-    OptimizingCallLinkInfo* callLinkInfo() const { return m_callLinkInfo; }
     JSObject* customSlotBase() const { return m_customSlotBase.get(); }
     std::optional<DOMAttributeAnnotation> domAttribute() const { return m_domAttribute; }
 
@@ -70,9 +62,7 @@ class GetterSetterAccessCase final : public ProxyableAccessCase {
     void dumpImpl(PrintStream&, CommaPrinter&, Indenter&) const;
     Ref<AccessCase> cloneImpl() const;
 
-
     WriteBarrier<JSObject> m_customSlotBase;
-    OptimizingCallLinkInfo* m_callLinkInfo { nullptr };
     CodePtr<CustomAccessorPtrTag> m_customAccessor;
     std::optional<DOMAttributeAnnotation> m_domAttribute;
 };