Skip to content

Commit 1edc620

Browse files
sosukesuzukisosukesuzuki
committed
[JSC] Baseline JIT should not construct GCOwnedDataScope on the compiler thread
https://bugs.webkit.org/show_bug.cgi?id=323102 Reviewed by Yusuke Suzuki. Heap::m_topGCOwnedDataScope is a debug-only, unsynchronized field that says whether a GCOwnedDataScope is live on the mutator's stack, and Heap::clearConcurrentRetainedDataIfPossible asserts it is null before dropping the old StringImpls it keeps alive for such scopes. Compiler and GC threads are covered by separate checks there. But JIT::emit_compareImpl and LOLJIT::emitCompareImpl read a single-character string constant through JSString::tryGetValue(), which returns a GCOwnedDataScope, so the JITWorker thread writes the field too. If the IncrementalSweeper timer fires while the JIT thread is inside that scope, the assertion aborts even though the ongoing-compilation check would have kept the strings alive. Read the constant through tryGetValueImpl() and StringImpl::at(0), as the DFG already does, and assert in setTopGCOwnedDataScopeIfNeeded and clearTopGCOwnedDataScopeIfNeeded that they never run on a compiler thread. GC threads still construct such scopes, since ErrorInstance::reconcileWeakReferencesAtGCEnd materializes stack traces into strings on the collector thread, so the field keeps ignoring them. This assertion fired in an ASan build in Bun's CI. Release builds are not affected. * Source/JavaScriptCore/heap/GCOwnedDataScope.cpp: (JSC::setTopGCOwnedDataScopeIfNeeded): (JSC::clearTopGCOwnedDataScopeIfNeeded): * Source/JavaScriptCore/jit/JITArithmetic.cpp: (JSC::JIT::emit_compareImpl): * Source/JavaScriptCore/lol/LOLJIT.cpp: (JSC::LOL::LOLJIT::emitCompareImpl): Canonical link: https://commits.webkit.org/320348@main
1 parent d3e5e15 commit 1edc620

3 files changed

Lines changed: 12 additions & 4 deletions

File tree

‎Source/JavaScriptCore/heap/GCOwnedDataScope.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ void setTopGCOwnedDataScopeIfNeeded(const JSCell* cell, const void* scope)
3636
{
3737
if (!cell)
3838
return;
39+
ASSERT(!isCompilationThread());
40+
if (Thread::mayBeGCThread())
41+
return;
3942
if (!cell->vm().heap.m_topGCOwnedDataScope)
4043
cell->vm().heap.m_topGCOwnedDataScope = scope;
4144
}
@@ -44,6 +47,9 @@ void clearTopGCOwnedDataScopeIfNeeded(const JSCell* cell, const void* scope)
4447
{
4548
if (!cell)
4649
return;
50+
ASSERT(!isCompilationThread());
51+
if (Thread::mayBeGCThread())
52+
return;
4753
if (cell->vm().heap.m_topGCOwnedDataScope == scope)
4854
cell->vm().heap.m_topGCOwnedDataScope = nullptr;
4955
}

‎Source/JavaScriptCore/jit/JITArithmetic.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,17 +233,18 @@ ALWAYS_INLINE void JIT::emit_compareImpl(VirtualRegister op1, VirtualRegister op
233233
// - constant int immediate to int immediate
234234
// - int immediate to int immediate
235235

236-
constexpr bool disallowAllocation = false;
237236
auto handleConstantCharOperand = [&](VirtualRegister left, VirtualRegister right, RelationalCondition cond) {
238237
if (!isOperandConstantChar(left))
239238
return false;
239+
JSString* string = asString(getConstantOperand(left));
240+
RELEASE_ASSERT(!string->isRope());
240241

241242
emitGetVirtualRegister(right, regT0);
242243
addSlowCase(branchIfNotCell(regT0));
243244
JumpList failures;
244245
emitLoadCharacterString(regT0, regT0, failures);
245246
addSlowCase(failures);
246-
emitCompare(commute(cond), regT0, Imm32(asString(getConstantOperand(left))->tryGetValue(disallowAllocation).data[0]));
247+
emitCompare(commute(cond), regT0, Imm32(string->tryGetValueImpl()->at(0)));
247248
return true;
248249
};
249250

‎Source/JavaScriptCore/lol/LOLJIT.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -961,17 +961,18 @@ ALWAYS_INLINE void LOLJIT::emitCompareImpl(VirtualRegister op1, GPRReg op1GPR, V
961961
// - constant int immediate to int immediate
962962
// - int immediate to int immediate
963963

964-
constexpr bool disallowAllocation = false;
965964
auto handleConstantCharOperand = [&](VirtualRegister left, GPRReg rightGPR, RelationalCondition cond) {
966965
if (!isOperandConstantChar(left))
967966
return false;
967+
JSString* string = asString(getConstantOperand(left));
968+
RELEASE_ASSERT(!string->isRope());
968969

969970
addSlowCase(branchIfNotCell(rightGPR));
970971
JumpList failures;
971972
// 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.
972973
emitLoadCharacterString(rightGPR, s_scratch, failures);
973974
addSlowCase(failures);
974-
emitCompare(commute(cond), s_scratch, Imm32(asString(getConstantOperand(left))->tryGetValue(disallowAllocation).data[0]));
975+
emitCompare(commute(cond), s_scratch, Imm32(string->tryGetValueImpl()->at(0)));
975976
return true;
976977
};
977978

0 commit comments

Comments
 (0)