Separate WeakRefList from ObjExt prefix#7365
Conversation
📝 WalkthroughWalkthroughAdds a MANAGED_WEAKREF type flag and propagates it alongside HAS_WEAKREF during type creation. Introduces an optional two-prefix object layout (ObjExt and WeakRefList) and updates allocation, deallocation, and weakref accessors to compute and handle these prefixes. Changes
Sequence Diagram(s)sequenceDiagram
participant TypeCreator as TypeCreator
participant Allocator as Allocator
participant Memory as PyInner
participant ObjExt as ObjExt
participant WeakRef as WeakRefList
TypeCreator->>Allocator: request allocation (flags include HAS_WEAKREF?/MANAGED_WEAKREF?)
Allocator->>Allocator: decide layout ([ObjExt?][WeakRefList?][PyInner])
Allocator->>Memory: allocate bytes for layout
Allocator->>ObjExt: initialize ObjExt (if present)
Allocator->>WeakRef: initialize WeakRefList (if present)
Allocator->>TypeCreator: return object pointer + provenance
TypeCreator->>Memory: set slots/flags (ensure MANAGED_WEAKREF when HAS_WEAKREF)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin managed-weakref |
43df2a2 to
28cc5f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/object/core.rs (1)
2426-2441: Use the computed weakref offset in the bootstrap allocator.
alloc_type_with_prefixes()already asksLayout::extend()where theWeakRefListlands, but then writes it atalloc_ptr.add(EXT_OFFSET)anyway. That duplicates the no-padding assumption instead of reusing the actual layout result, so this path can drift first ifWeakRefList's alignment ever changes.♻️ Proposed change
- let (layout, _ext_offset) = ext_layout.extend(weakref_layout).unwrap(); + let (layout, weakref_offset) = ext_layout.extend(weakref_layout).unwrap(); let (combined, inner_offset) = layout.extend(inner_layout).unwrap(); let combined = combined.pad_to_align(); @@ - let weakref_ptr = alloc_ptr.add(EXT_OFFSET) as *mut WeakRefList; + let weakref_ptr = alloc_ptr.add(weakref_offset) as *mut WeakRefList; weakref_ptr.write(WeakRefList::new());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/object/core.rs` around lines 2426 - 2441, The code ignores the computed weakref offset and uses the hardcoded EXT_OFFSET; change the binding let (layout, _ext_offset) = ext_layout.extend(weakref_layout).unwrap() to capture the offset (e.g. ext_offset) and use that value when computing the weakref pointer (replace alloc_ptr.add(EXT_OFFSET) with alloc_ptr.add(ext_offset as usize) or equivalent), so alloc_type_with_prefixes() uses the actual layout result; ensure the variable name matches uses and types (usize) for pointer arithmetic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/builtins/type.rs`:
- Around line 595-600: The final type-flag assembly must always normalize
MANAGED_WEAKREF from HAS_WEAKREF regardless of which branch set it; after
assembling flags (e.g., following the MRO inheritance block that touches
mro.iter() and slots.flags and the other similar branches), add a
post-normalization step that if slots.flags contains PyTypeFlags::HAS_WEAKREF
then also |= PyTypeFlags::MANAGED_WEAKREF so static/builtin types that declare
HAS_WEAKREF (and the type objects given a managed weakref preheader in
object/core.rs) get a consistent MANAGED_WEAKREF bit; apply the same
post-normalization in the other flag-assembly sites referenced (the branches
around the other occurrences).
---
Nitpick comments:
In `@crates/vm/src/object/core.rs`:
- Around line 2426-2441: The code ignores the computed weakref offset and uses
the hardcoded EXT_OFFSET; change the binding let (layout, _ext_offset) =
ext_layout.extend(weakref_layout).unwrap() to capture the offset (e.g.
ext_offset) and use that value when computing the weakref pointer (replace
alloc_ptr.add(EXT_OFFSET) with alloc_ptr.add(ext_offset as usize) or
equivalent), so alloc_type_with_prefixes() uses the actual layout result; ensure
the variable name matches uses and types (usize) for pointer arithmetic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b04db21e-8745-4b0c-84a5-7a043c705bf1
📒 Files selected for processing (4)
crates/vm/src/builtins/type.rscrates/vm/src/object/core.rscrates/vm/src/object/traverse_object.rscrates/vm/src/types/slot.rs
💤 Files with no reviewable changes (1)
- crates/vm/src/object/traverse_object.rs
28cc5f5 to
4ee068e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/object/core.rs`:
- Around line 322-335: The hard-coded WEAKREF_OFFSET/EXT_OFFSET assume no
padding between prefix structs and PyInner<T>, which can be false on some
targets; replace the fixed size_of::<WeakRefList>() / size_of::<ObjExt>()
offsets with offsets computed via core::alloc::Layout::extend (derive the offset
from Layout::new::<WeakRefList>().extend(Layout::new::<PyInner<()>>()) and
similarly for ObjExt) and use that computed offset in weakref_list_ref(),
ext_ref(), gc_clear_raw() (and the other occurrences around lines 395-410) so
the pointer arithmetic accounts for any padding/alignment introduced by
Layout::extend instead of assuming a padding-free layout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1f65fb19-7d09-4139-becf-956e7c4afea6
📒 Files selected for processing (4)
crates/vm/src/builtins/type.rscrates/vm/src/object/core.rscrates/vm/src/object/traverse_object.rscrates/vm/src/types/slot.rs
💤 Files with no reviewable changes (1)
- crates/vm/src/object/traverse_object.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/vm/src/types/slot.rs
- crates/vm/src/builtins/type.rs
4ee068e to
59ae6f1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/vm/src/object/core.rs (1)
322-335:⚠️ Potential issue | 🟠 MajorDerive the prefix offsets from
Layout::extend, notsize_of.The fixed
EXT_OFFSET/WEAKREF_OFFSETmath is still only proven againstPyInner<()>, but the unsafe consumers here operate on arbitraryPyInner<T>. If any payload pushesalign_of::<PyInner<T>>()above 8, these subtractions land before the real prefix and turnext_ref(),weakref_list_ref(), andgc_clear_raw()into invalid reads/writes. Please compute the negative offsets fromLayout::extend(..., Layout::new::<Self>())instead of hard-codingsize_of::<ObjExt>()/size_of::<WeakRefList>().Also applies to: 376-392, 395-410, 1759-1772
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/object/core.rs` around lines 322 - 335, Compute the prefix offsets using Layout::extend rather than size_of so they account for arbitrary PyInner<T> alignments: replace the hard-coded EXT_OFFSET and WEAKREF_OFFSET calculations with values derived from Layout::extend(Layout::new::<ObjExt>(), Layout::new::<PyInner<()>>()) (and similarly for WeakRefList) then subtract the size/offset returned by extend for the PyInner layout to produce the negative offsets used by ext_ref(), weakref_list_ref(), and gc_clear_raw(); ensure the new asserts validate using Layout::extend results (referencing EXT_OFFSET, WEAKREF_OFFSET, ObjExt, WeakRefList, PyInner, ext_ref, weakref_list_ref, gc_clear_raw) so offsets remain correct for any PyInner<T> alignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/vm/src/object/core.rs`:
- Around line 322-335: Compute the prefix offsets using Layout::extend rather
than size_of so they account for arbitrary PyInner<T> alignments: replace the
hard-coded EXT_OFFSET and WEAKREF_OFFSET calculations with values derived from
Layout::extend(Layout::new::<ObjExt>(), Layout::new::<PyInner<()>>()) (and
similarly for WeakRefList) then subtract the size/offset returned by extend for
the PyInner layout to produce the negative offsets used by ext_ref(),
weakref_list_ref(), and gc_clear_raw(); ensure the new asserts validate using
Layout::extend results (referencing EXT_OFFSET, WEAKREF_OFFSET, ObjExt,
WeakRefList, PyInner, ext_ref, weakref_list_ref, gc_clear_raw) so offsets remain
correct for any PyInner<T> alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d464627d-f6f4-4525-a7e3-e275a434bf56
📒 Files selected for processing (4)
crates/vm/src/builtins/type.rscrates/vm/src/object/core.rscrates/vm/src/object/traverse_object.rscrates/vm/src/types/slot.rs
💤 Files with no reviewable changes (1)
- crates/vm/src/object/traverse_object.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/types/slot.rs
- Remove weak_list from ObjExt, allocate WeakRefList as its own prefix slot before PyInner - Add MANAGED_WEAKREF flag (1 << 3) to PyTypeFlags - Normalize MANAGED_WEAKREF from HAS_WEAKREF after flag assembly - Use Layout::extend offsets in bootstrap allocator
59ae6f1 to
3913928
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/vm/src/object/core.rs (1)
322-326:⚠️ Potential issue | 🔴 CriticalThe prefix offsets are still assuming a padding-free layout.
new()/dealloc()build the allocation withLayout::extend, but the read/clear paths recover prefixes by subtractingsize_of::<WeakRefList>()/size_of::<ObjExt>(). That is only correct whenLayout::extendinserts no padding beforePyInner<T>. For a more-aligned payload,inner_offsetcan exceed those raw sizes, soext_ref(),weakref_list_ref(), andgc_clear_raw()can read the wrong address.💡 Minimal direction
-const EXT_OFFSET: usize = core::mem::size_of::<ObjExt>(); -const WEAKREF_OFFSET: usize = core::mem::size_of::<WeakRefList>(); +#[inline(always)] +fn weakref_to_inner_offset<T>() -> usize { + core::alloc::Layout::new::<WeakRefList>() + .extend(core::alloc::Layout::new::<PyInner<T>>()) + .unwrap() + .1 +} + +#[inline(always)] +fn ext_to_inner_offset<T>(has_weakref: bool) -> usize { + if has_weakref { + let (layout, _) = core::alloc::Layout::new::<ObjExt>() + .extend(core::alloc::Layout::new::<WeakRefList>()) + .unwrap(); + layout.extend(core::alloc::Layout::new::<PyInner<T>>()).unwrap().1 + } else { + core::alloc::Layout::new::<ObjExt>() + .extend(core::alloc::Layout::new::<PyInner<T>>()) + .unwrap() + .1 + } +}Use those computed offsets everywhere prefix pointers are reconstructed.
Also applies to: 376-410, 1759-1772
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/object/core.rs` around lines 322 - 326, The code incorrectly reconstructs prefix pointer addresses by subtracting raw sizes (size_of::<ObjExt>() / size_of::<WeakRefList>()) instead of using the computed allocation offsets—update all places that recover prefixes (e.g., in new(), dealloc(), ext_ref(), weakref_list_ref(), gc_clear_raw(), and the similar blocks around lines 376-410 and 1759-1772) to use the precomputed inner_offset/EXT_OFFSET/WEAKREF_OFFSET values produced when building the Layout (or the computed inner_offset variable) so the pointer arithmetic accounts for any padding inserted by Layout::extend; replace any direct size_of::<...>() subtractions with the corresponding computed offset variables when reconstructing prefix pointers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/vm/src/object/core.rs`:
- Around line 322-326: The code incorrectly reconstructs prefix pointer
addresses by subtracting raw sizes (size_of::<ObjExt>() /
size_of::<WeakRefList>()) instead of using the computed allocation
offsets—update all places that recover prefixes (e.g., in new(), dealloc(),
ext_ref(), weakref_list_ref(), gc_clear_raw(), and the similar blocks around
lines 376-410 and 1759-1772) to use the precomputed
inner_offset/EXT_OFFSET/WEAKREF_OFFSET values produced when building the Layout
(or the computed inner_offset variable) so the pointer arithmetic accounts for
any padding inserted by Layout::extend; replace any direct size_of::<...>()
subtractions with the corresponding computed offset variables when
reconstructing prefix pointers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f4b18d16-96f9-4050-87d9-aaf7f3cbaabe
📒 Files selected for processing (4)
crates/vm/src/builtins/type.rscrates/vm/src/object/core.rscrates/vm/src/object/traverse_object.rscrates/vm/src/types/slot.rs
💤 Files with no reviewable changes (1)
- crates/vm/src/object/traverse_object.rs
Summary
ObjExtprefix into two independent prefixes:ObjExt(dict + slots) andWeakRefList(weakref support)MANAGED_WEAKREFflag (1 << 3) matching CPython'sPy_TPFLAGS_MANAGED_WEAKREFMemory layout
Test plan
cargo test --workspacepassestest_weakrefpasses (137 tests)test_gc,test_class,test_descrpassSummary by CodeRabbit