Fix descriptor cache race in LOAD_ATTR method specialization

youknowone · youknowone · commit e2c097eb6830 · 2026-03-06T13:35:35.000+09:00
Add per-cache-base descriptor seqlock in CodeUnits and use it in try_read/write_cached_descriptor paths to prevent mixed type_version/descriptor reads under concurrent specialization. This fixes lock/RLock method mixups seen in threading/compileall stress runs. Also add cspell entries for stoptheworld and pystate.
diff --git a/.cspell.dict/cpython.txt b/.cspell.dict/cpython.txt
@@ -127,8 +127,8 @@ NEWLOCALS
 newsemlockobject
 nfrees
 nkwargs
-nlocalsplus
 nkwelts
+nlocalsplus
 Nondescriptor
 noninteger
 nops
@@ -160,6 +160,7 @@ pylifecycle
 pymain
 pyrepl
 PYTHONTRACEMALLOC
+PYTHONUTF
 pythonw
 PYTHREAD_NAME
 releasebuffer
@@ -171,9 +172,11 @@ saveall
 scls
 setdict
 setfunc
+setprofileallthreads
 SETREF
 setresult
 setslice
+settraceallthreads
 SLOTDEFINED
 SMALLBUF
 SOABI
@@ -190,6 +193,7 @@ subparams
 subscr
 sval
 swappedbytes
+sysdict
 templatelib
 testconsole
 ticketer
diff --git a/.cspell.json b/.cspell.json
@@ -152,11 +152,6 @@
     "IFEXEC",
     // "stat"
     "FIRMLINK",
-    // CPython internal names
-    "PYTHONUTF",
-    "sysdict",
-    "settraceallthreads",
-    "setprofileallthreads"
   ],
   // flagWords - list of words to be always considered incorrect
   "flagWords": [
diff --git a/crates/compiler-core/src/bytecode.rs b/crates/compiler-core/src/bytecode.rs
@@ -12,7 +12,7 @@ use core::{
     cell::UnsafeCell,
     hash, mem,
     ops::Deref,
-    sync::atomic::{AtomicU8, AtomicU16, AtomicUsize, Ordering},
+    sync::atomic::{AtomicU8, AtomicU16, AtomicU32, AtomicUsize, Ordering},
 };
 use itertools::Itertools;
 use malachite_bigint::BigInt;
@@ -415,6 +415,9 @@ pub struct CodeUnits {
     /// Single atomic load/store prevents torn reads when multiple threads
     /// specialize the same instruction concurrently.
     pointer_cache: Box<[AtomicUsize]>,
+    /// SeqLock counter per instruction cache base for descriptor payload writes.
+    /// odd = write in progress, even = quiescent.
+    descriptor_sequences: Box<[AtomicU32]>,
 }
 
 // SAFETY: All cache operations use atomic read/write instructions.
@@ -441,10 +444,16 @@ impl Clone for CodeUnits {
             .iter()
             .map(|c| AtomicUsize::new(c.load(Ordering::Relaxed)))
             .collect();
+        let descriptor_sequences = self
+            .descriptor_sequences
+            .iter()
+            .map(|c| AtomicU32::new(c.load(Ordering::Relaxed)))
+            .collect();
         Self {
             units: UnsafeCell::new(units),
             adaptive_counters,
             pointer_cache,
+            descriptor_sequences,
         }
     }
 }
@@ -491,10 +500,15 @@ impl From<Vec<CodeUnit>> for CodeUnits {
             .map(|_| AtomicUsize::new(0))
             .collect::<Vec<_>>()
             .into_boxed_slice();
+        let descriptor_sequences = (0..len)
+            .map(|_| AtomicU32::new(0))
+            .collect::<Vec<_>>()
+            .into_boxed_slice();
         Self {
             units: UnsafeCell::new(units),
             adaptive_counters,
             pointer_cache,
+            descriptor_sequences,
         }
     }
 }
@@ -641,6 +655,54 @@ impl CodeUnits {
         self.pointer_cache[index].load(Ordering::Relaxed)
     }
 
+    #[inline]
+    pub fn begin_descriptor_read(&self, index: usize) -> u32 {
+        let mut sequence = self.descriptor_sequences[index].load(Ordering::Acquire);
+        while (sequence & 1) != 0 {
+            core::hint::spin_loop();
+            sequence = self.descriptor_sequences[index].load(Ordering::Acquire);
+        }
+        sequence
+    }
+
+    #[inline]
+    pub fn end_descriptor_read(&self, index: usize, previous: u32) -> bool {
+        core::sync::atomic::fence(Ordering::Acquire);
+        self.descriptor_sequences[index].load(Ordering::Relaxed) == previous
+    }
+
+    #[inline]
+    pub fn begin_descriptor_write(&self, index: usize) {
+        let sequence = &self.descriptor_sequences[index];
+        let mut seq = sequence.load(Ordering::Acquire);
+        loop {
+            while (seq & 1) != 0 {
+                core::hint::spin_loop();
+                seq = sequence.load(Ordering::Acquire);
+            }
+            match sequence.compare_exchange_weak(
+                seq,
+                seq.wrapping_add(1),
+                Ordering::AcqRel,
+                Ordering::Acquire,
+            ) {
+                Ok(_) => {
+                    core::sync::atomic::fence(Ordering::Release);
+                    break;
+                }
+                Err(observed) => {
+                    core::hint::spin_loop();
+                    seq = observed;
+                }
+            }
+        }
+    }
+
+    #[inline]
+    pub fn end_descriptor_write(&self, index: usize) {
+        self.descriptor_sequences[index].fetch_add(1, Ordering::Release);
+    }
+
     /// Read adaptive counter bits for instruction at `index`.
     /// Uses Relaxed atomic load.
     pub fn read_adaptive_counter(&self, index: usize) -> u16 {
diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs
@@ -7030,18 +7030,50 @@ impl ExecutingFrame<'_> {
         cache_base: usize,
         expected_type_version: u32,
     ) -> Option<PyObjectRef> {
-        let descr_ptr = self.code.instructions.read_cache_ptr(cache_base + 5);
-        if descr_ptr == 0 {
-            return None;
-        }
-        let cloned = unsafe { PyObject::try_to_owned_from_ptr(descr_ptr as *mut PyObject) }?;
-        if self.code.instructions.read_cache_u32(cache_base + 1) == expected_type_version
-            && self.code.instructions.read_cache_ptr(cache_base + 5) == descr_ptr
-        {
-            Some(cloned)
-        } else {
-            drop(cloned);
-            None
+        loop {
+            let sequence = self.code.instructions.begin_descriptor_read(cache_base);
+            let version_before = self.code.instructions.read_cache_u32(cache_base + 1);
+            if version_before != expected_type_version || version_before == 0 {
+                if self
+                    .code
+                    .instructions
+                    .end_descriptor_read(cache_base, sequence)
+                {
+                    return None;
+                }
+                continue;
+            }
+
+            let descr_ptr = self.code.instructions.read_cache_ptr(cache_base + 5);
+            if descr_ptr == 0 {
+                if self
+                    .code
+                    .instructions
+                    .end_descriptor_read(cache_base, sequence)
+                {
+                    return None;
+                }
+                continue;
+            }
+
+            let cloned = unsafe { PyObject::try_to_owned_from_ptr(descr_ptr as *mut PyObject) };
+            let version_after = self.code.instructions.read_cache_u32(cache_base + 1);
+            let ptr_after = self.code.instructions.read_cache_ptr(cache_base + 5);
+            let stable = self
+                .code
+                .instructions
+                .end_descriptor_read(cache_base, sequence);
+
+            if let Some(cloned) = cloned
+                && stable
+                && version_after == expected_type_version
+                && ptr_after == descr_ptr
+            {
+                return Some(cloned);
+            }
+            if stable {
+                return None;
+            }
         }
     }
 
@@ -7054,6 +7086,7 @@ impl ExecutingFrame<'_> {
     ) {
         // Publish descriptor cache atomically as a tuple:
         // invalidate version first, then write payload, then publish version.
+        self.code.instructions.begin_descriptor_write(cache_base);
         unsafe {
             self.code.instructions.write_cache_u32(cache_base + 1, 0);
             self.code
@@ -7063,6 +7096,7 @@ impl ExecutingFrame<'_> {
                 .instructions
                 .write_cache_u32(cache_base + 1, type_version);
         }
+        self.code.instructions.end_descriptor_write(cache_base);
     }
 
     #[inline]
@@ -7074,6 +7108,7 @@ impl ExecutingFrame<'_> {
         descr_ptr: usize,
     ) {
         // Same publish protocol as write_cached_descriptor(), plus metaclass guard.
+        self.code.instructions.begin_descriptor_write(cache_base);
         unsafe {
             self.code.instructions.write_cache_u32(cache_base + 1, 0);
             self.code
@@ -7086,6 +7121,7 @@ impl ExecutingFrame<'_> {
                 .instructions
                 .write_cache_u32(cache_base + 1, type_version);
         }
+        self.code.instructions.end_descriptor_write(cache_base);
     }
 
     fn load_attr(&mut self, vm: &VirtualMachine, oparg: LoadAttr) -> FrameResult {
