From 8f84fea3772d5ee357efd943d22714a3e1a62b87 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" <69878+youknowone@users.noreply.github.com> Date: Tue, 29 Sep 2026 01:59:05 +0900 Subject: [PATCH] Fix 32-bit member layout assertions and address member descriptor review (#8882) * Address review findings on member descriptors Member definitions carry an internal `PY_ATOMIC` flag taken from `MemberLayout::ATOMIC`, and loads use an atomic access only for atomic storage; extension members never set it. `XMLParser` publishes its new state, `entity` and `target` only after every handler lookup succeeds. `subs_parameters` reads type names only for the error. The unraisable hook prints `` when `__module__` fails or is not a str. The builtin function `__reduce__` decides on the bound object alone. Assisted-by: Grok:4.7 Assisted-by: Claude:claude-opus-5-5 * Fix member layout assertions on 32-bit targets The tp_flags assertion compared AtomicU64 with u64 alignment, which differs on i686. The kevent assertion required usize, isize, c_ulong and c_long to be 64-bit. Both now check only what the member cells need. Assisted-by: Grok:4.7 Assisted-by: Claude:claude-opus-5-5 * Use bitflags for member definition flags PY_READONLY, PY_AUDIT_READ, PY_RELATIVE_OFFSET and PY_ATOMIC become the repr(transparent) PyMemberFlags. C extension flags are read with from_bits_retain. Assisted-by: Grok:4.7 Assisted-by: Claude:claude-opus-5-5 --- crates/capi/src/descrobject.rs | 13 +- crates/derive-impl/src/pyclass.rs | 52 ++++-- crates/stdlib/src/elementtree.rs | 9 +- crates/stdlib/src/select.rs | 9 +- crates/vm/src/builtins/builtin_func.rs | 19 +- crates/vm/src/builtins/descriptor.rs | 246 ++++++++++++++----------- crates/vm/src/builtins/genericalias.rs | 19 +- crates/vm/src/builtins/mod.rs | 4 +- crates/vm/src/builtins/type.rs | 3 +- crates/vm/src/stdlib/sys.rs | 13 +- crates/vm/src/types/slot.rs | 5 +- crates/vm/src/vm/context.rs | 8 +- 12 files changed, 246 insertions(+), 154 deletions(-) diff --git a/crates/capi/src/descrobject.rs b/crates/capi/src/descrobject.rs index ea27600bb..c600f2d01 100644 --- a/crates/capi/src/descrobject.rs +++ b/crates/capi/src/descrobject.rs @@ -5,8 +5,8 @@ use crate::pystate::with_vm; use crate::util::{CStrExt, FfiPtrExt}; use core::ffi::{c_char, c_int, c_void}; use rustpython_vm::builtins::{ - DescriptorMemberDef, MemberAccess, MemberKind, PY_RELATIVE_OFFSET, PyDescriptorOwned, PyGetSet, - PyMappingProxy, PyMemberDescriptor, PyType, + DescriptorMemberDef, MemberAccess, MemberKind, PyDescriptorOwned, PyGetSet, PyMappingProxy, + PyMemberDescriptor, PyMemberFlags, PyType, }; use rustpython_vm::common::lock::PyRwLock; use rustpython_vm::function::{ItemDoc, PySetterValue}; @@ -127,12 +127,15 @@ impl PyMemberDef { ))); }; let mut offset = self.offset; - let mut flags = self.flags; - if flags & PY_RELATIVE_OFFSET != 0 { + // Unknown bits belong to the extension and are kept. + let mut flags = PyMemberFlags::from_bits_retain(self.flags); + // Extension members never carry the internal atomic-storage bit. + flags.remove(PyMemberFlags::ATOMIC); + if flags.contains(PyMemberFlags::RELATIVE_OFFSET) { // type creation adds tp_basicsize and clears the flag before GetOne. // `slots.basicsize` is already the full tp_basicsize. offset += ty.slots.basicsize as isize; - flags &= !PY_RELATIVE_OFFSET; + flags.remove(PyMemberFlags::RELATIVE_OFFSET); } let doc = unsafe { self.doc.try_as_str_opt(vm) }?.map_or(ItemDoc::NONE, |doc| { diff --git a/crates/derive-impl/src/pyclass.rs b/crates/derive-impl/src/pyclass.rs index ccb6a7543..8441bf0d0 100644 --- a/crates/derive-impl/src/pyclass.rs +++ b/crates/derive-impl/src/pyclass.rs @@ -2122,17 +2122,32 @@ fn member_layout_tokens(readonly: bool) -> TokenStream { } fn member_flag_tokens(readonly: bool, audit_read: bool) -> TokenStream { - let mut flag_parts = Vec::new(); - if readonly { - flag_parts.push(quote!(::rustpython_vm::builtins::descriptor::PY_READONLY)); + match (readonly, audit_read) { + (false, false) => { + quote!(::rustpython_vm::builtins::descriptor::PyMemberFlags::empty()) + } + (true, false) => { + quote!(::rustpython_vm::builtins::descriptor::PyMemberFlags::READONLY) + } + (false, true) => { + quote!(::rustpython_vm::builtins::descriptor::PyMemberFlags::AUDIT_READ) + } + (true, true) => quote!( + ::rustpython_vm::builtins::descriptor::PyMemberFlags::READONLY + .union(::rustpython_vm::builtins::descriptor::PyMemberFlags::AUDIT_READ) + ), } - if audit_read { - flag_parts.push(quote!(::rustpython_vm::builtins::descriptor::PY_AUDIT_READ)); - } - if flag_parts.is_empty() { - quote!(0) - } else { - quote!(#(#flag_parts)|*) +} + +fn member_atomic_flag(field_tokens: &TokenStream) -> TokenStream { + quote! { + if ::rustpython_vm::builtins::descriptor::member_atomic_of( + |payload: &Self| &payload.#field_tokens, + ) { + ::rustpython_vm::builtins::descriptor::PyMemberFlags::ATOMIC + } else { + ::rustpython_vm::builtins::descriptor::PyMemberFlags::empty() + } } } @@ -2196,7 +2211,7 @@ fn build_member( "#[pymember(path = ...)] is only valid on a field", )); } - let (offset, check, kind_tokens) = if let Some((field, index)) = field { + let (offset, check, kind_tokens, atomic_flag) = if let Some((field, index)) = field { let index_tokens = match &field.ident { Some(ident) => ident.to_token_stream(), None => syn::Index::from(index).into_token_stream(), @@ -2211,6 +2226,9 @@ fn build_member( + ::core::mem::offset_of!(Self, #field_tokens) as isize }; let layout = member_layout_tokens(readonly); + let atomic_flag = member_atomic_flag(&field_tokens); + // Writable fields must implement `MemberCell`, which only atomic + // storage does. That bound is the check. let check = quote_spanned! { attr.span() => let _ = |payload: *const Self| { fn assert_member_field(_: *const T) {} @@ -2221,7 +2239,7 @@ fn build_member( }; }; let kind_tokens = member_kind_tokens(kind.as_deref(), Some(&field_tokens), attr.span())?; - (offset, check, kind_tokens) + (offset, check, kind_tokens, atomic_flag) } else { let Some(offset_expr) = offset_expr else { return Err(syn::Error::new( @@ -2230,19 +2248,25 @@ fn build_member( )); }; let kind_tokens = member_kind_tokens(kind.as_deref(), None, attr.span())?; - (offset_expr, quote!(), kind_tokens) + ( + offset_expr, + quote!(), + kind_tokens, + quote!(::rustpython_vm::builtins::descriptor::PyMemberFlags::empty()), + ) }; let doc = if meta.suppress_doc()? { quote!(::rustpython_vm::function::ItemDoc::NONE) } else { attr_doc_expr(Some(class_ty), &name, rust_doc) }; + let base_flags = member_flag_tokens(readonly, audit_read); Ok(BuiltMember { name, cfgs: cfgs.to_vec(), kind: kind_tokens, offset, - flags: member_flag_tokens(readonly, audit_read), + flags: quote!(#base_flags.union(#atomic_flag)), doc, check, span: attr.span(), diff --git a/crates/stdlib/src/elementtree.rs b/crates/stdlib/src/elementtree.rs index 857b4d980..37c60c127 100644 --- a/crates/stdlib/src/elementtree.rs +++ b/crates/stdlib/src/elementtree.rs @@ -1968,8 +1968,8 @@ pub(crate) mod _elementtree { }; let entity = vm.ctx.new_dict(); - let _previous = unsafe { zelf.entity.swap(Some(entity.into())) }; - let _previous = unsafe { zelf.target.swap(Some(target.clone())) }; + // Lookups can fail. Build the whole state before publishing it, so a + // failed re-init leaves the previous parser, entity, and target in place. let handlers = XMLParserState { parser: Some(parser.clone()), names: Some(vm.ctx.new_dict()), @@ -1992,6 +1992,11 @@ pub(crate) mod _elementtree { let has_pi = handlers.handle_pi.is_some(); let has_ns = handlers.handle_start_ns.is_some() || handlers.handle_end_ns.is_some(); *zelf.state.write() = handlers; + // SAFETY: `swap` returns the previous object. `_previous` holds it + // until `init` returns, so a `deref` of the old pointer stays valid + // for the rest of this call. + let _previous = unsafe { zelf.entity.swap(Some(entity.into())) }; + let _previous = unsafe { zelf.target.swap(Some(target)) }; // Expat calls back into these; they dispatch straight to the // target (and, when it is our own TreeBuilder, straight into diff --git a/crates/stdlib/src/select.rs b/crates/stdlib/src/select.rs index d55fe8312..2232fc8e3 100644 --- a/crates/stdlib/src/select.rs +++ b/crates/stdlib/src/select.rs @@ -721,11 +721,12 @@ mod decl { udata: AtomicU64, } + // `ident`/`udata` live in `AtomicU64` and `data` in `AtomicI64`. + // Pointer-sized inputs widen into those cells. The cells are not + // `usize`/`c_long`, so those widths need not match. const _: () = assert!( - core::mem::size_of::() == core::mem::size_of::() - && core::mem::size_of::() == core::mem::size_of::() - && core::mem::size_of::() == core::mem::size_of::() - && core::mem::size_of::() == core::mem::size_of::() + core::mem::size_of::() <= core::mem::size_of::() + && core::mem::size_of::() <= core::mem::size_of::() ); #[derive(FromArgs)] diff --git a/crates/vm/src/builtins/builtin_func.rs b/crates/vm/src/builtins/builtin_func.rs index 9747f4571..79e2c1ca7 100644 --- a/crates/vm/src/builtins/builtin_func.rs +++ b/crates/vm/src/builtins/builtin_func.rs @@ -201,16 +201,23 @@ impl PyNativeFunction { vm.ctx.none() } - // meth_reduce in CPython + // meth_reduce: the name when unbound or bound to a module, otherwise + // `(getattr, (self, name))`. `__module__` is not part of the decision. #[pymethod] fn __reduce__(zelf: NativeFunctionOrMethod, vm: &VirtualMachine) -> PyResult { let zelf = zelf.0; - if zelf.zelf.is_none() || zelf.module.deref().is_some() { - Ok(vm.ctx.new_str(zelf.value.name).into()) - } else { + // PyModule_Check: the type or a subtype, same as meth_reduce. + if let Some(bound) = zelf + .zelf + .as_ref() + .filter(|bound| !bound.class().is_subtype(vm.ctx.types.module_type)) + { let getattr = vm.builtins.get_attr("getattr", vm)?; - let target = zelf.zelf.clone().unwrap(); - Ok(vm.new_tuple((getattr, (target, zelf.value.name))).into()) + Ok(vm + .new_tuple((getattr, (bound.clone(), zelf.value.name))) + .into()) + } else { + Ok(vm.ctx.new_str(zelf.value.name).into()) } } diff --git a/crates/vm/src/builtins/descriptor.rs b/crates/vm/src/builtins/descriptor.rs index 8456d179e..83fa815af 100644 --- a/crates/vm/src/builtins/descriptor.rs +++ b/crates/vm/src/builtins/descriptor.rs @@ -378,15 +378,31 @@ impl MemberKind { } } -pub const PY_READONLY: i32 = 1; -#[doc(hidden)] -pub const PY_AUDIT_READ: i32 = 2; -pub const PY_RELATIVE_OFFSET: i32 = 8; +bitflags::bitflags! { + #[derive(Copy, Clone, Debug, PartialEq, Eq)] + #[repr(transparent)] + pub struct PyMemberFlags: i32 { + const READONLY = 1; + const AUDIT_READ = 2; + // `_Py_WRITE_RESTRICTED` (4) is deprecated. The bit is reserved and must not be reused. + const RELATIVE_OFFSET = 8; + /// The field is atomic storage (`Atomic*` or `PyAtomicTypeFlags`). + /// + /// Not a public member flag. The defined flags are `Py_READONLY` (1), + /// `Py_AUDIT_READ` (2), `_Py_WRITE_RESTRICTED` (4, deprecated, do not reuse), + /// and `Py_RELATIVE_OFFSET` (8). This bit is set only by `#[pymember]`. + /// Extension members leave it clear: a readonly extension member is a plain + /// load, and a writable one keeps an atomic access. + const ATOMIC = 0x10; + } +} /// Kind of a `#[pymember]` field. The macro reads [`MemberLayout::KIND`]. #[doc(hidden)] pub trait MemberLayout { const KIND: MemberKind; + /// `true` when the field is an atomic cell, not a plain integer or pointer. + const ATOMIC: bool = false; } /// Writable `#[pymember]` field. Only an atomic cell may change after publication. @@ -402,11 +418,19 @@ pub const fn member_kind_of( ::KIND } +/// [`MemberLayout::ATOMIC`] of the field named by `probe`. `probe` is not called. +#[doc(hidden)] +#[must_use] +pub const fn member_atomic_of(_: for<'a> fn(&'a Owner) -> &'a T) -> bool { + ::ATOMIC +} + impl MemberLayout for i16 { const KIND: MemberKind = MemberKind::Short; } impl MemberLayout for core::sync::atomic::AtomicI16 { const KIND: MemberKind = MemberKind::Short; + const ATOMIC: bool = true; } impl MemberCell for core::sync::atomic::AtomicI16 {} @@ -415,6 +439,7 @@ impl MemberLayout for u16 { } impl MemberLayout for core::sync::atomic::AtomicU16 { const KIND: MemberKind = MemberKind::UShort; + const ATOMIC: bool = true; } impl MemberCell for core::sync::atomic::AtomicU16 {} @@ -424,6 +449,7 @@ impl MemberLayout for i64 { } impl MemberLayout for core::sync::atomic::AtomicI64 { const KIND: MemberKind = MemberKind::from_i64(); + const ATOMIC: bool = true; } impl MemberCell for core::sync::atomic::AtomicI64 {} @@ -432,6 +458,7 @@ impl MemberLayout for u64 { } impl MemberLayout for core::sync::atomic::AtomicU64 { const KIND: MemberKind = MemberKind::from_u64(); + const ATOMIC: bool = true; } impl MemberCell for core::sync::atomic::AtomicU64 {} @@ -476,6 +503,7 @@ impl MemberLayout for bool { } impl MemberLayout for core::sync::atomic::AtomicBool { const KIND: MemberKind = MemberKind::Bool; + const ATOMIC: bool = true; } impl MemberCell for core::sync::atomic::AtomicBool {} @@ -484,6 +512,7 @@ impl MemberLayout for i32 { } impl MemberLayout for core::sync::atomic::AtomicI32 { const KIND: MemberKind = MemberKind::Int; + const ATOMIC: bool = true; } impl MemberCell for core::sync::atomic::AtomicI32 {} @@ -492,6 +521,7 @@ impl MemberLayout for u32 { } impl MemberLayout for core::sync::atomic::AtomicU32 { const KIND: MemberKind = MemberKind::Uint; + const ATOMIC: bool = true; } impl MemberCell for core::sync::atomic::AtomicU32 {} @@ -512,6 +542,7 @@ impl MemberLayout for Option> { } impl MemberLayout for crate::object::PyAtomicRef { const KIND: MemberKind = MemberKind::Object; + const ATOMIC: bool = true; } // `PyAtomicRef` is the same pointer-sized cell as `PyAtomicRef` // (`PhantomData` is zero-sized). Readonly object members may use it. The @@ -519,12 +550,15 @@ impl MemberLayout for crate::object::PyAtomicRef { // `PyAtomicRef` or `PyAtomicRef>` (`MemberCell`). impl MemberLayout for crate::object::PyAtomicRef { const KIND: MemberKind = MemberKind::Object; + const ATOMIC: bool = true; } impl MemberLayout for crate::object::PyAtomicRef> { const KIND: MemberKind = MemberKind::Object; + const ATOMIC: bool = true; } impl MemberLayout for crate::object::PyAtomicRef> { const KIND: MemberKind = MemberKind::Object; + const ATOMIC: bool = true; } impl MemberLayout for &'static crate::builtins::PyStrInterned { const KIND: MemberKind = MemberKind::Object; @@ -540,6 +574,7 @@ impl MemberLayout for f64 { } impl MemberLayout for crate::common::atomic::AtomicF64 { const KIND: MemberKind = MemberKind::Double; + const ATOMIC: bool = true; } impl MemberCell for crate::common::atomic::AtomicF64 {} @@ -548,6 +583,7 @@ impl MemberLayout for isize { } impl MemberLayout for core::sync::atomic::AtomicIsize { const KIND: MemberKind = MemberKind::PySsizeT; + const ATOMIC: bool = true; } impl MemberCell for core::sync::atomic::AtomicIsize {} @@ -577,6 +613,7 @@ const _: () = assert!( ); impl MemberLayout for core::sync::atomic::AtomicPtr { const KIND: MemberKind = MemberKind::Object; + const ATOMIC: bool = true; } /// Where `PyMemberDef.offset` points. @@ -598,26 +635,37 @@ pub struct PyMemberDefLayout { _name: *const core::ffi::c_char, _type: core::ffi::c_int, _offset: isize, - _flags: core::ffi::c_int, + _flags: PyMemberFlags, _doc: *const core::ffi::c_char, } +const _: () = assert!( + size_of::() == size_of::() + && align_of::() == align_of::() +); + /// Same fields as `PyMemberDef`: name, type, offset, flags, doc. pub struct PyMemberDef { pub name: String, pub kind: MemberKind, pub offset: isize, - pub flags: i32, + pub flags: PyMemberFlags, pub doc: ItemDoc, } impl PyMemberDef { pub(crate) fn readonly(&self) -> bool { - self.flags & PY_READONLY != 0 + self.flags.contains(PyMemberFlags::READONLY) + } + + /// Atomic load when the field is atomic storage. Writable members are + /// stored as cells even when an extension did not set [`PyMemberFlags::ATOMIC`]. + pub(crate) fn atomic_storage(&self) -> bool { + self.flags.contains(PyMemberFlags::ATOMIC) || !self.readonly() } pub(crate) fn audit_read(&self) -> bool { - self.flags & PY_AUDIT_READ != 0 + self.flags.contains(PyMemberFlags::AUDIT_READ) } } @@ -639,7 +687,7 @@ pub struct PyMemberSpec { pub name: &'static str, pub kind: MemberKind, pub offset: isize, - pub flags: i32, + pub flags: PyMemberFlags, pub doc: ItemDoc, } @@ -651,7 +699,7 @@ impl PyMemberSpec { name: "", kind: MemberKind::Object, offset: 0, - flags: 0, + flags: PyMemberFlags::empty(), doc: ItemDoc::NONE, }; let mut out = [EMPTY; N]; @@ -707,7 +755,7 @@ impl PyMemberDescriptor { } fn get(&self, obj: PyObjectRef, vm: &VirtualMachine) -> PyResult { - if self.member.flags & PY_AUDIT_READ != 0 { + if self.member.audit_read() { vm.audit("object.__getattr__", || { (obj.clone(), vm.ctx.new_str(self.member.name.as_str())) })?; @@ -836,91 +884,87 @@ fn warn_member(vm: &VirtualMachine, message: &str) -> PyResult<()> { ) } -fn load_i32(obj: &PyObject, offset: isize, readonly: bool) -> i32 { +fn load_i32(obj: &PyObject, offset: isize, atomic: bool) -> i32 { let addr = member_addr(obj, offset); - if readonly { - // SAFETY: a readonly int member addresses an `i32` that is not written - // after publication. - unsafe { addr.cast::().read() } - } else { - // SAFETY: a writable int member addresses an aligned `AtomicI32`. + if atomic { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned `AtomicI32`. unsafe { (*addr.cast::()) .load(core::sync::atomic::Ordering::Relaxed) } + } else { + // SAFETY: a plain int member addresses an `i32` that is not written + // after publication. The field may be only 4-byte aligned. + unsafe { addr.cast::().read() } } } -fn load_i16(obj: &PyObject, offset: isize, readonly: bool) -> i16 { +fn load_i16(obj: &PyObject, offset: isize, atomic: bool) -> i16 { let addr = member_addr(obj, offset); - if readonly { - // SAFETY: a readonly short member addresses an `i16` that is not written - // after publication. - unsafe { addr.cast::().read() } - } else { - // SAFETY: a writable short member addresses an aligned `AtomicI16`. + if atomic { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned `AtomicI16`. unsafe { (*addr.cast::()) .load(core::sync::atomic::Ordering::Relaxed) } + } else { + // SAFETY: a plain short member addresses an `i16` that is not written + // after publication. + unsafe { addr.cast::().read() } } } -fn load_u16(obj: &PyObject, offset: isize, readonly: bool) -> u16 { +fn load_u16(obj: &PyObject, offset: isize, atomic: bool) -> u16 { let addr = member_addr(obj, offset); - if readonly { - // SAFETY: a readonly unsigned short member addresses a `u16` that is not - // written after publication. - unsafe { addr.cast::().read() } - } else { - // SAFETY: a writable unsigned short member addresses an aligned `AtomicU16`. + if atomic { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned `AtomicU16`. unsafe { (*addr.cast::()) .load(core::sync::atomic::Ordering::Relaxed) } + } else { + // SAFETY: a plain unsigned short member addresses a `u16` that is not + // written after publication. + unsafe { addr.cast::().read() } } } -fn load_i64(obj: &PyObject, offset: isize, readonly: bool) -> i64 { +fn load_i64(obj: &PyObject, offset: isize, atomic: bool) -> i64 { let addr = member_addr(obj, offset); - if readonly { - // SAFETY: a readonly `long long` member addresses an `i64` that is not - // written after publication. - unsafe { addr.cast::().read() } - } else { - // SAFETY: a writable `long long` member addresses an aligned `AtomicI64`. + if atomic { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned `AtomicI64`. unsafe { (*addr.cast::()) .load(core::sync::atomic::Ordering::Relaxed) } + } else { + // SAFETY: a plain `long long` member addresses an `i64` that is not + // written after publication. The field may be only 4-byte aligned. + unsafe { addr.cast::().read() } } } -fn load_u64(obj: &PyObject, offset: isize, readonly: bool) -> u64 { +fn load_u64(obj: &PyObject, offset: isize, atomic: bool) -> u64 { let addr = member_addr(obj, offset); - if readonly { - // SAFETY: a readonly `unsigned long long` member is a `u64` or an - // `AtomicU64` of the same size and alignment. A relaxed atomic load - // reads either; a plain integer is not written after publication, and - // an atomic word (type flags) is updated in place. + if atomic { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned `AtomicU64`. unsafe { (*addr.cast::()) .load(core::sync::atomic::Ordering::Relaxed) } } else { - // SAFETY: a writable `unsigned long long` member addresses an aligned `AtomicU64`. - unsafe { - (*addr.cast::()) - .load(core::sync::atomic::Ordering::Relaxed) - } + // SAFETY: a plain `unsigned long long` member addresses a `u64` that is + // not written after publication. The field may be only 4-byte aligned, + // so this is a plain read and not an `AtomicU64` access. + unsafe { addr.cast::().read() } } } #[allow(clippy::unnecessary_cast)] // `c_long` is `i32` or `i64` -fn load_c_long(obj: &PyObject, offset: isize, readonly: bool) -> core::ffi::c_long { +fn load_c_long(obj: &PyObject, offset: isize, atomic: bool) -> core::ffi::c_long { let addr = member_addr(obj, offset); - if readonly { - // SAFETY: a readonly `long` member addresses a `c_long` that is not + if !atomic { + // SAFETY: a plain `long` member addresses a `c_long` that is not // written after publication. unsafe { addr.cast::().read() } } else if size_of::() == 8 { @@ -961,20 +1005,12 @@ fn store_c_long(obj: &PyObject, offset: isize, value: core::ffi::c_long) { } #[allow(clippy::unnecessary_cast)] // `c_ulong` is `u32` or `u64` -fn load_c_ulong(obj: &PyObject, offset: isize, readonly: bool) -> core::ffi::c_ulong { +fn load_c_ulong(obj: &PyObject, offset: isize, atomic: bool) -> core::ffi::c_ulong { let addr = member_addr(obj, offset); - if readonly && size_of::() == 8 { - // SAFETY: a readonly `unsigned long` member is a `c_ulong` or an - // `AtomicU64` of the same size and alignment. A relaxed atomic load - // reads either; a plain integer is not written after publication, and - // an atomic word (type flags) is updated in place. - unsafe { - (*addr.cast::()) - .load(core::sync::atomic::Ordering::Relaxed) as core::ffi::c_ulong - } - } else if readonly { - // SAFETY: a readonly `unsigned long` member addresses a `c_ulong` that - // is not written after publication. `c_ulong` is 4 bytes on this target. + if !atomic { + // SAFETY: a plain `unsigned long` member addresses a `c_ulong` that is + // not written after publication. A plain integer may be only 4-byte + // aligned, so this is not an atomic access. unsafe { addr.cast::().read() } } else if size_of::() == 8 { // SAFETY: a writable `unsigned long` member addresses an aligned `AtomicU64`. @@ -1013,18 +1049,18 @@ fn store_c_ulong(obj: &PyObject, offset: isize, value: core::ffi::c_ulong) { } } -fn load_u32(obj: &PyObject, offset: isize, readonly: bool) -> u32 { +fn load_u32(obj: &PyObject, offset: isize, atomic: bool) -> u32 { let addr = member_addr(obj, offset); - if readonly { - // SAFETY: a readonly uint member addresses a `u32` that is not written - // after publication. - unsafe { addr.cast::().read() } - } else { - // SAFETY: a writable uint member addresses an aligned `AtomicU32`. + if atomic { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned `AtomicU32`. unsafe { (*addr.cast::()) .load(core::sync::atomic::Ordering::Relaxed) } + } else { + // SAFETY: a plain uint member addresses a `u32` that is not written + // after publication. + unsafe { addr.cast::().read() } } } @@ -1075,78 +1111,82 @@ fn member_get_one( } }, MemberKind::Bool => { - // SAFETY: a bool member addresses an `AtomicBool` or a `bool` - // (one byte, naturally aligned). The macro rejects any other field - // type. A plain `bool` is not written after the object is published. - let raw = unsafe { - (*member_addr(obj, offset).cast::()) - .load(core::sync::atomic::Ordering::Relaxed) + let raw = if member.atomic_storage() { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned + // `AtomicBool`. + unsafe { + (*member_addr(obj, offset).cast::()) + .load(core::sync::atomic::Ordering::Relaxed) + } + } else { + // SAFETY: a plain bool member is one byte and is not written + // after publication. + unsafe { member_addr(obj, offset).cast::().read() } }; vm.ctx.new_bool(raw).into() } MemberKind::Int => vm .ctx - .new_int(load_i32(obj, offset, member.readonly())) + .new_int(load_i32(obj, offset, member.atomic_storage())) .into(), MemberKind::Uint => vm .ctx - .new_int(load_u32(obj, offset, member.readonly())) + .new_int(load_u32(obj, offset, member.atomic_storage())) .into(), MemberKind::Double => { - let raw = if member.readonly() { - // SAFETY: a readonly double member addresses an `f64` that is - // not written after publication. A plain `f64` may be only - // 4-byte aligned, so this is a plain read and not an atomic - // access. - unsafe { member_addr(obj, offset).cast::().read() } - } else { - // SAFETY: a writable double member addresses an aligned - // `AtomicF64`. The macro accepts only `MemberCell`. + let raw = if member.atomic_storage() { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned + // `AtomicF64`. unsafe { (*member_addr(obj, offset).cast::()) .load(core::sync::atomic::Ordering::Relaxed) } + } else { + // SAFETY: a plain double member addresses an `f64` that is not + // written after publication. A plain `f64` may be only 4-byte + // aligned, so this is a plain read and not an atomic access. + unsafe { member_addr(obj, offset).cast::().read() } }; vm.ctx.new_float(raw).into() } MemberKind::PySsizeT => { - let raw = if member.readonly() { - // SAFETY: a readonly `Py_ssize_t` member addresses an `isize` - // that is not written after publication. - unsafe { member_addr(obj, offset).cast::().read() } - } else { - // SAFETY: a writable `Py_ssize_t` member addresses an aligned - // `AtomicIsize`. The macro accepts only `MemberCell`. + let raw = if member.atomic_storage() { + // SAFETY: `PyMemberFlags::ATOMIC` or a writable member addresses an aligned + // `AtomicIsize`. unsafe { (*member_addr(obj, offset).cast::()) .load(core::sync::atomic::Ordering::Relaxed) } + } else { + // SAFETY: a plain `Py_ssize_t` member addresses an `isize` that + // is not written after publication. + unsafe { member_addr(obj, offset).cast::().read() } }; vm.ctx.new_int(raw).into() } MemberKind::Short => vm .ctx - .new_int(load_i16(obj, offset, member.readonly())) + .new_int(load_i16(obj, offset, member.atomic_storage())) .into(), MemberKind::UShort => vm .ctx - .new_int(load_u16(obj, offset, member.readonly())) + .new_int(load_u16(obj, offset, member.atomic_storage())) .into(), MemberKind::Long => vm .ctx - .new_int(load_c_long(obj, offset, member.readonly())) + .new_int(load_c_long(obj, offset, member.atomic_storage())) .into(), MemberKind::LongLong => vm .ctx - .new_int(load_i64(obj, offset, member.readonly())) + .new_int(load_i64(obj, offset, member.atomic_storage())) .into(), MemberKind::ULong => vm .ctx - .new_int(load_c_ulong(obj, offset, member.readonly())) + .new_int(load_c_ulong(obj, offset, member.atomic_storage())) .into(), MemberKind::ULongLong => vm .ctx - .new_int(load_u64(obj, offset, member.readonly())) + .new_int(load_u64(obj, offset, member.atomic_storage())) .into(), MemberKind::String => { // SAFETY: a string member addresses a `*const c_char` that is not diff --git a/crates/vm/src/builtins/genericalias.rs b/crates/vm/src/builtins/genericalias.rs index 59ec0e430..9dfcfd2e1 100644 --- a/crates/vm/src/builtins/genericalias.rs +++ b/crates/vm/src/builtins/genericalias.rs @@ -562,13 +562,18 @@ pub(crate) fn subs_parameters( }; if unpack { - let arg_type = arg.class().fully_qualified_name(vm)?; - let substituted_type = substituted_arg.class().fully_qualified_name(vm)?; - let tuple = substituted_arg.try_to_ref::(vm).map_err(|_| { - vm.new_type_error(format!( - "expected __typing_subst__ of {arg_type} objects to return a tuple, not {substituted_type}", - )) - })?; + // Names are read only for the error. A failing `__module__` lookup + // must not reject a substitution that did return a tuple. + let tuple = match substituted_arg.try_to_ref::(vm) { + Ok(tuple) => tuple, + Err(_) => { + let arg_type = arg.class().fully_qualified_name(vm)?; + let substituted_type = substituted_arg.class().fully_qualified_name(vm)?; + return Err(vm.new_type_error(format!( + "expected __typing_subst__ of {arg_type} objects to return a tuple, not {substituted_type}", + ))); + } + }; for elem in tuple { new_args.push(elem.clone()); } diff --git a/crates/vm/src/builtins/mod.rs b/crates/vm/src/builtins/mod.rs index f74660c3a..c7404fcd1 100644 --- a/crates/vm/src/builtins/mod.rs +++ b/crates/vm/src/builtins/mod.rs @@ -100,8 +100,8 @@ pub(crate) mod union_; pub use union_::{PyUnion, make_union}; pub mod descriptor; pub use descriptor::{ - MemberAccess, MemberKind, PY_READONLY, PY_RELATIVE_OFFSET, PyDescriptorOwned, - PyMemberDef as DescriptorMemberDef, PyMemberDescriptor, + MemberAccess, MemberKind, PyDescriptorOwned, PyMemberDef as DescriptorMemberDef, + PyMemberDescriptor, PyMemberFlags, }; pub use float::float_from_string as parse_float_from_string; diff --git a/crates/vm/src/builtins/type.rs b/crates/vm/src/builtins/type.rs index 91f10b267..e2d421e51 100644 --- a/crates/vm/src/builtins/type.rs +++ b/crates/vm/src/builtins/type.rs @@ -9,6 +9,7 @@ use crate::{ PyBaseExceptionRef, descriptor::{ MemberAccess, MemberKind, PyDescriptorOwned, PyMemberDef, PyMemberDescriptor, + PyMemberFlags, }, function::{PyCellRef, PyFunction}, tuple::{IntoPyTuple, PyTuple}, @@ -2772,7 +2773,7 @@ impl Constructor for PyType { name: mangled_name.clone(), kind: MemberKind::ObjectEx, offset: crate::object::slot_member_offset(offset), - flags: 0, + flags: PyMemberFlags::empty(), doc: ItemDoc::NONE, }; let attr_name = vm.ctx.intern_str(mangled_name.as_str()); diff --git a/crates/vm/src/stdlib/sys.rs b/crates/vm/src/stdlib/sys.rs index 6914d6243..138a60909 100644 --- a/crates/vm/src/stdlib/sys.rs +++ b/crates/vm/src/stdlib/sys.rs @@ -1186,15 +1186,20 @@ pub mod sys { .fast_issubclass(vm.ctx.exceptions.base_exception_type) ); - // Print module name (if not builtins or __main__) - let module_name = unraisable.exc_type.__module__(vm)?; - if let Ok(module_str) = module_name.downcast::() { + // Print module name (if not builtins or __main__). + // A failed lookup or a non-str `__module__` is discarded: print + // `` with no trailing `.`, then the qualname. + let module_name = unraisable.exc_type.__module__(vm).ok(); + if let Some(module_str) = module_name + .as_ref() + .and_then(|name| name.downcast_ref::()) + { let module = module_str.as_wtf8(); if module != "builtins" && module != "__main__" { write!(stderr, "{module}."); } } else { - write!(stderr, "."); + write!(stderr, ""); } // Print qualname diff --git a/crates/vm/src/types/slot.rs b/crates/vm/src/types/slot.rs index f838ff4c1..65011572b 100644 --- a/crates/vm/src/types/slot.rs +++ b/crates/vm/src/types/slot.rs @@ -385,13 +385,13 @@ impl core::ops::BitOrAssign for PyAtomicTypeFlags { } } +// `__flags__` is `PyMemberFlags::ATOMIC`, so the load reads this field as an `AtomicU64`. +// A plain `u64` may be less aligned; the load does not use that alignment. const _: () = assert!( core::mem::size_of::() == core::mem::size_of::() && core::mem::align_of::() == core::mem::align_of::() - && core::mem::size_of::() == core::mem::size_of::() - && core::mem::align_of::() == core::mem::align_of::() ); impl crate::builtins::descriptor::MemberLayout for PyAtomicTypeFlags { @@ -402,6 +402,7 @@ impl crate::builtins::descriptor::MemberLayout for PyAtomicTypeFlags { crate::builtins::descriptor::MemberKind::ULongLong } }; + const ATOMIC: bool = true; } pub(crate) type GenericMethod = fn(&PyObject, FuncArgs, &VirtualMachine) -> PyResult; diff --git a/crates/vm/src/vm/context.rs b/crates/vm/src/vm/context.rs index 781525ad7..c3c744e88 100644 --- a/crates/vm/src/vm/context.rs +++ b/crates/vm/src/vm/context.rs @@ -7,8 +7,8 @@ use crate::{ bool_::PyBool, code::{self, PyCode}, descriptor::{ - MemberAccess, MemberKind, PY_READONLY, PyDescriptorOwned, PyMemberDef, - PyMemberDescriptor, + MemberAccess, MemberKind, PyDescriptorOwned, PyMemberDef, PyMemberDescriptor, + PyMemberFlags, }, getset::PyGetSet, object, pystr, @@ -668,7 +668,7 @@ impl Context { name: &str, kind: MemberKind, offset: isize, - flags: i32, + flags: PyMemberFlags, class: &'static Py, doc: ItemDoc, ) -> PyRef { @@ -706,7 +706,7 @@ impl Context { name: name.to_owned(), kind: MemberKind::Object, offset: index as isize, - flags: PY_READONLY, + flags: PyMemberFlags::READONLY, doc: ItemDoc::NONE, }, access: MemberAccess::TupleItem,