From dd0e57ec5e15f3707a2b30076c55ea0e7b620e61 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Mon, 17 Aug 2026 10:06:17 +0900 Subject: [PATCH 1/4] _ast: address the converted tree's values by shadow slot `module_to_object` evaluates a node's field array left to right and every field's construction allocates, so a list built for an earlier field sat in the array as a pointer that a collection under a later sibling had left behind. `Converter::pin` now returns `Rooted`, the shadow slot the value was published at, and every producer in the ast-to-object direction returns `Rooted` in place of `PyObjectRef`. `node` reads each field out of its slot at the `setattr_str` that stores it, `list` reads its members back at the `w_list_new` that pins them, and the call sites that built a value from a `pyre_object::w_*` constructor publish it the same way. `pin_slot` and the note recording the residual window are gone. `ast.dump`, `ast.unparse` and `compile()` over 36 sources fail under `PYPY_GC_NURSERY=4096` and `=1` before this commit -- the first with `TypeError: descriptor '__iter__' requires a 'list' object but received a 'list'` -- and pass after. The object-to-ast direction copies a list's members out as bare pointers too, but no stress level made it fail. Assisted-by: Claude --- .../src/module/_ast/convert.rs | 272 +++++++++--------- 1 file changed, 136 insertions(+), 136 deletions(-) diff --git a/pyre/pyre-interpreter/src/module/_ast/convert.rs b/pyre/pyre-interpreter/src/module/_ast/convert.rs index 0e877e599bb..1277759ba54 100644 --- a/pyre/pyre-interpreter/src/module/_ast/convert.rs +++ b/pyre/pyre-interpreter/src/module/_ast/convert.rs @@ -1732,12 +1732,13 @@ fn module_to_object( module: ast::Mod, source: &str, mode: crate::compile::Mode, - ast_module: PyObjectRef, + module_object: PyObjectRef, ) -> crate::PyResult { let _roots = pyre_object::gc_roots::push_roots(); - pyre_object::gc_roots::pin_root(ast_module); + let ast_module = Rooted(pyre_object::gc_roots::shadow_stack_len()); + pyre_object::gc_roots::pin_root(module_object); let converter = Converter { source, ast_module }; - match module { + let root = match module { ast::Mod::Expression(module) => converter.node( "Expression", None, @@ -1749,96 +1750,91 @@ fn module_to_object( } else { "Module" }; - // `converter.list` allocates the `type_ignores` list, so read the - // body back from its own slot after that sibling is built. - let body_slot = converter.pin_slot(converter.stmt_list(&module.body)?); + let body = converter.stmt_list(&module.body)?; if root_name == "Module" { let type_ignores = converter.list(Vec::new()); converter.node( root_name, None, - &[ - ("body", pyre_object::gc_roots::shadow_stack_get(body_slot)), - ("type_ignores", type_ignores), - ], + &[("body", body), ("type_ignores", type_ignores)], ) } else { - converter.node( - root_name, - None, - &[("body", pyre_object::gc_roots::shadow_stack_get(body_slot))], - ) + converter.node(root_name, None, &[("body", body)]) } } + }?; + Ok(root.get()) +} + +/// A value published in [`module_to_object`]'s root scope, held as its shadow +/// slot rather than as a pointer. +/// +/// Every value the tree is built from is produced by a call that allocates, and +/// a node's fields are produced one after another before the node exists to +/// hold any of them: a `PyObjectRef` copy of the first field addresses the +/// pre-move object by the time the last one is built. Only the slot survives +/// that, so nothing here passes a bare `PyObjectRef` around — a value is read +/// out of its slot at the point it is used and nowhere earlier. +#[derive(Clone, Copy)] +struct Rooted(usize); + +impl Rooted { + fn get(self) -> PyObjectRef { + pyre_object::gc_roots::shadow_stack_get(self.0) } } +type RootedResult = Result; + struct Converter<'a> { source: &'a str, - ast_module: PyObjectRef, + ast_module: Rooted, } impl Converter<'_> { - /// Publish `value` as a root of `module_to_object`'s scope and hand back - /// the published slot's contents, not the caller's copy — the pin is what - /// makes the collector forward it, and reading the pre-pin local back would - /// discard that forwarding. - /// - /// STILL OPEN: the value is fresh when it is returned, but a caller that - /// builds a `&[(&str, PyObjectRef)]` field array allocates for the sibling - /// elements before [`Converter::node`] receives it, and only the shadow - /// slot is forwarded across that window. Lists are the only movable field - /// values here (nodes are instances, and the rest are str/int/None), so - /// closing it means having the list-producing helpers return slots and - /// `node` take them — a change across all 77 `node` call sites, not a - /// rooting patch. Reproduces only under `PYPY_GC_NURSERY=1` - /// (`ast.unparse` emits invalid source); the default nursery is unaffected. - fn pin(&self, value: PyObjectRef) -> PyObjectRef { - let slot = self.pin_slot(value); - pyre_object::gc_roots::shadow_stack_get(slot) - } - - /// `pin`, returning the slot index so a caller that runs Python between the - /// pin and the use can re-read the value at each use. - fn pin_slot(&self, value: PyObjectRef) -> usize { + /// Publish `value` as a root of `module_to_object`'s scope. The pin is + /// what makes the collector forward it; the slot is how a later use finds + /// where it was forwarded to. + fn pin(&self, value: PyObjectRef) -> Rooted { let slot = pyre_object::gc_roots::shadow_stack_len(); pyre_object::gc_roots::pin_root(value); - slot + Rooted(slot) } - fn list(&self, values: Vec) -> PyObjectRef { - self.pin(pyre_object::w_list_new(values)) + fn list(&self, values: Vec) -> Rooted { + // Read the members back only here: `w_list_new` pins what it is handed, + // so the vector it receives has to be current at the call, and building + // it any earlier would hand over addresses the members have left. + self.pin(pyre_object::w_list_new( + values.into_iter().map(Rooted::get).collect(), + )) } - fn string(&self, value: &str) -> PyObjectRef { + fn string(&self, value: &str) -> Rooted { self.pin(pyre_object::w_str_new(value)) } - fn optional(&self, value: Option) -> PyObjectRef { - value.unwrap_or_else(pyre_object::w_none) + fn none(&self) -> Rooted { + self.pin(pyre_object::w_none()) + } + + fn optional(&self, value: Option) -> Rooted { + value.unwrap_or_else(|| self.none()) } fn node( &self, name: &str, range: Option<(u32, u32)>, - fields: &[(&str, PyObjectRef)], - ) -> crate::PyResult { - let node_type = crate::baseobjspace::getattr_str(self.ast_module, name)?; - // Every `setattr_str` below runs Python, so the node and the field - // values move under the loop. Publish them and read each back at the - // store that consumes it. - let node_slot = self.pin_slot(pyre_object::w_instance_new(node_type)); - let value_base = pyre_object::gc_roots::shadow_stack_len(); - for &(_, value) in fields { - pyre_object::gc_roots::pin_root(value); - } - for (index, &(field, _)) in fields.iter().enumerate() { - crate::baseobjspace::setattr_str( - pyre_object::gc_roots::shadow_stack_get(node_slot), - field, - pyre_object::gc_roots::shadow_stack_get(value_base + index), - )?; + fields: &[(&str, Rooted)], + ) -> RootedResult { + let node_type = crate::baseobjspace::getattr_str(self.ast_module.get(), name)?; + let node = self.pin(pyre_object::w_instance_new(node_type)); + // Every `setattr_str` below runs Python, so the node and the remaining + // field values move under the loop; each is read at the store that + // consumes it. + for &(field, value) in fields { + crate::baseobjspace::setattr_str(node.get(), field, value.get())?; } if let Some((start, end)) = range { let (lineno, col_offset) = self.location(start as usize); @@ -1852,14 +1848,10 @@ impl Converter<'_> { // Box the position before reading the node back: `w_int_new` // allocates, so a receiver read ahead of it is the pre-move one. let w_value = pyre_object::w_int_new(value as i64); - crate::baseobjspace::setattr_str( - pyre_object::gc_roots::shadow_stack_get(node_slot), - field, - w_value, - )?; + crate::baseobjspace::setattr_str(node.get(), field, w_value)?; } } - Ok(pyre_object::gc_roots::shadow_stack_get(node_slot)) + Ok(node) } fn location(&self, offset: usize) -> (usize, usize) { @@ -1876,7 +1868,7 @@ impl Converter<'_> { ) } - fn stmt_list(&self, stmts: &[ast::Stmt]) -> crate::PyResult { + fn stmt_list(&self, stmts: &[ast::Stmt]) -> RootedResult { stmts .iter() .map(|stmt| self.stmt(stmt)) @@ -1884,7 +1876,7 @@ impl Converter<'_> { .map(|items| self.list(items)) } - fn expr_list(&self, exprs: &[ast::Expr]) -> crate::PyResult { + fn expr_list(&self, exprs: &[ast::Expr]) -> RootedResult { exprs .iter() .map(|expr| self.expr(expr)) @@ -1892,7 +1884,7 @@ impl Converter<'_> { .map(|items| self.list(items)) } - fn name_list>(&self, names: &[T]) -> PyObjectRef { + fn name_list>(&self, names: &[T]) -> Rooted { self.list( names .iter() @@ -1901,7 +1893,7 @@ impl Converter<'_> { ) } - fn stmt(&self, stmt: &ast::Stmt) -> crate::PyResult { + fn stmt(&self, stmt: &ast::Stmt) -> RootedResult { use ast::Stmt; match stmt { Stmt::FunctionDef(node) => { @@ -2030,9 +2022,9 @@ impl Converter<'_> { ), ( "simple", - pyre_object::w_int_new( - node.runtime_simple.unwrap_or(node.simple as i32) as i64 - ), + self.pin(pyre_object::w_int_new( + node.runtime_simple.unwrap_or(node.simple as i32) as i64, + )), ), ], ), @@ -2074,7 +2066,12 @@ impl Converter<'_> { ], )?]; } else { - orelse = unsafe { pyre_object::w_list_items_copy_as_vec(body) }; + // The members come out of the list as bare pointers, so + // each is published before the next clause allocates. + orelse = unsafe { pyre_object::w_list_items_copy_as_vec(body.get()) } + .into_iter() + .map(|item| self.pin(item)) + .collect(); } } self.node( @@ -2150,9 +2147,9 @@ impl Converter<'_> { ("names", self.aliases(&node.names)?), ( "level", - pyre_object::w_int_new( - node.runtime_level.unwrap_or(node.level as i32) as i64 - ), + self.pin(pyre_object::w_int_new( + node.runtime_level.unwrap_or(node.level as i32) as i64, + )), ), ], ), @@ -2195,7 +2192,7 @@ impl Converter<'_> { } } - fn expr(&self, expr: &ast::Expr) -> crate::PyResult { + fn expr(&self, expr: &ast::Expr) -> RootedResult { use ast::Expr; match expr { Expr::BoolOp(n) => self.node( @@ -2358,7 +2355,7 @@ impl Converter<'_> { if n.value.is_unicode() { self.string("u") } else { - pyre_object::w_none() + self.none() }, ), Expr::BytesLiteral(n) => self.constant( @@ -2366,25 +2363,21 @@ impl Converter<'_> { self.pin(pyre_object::w_bytes_from_bytes( &n.value.bytes().collect::>(), )), - pyre_object::w_none(), - ), - Expr::NumberLiteral(n) => self.constant( - range(n.range), - self.number(&n.value)?, - pyre_object::w_none(), + self.none(), ), + Expr::NumberLiteral(n) => { + self.constant(range(n.range), self.number(&n.value)?, self.none()) + } Expr::BooleanLiteral(n) => self.constant( range(n.range), - pyre_object::w_bool_from(n.value), - pyre_object::w_none(), + self.pin(pyre_object::w_bool_from(n.value)), + self.none(), ), - Expr::NoneLiteral(n) => { - self.constant(range(n.range), pyre_object::w_none(), pyre_object::w_none()) - } + Expr::NoneLiteral(n) => self.constant(range(n.range), self.none(), self.none()), Expr::EllipsisLiteral(n) => self.constant( range(n.range), - pyre_object::w_ellipsis(), - pyre_object::w_none(), + self.pin(pyre_object::w_ellipsis()), + self.none(), ), Expr::Constant(n) => self.constant( range(n.range), @@ -2466,7 +2459,7 @@ impl Converter<'_> { } } - fn fstring(&self, node: &ast::ExprFString) -> crate::PyResult { + fn fstring(&self, node: &ast::ExprFString) -> RootedResult { if let Some(values) = node.runtime_joined_str.as_deref() { return self.node( "JoinedStr", @@ -2511,12 +2504,12 @@ impl Converter<'_> { ) } - fn joined_values(&self, parts: Vec) -> Result, crate::PyError> { + fn joined_values(&self, parts: Vec) -> Result, crate::PyError> { parts .into_iter() .map(|part| match part { JoinedPart::Literal { start, end, value } => { - self.constant((start, end), self.string(&value), pyre_object::w_none()) + self.constant((start, end), self.string(&value), self.none()) } JoinedPart::Value(value) => Ok(value), }) @@ -2568,7 +2561,7 @@ impl Converter<'_> { ("value", self.expr(&interpolation.expression)?), ( "conversion", - pyre_object::w_int_new(conversion as i8 as i64), + self.pin(pyre_object::w_int_new(conversion as i8 as i64)), ), ("format_spec", self.optional(format_spec)), ], @@ -2605,7 +2598,7 @@ impl Converter<'_> { ); } - fn match_case(&self, case: &ast::MatchCase) -> crate::PyResult { + fn match_case(&self, case: &ast::MatchCase) -> RootedResult { self.node( "match_case", None, @@ -2620,7 +2613,7 @@ impl Converter<'_> { ) } - fn pattern(&self, pattern: &ast::Pattern) -> crate::PyResult { + fn pattern(&self, pattern: &ast::Pattern) -> RootedResult { match pattern { ast::Pattern::MatchValue(node) => self.node( "MatchValue", @@ -2633,9 +2626,9 @@ impl Converter<'_> { &[( "value", match node.value { - ast::Singleton::None => pyre_object::w_none(), - ast::Singleton::True => pyre_object::w_bool_from(true), - ast::Singleton::False => pyre_object::w_bool_from(false), + ast::Singleton::None => self.none(), + ast::Singleton::True => self.pin(pyre_object::w_bool_from(true)), + ast::Singleton::False => self.pin(pyre_object::w_bool_from(false)), }, )], ), @@ -2715,7 +2708,7 @@ impl Converter<'_> { } } - fn pattern_list(&self, patterns: &[ast::Pattern]) -> crate::PyResult { + fn pattern_list(&self, patterns: &[ast::Pattern]) -> RootedResult { patterns .iter() .map(|pattern| self.pattern(pattern)) @@ -2723,16 +2716,11 @@ impl Converter<'_> { .map(|patterns| self.list(patterns)) } - fn constant( - &self, - range: (u32, u32), - value: PyObjectRef, - kind: PyObjectRef, - ) -> crate::PyResult { + fn constant(&self, range: (u32, u32), value: Rooted, kind: Rooted) -> RootedResult { self.node("Constant", Some(range), &[("value", value), ("kind", kind)]) } - fn number(&self, value: &ast::Number) -> crate::PyResult { + fn number(&self, value: &ast::Number) -> RootedResult { Ok(match value { ast::Number::Int(value) => { // Ruff's Int stores an overflowing non-decimal literal by @@ -2741,7 +2729,11 @@ impl Converter<'_> { // with the token's radix instead of decimal int(). let spelling = value.to_string(); let source = self.string(&spelling); - crate::builtins::parse_int_from_str(source, &spelling, 0)? + self.pin(crate::builtins::parse_int_from_str( + source.get(), + &spelling, + 0, + )?) } ast::Number::Float(value) => self.pin(pyre_object::w_float_new(*value)), ast::Number::Complex { real, imag } => { @@ -2750,10 +2742,10 @@ impl Converter<'_> { }) } - fn constant_value(&self, value: &ast::ConstantValue) -> crate::PyResult { + fn constant_value(&self, value: &ast::ConstantValue) -> RootedResult { Ok(match value { - ast::ConstantValue::None => pyre_object::w_none(), - ast::ConstantValue::Boolean(value) => pyre_object::w_bool_from(*value), + ast::ConstantValue::None => self.none(), + ast::ConstantValue::Boolean(value) => self.pin(pyre_object::w_bool_from(*value)), ast::ConstantValue::Str(value) => self.string(value), ast::ConstantValue::Bytes(value) => self.pin(pyre_object::w_bytes_from_bytes(value)), ast::ConstantValue::Integer(value) => { @@ -2763,19 +2755,24 @@ impl Converter<'_> { // spelling, so let the same internal parser infer that radix // rather than feeding a hexadecimal token to decimal int(). let source = self.string(value); - crate::builtins::parse_int_from_str(source, value, 0)? + self.pin(crate::builtins::parse_int_from_str(source.get(), value, 0)?) } ast::ConstantValue::Float(value) => self.pin(pyre_object::w_float_new(*value)), ast::ConstantValue::Complex { real, imag } => { self.pin(pyre_object::w_complex_new(*real, *imag)) } - ast::ConstantValue::Ellipsis => pyre_object::w_ellipsis(), + ast::ConstantValue::Ellipsis => self.pin(pyre_object::w_ellipsis()), ast::ConstantValue::Tuple(values) => { let values = values .iter() .map(|v| self.constant_value(v)) .collect::, _>>()?; - self.pin(pyre_object::w_tuple_new(values)) + // A tuple header never moves, but it is untraced until it is + // pinned, so its members are read only once there is nothing + // left to allocate before the store. + self.pin(pyre_object::w_tuple_new( + values.into_iter().map(Rooted::get).collect(), + )) } ast::ConstantValue::Frozenset(_) => { return Err(crate::PyError::not_implemented( @@ -2785,12 +2782,12 @@ impl Converter<'_> { }) } - fn singleton(&self, name: &str) -> crate::PyResult { - let typ = crate::baseobjspace::getattr_str(self.ast_module, name)?; + fn singleton(&self, name: &str) -> RootedResult { + let typ = crate::baseobjspace::getattr_str(self.ast_module.get(), name)?; Ok(self.pin(pyre_object::w_instance_new(typ))) } - fn context(&self, value: ast::ExprContext) -> crate::PyResult { + fn context(&self, value: ast::ExprContext) -> RootedResult { self.singleton(match value { ast::ExprContext::Load => "Load", ast::ExprContext::Store => "Store", @@ -2798,13 +2795,13 @@ impl Converter<'_> { ast::ExprContext::Invalid => "Load", }) } - fn boolop(&self, value: ast::BoolOp) -> crate::PyResult { + fn boolop(&self, value: ast::BoolOp) -> RootedResult { self.singleton(match value { ast::BoolOp::And => "And", ast::BoolOp::Or => "Or", }) } - fn operator(&self, value: ast::Operator) -> crate::PyResult { + fn operator(&self, value: ast::Operator) -> RootedResult { self.singleton(match value { ast::Operator::Add => "Add", ast::Operator::Sub => "Sub", @@ -2821,7 +2818,7 @@ impl Converter<'_> { ast::Operator::FloorDiv => "FloorDiv", }) } - fn unaryop(&self, value: ast::UnaryOp) -> crate::PyResult { + fn unaryop(&self, value: ast::UnaryOp) -> RootedResult { self.singleton(match value { ast::UnaryOp::Invert => "Invert", ast::UnaryOp::Not => "Not", @@ -2829,7 +2826,7 @@ impl Converter<'_> { ast::UnaryOp::USub => "USub", }) } - fn cmpop(&self, value: ast::CmpOp) -> crate::PyResult { + fn cmpop(&self, value: ast::CmpOp) -> RootedResult { self.singleton(match value { ast::CmpOp::Eq => "Eq", ast::CmpOp::NotEq => "NotEq", @@ -2844,14 +2841,14 @@ impl Converter<'_> { }) } - fn parameters_opt(&self, parameters: Option<&ast::Parameters>) -> crate::PyResult { + fn parameters_opt(&self, parameters: Option<&ast::Parameters>) -> RootedResult { match parameters { Some(p) => self.parameters(p), None => self.parameters(&ast::Parameters::default()), } } - fn parameters(&self, p: &ast::Parameters) -> crate::PyResult { + fn parameters(&self, p: &ast::Parameters) -> RootedResult { let posonlyargs = p .posonlyargs .iter() @@ -2908,7 +2905,7 @@ impl Converter<'_> { ) } - fn parameter(&self, p: &ast::Parameter) -> crate::PyResult { + fn parameter(&self, p: &ast::Parameter) -> RootedResult { self.node( "arg", Some(range(p.range)), @@ -2918,12 +2915,12 @@ impl Converter<'_> { "annotation", self.optional(p.annotation.as_deref().map(|v| self.expr(v)).transpose()?), ), - ("type_comment", pyre_object::w_none()), + ("type_comment", self.none()), ], ) } - fn keyword_list(&self, keywords: &[ast::Keyword]) -> crate::PyResult { + fn keyword_list(&self, keywords: &[ast::Keyword]) -> RootedResult { keywords .iter() .map(|k| { @@ -2943,7 +2940,7 @@ impl Converter<'_> { .map(|items| self.list(items)) } - fn aliases(&self, aliases: &[ast::Alias]) -> crate::PyResult { + fn aliases(&self, aliases: &[ast::Alias]) -> RootedResult { aliases .iter() .map(|a| { @@ -2963,7 +2960,7 @@ impl Converter<'_> { .map(|items| self.list(items)) } - fn with_items(&self, items: &[ast::WithItem]) -> crate::PyResult { + fn with_items(&self, items: &[ast::WithItem]) -> RootedResult { items .iter() .map(|item| { @@ -2988,7 +2985,7 @@ impl Converter<'_> { .map(|items| self.list(items)) } - fn comprehensions(&self, comprehensions: &[ast::Comprehension]) -> crate::PyResult { + fn comprehensions(&self, comprehensions: &[ast::Comprehension]) -> RootedResult { comprehensions .iter() .map(|c| { @@ -2999,7 +2996,10 @@ impl Converter<'_> { ("target", self.expr(&c.target)?), ("iter", self.expr(&c.iter)?), ("ifs", self.expr_list(&c.ifs)?), - ("is_async", pyre_object::w_int_new(c.is_async as i64)), + ( + "is_async", + self.pin(pyre_object::w_int_new(c.is_async as i64)), + ), ], ) }) @@ -3007,7 +3007,7 @@ impl Converter<'_> { .map(|items| self.list(items)) } - fn handlers(&self, handlers: &[ast::ExceptHandler]) -> crate::PyResult { + fn handlers(&self, handlers: &[ast::ExceptHandler]) -> RootedResult { handlers .iter() .map(|handler| match handler { @@ -3031,7 +3031,7 @@ impl Converter<'_> { .map(|items| self.list(items)) } - fn type_params(&self, params: Option<&ast::TypeParams>) -> crate::PyResult { + fn type_params(&self, params: Option<&ast::TypeParams>) -> RootedResult { let Some(params) = params else { return Ok(self.list(Vec::new())); }; @@ -3100,7 +3100,7 @@ fn class_name(object: PyObjectRef) -> &'static str { /// reaches the tree as one node, not one per piece. enum JoinedPart { Literal { start: u32, end: u32, value: String }, - Value(PyObjectRef), + Value(Rooted), } fn push_literal(parts: &mut Vec, (start, end): (u32, u32), value: &str) { From 5629bfff14ccf66b0096fae1dff676f74b83d53d Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Mon, 17 Aug 2026 12:13:37 +0900 Subject: [PATCH 2/4] type.__new__: root the __set_name__ entries and the __init_subclass__ keywords `type_descr_new` copies the finished class dict out with `w_dict_items` and then calls `__set_name__` on each entry. That call runs Python, and a class body's values include lists and dicts -- the two kinds whose headers move -- so the pairs still sitting in the native vector addressed objects that had been moved. `PYPY_GC_NURSERY=4096` segfaults in `baseobjspace::set_name` reading such a value's type pointer, with `builtins::type_descr_new` and `call::build_class_inner` under it; the whole CPython `test_dictviews` module goes CRASH -> PASS at that nursery. `call_init_subclass_on_bases` holds its keyword pairs across `super_check`, `w_super_new` and the `__init_subclass__` lookup, which run Python for the same reason, and a class keyword's value can be a list or a dict. That one is from reading the function rather than from a backtrace. Both now pin the flattened pairs and read each back at the call that consumes it. The repro builds classes through `type(name, bases, dict)` and through a metaclass, with class bodies and class keywords holding lists and dicts: 201 assertions, rc=139 under `PYPY_GC_NURSERY=4096` before and clean after, at the default nursery and at `=1` too. Assisted-by: Claude --- pyre/pyre-interpreter/src/builtins.rs | 16 ++++++++++++++-- pyre/pyre-interpreter/src/call.rs | 26 ++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index 3d013dc6dea..1e3f12c6684 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -5896,9 +5896,21 @@ fn type_descr_new_with_metaclass( // re-enter the type's dict. let dict_obj = pyre_object::gc_roots::shadow_stack_get(dict_root); let set_name_entries = unsafe { pyre_object::w_dict_items(dict_obj) }; - for (key, v) in set_name_entries { + // The pairs sit in a native Vec the collector does not walk and every + // `__set_name__` runs Python, so a class body's list and dict values + // move out from under the entries still to come. Pin them and read + // each back at the call that consumes it. + let _entry_roots = pyre_object::gc_roots::push_roots(); + let flat: Vec = set_name_entries + .iter() + .flat_map(|&(key, value)| [key, value]) + .collect(); + let entry_base = pyre_object::gc_roots::pin_roots(&flat); + for index in 0..set_name_entries.len() { + let key = pyre_object::gc_roots::shadow_stack_get(entry_base + index * 2); if unsafe { pyre_object::is_str(key) } { - unsafe { crate::baseobjspace::set_name(w_type, key, v) }?; + let value = pyre_object::gc_roots::shadow_stack_get(entry_base + index * 2 + 1); + unsafe { crate::baseobjspace::set_name(w_type, key, value) }?; } } diff --git a/pyre/pyre-interpreter/src/call.rs b/pyre/pyre-interpreter/src/call.rs index f070be59cbd..3a7ce5bbb9b 100644 --- a/pyre/pyre-interpreter/src/call.rs +++ b/pyre/pyre-interpreter/src/call.rs @@ -5046,16 +5046,34 @@ pub(crate) fn call_init_subclass_on_bases( // proxy. This matters for a custom metaclass mro() that omits the // nascent class: `super(w_type, w_type)` must reject that incomplete // hierarchy instead of manufacturing an invalid proxy. + // The keywords are a raw copy the collector cannot see, and `super_check`, + // the `__init_subclass__` lookup and a `__getattr__` under it all run + // Python. A class keyword's value can be a list or a dict, so pin the + // pairs here and read them back where the call's keywords are built. + let _roots = pyre_object::gc_roots::push_roots(); + let flat: Vec = init_subclass_kwargs + .iter() + .flat_map(|&(key, value)| [key, value]) + .collect(); + let kwarg_base = pyre_object::gc_roots::pin_roots(&flat); let w_objtype = crate::builtins::super_check(w_type, w_type)?; let w_super = pyre_object::descriptor::w_super_new(w_type, w_objtype, w_type); let w_func = crate::baseobjspace::getattr_str(w_super, "__init_subclass__")?; // typeobject.py:1025-1026 — `args = __args__.replace_arguments([])` then // `space.call_args(w_func, args)`: keywords only, no positionals, and no // frame, because `call_args` (descroperation.py:189) never takes one. - let kwds: Vec<(Wtf8Buf, PyObjectRef)> = init_subclass_kwargs - .iter() - .filter(|(k, _)| unsafe { pyre_object::is_str(*k) }) - .map(|(k, v)| (unsafe { pyre_object::w_str_get_wtf8(*k) }.to_owned(), *v)) + let kwds: Vec<(Wtf8Buf, PyObjectRef)> = (0..init_subclass_kwargs.len()) + .filter_map(|index| { + let key = pyre_object::gc_roots::shadow_stack_get(kwarg_base + index * 2); + if !unsafe { pyre_object::is_str(key) } { + return None; + } + let value = pyre_object::gc_roots::shadow_stack_get(kwarg_base + index * 2 + 1); + Some(( + unsafe { pyre_object::w_str_get_wtf8(key) }.to_owned(), + value, + )) + }) .collect(); call_with_kwargs_in_ctx(take_last_exec_ctx(), w_func, &[], &kwds)?; Ok(()) From 41a22884bcf5c90464d964d0ff99f145224930e5 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Mon, 17 Aug 2026 16:19:04 +0900 Subject: [PATCH 3/4] gc: root the values eight interpreter paths held across a collection point `__build_class__` mints both bases tuples into Rust locals nothing traces, so a class body long enough to span a major cycle lets Sweeping free the tuple `w_type_new` then stores into `W_TypeObject.bases`; the crash reproduces at the default nursery. `real_build_class` pins both at their mints for the whole of `build_class_inner`, its one caller. The same shape elsewhere: - `update_bases` kept the tuples `__mro_entries__` returned in a native vector across the next base's `getattr_str`. - `type_descr_new_with_metaclass` minted the `(object,)` default bases with no other referrer and used it through `__set_name__`. - `array_descr_new` read its initializer out of the native argument slice after the 'u' deprecation warning ran Python. - `array_extend_iterable` held the iterator it minted across `next`. - `save_global_or_reduce` re-read the object after `__reduce_ex__` ran. - `_json`'s sequence and dict encoders named `obj` in their `map_err` closures after the child encoders ran, so a note reported the type of whatever occupied the cell; `encode_dict` also held the `items()` result across `sorted`. - `select.select` collected each fd sequence from the argument slice in turn, so the second and third were pre-move addresses once the first had run `__iter__` and `fileno()`. - `set_intersect_update` and `w_set_difference_update_from_set` left their accumulators unreferenced across the `__eq__` a bucket probe runs, and `set_method_intersection` held the set each `set_intersect_update` returned across the next operand's iterable drain. A tuple, type, set and array are allocated stable and never move, so those sites pin for liveness and keep reading the local; a list or dict header moves, so those read the value back from its slot at each consumer. Assisted-by: Claude --- pyre/pyre-interpreter/src/builtins.rs | 8 ++++ pyre/pyre-interpreter/src/call.rs | 36 +++++++++++++++-- pyre/pyre-interpreter/src/module/_json/mod.rs | 39 ++++++++++++++----- .../src/module/_pickle/pickler.rs | 27 +++++++++++-- pyre/pyre-interpreter/src/module/array/mod.rs | 21 +++++++++- .../src/module/select/interp_select.rs | 38 ++++++++++++++---- pyre/pyre-interpreter/src/typedef.rs | 26 ++++++++++--- pyre/pyre-object/src/setobject.rs | 5 +++ 8 files changed, 171 insertions(+), 29 deletions(-) diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index 1e3f12c6684..92efc91d67a 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -5770,6 +5770,13 @@ fn type_descr_new_with_metaclass( // typeobject.py:954 — `W_TypeObject.__init__` is the site that reads // the bases as `bases_w or [space.w_object]`, so the `(object,)` // default belongs to construction and to nothing that runs before it. + // On the empty-bases arm this is a `(object,)` tuple minted right here + // with no other referrer, and it has to survive `calculate_metaclass`, + // the namespace copy, `validate_c3_mro`, `create_all_slots`, + // `__set_name__` and `__init_subclass__` before anything else refers to + // it. A tuple never moves, so the plain uses below stay valid + // addresses; the pin is what keeps a major cycle from sweeping it. + let _effective_bases_roots = pyre_object::gc_roots::push_roots(); let w_effective_bases = if bases.is_null() || !unsafe { is_tuple(bases) } || unsafe { w_tuple_len(bases) } == 0 { @@ -5782,6 +5789,7 @@ fn type_descr_new_with_metaclass( } else { bases }; + pyre_object::gc_roots::pin_root(w_effective_bases); // calculate_metaclass — delegate to winner if different let default_meta = if w_metaclass.is_null() { crate::typedef::w_type() diff --git a/pyre/pyre-interpreter/src/call.rs b/pyre/pyre-interpreter/src/call.rs index 3a7ce5bbb9b..2263cc050b8 100644 --- a/pyre/pyre-interpreter/src/call.rs +++ b/pyre/pyre-interpreter/src/call.rs @@ -4029,6 +4029,13 @@ fn update_bases( base_args: &[PyObjectRef], w_orig_bases: PyObjectRef, ) -> Result<(Vec, bool), crate::PyError> { + // The entries `__mro_entries__` contributes are reachable only from the + // tuple it returned and from this native vector, neither of which the + // collector walks, while a later iteration's `getattr_str` and + // `__mro_entries__` call both run Python. Pinning each returned tuple + // keeps its entries traced for the rest of the walk; the caller's + // `w_tuple_new` re-pins them before it allocates. + let _entry_roots = pyre_object::gc_roots::push_roots(); let mut new_bases: Option> = None; for (i, &w_base) in base_args.iter().enumerate() { if unsafe { pyre_object::is_type(w_base) } { @@ -4059,6 +4066,7 @@ fn update_bases( "__mro_entries__ must return a tuple", )); } + pyre_object::gc_roots::pin_root(w_new_base); if new_bases.is_none() { new_bases = Some(base_args[..i].to_vec()); } @@ -4166,15 +4174,29 @@ pub(crate) fn real_build_class(args: &[PyObjectRef]) -> Result Result Result { let _roots = gc_roots::push_roots(); + // `obj` is the sequence being encoded — a movable header — and the child + // encoders below run arbitrary Python. The `map_err` closure must read it + // back out of the slot; capturing the parameter would name the pre-move + // address and report the type of whatever now occupies that cell. + let obj_slot = gc_roots::shadow_stack_len(); + gc_roots::pin_root(obj); let iter = crate::baseobjspace::iter(obj)?; let iter_slot = gc_roots::shadow_stack_len(); gc_roots::pin_root(iter); @@ -1028,7 +1034,7 @@ fn encode_sequence( err, format!( "when serializing {} item {item_index}", - short_type_name(obj) + short_type_name(gc_roots::shadow_stack_get(obj_slot)) ), ) })?; @@ -1086,20 +1092,32 @@ fn encode_dict( // CPython's encoder iterates `items()`. Keeping the returned Python // iterable as the owner makes mutations from re-entrant key encoders // visible and avoids holding raw pointers into a mutable list. + // + // `obj` and the `items()` result are both movable headers, and the + // `sort_keys` attribute read, the `sorted` lookup and the sort itself all + // run Python between the two. Each is pinned where it is produced and read + // back where it is used. + let _roots = gc_roots::push_roots(); + let obj_slot = gc_roots::shadow_stack_len(); + gc_roots::pin_root(obj); let items = crate::call::call_function_impl_result( crate::baseobjspace::getattr_str(obj, "items")?, &[], )?; - let items = if crate::baseobjspace::is_true(encoder_attr(self_obj, "sort_keys")?)? { + let mut items_slot = gc_roots::shadow_stack_len(); + gc_roots::pin_root(items); + if crate::baseobjspace::is_true(encoder_attr(self_obj, "sort_keys")?)? { let builtins = crate::importing::get_sys_module("builtins") .ok_or_else(|| PyError::runtime_error("builtins module is unavailable"))?; let sorted = crate::baseobjspace::getattr_str(builtins, "sorted")?; - crate::call::call_function_impl_result(sorted, &[items])? - } else { - items - }; - let _roots = gc_roots::push_roots(); - let iter = crate::baseobjspace::iter(items)?; + let sorted_items = crate::call::call_function_impl_result( + sorted, + &[gc_roots::shadow_stack_get(items_slot)], + )?; + items_slot = gc_roots::shadow_stack_len(); + gc_roots::pin_root(sorted_items); + } + let iter = crate::baseobjspace::iter(gc_roots::shadow_stack_get(items_slot))?; let iter_slot = gc_roots::shadow_stack_len(); gc_roots::pin_root(iter); let item_separator = require_string(encoder_attr(self_obj, "item_separator")?)?.to_wtf8_buf(); @@ -1162,7 +1180,10 @@ fn encode_dict( add_json_note( err, crate::display::wtf8_format!( - format!("when serializing {} item ", short_type_name(obj)), + format!( + "when serializing {} item ", + short_type_name(gc_roots::shadow_stack_get(obj_slot)) + ), key_repr ), ) diff --git a/pyre/pyre-interpreter/src/module/_pickle/pickler.rs b/pyre/pyre-interpreter/src/module/_pickle/pickler.rs index 85116b331f4..f6a62afa91b 100644 --- a/pyre/pyre-interpreter/src/module/_pickle/pickler.rs +++ b/pyre/pyre-interpreter/src/module/_pickle/pickler.rs @@ -1362,6 +1362,13 @@ fn save_global_or_reduce( buf: &mut Framer, w_obj: PyObjectRef, ) -> Result<(), PyError> { + // The reduce protocol below runs Python — the `dispatch_table` lookup and + // `__reduce_ex__` — and the object being pickled can be a `list` or `dict` + // subclass, whose header moves. Pin it and re-read it from the slot at the + // two consumers that sit after one of those calls. + let _roots = pyre_object::gc_roots::push_roots(); + let obj_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(w_obj); // CPython 3.14 pickle.py dispatch[type] = save_type: only a class whose // exact metaclass is `type` takes this built-in dispatch entry. A class // with a custom metaclass must consult dispatch_table first. @@ -1381,6 +1388,7 @@ fn save_global_or_reduce( // A `dispatch_table` reduce function registered for this exact type takes // precedence over `__reduce_ex__`. if let Some(w_rv) = dispatch_table_reduce(ctx, w_obj)? { + let w_obj = pyre_object::gc_roots::shadow_stack_get(obj_slot); return save_reduce_value(ctx, buf, w_obj, w_rv); } @@ -1392,13 +1400,26 @@ fn save_global_or_reduce( // Everything else goes through the reduce protocol. let w_rv = match crate::baseobjspace::findattr_result(w_obj, "__reduce_ex__")? { - Some(reduce_ex) => call_fn(reduce_ex, &[pyre_object::w_int_new(ctx.proto)])?, - None => match crate::baseobjspace::findattr_result(w_obj, "__reduce__")? { + Some(reduce_ex) => { + // A bound method the lookup minted has no other referrer, and + // boxing the protocol number allocates before the call reaches it. + pyre_object::gc_roots::pin_root(reduce_ex); + call_fn(reduce_ex, &[pyre_object::w_int_new(ctx.proto)])? + } + None => match crate::baseobjspace::findattr_result( + pyre_object::gc_roots::shadow_stack_get(obj_slot), + "__reduce__", + )? { Some(reduce) => call_fn(reduce, &[])?, None => return Err(pickling_error("Can't pickle object: no __reduce_ex__")), }, }; - save_reduce_value(ctx, buf, w_obj, w_rv) + save_reduce_value( + ctx, + buf, + pyre_object::gc_roots::shadow_stack_get(obj_slot), + w_rv, + ) } /// CPython 3.14 `pickle._Pickler.save_type`, line by line. diff --git a/pyre/pyre-interpreter/src/module/array/mod.rs b/pyre/pyre-interpreter/src/module/array/mod.rs index 9c3333af924..26bd3073954 100644 --- a/pyre/pyre-interpreter/src/module/array/mod.rs +++ b/pyre/pyre-interpreter/src/module/array/mod.rs @@ -187,7 +187,14 @@ fn array_extend_iterable( return Ok(()); } } + // The iterator is minted here and nothing else refers to it, while `next` + // and `array_append` both run Python. An iterator and an array are stable + // allocations, so neither address goes stale; what the scope buys is that + // an unmarked block is not swept out from under the loop. + let _roots = pyre_object::gc_roots::push_roots(); + pyre_object::gc_roots::pin_root(obj); let w_iter = crate::baseobjspace::iter(w_iterable)?; + pyre_object::gc_roots::pin_root(w_iter); loop { match crate::baseobjspace::next(w_iter) { Ok(w_item) => array_append(obj, w_item)?, @@ -230,6 +237,14 @@ fn array_descr_new(args: &[PyObjectRef]) -> PyResult { pos.len() - 1 ))); } + // `pos` is a native slice into the flat ABI's argument buffer. The gateway + // keeps the arguments alive, but it does not rewrite this copy, and the 'u' + // deprecation warning below runs Python: a `list` or `dict` initializer is + // movable and leaves its pre-move address behind in the slice. Pin the + // arguments here, before that warning, and read the initializer back out of + // its slot where it is used. + let _pos_roots = pyre_object::gc_roots::push_roots(); + let pos_base = pyre_object::gc_roots::pin_roots(pos); let cls = pos[0]; let canonical = crate::typedef::gettypefor(&pyre_object::interp_array::ARRAY_TYPE) .map_or(PY_NULL, |ty| ty.as_ptr()); @@ -284,13 +299,17 @@ fn array_descr_new(args: &[PyObjectRef]) -> PyResult { )?; } let obj = arr::w_array_new(typecode, itemsize); + // Nothing refers to the fresh array yet and every initializer below runs + // Python. An array is stable, so the local stays a valid address; the pin + // is what stops a major cycle sweeping it as unreachable. + pyre_object::gc_roots::pin_root(obj); // Subclass: retag the fresh array with the requested class. if !cls.is_null() && unsafe { pyre_object::is_type(cls) } && !std::ptr::eq(cls, canonical) { crate::typedef::tag_subclass_instance(obj, cls); } // Optional initializer. if pos.len() >= 3 { - let w_init = pos[2]; + let w_init = pyre_object::gc_roots::shadow_stack_get(pos_base + 2); if unsafe { pyre_object::is_str(w_init) } { if matches!(typecode, b'u' | b'w') { array_fromunicode(obj, w_init)?; diff --git a/pyre/pyre-interpreter/src/module/select/interp_select.rs b/pyre/pyre-interpreter/src/module/select/interp_select.rs index e607ac96f32..676a544d22d 100644 --- a/pyre/pyre-interpreter/src/module/select/interp_select.rs +++ b/pyre/pyre-interpreter/src/module/select/interp_select.rs @@ -341,9 +341,22 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { Ok(out) } - let rfds = collect_fds(args[0])?; - let wfds = collect_fds(args[1])?; - let xfds = collect_fds(args[2])?; + // `args` is a native slice the gateway copied out of its own + // slots: it keeps the arguments alive but cannot rewrite this + // copy, and each `collect_fds` runs Python twice — the + // iteration protocol and `fileno()`. The three fd sequences + // are usually lists, whose header moves, so reading the second + // and third out of the slice after the first was collected + // hands `unpackiterable` a pre-move address. Pin them here and + // read each back at its own call. + let arg_roots = pyre_object::gc_roots::push_roots(); + let args_base = arg_roots.base(); + arg_roots.pin_root(args[0]); + arg_roots.pin_root(args[1]); + arg_roots.pin_root(args[2]); + let rfds = collect_fds(arg_roots.get(args_base))?; + let wfds = collect_fds(arg_roots.get(args_base + 1))?; + let xfds = collect_fds(arg_roots.get(args_base + 2))?; // The first `select()` argument: POSIX scans descriptors // `0..nfds`, so it must exceed the highest one. @@ -457,10 +470,21 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { pyre_object::w_list_new(items) } - let r_ready = build_ready(&mut rset, &rfds); - let w_ready = build_ready(&mut wset, &wfds); - let x_ready = build_ready(&mut xset, &xfds); - Ok(pyre_object::w_tuple_new(vec![r_ready, w_ready, x_ready])) + // Each list is freshly minted and a list header moves, so the + // allocation the next `build_ready` performs can relocate the + // previous one and leave its pre-move address in the local. + // Pin each at its mint and read all three back where the tuple + // is built. + let roots = pyre_object::gc_roots::push_roots(); + let ready_base = roots.base(); + roots.pin_root(build_ready(&mut rset, &rfds)); + roots.pin_root(build_ready(&mut wset, &wfds)); + roots.pin_root(build_ready(&mut xset, &xfds)); + Ok(pyre_object::w_tuple_new(vec![ + roots.get(ready_base), + roots.get(ready_base + 1), + roots.get(ready_base + 2), + ])) } #[cfg(not(all(any(unix, windows), feature = "host_env")))] { diff --git a/pyre/pyre-interpreter/src/typedef.rs b/pyre/pyre-interpreter/src/typedef.rs index 1b2c854f979..aef68f60b98 100644 --- a/pyre/pyre-interpreter/src/typedef.rs +++ b/pyre/pyre-interpreter/src/typedef.rs @@ -25967,11 +25967,15 @@ fn set_intersect_update( } else { (w_set, w_other) }; - let result = pyre_object::w_set_new(); // The three sets are old-gen allocations and keep their addresses across - // a collection, but their elements are young and move, so each key is - // re-read from the table the collector rewrites rather than carried - // across the `eq_w` a bucket probe can run. + // a collection, but staying put is not staying alive: `result` has no + // referrer yet, and the `eq_w` a bucket probe runs is a collection point + // that would sweep it. Their elements are young and move, so each key + // is re-read from the table the collector rewrites rather than carried + // across that probe. + let _roots = pyre_object::gc_roots::push_roots(); + let result = pyre_object::w_set_new(); + pyre_object::gc_roots::pin_root(result); let mut i = 0; while let Some(key) = pyre_object::w_set_key_at(keep, i) { if pyre_object::w_set_contains_key_checked(probe, key) @@ -26027,18 +26031,30 @@ pub(crate) fn set_method_intersection( // `setobject.py` — the seed and every operand become sets, and a // set operand is intersected as it stands rather than rebuilt. + // Every set here is freshly minted with no referrer: the seed, each operand + // `set_newobj_intersection` builds out of a non-set, and the accumulator + // each `set_intersect_update` hands back — whose own root scope ends when it + // returns. A set is old-gen and never moves, so the locals stay valid + // addresses; what they need is to survive the collection points around + // them, the iterable drain and the `__eq__` a bucket probe runs. + let _roots = pyre_object::gc_roots::push_roots(); let mut result = set_newobj_intersection(others_w[0])?; + pyre_object::gc_roots::pin_root(result); for &w_other in &others_w[1..] { let w_other_as_set = if unsafe { pyre_object::is_set_or_frozenset(w_other) } { w_other } else { - set_newobj_intersection(w_other)? + let w_fresh = set_newobj_intersection(w_other)?; + pyre_object::gc_roots::pin_root(w_fresh); + w_fresh }; result = set_intersect_update(result, w_other_as_set)?; + pyre_object::gc_roots::pin_root(result); } unsafe { if pyre_object::is_frozenset(args[0]) { let w_frozenset = pyre_object::w_frozenset_new(); + pyre_object::gc_roots::pin_root(w_frozenset); pyre_object::w_set_copy_storage_from(w_frozenset, result); return Ok(w_frozenset); } diff --git a/pyre/pyre-object/src/setobject.rs b/pyre/pyre-object/src/setobject.rs index 7efdb3b11ea..d284b7d3458 100644 --- a/pyre/pyre-object/src/setobject.rs +++ b/pyre/pyre-object/src/setobject.rs @@ -742,7 +742,12 @@ pub unsafe fn w_set_difference_update_from_set( // storage wholesale. Besides the complexity bound, this preserves the // exact contains-with-hash callback direction of the upstream strategy. if w_set_len(dst) < w_set_len(src) { + // A set is old-gen and never moves, but the difference accumulator has + // no referrer until `w_set_copy_storage_from` below: the `eq_w` a bucket + // probe runs is a collection point that would otherwise sweep it. + let _roots = crate::gc_roots::push_roots(); let result = w_set_new(); + crate::gc_roots::pin_root(result); let dst_items = (*(dst as *const W_SetObject)).items; let dst_len = (*dst_items).len(); let mut i = 0; From 1e608da51a162d54842b4f901a19a9cf11b88f84 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Mon, 17 Aug 2026 16:19:14 +0900 Subject: [PATCH 4/4] gc: name a stale pointer where a forwarded header answered a flag test `FORWARDED_MARKER` sets every bit `has_flag` reads, so a forwarded header answers `HAS_SHADOW` and the map lookup behind it fails with "GCFLAG_HAS_SHADOW but no shadow found". `find_shadow` and the major marking visitor assert the header is not forwarded before that test. `has_flag` is unchanged: incminimark.py:2167-2216 relies on the all-bits form and orders `is_forwarded` ahead of the shadow arm instead, which `copy_nursery_object` already mirrors. `walk_raw_function_roots`' comment attributed a stale nursery ref to `Nursery::reset` zero-filling the region; on native it only rewinds the free pointer, and the zero fill is the wasm32 arm. Assisted-by: Claude --- majit/majit-gc/src/collector.rs | 32 +++++++++++++++++++++++-------- pyre/pyre-interpreter/src/eval.rs | 8 ++++++-- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/majit/majit-gc/src/collector.rs b/majit/majit-gc/src/collector.rs index 1e78043ad6b..b76bc6f5d58 100644 --- a/majit/majit-gc/src/collector.rs +++ b/majit/majit-gc/src/collector.rs @@ -3178,6 +3178,14 @@ impl MiniMarkGC { /// if this is the first request. fn find_shadow(&mut self, obj_addr: usize) -> usize { let hdr = unsafe { *((obj_addr - GcHeader::SIZE) as *const GcHeader) }; + // A forwarded header is `FORWARDED_MARKER`, whose every flag bit reads + // set, so it would answer the test below and then die on the map lookup + // under the shadow message. `_find_shadow`'s precondition is that the + // object has not been copied yet, so name the real fault here. + assert!( + !hdr.is_forwarded(), + "stale pointer into the nursery: find_shadow reached a forwarded header at {obj_addr:#x}" + ); if hdr.has_flag(flags::HAS_SHADOW) { // incminimark.py:2855-2857 `ll_assert(shadow != NULL, // "GCFLAG_HAS_SHADOW but no shadow found")`. HAS_SHADOW @@ -4891,14 +4899,22 @@ impl MiniMarkGC { // block. Upstream normally reaches the shadow through // GCFLAG_HAS_SHADOW during the leading minor; this is the equivalent // for pyre's oldgen-only non-moving major. - if self.is_in_nursery(obj_addr) - && unsafe { (*header_of(obj_addr)).has_flag(flags::HAS_SHADOW) } - { - let shadow_obj = *self - .nursery_objects_shadows - .get(&obj_addr) - .expect("GCFLAG_HAS_SHADOW but no shadow found"); - unsafe { (*header_of(shadow_obj)).set_flag(flags::VISITED) }; + if self.is_in_nursery(obj_addr) { + // Every flag bit of `FORWARDED_MARKER` reads set, so a worklist + // entry a minor collection forwarded would pass the shadow test + // below and then fail the map lookup under a message about the + // shadow map. The fault is the stale worklist entry; say so. + assert!( + !unsafe { (*header_of(obj_addr)).is_forwarded() }, + "stale major worklist entry: forwarded header at {obj_addr:#x}" + ); + if unsafe { (*header_of(obj_addr)).has_flag(flags::HAS_SHADOW) } { + let shadow_obj = *self + .nursery_objects_shadows + .get(&obj_addr) + .expect("GCFLAG_HAS_SHADOW but no shadow found"); + unsafe { (*header_of(shadow_obj)).set_flag(flags::VISITED) }; + } } let custom_trace; let (item_size, length_offset, fixed_size, items_have_gc_ptrs); diff --git a/pyre/pyre-interpreter/src/eval.rs b/pyre/pyre-interpreter/src/eval.rs index ed386e7c03d..424667ded49 100644 --- a/pyre/pyre-interpreter/src/eval.rs +++ b/pyre/pyre-interpreter/src/eval.rs @@ -231,8 +231,12 @@ impl Drop for FrameAnchor { /// pyre's JIT-compiled code allocates W_IntObject / result boxes into the /// nursery (`NewWithVtable` → `gc_alloc_typed_nursery_shim`). When the /// nursery fills and a minor collection runs, only registered roots are -/// forwarded — unforwarded nursery refs become stale after -/// `Nursery::reset` zero-fills the region. The interpreter stores live +/// forwarded — an unforwarded nursery ref is left addressing a corpse. +/// `Nursery::reset` only rewinds the free pointer on native (it zero-fills +/// on wasm32, and writes the 0xAA poison only when that debug mode is on), +/// so the corpse keeps its forwarding header until something is allocated +/// over it and the stale ref reads whichever of the two it finds. The +/// interpreter stores live /// refs in `PyFrame.locals_cells_stack_w`; without this walker those /// slots turn into NULL-`ob_type` stale pointers on the next LOAD_FAST /// (reproduced by `inline_helper` n >= 10000).