Remove the December 2025 LTO de-inlining stopgap now that WTF::opaque() is volatile

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<HeapCell*>(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;

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<HeapCell*>(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<HeapCell*>(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)
 {

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;
 }

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)
 {

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<const HeapCell*>(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)
 {

Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp

@@ -97,7 +97,7 @@ void JSFinalizationRegistry::destroy(JSCell* table)
     static_cast<JSFinalizationRegistry*>(table)->~JSFinalizationRegistry();
 }
 
-NEVER_INLINE void JSFinalizationRegistry::finalizeUnconditionally(VM& vm, CollectionScope)
+void JSFinalizationRegistry::finalizeUnconditionally(VM& vm, CollectionScope)
 {
     Locker locker { cellLock() };
 

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<typename... Arguments>
-    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<typename T>
-    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);

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