diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp index 0a4e2038f374..5bb60987462d 100644 --- a/Source/JavaScriptCore/heap/Heap.cpp +++ b/Source/JavaScriptCore/heap/Heap.cpp @@ -135,18 +135,6 @@ namespace JSC { -// NEVER_INLINE to prevent LTO from inlining this function, which can break -// compiler barriers in MarkedBlock::isMarked on x86_64. -NEVER_INLINE bool Heap::isMarked(const void* rawCell) -{ - ASSERT(!m_isMarkingForGCVerifier); - HeapCell* cell = std::bit_cast(rawCell); - if (cell->isPreciseAllocation()) - return cell->preciseAllocation().isMarked(); - MarkedBlock& block = cell->markedBlock(); - return block.isMarked(m_objectSpace.markingVersion(), cell); -} - namespace HeapInternal { static constexpr bool verbose = false; static constexpr bool verboseStop = false; diff --git a/Source/JavaScriptCore/heap/HeapInlines.h b/Source/JavaScriptCore/heap/HeapInlines.h index 9c6647adfee2..4eb3938995b0 100644 --- a/Source/JavaScriptCore/heap/HeapInlines.h +++ b/Source/JavaScriptCore/heap/HeapInlines.h @@ -56,16 +56,15 @@ inline JSC::Heap* Heap::heap(const JSValue v) return heap(v.asCell()); } -// Defined in Heap.cpp with NEVER_INLINE to prevent LTO from breaking compiler barriers -// ALWAYS_INLINE bool Heap::isMarked(const void* rawCell) -// { -// ASSERT(!m_isMarkingForGCVerifier); -// HeapCell* cell = std::bit_cast(rawCell); -// if (cell->isPreciseAllocation()) -// return cell->preciseAllocation().isMarked(); -// MarkedBlock& block = cell->markedBlock(); -// return block.isMarked(m_objectSpace.markingVersion(), cell); -// } +ALWAYS_INLINE bool Heap::isMarked(const void* rawCell) +{ + ASSERT(!m_isMarkingForGCVerifier); + HeapCell* cell = std::bit_cast(rawCell); + if (cell->isPreciseAllocation()) + return cell->preciseAllocation().isMarked(); + MarkedBlock& block = cell->markedBlock(); + return block.isMarked(m_objectSpace.markingVersion(), cell); +} ALWAYS_INLINE bool Heap::testAndSetMarked(HeapVersion markingVersion, const void* rawCell) { diff --git a/Source/JavaScriptCore/heap/MarkedBlock.cpp b/Source/JavaScriptCore/heap/MarkedBlock.cpp index 4430cbd053e2..4690d7916321 100644 --- a/Source/JavaScriptCore/heap/MarkedBlock.cpp +++ b/Source/JavaScriptCore/heap/MarkedBlock.cpp @@ -43,81 +43,6 @@ WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN namespace JSC { -// NEVER_INLINE to prevent LTO from inlining this function, which can break -// compiler barriers (loadLoadFence/compilerFence) on x86_64. -NEVER_INLINE bool MarkedBlock::isMarked(HeapVersion markingVersion, const void* p) -{ - HeapVersion version; - Dependency dependency = Dependency::loadAndFence(&header().m_markingVersion, version); - if (version != markingVersion) [[unlikely]] - return false; - return header().m_marks.concurrentGet(atomNumber(p), dependency); -} - -// NEVER_INLINE to prevent LTO from inlining this function, which can break -// compiler barriers (Dependency::fence/loadLoadFence/compilerFence) on x86_64. -NEVER_INLINE bool MarkedBlock::Handle::isLive(HeapVersion markingVersion, HeapVersion newlyAllocatedVersion, bool isMarking, const HeapCell* cell) -{ - m_directory->assertIsMutatorOrMutatorIsStopped(); - if (m_directory->isAllocated(this)) - return true; - - MarkedBlock& block = this->block(); - MarkedBlock::Header& header = block.header(); - - auto count = header.m_lock.tryOptimisticFencelessRead(); - if (count.value) { - Dependency fenceBefore = Dependency::fence(count.input); - MarkedBlock& fencedBlock = *fenceBefore.consume(&block); - MarkedBlock::Header& fencedHeader = fencedBlock.header(); - MarkedBlock::Handle* fencedThis = fenceBefore.consume(this); - - ASSERT_UNUSED(fencedThis, !fencedThis->isFreeListed()); - - HeapVersion myNewlyAllocatedVersion = fencedHeader.m_newlyAllocatedVersion; - if (myNewlyAllocatedVersion == newlyAllocatedVersion) { - bool result = fencedBlock.isNewlyAllocated(cell); - if (header.m_lock.fencelessValidate(count.value, Dependency::fence(result))) - return result; - } else { - HeapVersion myMarkingVersion = fencedHeader.m_markingVersion; - if (myMarkingVersion != markingVersion - && (!isMarking || !fencedBlock.marksConveyLivenessDuringMarking(myMarkingVersion, markingVersion))) { - if (header.m_lock.fencelessValidate(count.value, Dependency::fence(myMarkingVersion))) - return false; - } else { - bool result = fencedHeader.m_marks.get(block.atomNumber(cell)); - if (header.m_lock.fencelessValidate(count.value, Dependency::fence(result))) - return result; - } - } - } - - Locker locker { header.m_lock }; - - ASSERT(!isFreeListed()); - - HeapVersion myNewlyAllocatedVersion = header.m_newlyAllocatedVersion; - if (myNewlyAllocatedVersion == newlyAllocatedVersion) - return block.isNewlyAllocated(cell); - - if (block.areMarksStale(markingVersion)) { - if (!isMarking) - return false; - if (!block.marksConveyLivenessDuringMarking(markingVersion)) - return false; - } - - return header.m_marks.get(block.atomNumber(cell)); -} - -// NEVER_INLINE to prevent LTO from inlining this function, which can break -// compiler barriers on x86_64. -NEVER_INLINE bool MarkedBlock::Handle::isLive(const HeapCell* cell) -{ - return isLive(space()->markingVersion(), space()->newlyAllocatedVersion(), space()->isMarking(), cell); -} - namespace MarkedBlockInternal { static constexpr bool verbose = false; } diff --git a/Source/JavaScriptCore/heap/MarkedBlock.h b/Source/JavaScriptCore/heap/MarkedBlock.h index 1bf8d98a2bae..da473e22137c 100644 --- a/Source/JavaScriptCore/heap/MarkedBlock.h +++ b/Source/JavaScriptCore/heap/MarkedBlock.h @@ -606,15 +606,14 @@ inline bool MarkedBlock::isMarkedRaw(const void* p) return header().m_marks.get(atomNumber(p)); } -// Defined in MarkedBlock.cpp with NEVER_INLINE to prevent LTO from breaking compiler barriers -// inline bool MarkedBlock::isMarked(HeapVersion markingVersion, const void* p) -// { -// HeapVersion version; -// Dependency dependency = Dependency::loadAndFence(&header().m_markingVersion, version); -// if (version != markingVersion) [[unlikely]] -// return false; -// return header().m_marks.concurrentGet(atomNumber(p), dependency); -// } +inline bool MarkedBlock::isMarked(HeapVersion markingVersion, const void* p) +{ + HeapVersion version; + Dependency dependency = Dependency::loadAndFence(&header().m_markingVersion, version); + if (version != markingVersion) [[unlikely]] + return false; + return header().m_marks.concurrentGet(atomNumber(p), dependency); +} inline bool MarkedBlock::isMarked(const void* p, Dependency dependency) { diff --git a/Source/JavaScriptCore/heap/MarkedBlockInlines.h b/Source/JavaScriptCore/heap/MarkedBlockInlines.h index 4a2369a90c07..d3739a07099f 100644 --- a/Source/JavaScriptCore/heap/MarkedBlockInlines.h +++ b/Source/JavaScriptCore/heap/MarkedBlockInlines.h @@ -99,8 +99,96 @@ inline bool MarkedBlock::Handle::isAllocated() return m_directory->isAllocated(this); } -// Defined in MarkedBlock.cpp with NEVER_INLINE to prevent LTO from breaking compiler barriers -// ALWAYS_INLINE bool MarkedBlock::Handle::isLive(HeapVersion markingVersion, HeapVersion newlyAllocatedVersion, bool isMarking, const HeapCell* cell) +ALWAYS_INLINE bool MarkedBlock::Handle::isLive(HeapVersion markingVersion, HeapVersion newlyAllocatedVersion, bool isMarking, const HeapCell* cell) +{ + m_directory->assertIsMutatorOrMutatorIsStopped(); + if (m_directory->isAllocated(this)) + return true; + + // We need to do this while holding the lock because marks might be stale. In that case, newly + // allocated will not yet be valid. Consider this interleaving. + // + // One thread is doing this: + // + // 1) IsLiveChecksNewlyAllocated: We check if newly allocated is valid. If it is valid, and the bit is + // set, we return true. Let's assume that this executes atomically. It doesn't have to in general, + // but we can assume that for the purpose of seeing this bug. + // + // 2) IsLiveChecksMarks: Having failed that, we check the mark bits. This step implies the rest of + // this function. It happens under a lock so it's atomic. + // + // Another thread is doing: + // + // 1) AboutToMarkSlow: This is the entire aboutToMarkSlow function, and let's say it's atomic. It + // sorta is since it holds a lock, but that doesn't actually make it atomic with respect to + // IsLiveChecksNewlyAllocated, since that does not hold a lock in our scenario. + // + // The harmful interleaving happens if we start out with a block that has stale mark bits that + // nonetheless convey liveness during marking (the off-by-one version trick). The interleaving is + // just: + // + // IsLiveChecksNewlyAllocated AboutToMarkSlow IsLiveChecksMarks + // + // We started with valid marks but invalid newly allocated. So, the first part doesn't think that + // anything is live, but dutifully drops down to the marks step. But in the meantime, we clear the + // mark bits and transfer their contents into newlyAllocated. So IsLiveChecksMarks also sees nothing + // live. Ooops! + // + // Fortunately, since this is just a read critical section, we can use a CountingLock. + // + // Probably many users of CountingLock could use its lambda-based and locker-based APIs. But here, we + // need to ensure that everything is ALWAYS_INLINE. It's hard to do that when using lambdas. It's + // more reliable to write it inline instead. Empirically, it seems like how inline this is has some + // impact on perf - around 2% on splay if you get it wrong. + + MarkedBlock& block = this->block(); + MarkedBlock::Header& header = block.header(); + + auto count = header.m_lock.tryOptimisticFencelessRead(); + if (count.value) { + Dependency fenceBefore = Dependency::fence(count.input); + MarkedBlock& fencedBlock = *fenceBefore.consume(&block); + MarkedBlock::Header& fencedHeader = fencedBlock.header(); + MarkedBlock::Handle* fencedThis = fenceBefore.consume(this); + + ASSERT_UNUSED(fencedThis, !fencedThis->isFreeListed()); + + HeapVersion myNewlyAllocatedVersion = fencedHeader.m_newlyAllocatedVersion; + if (myNewlyAllocatedVersion == newlyAllocatedVersion) { + bool result = fencedBlock.isNewlyAllocated(cell); + if (header.m_lock.fencelessValidate(count.value, Dependency::fence(result))) + return result; + } else { + HeapVersion myMarkingVersion = fencedHeader.m_markingVersion; + if (myMarkingVersion != markingVersion + && (!isMarking || !fencedBlock.marksConveyLivenessDuringMarking(myMarkingVersion, markingVersion))) { + if (header.m_lock.fencelessValidate(count.value, Dependency::fence(myMarkingVersion))) + return false; + } else { + bool result = fencedHeader.m_marks.get(block.atomNumber(cell)); + if (header.m_lock.fencelessValidate(count.value, Dependency::fence(result))) + return result; + } + } + } + + Locker locker { header.m_lock }; + + ASSERT(!isFreeListed()); + + HeapVersion myNewlyAllocatedVersion = header.m_newlyAllocatedVersion; + if (myNewlyAllocatedVersion == newlyAllocatedVersion) + return block.isNewlyAllocated(cell); + + if (block.areMarksStale(markingVersion)) { + if (!isMarking) + return false; + if (!block.marksConveyLivenessDuringMarking(markingVersion)) + return false; + } + + return header.m_marks.get(block.atomNumber(cell)); +} inline bool MarkedBlock::Handle::isLiveCell(HeapVersion markingVersion, HeapVersion newlyAllocatedVersion, bool isMarking, const void* p) { @@ -109,11 +197,10 @@ inline bool MarkedBlock::Handle::isLiveCell(HeapVersion markingVersion, HeapVers return isLive(markingVersion, newlyAllocatedVersion, isMarking, static_cast(p)); } -// Defined in MarkedBlock.cpp with NEVER_INLINE to prevent LTO from breaking compiler barriers -// inline bool MarkedBlock::Handle::isLive(const HeapCell* cell) -// { -// return isLive(space()->markingVersion(), space()->newlyAllocatedVersion(), space()->isMarking(), cell); -// } +inline bool MarkedBlock::Handle::isLive(const HeapCell* cell) +{ + return isLive(space()->markingVersion(), space()->newlyAllocatedVersion(), space()->isMarking(), cell); +} inline bool MarkedBlock::Handle::isLiveCell(const void* p) { diff --git a/Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp b/Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp index f18bce58ea36..0809d0a8cd53 100644 --- a/Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp +++ b/Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp @@ -97,7 +97,7 @@ void JSFinalizationRegistry::destroy(JSCell* table) static_cast(table)->~JSFinalizationRegistry(); } -NEVER_INLINE void JSFinalizationRegistry::finalizeUnconditionally(VM& vm, CollectionScope) +void JSFinalizationRegistry::finalizeUnconditionally(VM& vm, CollectionScope) { Locker locker { cellLock() }; diff --git a/Source/WTF/wtf/Atomics.h b/Source/WTF/wtf/Atomics.h index 657197774028..14a4467d8a56 100644 --- a/Source/WTF/wtf/Atomics.h +++ b/Source/WTF/wtf/Atomics.h @@ -291,12 +291,10 @@ inline void x86_cpuid() : "memory"); } -// Use std::atomic_thread_fence instead of compilerFence to prevent LTO from -// optimizing away the barrier on x86_64. -inline void loadLoadFence() { std::atomic_thread_fence(std::memory_order_acquire); } -inline void loadStoreFence() { std::atomic_thread_fence(std::memory_order_acquire); } +inline void loadLoadFence() { compilerFence(); } +inline void loadStoreFence() { compilerFence(); } inline void storeLoadFence() { x86_ortop(); } -inline void storeStoreFence() { std::atomic_thread_fence(std::memory_order_release); } +inline void storeStoreFence() { compilerFence(); } inline void crossModifyingCodeFence() { x86_cpuid(); } #else @@ -371,7 +369,7 @@ class Dependency { // produces zero, but it's concealed from the compiler. The CPU understands this dummy op to be a // phantom dependency. template - NEVER_INLINE static Dependency fence(Arguments... arguments) + static Dependency fence(Arguments... arguments) { InternalDependencyType input = opaqueMixture(arguments...); InternalDependencyType output; @@ -437,7 +435,7 @@ class Dependency { // value, similar to above. The fix here is to obscure the pointer we're loading from from // the compiler. template - NEVER_INLINE static Dependency loadAndFence(const T* pointer, T& output) + static Dependency loadAndFence(const T* pointer, T& output) { #if CPU(ARM64) || CPU(ARM) T value = *opaque(pointer); diff --git a/Source/WTF/wtf/Compiler.h b/Source/WTF/wtf/Compiler.h index 717a9db83470..44d1486e5cdf 100644 --- a/Source/WTF/wtf/Compiler.h +++ b/Source/WTF/wtf/Compiler.h @@ -191,16 +191,24 @@ /* ALWAYS_INLINE */ -/* TEMPORARY: Replace ALWAYS_INLINE with plain inline (no __always_inline__ attribute) - * to debug LTO inlining issues on Alpine Linux. - * This helps identify if the always_inline attribute is causing the FinalizationRegistry bug. */ +/* In GCC functions marked with no_sanitize_address cannot call functions that are marked with always_inline and not marked with no_sanitize_address. + * Therefore we need to give up on the enforcement of ALWAYS_INLINE when building with ASAN. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 */ +#if !defined(ALWAYS_INLINE) && defined(NDEBUG) && !(COMPILER(GCC) && ASAN_ENABLED) +#define ALWAYS_INLINE inline __attribute__((__always_inline__)) +#endif + #if !defined(ALWAYS_INLINE) #define ALWAYS_INLINE inline #endif /* ALWAYS_INLINE_LAMBDA */ -/* TEMPORARY: Disable ALWAYS_INLINE_LAMBDA to debug LTO inlining issues on Alpine Linux. */ +/* In GCC functions marked with no_sanitize_address cannot call functions that are marked with always_inline and not marked with no_sanitize_address. + * Therefore we need to give up on the enforcement of ALWAYS_INLINE_LAMBDA when building with ASAN. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 */ +#if !defined(ALWAYS_INLINE_LAMBDA) && defined(NDEBUG) && !(COMPILER(GCC) && ASAN_ENABLED) +#define ALWAYS_INLINE_LAMBDA __attribute__((__always_inline__)) +#endif + #if !defined(ALWAYS_INLINE_LAMBDA) #define ALWAYS_INLINE_LAMBDA #endif