JIT: read single-character string constants without a GCOwnedDataScope on the compiler thread

Source/JavaScriptCore/heap/GCOwnedDataScope.cpp

@@ -32,10 +32,16 @@ namespace JSC {
 
 #if ASSERT_ENABLED
 
+// Heap::m_topGCOwnedDataScope tracks the scopes on the mutator's stack, and it is not synchronized.
+// Heap::clearConcurrentRetainedDataIfPossible asserts it is null before it drops the strings it kept
+// alive for those scopes. Compiler and GC threads have their own guards there (no ongoing compilation,
+// mutator not fenced), so a scope constructed on one of them must not touch the mutator's tracker.
 void setTopGCOwnedDataScopeIfNeeded(const JSCell* cell, const void* scope)
 {
     if (!cell)
         return;
+    if (isCompilationThread() || Thread::mayBeGCThread())
+        return;
     if (!cell->vm().heap.m_topGCOwnedDataScope)
         cell->vm().heap.m_topGCOwnedDataScope = scope;
 }
@@ -44,6 +50,8 @@ void clearTopGCOwnedDataScopeIfNeeded(const JSCell* cell, const void* scope)
 {
     if (!cell)
         return;
+    if (isCompilationThread() || Thread::mayBeGCThread())
+        return;
     if (cell->vm().heap.m_topGCOwnedDataScope == scope)
         cell->vm().heap.m_topGCOwnedDataScope = nullptr;
 }

Source/JavaScriptCore/jit/JITArithmetic.cpp

@@ -234,17 +234,23 @@ ALWAYS_INLINE void JIT::emit_compareImpl(VirtualRegister op1, VirtualRegister op
     // - constant int immediate to int immediate
     // - int immediate to int immediate
 
-    constexpr bool disallowAllocation = false;
     auto handleConstantCharOperand = [&](VirtualRegister left, VirtualRegister right, RelationalCondition cond) {
         if (!isOperandConstantChar(left))
             return false;
+        // This may run on a compiler thread, so read the StringImpl directly rather than through
+        // tryGetValue(), whose GCOwnedDataScope is only meant to be constructed on the mutator.
+        // Read the character at once: the mutator may atomize the constant at any time, and a GC
+        // during this compile then drops the old StringImpl. A single character constant is never a rope.
+        const StringImpl* impl = asString(getConstantOperand(left))->tryGetValueImpl();
+        RELEASE_ASSERT(impl);
+        char16_t character = impl->at(0);
 
         emitGetVirtualRegister(right, jsRegT10);
         addSlowCase(branchIfNotCell(jsRegT10));
         JumpList failures;
         emitLoadCharacterString(jsRegT10.payloadGPR(), jsRegT10.payloadGPR(), failures);
         addSlowCase(failures);
-        emitCompare(commute(cond), jsRegT10, Imm32(asString(getConstantOperand(left))->tryGetValue(disallowAllocation).data[0]));
+        emitCompare(commute(cond), jsRegT10, Imm32(character));
         return true;
     };
 

Source/JavaScriptCore/lol/LOLJIT.cpp

@@ -961,17 +961,23 @@ ALWAYS_INLINE void LOLJIT::emitCompareImpl(VirtualRegister op1, JSValueRegs op1R
     // - constant int immediate to int immediate
     // - int immediate to int immediate
 
-    constexpr bool disallowAllocation = false;
     auto handleConstantCharOperand = [&](VirtualRegister left, JSValueRegs rightRegs, RelationalCondition cond) {
         if (!isOperandConstantChar(left))
             return false;
+        // This may run on a compiler thread, so read the StringImpl directly rather than through
+        // tryGetValue(), whose GCOwnedDataScope is only meant to be constructed on the mutator.
+        // Read the character at once: the mutator may atomize the constant at any time, and a GC
+        // during this compile then drops the old StringImpl. A single character constant is never a rope.
+        const StringImpl* impl = asString(getConstantOperand(left))->tryGetValueImpl();
+        RELEASE_ASSERT(impl);
+        char16_t character = impl->at(0);
 
         addSlowCase(branchIfNotCell(rightRegs));
         JumpList failures;
         // FIXME: We could deduplicate the String's data load in emitLoadCharacterString if we had an extra scratch but we'd have to teach the register allocator about constants to do that unless we wanted to have the scratch in all cases, which doesn't seem worth it.
         emitLoadCharacterString(rightRegs.payloadGPR(), s_scratch, failures);
         addSlowCase(failures);
-        emitCompare(commute(cond), JSValueRegs { s_scratch }, Imm32(asString(getConstantOperand(left))->tryGetValue(disallowAllocation).data[0]));
+        emitCompare(commute(cond), JSValueRegs { s_scratch }, Imm32(character));
         return true;
     };