diff --git a/pyre/bench/synth/foriter_inplace_immutable.cranelift.jitstats b/pyre/bench/synth/foriter_inplace_immutable.cranelift.jitstats index cbfba7e5e41..a0796ff2cd2 100644 --- a/pyre/bench/synth/foriter_inplace_immutable.cranelift.jitstats +++ b/pyre/bench/synth/foriter_inplace_immutable.cranelift.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=1 +bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=202 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=1 internal_compile_panics=0 loops_aborted=0 -loops_compiled=1 +loops_compiled=2 diff --git a/pyre/bench/synth/inline_callee_constructs_object.cranelift.jitstats b/pyre/bench/synth/inline_callee_constructs_object.cranelift.jitstats index 114b48b9fd0..62c98090fec 100644 --- a/pyre/bench/synth/inline_callee_constructs_object.cranelift.jitstats +++ b/pyre/bench/synth/inline_callee_constructs_object.cranelift.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=1 +bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=201 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=1 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/bench/synth/locals_forced_frame.cranelift.jitstats b/pyre/bench/synth/locals_forced_frame.cranelift.jitstats new file mode 100644 index 00000000000..c630148fca7 --- /dev/null +++ b/pyre/bench/synth/locals_forced_frame.cranelift.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=3 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=2 diff --git a/pyre/bench/synth/locals_forced_frame.dynasm.jitstats b/pyre/bench/synth/locals_forced_frame.dynasm.jitstats new file mode 100644 index 00000000000..c630148fca7 --- /dev/null +++ b/pyre/bench/synth/locals_forced_frame.dynasm.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=3 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=2 diff --git a/pyre/bench/synth/locals_forced_frame.py b/pyre/bench/synth/locals_forced_frame.py new file mode 100644 index 00000000000..64845b81bd7 --- /dev/null +++ b/pyre/bench/synth/locals_forced_frame.py @@ -0,0 +1,35 @@ +# `locals()` inside a hot `for` loop must report the frame's live values. +# +# `locals()` reaches its frame through `gettopframe_nohidden`, which walks the +# `f_backref` chain without forcing anything, and `fast2locals` then reads +# `locals_cells_stack_w` directly. Once the loop compiles, an unforced +# virtualizable still holds whatever the frame last wrote out, so the mapping +# comes back with correct KEYS (they come from the code object) and stale +# VALUES -- a key-only assertion does not see it. +# +# The shape matters: only a `FOR_ITER` loop reproduces. The same body written +# with `while` reports live values whether or not the frame is forced. + + +def probe(): + seen = [] + for i in range(200000): + a = i * 2 + b = a + 1 + if i == 5000 or i == 199999: + d = locals() + seen.append((d.get("i"), d.get("a"), d.get("b"))) + return seen + + +def probe_vars(): + out = None + for i in range(200000): + a = i * 2 + if i == 199999: + out = vars().get("a") + return out + + +print(probe()) +print(probe_vars()) diff --git a/pyre/bench/synth/locals_forced_frame.wasm.jitstats b/pyre/bench/synth/locals_forced_frame.wasm.jitstats new file mode 100644 index 00000000000..c630148fca7 --- /dev/null +++ b/pyre/bench/synth/locals_forced_frame.wasm.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=3 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=2 diff --git a/pyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstats b/pyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstats index 5bf2ef31976..1aaa522e304 100644 --- a/pyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstats +++ b/pyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=1 +bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=203 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=2 internal_compile_panics=0 loops_aborted=0 loops_compiled=3 diff --git a/pyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstats b/pyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstats index 5bf2ef31976..1aaa522e304 100644 --- a/pyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstats +++ b/pyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=1 +bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=203 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=2 internal_compile_panics=0 loops_aborted=0 loops_compiled=3 diff --git a/pyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstats b/pyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstats index 01fbf6998ec..1aaa522e304 100644 --- a/pyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstats +++ b/pyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=2 +bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=404 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=2 internal_compile_panics=0 loops_aborted=0 loops_compiled=3 diff --git a/pyre/bench/synth/pickle_terminal_raise_resume.cranelift.jitstats b/pyre/bench/synth/pickle_terminal_raise_resume.cranelift.jitstats index db0d2de8154..8fd36564d41 100644 --- a/pyre/bench/synth/pickle_terminal_raise_resume.cranelift.jitstats +++ b/pyre/bench/synth/pickle_terminal_raise_resume.cranelift.jitstats @@ -5,7 +5,7 @@ descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=656 +guard_failures=338 internal_compile_panics=0 loops_aborted=1 -loops_compiled=31 +loops_compiled=35 diff --git a/pyre/bench/synth/pickle_terminal_raise_resume.dynasm.jitstats b/pyre/bench/synth/pickle_terminal_raise_resume.dynasm.jitstats index 52e6ae5ab59..8fd36564d41 100644 --- a/pyre/bench/synth/pickle_terminal_raise_resume.dynasm.jitstats +++ b/pyre/bench/synth/pickle_terminal_raise_resume.dynasm.jitstats @@ -5,7 +5,7 @@ descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=354 +guard_failures=338 internal_compile_panics=0 loops_aborted=1 -loops_compiled=31 +loops_compiled=35 diff --git a/pyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstats b/pyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstats index b6607e90cd0..52e7c49250e 100644 --- a/pyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstats +++ b/pyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstats @@ -5,7 +5,7 @@ descr_set_stale_absent=0 fbw_rolled_back_with_effects=5 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=355 +guard_failures=339 internal_compile_panics=0 loops_aborted=13 -loops_compiled=68 +loops_compiled=72 diff --git a/pyre/bench/synth/polymorphic_binary_receiver.cranelift.jitstats b/pyre/bench/synth/polymorphic_binary_receiver.cranelift.jitstats index 62cb559a364..f81573247ba 100644 --- a/pyre/bench/synth/polymorphic_binary_receiver.cranelift.jitstats +++ b/pyre/bench/synth/polymorphic_binary_receiver.cranelift.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=4 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=1045 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=788 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 diff --git a/pyre/bench/synth/polymorphic_binary_receiver.dynasm.jitstats b/pyre/bench/synth/polymorphic_binary_receiver.dynasm.jitstats index 62cb559a364..f81573247ba 100644 --- a/pyre/bench/synth/polymorphic_binary_receiver.dynasm.jitstats +++ b/pyre/bench/synth/polymorphic_binary_receiver.dynasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=4 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=1045 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=788 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 diff --git a/pyre/bench/synth/polymorphic_binary_receiver.wasm.jitstats b/pyre/bench/synth/polymorphic_binary_receiver.wasm.jitstats index 62cb559a364..f81573247ba 100644 --- a/pyre/bench/synth/polymorphic_binary_receiver.wasm.jitstats +++ b/pyre/bench/synth/polymorphic_binary_receiver.wasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=4 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=1045 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=788 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 diff --git a/pyre/bench/synth/sre_pattern_methods.cranelift.jitstats b/pyre/bench/synth/sre_pattern_methods.cranelift.jitstats index b549d05062f..235d297fa8f 100644 --- a/pyre/bench/synth/sre_pattern_methods.cranelift.jitstats +++ b/pyre/bench/synth/sre_pattern_methods.cranelift.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=11 +bridges_compiled=5 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=2291 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=1012 internal_compile_panics=0 loops_aborted=0 loops_compiled=7 diff --git a/pyre/bench/synth/sre_pattern_methods.dynasm.jitstats b/pyre/bench/synth/sre_pattern_methods.dynasm.jitstats index b549d05062f..235d297fa8f 100644 --- a/pyre/bench/synth/sre_pattern_methods.dynasm.jitstats +++ b/pyre/bench/synth/sre_pattern_methods.dynasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=11 +bridges_compiled=5 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=2291 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=1012 internal_compile_panics=0 loops_aborted=0 loops_compiled=7 diff --git a/pyre/bench/synth/sre_pattern_methods.wasm.jitstats b/pyre/bench/synth/sre_pattern_methods.wasm.jitstats index e526333d4a8..bedde7b54df 100644 --- a/pyre/bench/synth/sre_pattern_methods.wasm.jitstats +++ b/pyre/bench/synth/sre_pattern_methods.wasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=11 +bridges_compiled=5 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=2292 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=1013 internal_compile_panics=0 loops_aborted=0 loops_compiled=7 diff --git a/pyre/bench/synth/sre_wasm_min.cranelift.jitstats b/pyre/bench/synth/sre_wasm_min.cranelift.jitstats index 00169699651..bb8f5f34145 100644 --- a/pyre/bench/synth/sre_wasm_min.cranelift.jitstats +++ b/pyre/bench/synth/sre_wasm_min.cranelift.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=8 +bridges_compiled=4 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=1849 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=803 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 diff --git a/pyre/bench/synth/sre_wasm_min.dynasm.jitstats b/pyre/bench/synth/sre_wasm_min.dynasm.jitstats index 00169699651..bb8f5f34145 100644 --- a/pyre/bench/synth/sre_wasm_min.dynasm.jitstats +++ b/pyre/bench/synth/sre_wasm_min.dynasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=8 +bridges_compiled=4 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=1849 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=803 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 diff --git a/pyre/bench/synth/sre_wasm_min.wasm.jitstats b/pyre/bench/synth/sre_wasm_min.wasm.jitstats index 00169699651..bb8f5f34145 100644 --- a/pyre/bench/synth/sre_wasm_min.wasm.jitstats +++ b/pyre/bench/synth/sre_wasm_min.wasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=8 +bridges_compiled=4 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=1849 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=803 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 diff --git a/pyre/bench/synth/sre_wasm_min1.cranelift.jitstats b/pyre/bench/synth/sre_wasm_min1.cranelift.jitstats index 26e4d7b2337..fc1aa727ecc 100644 --- a/pyre/bench/synth/sre_wasm_min1.cranelift.jitstats +++ b/pyre/bench/synth/sre_wasm_min1.cranelift.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=4 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=803 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=603 internal_compile_panics=0 loops_aborted=0 loops_compiled=4 diff --git a/pyre/bench/synth/sre_wasm_min1.dynasm.jitstats b/pyre/bench/synth/sre_wasm_min1.dynasm.jitstats index 26e4d7b2337..fc1aa727ecc 100644 --- a/pyre/bench/synth/sre_wasm_min1.dynasm.jitstats +++ b/pyre/bench/synth/sre_wasm_min1.dynasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=4 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=803 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=603 internal_compile_panics=0 loops_aborted=0 loops_compiled=4 diff --git a/pyre/bench/synth/sre_wasm_min1.wasm.jitstats b/pyre/bench/synth/sre_wasm_min1.wasm.jitstats index 26e4d7b2337..fc1aa727ecc 100644 --- a/pyre/bench/synth/sre_wasm_min1.wasm.jitstats +++ b/pyre/bench/synth/sre_wasm_min1.wasm.jitstats @@ -1,9 +1,11 @@ -bridges_compiled=4 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_rolled_back_with_effects=0 -guard_failures=803 +field_pos_attached_misplaced=0 +field_pos_spec_misplaced=0 +guard_failures=603 internal_compile_panics=0 loops_aborted=0 loops_compiled=4 diff --git a/pyre/pyre-interpreter/src/baseobjspace.rs b/pyre/pyre-interpreter/src/baseobjspace.rs index eeeafaa44dd..bf455c379ab 100644 --- a/pyre/pyre-interpreter/src/baseobjspace.rs +++ b/pyre/pyre-interpreter/src/baseobjspace.rs @@ -4909,6 +4909,25 @@ pub(crate) fn setdictvalue_native(obj: PyObjectRef, name: &str, value: PyObjectR /// key whose hash collides can run a user `__eq__`, and the raw accessor /// reports that as an ordinary miss — the attribute would look absent. fn getdictvalue(obj: PyObjectRef, name: &str) -> Result, PyError> { + // mapdict.py:846-847 `MapdictDictSupport.getdictvalue` overrides the + // `W_Root` default for every mapdict carrier: + // + // ```python + // def getdictvalue(self, space, attrname): + // return self._get_mapdict_map().read(self, attrname, DICT) + // ``` + // + // Reading through `getdict` instead would materialise the + // `("dict", SPECIAL)` wrapper and change the instance's map — see + // [`setdictvalue`]. + if unsafe { crate::objspace::std::mapdict::has_mapdict_storage(obj) } { + return Ok(unsafe { + crate::objspace::std::mapdict::instance_node_getdictvalue( + obj, + rustpython_wtf8::Wtf8::new(name), + ) + }); + } let w_dict = getdict_backing(obj)?; if w_dict.is_null() { return Ok(None); @@ -8321,7 +8340,7 @@ pub fn truncatedint_w(obj: PyObjectRef) -> Result { } } -/// pypy/interpreter/baseobjspace.py:1976-1982 `c_int_w(w_obj)`. +/// pypy/interpreter/baseobjspace.py:2062-2068 `c_int_w(w_obj)`. /// /// ```python /// def c_int_w(self, w_obj): @@ -8331,10 +8350,11 @@ pub fn truncatedint_w(obj: PyObjectRef) -> Result { /// return value /// ``` /// -/// Used by `@unwrap_spec(name="c_int")` (gateway.py). The only caller -/// today is `sys.setrecursionlimit` (pypy/module/sys/vm.py:63), whose -/// argument is typed as `c_int`; values outside the 32-bit signed -/// range surface as `OverflowError` rather than a silent clamp. +/// The port of `@unwrap_spec(name="c_int")` (gateway.py): values outside the +/// 32-bit signed range surface as `OverflowError` rather than a silent clamp. +/// Which sites want it is decided per argument by the upstream `unwrap_spec` — +/// a `c_int` argument narrows here, while a plain `int` one stays machine-word +/// wide (RPython's `int`) and is range-checked at its own bound instead. pub fn c_int_w(obj: PyObjectRef) -> Result { let value = gateway_int_w(obj)?; if !(i32::MIN as i64..=i32::MAX as i64).contains(&value) { @@ -11712,15 +11732,33 @@ pub(crate) fn setdictvalue( name: &str, value: PyObjectRef, ) -> Result { + // mapdict.py:849-850 `MapdictDictSupport.setdictvalue` overrides the + // `W_Root` default above for every mapdict carrier: + // + // ```python + // def setdictvalue(self, space, attrname, w_value): + // return self._get_mapdict_map().write(self, attrname, DICT, w_value) + // ``` + // + // The write goes to the map, not through `getdict`. Routing it through + // `getdict` instead materialises the `("dict", SPECIAL)` wrapper, which + // adds an attribute to the instance's map — a shape change that must only + // happen when app-level code actually asks for `__dict__`. A `NoDict` + // terminator answers `false` here (`write_terminator`), which is the + // AttributeError signal the caller expects. + if unsafe { crate::objspace::std::mapdict::has_mapdict_storage(obj) } { + return Ok(unsafe { + crate::objspace::std::mapdict::instance_node_setdictvalue( + obj, + rustpython_wtf8::Wtf8::new(name), + value, + ) + }); + } let w_dict = getdict_backing(obj)?; if w_dict.is_null() { return Ok(false); } - // For a user instance, `getdict` returns the MapDictStrategy view, so this - // `setitem_str` routes straight to the instance map+storage - // (MapDictStrategy.setitem_str → setdictvalue → map.write DICT, - // mapdict.py:849-850). The earlier C1 explicit `instance_node_setdictvalue` - // dual-write is now subsumed by that routing and removed. unsafe { pyre_object::w_dict_setitem_str(w_dict, name, value) }; Ok(true) } diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index 1da43124bff..16c8cfbcb55 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -4034,6 +4034,52 @@ pub fn is_builtin_len_function(callable: PyObjectRef) -> bool { } } +/// True iff `callable` is the builtin `locals` function object. +/// +/// The JIT walker uses this to recognize the `locals()` residual it can +/// lower to modelled fastlocals reads; a name rebound to anything else +/// carries a different builtin code and answers `false`. +pub fn is_builtin_locals_function(callable: PyObjectRef) -> bool { + is_builtin_code_function(callable, builtin_locals) +} + +/// True iff `callable` is the builtin `vars` function object. +/// +/// `vars()` with no argument is `locals()` (`app_inspect.py:21-24` → +/// `interp_inspect.py:7-11`), so the walker recognizes both under one fold. +pub fn is_builtin_vars_function(callable: PyObjectRef) -> bool { + is_builtin_code_function(callable, builtin_vars) +} + +/// True iff `callable` is the builtin `dir` function object. +/// +/// Zero-argument `dir()` resolves its frame through the same +/// `topframe_for_locals` as `locals()` and reports that frame's sorted key +/// set, so the walker folds it through the same modelled fastlocals +/// expansion; `dir(obj)` carries an argument and never reaches the fold. +pub fn is_builtin_dir_function(callable: PyObjectRef) -> bool { + is_builtin_code_function(callable, builtin_dir) +} + +/// Shared identity test behind the `is_builtin_*_function` predicates: the +/// callable is a function whose code is the builtin-code wrapper around +/// `expected`. +fn is_builtin_code_function( + callable: PyObjectRef, + expected: crate::gateway::BuiltinCodeFn, +) -> bool { + unsafe { + if callable.is_null() || !crate::is_function(callable) { + return false; + } + let code = crate::function_get_code(callable) as PyObjectRef; + if code.is_null() || !crate::gateway::is_builtin_code(code) { + return false; + } + crate::gateway::builtin_code_fn_eq(crate::gateway::builtin_code_get(code), expected) + } +} + /// True iff `callable` is the canonical builtin `repr` function object. /// The JIT walker uses the builtin-code identity to distinguish it from an /// arbitrary replacement stored under the same global name. @@ -11190,22 +11236,23 @@ fn builtin_globals(args: &[PyObjectRef]) -> Result /// The frame whose locals `locals()` / `vars()` / `dir()` report on /// (`interp_inspect.py:7-11 locals` — `ec.gettopframe_nohidden()`). /// -/// Going through `gettopframe_nohidden` is what makes the frame's fastlocals -/// readable, but the mechanism is the VREF force on its first line, not a -/// per-frame `force_frame`: the walk itself only follows `f_backref` and tests -/// `hide()`. That materializes the top frame and nothing below it, which is -/// exactly what this consumer needs — `locals()` reports on the top frame. -/// `getdictscope` reads `locals_cells_stack_w` directly, so an unforced -/// virtualizable hands back an array of nulls, which `fast2locals` renders as -/// an EMPTY mapping rather than a stale one. Reading `CURRENT_FRAME` instead -/// skips the vref force, and under the JIT the caller then sees no locals at -/// all. +/// `gettopframe_nohidden` forces the VREF on its first line and then only +/// follows `f_backref` and tests `hide()`; it runs no per-frame `force_frame`. +/// That vref force is NOT sufficient here, measured: in a compiled `FOR_ITER` +/// loop `locals()` reported `a = 2082, i = 1041` where the frame held +/// `10000 / 5000`. `getdictscope` reads `locals_cells_stack_w` directly, and an +/// unforced virtualizable holds the values the frame last wrote out — so the +/// mapping comes back with correct KEYS (those come from the code object) and +/// STALE values, which no key-only check can see. Hence the explicit force +/// below. Reading `CURRENT_FRAME` instead would skip the vref force too. fn topframe_for_locals() -> *mut crate::PyFrame { let ec = crate::call::getexecutioncontext() as *mut crate::PyExecutionContext; if ec.is_null() { return std::ptr::null_mut(); } - unsafe { (*ec).gettopframe_nohidden() } + let frame = unsafe { (*ec).gettopframe_nohidden() }; + crate::executioncontext::force_frame_before_locals_read(frame); + frame } fn builtin_locals(args: &[PyObjectRef]) -> Result { @@ -11369,6 +11416,54 @@ pub(crate) fn type_dir_default(w_type: PyObjectRef) -> Result Result { + // `_dir_locals` reads the key set through `PyMapping_Keys(locals)`, + // so a locals mapping that overrides `keys()` (`eval(src, {}, A())` + // with `A(dict)`) is honoured; only an exact dict takes the + // `PyDict_Keys` fast path that iterates the storage directly. + let w_keys = if unsafe { + pyre_object::pyobject::is_exact_type(w_locals_dict, &pyre_object::pyobject::DICT_TYPE) + } { + w_locals_dict + } else { + let keys_meth = crate::baseobjspace::getattr_str(w_locals_dict, "keys")?; + crate::call_and_check(keys_meth, &[])? + }; + let keys_iter = crate::baseobjspace::iter(w_keys)?; + let keys = collect_iterable(keys_iter)?; + builtin_sorted(&[w_list_new(keys)]) +} + +/// Trace helper finishing the modelled no-argument `dir()`: sort the key set +/// of the mapping the modelled `fast2locals` expansion just built. +/// +/// Takes the mapping, never a `PyFrame` — that is what keeps `force_frame` +/// unreachable from the emitted call, so nothing on the folded path arms +/// `force_virtualizable_if_necessary`. Declared non-forcing and +/// `CannotRaise`: the input is always the exact dict the fold allocated, so +/// the key enumeration takes the storage fast path and the sort compares +/// `str` keys only. An error is nevertheless reported as `PY_NULL` rather +/// than published, and the caller side-exits on it, so the residual `dir()` +/// re-runs and raises from the interpreter. +pub extern "C" fn jit_dir_names_from_locals(mapping: i64) -> i64 { + let _roots = pyre_object::gc_roots::push_roots(); + let mapping_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(mapping as PyObjectRef); + let mapping = pyre_object::gc_roots::shadow_stack_get(mapping_slot); + match dir_names_from_locals_mapping(mapping) { + Ok(names) => names as i64, + Err(_) => pyre_object::PY_NULL as i64, + } +} + /// `dir([obj])` — PyPy: pypy/module/__builtin__/app_inspect.py dir /// /// Without argument: names in the current local scope (not supported). @@ -11397,21 +11492,7 @@ pub(crate) fn builtin_dir(args: &[PyObjectRef]) -> Result 1 { return Err(crate::PyError::type_error(format!( diff --git a/pyre/pyre-interpreter/src/display.rs b/pyre/pyre-interpreter/src/display.rs index d79c4271851..0db95df41a9 100644 --- a/pyre/pyre-interpreter/src/display.rs +++ b/pyre/pyre-interpreter/src/display.rs @@ -998,6 +998,12 @@ pub unsafe fn py_str_wtf8(obj: PyObjectRef) -> Result { pyre_object::tagged_int::untag_int(obj) ))); } + // The native `__str__` implementations reached below re-enter + // `py_str_wtf8` without pushing a Python frame — a one-element + // `BaseException.args` holding the exception itself recurses here + // forever. Guard the stack so `str(e)` raises RecursionError instead of + // overflowing. + crate::stack_check::stack_check()?; if obj.is_null() { return Ok(Wtf8Buf::from_string("NULL".to_string())); } @@ -1091,6 +1097,13 @@ pub(crate) unsafe fn base_exception_str(obj: PyObjectRef) -> Result Result { + // `space.str(self.args_w[0])` re-enters `py_str_wtf8` on the element, which + // for `e.args = (e,)` lands back here. The re-entry pushes no Python frame + // and sits in tail position, so neither the frame counter nor the stack + // pointer moves and `stack_check` alone cannot see the cycle. Spend a + // recursion unit for this dispatch level, as the equally frameless + // `A.__call__ = A()` chain does. + let _depth = crate::call::enter_native_dispatch(); unsafe { let args = pyre_object::interp_exceptions::w_exception_get_args(obj); if args.is_null() { diff --git a/pyre/pyre-interpreter/src/executioncontext.rs b/pyre/pyre-interpreter/src/executioncontext.rs index f1b0a0cc89e..8cd77f25ddf 100644 --- a/pyre/pyre-interpreter/src/executioncontext.rs +++ b/pyre/pyre-interpreter/src/executioncontext.rs @@ -52,11 +52,11 @@ pub fn force_frame(frame: *mut PyFrame) { /// whole synthetic corpus green throughout. /// /// [`PyExecutionContext::gettopframe_nohidden`] does not substitute. It forces -/// the VREF of the frame it starts from and then walks `f_backref` unforced, so -/// it materializes the top frame only — enough for `locals()`, which reports on -/// that frame, and nothing for a frame handed out some other way (a traceback's -/// `tb_frame`), which reaches `fast2locals` unforced; its null slots render as -/// an EMPTY mapping rather than a stale one. +/// the VREF of the frame it starts from and then walks `f_backref` unforced, and +/// the vref force alone does not materialize `locals_cells_stack_w`: measured, +/// `locals()` on a compiled frame reached that way reports the values the frame +/// last wrote out, under correct keys. So the top frame needs this call as much +/// as one handed out some other way (a traceback's `tb_frame`) does. /// /// # Safety /// `frame` must be a live `PyFrame` (or null). diff --git a/pyre/pyre-interpreter/src/host_seam.rs b/pyre/pyre-interpreter/src/host_seam.rs index 59d9bb51b2e..0e361936c80 100644 --- a/pyre/pyre-interpreter/src/host_seam.rs +++ b/pyre/pyre-interpreter/src/host_seam.rs @@ -499,6 +499,22 @@ pub fn emit_stdout(bytes: &[u8]) { { let _ = ops::write(1, bytes); } + flush_stdout_when_unbuffered(); +} + +/// Settle fd 1 when `-u` / PYTHONUNBUFFERED asked for no buffering. +/// +/// `std::io::Stdout` is a `LineWriter`, so without this a value carrying no +/// newline waits in its buffer until process exit — which is the opposite of +/// what the flag requests, and differs from the block buffering an unflagged +/// run should get. Under sandbox fd 1 is already written through unbuffered +/// `ll_os_write` requests, so there is nothing to settle. +pub fn flush_stdout_when_unbuffered() { + #[cfg(not(feature = "sandbox"))] + if crate::importing::unbuffered_flag() { + use std::io::Write; + let _ = std::io::stdout().flush(); + } } /// Emit bytes to the interpreter's stderr (fd 2). diff --git a/pyre/pyre-interpreter/src/launch_env.rs b/pyre/pyre-interpreter/src/launch_env.rs index ce593e99205..332c25377be 100644 --- a/pyre/pyre-interpreter/src/launch_env.rs +++ b/pyre/pyre-interpreter/src/launch_env.rs @@ -153,7 +153,26 @@ fn is_set_nonempty(name: &str) -> bool { read_raw(name).is_some_and(|value| !value.is_empty()) } +/// Whether the effective `LC_CTYPE` is the legacy C/POSIX locale, which is what +/// coerces utf8_mode to 1; every named locale (en_US, C.UTF-8, …) leaves it 0. fn locale_implies_utf8_mode() -> bool { + // `_Py_SetLocaleFromEnv(LC_CTYPE)` and read the answer back, so the value + // tested is the locale the C library actually installed rather than the + // variables that asked for it. The two differ whenever the environment + // names a locale the system cannot set: `setlocale` keeps C there, and + // utf8_mode has to follow the installed locale, not the unusable name. + // + // Only when the process environment is the authority — an embedding that + // supplied its own table is describing an environment this process does not + // have, and `setlocale("")` would answer from the wrong one, so that path + // keeps the string cascade below. wasm32 has no locale database at all and + // reaches the cascade the same way. + #[cfg(all(unix, feature = "host_env", not(feature = "sandbox")))] + if LAUNCH_ENV.lock().unwrap().is_none() { + rustpython_host_env::locale::setlocale(libc::LC_CTYPE, Some(c"")); + let effective = rustpython_host_env::locale::setlocale(libc::LC_CTYPE, None); + return matches!(effective.as_deref(), None | Some(b"C") | Some(b"POSIX")); + } // An empty variable is treated as unset (`setlocale` POSIX semantics) and // falls through to the next, so `LC_ALL= LC_CTYPE=en_US.UTF-8` resolves to // en_US.UTF-8, not C. diff --git a/pyre/pyre-interpreter/src/lib.rs b/pyre/pyre-interpreter/src/lib.rs index 9c9f0b571de..74b78928eb0 100644 --- a/pyre/pyre-interpreter/src/lib.rs +++ b/pyre/pyre-interpreter/src/lib.rs @@ -71,6 +71,19 @@ pub mod host_seam { } use std::io::Write; let _ = std::io::stdout().write_all(bytes); + flush_stdout_when_unbuffered(); + } + + /// Settle fd 1 when `-u` / PYTHONUNBUFFERED asked for no buffering. + /// + /// `std::io::Stdout` is a `LineWriter`, so without this a value carrying no + /// newline waits in its buffer until process exit — the opposite of what + /// the flag requests. + pub fn flush_stdout_when_unbuffered() { + if crate::importing::unbuffered_flag() { + use std::io::Write; + let _ = std::io::stdout().flush(); + } } /// Emit bytes to the interpreter's stderr (fd 2). @@ -1144,6 +1157,7 @@ pub fn print_output(s: &str) { let _ = crate::host_seam::ops::write(1, s.as_bytes()); #[cfg(not(all(unix, feature = "sandbox")))] print!("{s}"); + crate::host_seam::flush_stdout_when_unbuffered(); } /// Set a hook that receives everything the interpreter writes to fd 2 — diff --git a/pyre/pyre-interpreter/src/module/math/interp_math.rs b/pyre/pyre-interpreter/src/module/math/interp_math.rs index 3cc4921df81..0fca6beb12c 100644 --- a/pyre/pyre-interpreter/src/module/math/interp_math.rs +++ b/pyre/pyre-interpreter/src/module/math/interp_math.rs @@ -749,8 +749,10 @@ pub fn factorial(args: &[PyObjectRef]) -> PyResult { // raise TypeError("'%s' object cannot be interpreted as an integer" // % type(n).__name__) // The check is on `__index__` alone, so floats are rejected for the same - // reason strings are rather than by a numeric-value test. - if unsafe { crate::baseobjspace::lookup_special(args[0], "__index__") }?.is_none() { + // reason strings are rather than by a numeric-value test. `dir(n)` only + // reports membership, so the check must not bind the descriptor — the one + // binding belongs to `get_bigint` below. + if unsafe { crate::baseobjspace::lookup(args[0], "__index__") }.is_none() { return Err(crate::PyError::type_error(format!( "'{}' object cannot be interpreted as an integer", crate::baseobjspace::object_functionstr_type_name(args[0]) diff --git a/pyre/pyre-interpreter/src/module/posix/interp_posix.rs b/pyre/pyre-interpreter/src/module/posix/interp_posix.rs index 1f65b078bc5..4e4b45ffa28 100644 --- a/pyre/pyre-interpreter/src/module/posix/interp_posix.rs +++ b/pyre/pyre-interpreter/src/module/posix/interp_posix.rs @@ -5291,8 +5291,19 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { "configuration names must be strings or integers", )); } - let value = crate::baseobjspace::int_w(crate::baseobjspace::space_index(w)?)?; - Ok(value as i32) + // `conv_confname` narrows to a C `int` and reports a value that does + // not fit rather than truncating it. A truncated name reaches the + // syscall as an unrelated one — `2**40` narrows to 0 — and comes + // back EINVAL, which reads as "no such configuration option" for a + // name the caller never asked about. The object is an int here, so + // an `int_w` that fails did so on width. + let too_large = + || crate::PyError::overflow_error("Python int too large to convert to C int"); + let value = + crate::baseobjspace::int_w(crate::baseobjspace::space_index(w)?).map_err( + |_| too_large(), + )?; + i32::try_from(value).map_err(|_| too_large()) } // os.pathconf(path, name) -> int | None @@ -5367,7 +5378,12 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { ) })? } else { - crate::baseobjspace::int_w(args[0])? as i32 + // Narrowed, not truncated — see `confname_arg`. + i32::try_from(crate::baseobjspace::int_w(args[0])?).map_err(|_| { + crate::PyError::overflow_error( + "Python int too large to convert to C int", + ) + })? }; let v = host_posix::sysconf(name).map_err(|e| io_err(e, ""))?; Ok(pyre_object::w_int_new(v as i64)) diff --git a/pyre/pyre-interpreter/src/module/signal/interp_signal.rs b/pyre/pyre-interpreter/src/module/signal/interp_signal.rs index 97dd211368f..7656ffe3a10 100644 --- a/pyre/pyre-interpreter/src/module/signal/interp_signal.rs +++ b/pyre/pyre-interpreter/src/module/signal/interp_signal.rs @@ -178,13 +178,18 @@ pub unsafe fn walk_signal_handler_roots_area( /// (`gateway.py:646-665` → `int_w(allow_conversion=True)`), which runs /// `__index__`/`__int__` and accepts `int` subclasses, so route through /// the matching helper rather than an exact-tag check. -fn signum_arg(w_signum: PyObjectRef) -> Result { - Ok(crate::baseobjspace::gateway_int_w(w_signum)? as i32) +/// The unwrapped value stays machine-word wide, as the RPython `int` it +/// carries upstream does. Narrowing here instead would truncate the argument +/// before `check_signum_in_range` ever sees it, and a number that aliases a +/// valid signal in its low 32 bits — `(1 << 32) | SIGINT` — would pass the +/// check and act on the signal it aliases. +fn signum_arg(w_signum: PyObjectRef) -> Result { + crate::baseobjspace::gateway_int_w(w_signum) } /// interp_signal.py:285-288 `check_signum_in_range`. -fn check_signum_in_range(signum: i32) -> Result<(), crate::PyError> { - if (1..signalstate::NSIG).contains(&signum) { +fn check_signum_in_range(signum: i64) -> Result<(), crate::PyError> { + if (1..signalstate::NSIG as i64).contains(&signum) { Ok(()) } else { Err(crate::PyError::value_error("signal number out of range")) @@ -197,6 +202,8 @@ fn signal_signal(w_signum: PyObjectRef, w_handler: PyObjectRef) -> Result Result Result { let signum = signum_arg(w_signum)?; check_signum_in_range(signum)?; + let signum = signum as i32; let h = get_handler(signum); Ok(if h.is_null() { pyre_object::w_int_new(0) @@ -906,13 +914,12 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { ) })?; for it in signal_set_items(args[0])? { - let signum = (unsafe { pyre_object::w_int_get_value(it) }) as i32; + // Range-check before narrowing, or a number that + // aliases a valid signal in its low 32 bits passes. + let signum = unsafe { pyre_object::w_int_get_value(it) }; // interp_signal.py:285-288 check_signum_in_range - if !(1..signalstate::NSIG).contains(&signum) { - return Err(crate::PyError::value_error( - "signal number out of range", - )); - } + check_signum_in_range(signum)?; + let signum = signum as i32; rustpython_host_env::signal::sigaddset(&mut set, signum).map_err( |e| { crate::PyError::os_error_with_errno( diff --git a/pyre/pyre-interpreter/src/module/sys/vm.rs b/pyre/pyre-interpreter/src/module/sys/vm.rs index d4d2c43d4c9..627b3a98e6a 100644 --- a/pyre/pyre-interpreter/src/module/sys/vm.rs +++ b/pyre/pyre-interpreter/src/module/sys/vm.rs @@ -2624,6 +2624,7 @@ fn make_std_stream(name: &'static str, fd: i32) -> PyObjectRef { #[cfg(feature = "sandbox")] crate::host_seam::ops::write(1, &bytes) .map_err(|e| crate::host_seam::seam_os_err(e, ""))?; + crate::host_seam::flush_stdout_when_unbuffered(); } return Ok(w_int_new(unsafe { w_str_len(s_obj) } as i64)); } diff --git a/pyre/pyre-interpreter/src/module/zlib/mod.rs b/pyre/pyre-interpreter/src/module/zlib/mod.rs index b3529cc2d26..a82b7ea20bf 100644 --- a/pyre/pyre-interpreter/src/module/zlib/mod.rs +++ b/pyre/pyre-interpreter/src/module/zlib/mod.rs @@ -183,8 +183,11 @@ fn init_compress_type(ns: PyObjectRef) { return Err(crate::PyError::type_error("flush() missing self")); } let id = get_id(args[0]); + // interp_zlib.py:196 `@unwrap_spec(mode="c_int")` — the + // converter reports a value outside the C `int` range rather + // than truncating it into a different flush mode. let mode = match args.get(1).copied() { - Some(o) if !unsafe { is_none(o) } => crate::baseobjspace::int_w(o)? as i32, + Some(o) if !unsafe { is_none(o) } => crate::baseobjspace::c_int_w(o)?, _ => backend::Z_FINISH, }; let mut reg = COMPRESSORS.lock().unwrap(); diff --git a/pyre/pyre-interpreter/src/objspace/std/mapdict.rs b/pyre/pyre-interpreter/src/objspace/std/mapdict.rs index d2fe3503081..6eb96a1d8a8 100644 --- a/pyre/pyre-interpreter/src/objspace/std/mapdict.rs +++ b/pyre/pyre-interpreter/src/objspace/std/mapdict.rs @@ -603,9 +603,11 @@ pub unsafe fn instance_setclass(obj: PyObjectRef, w_cls: PyObjectRef) { /// `setdictvalue` routed to the mapdict node layer (mapdict.py:849-850 /// `MapdictDictSupport.setdictvalue` → `map.write(self, attrname, DICT, -/// w_value)`, dispatch at mapdict.py:68-75). C1 calls this alongside the legacy -/// INSTANCE_DICT store so map+storage tracks every user-instance DICT write and -/// can become the read authority in C2. +/// w_value)`, dispatch at mapdict.py:68-75). +/// +/// Returns `map.write`'s flag: `false` when the map is rooted at a +/// `NoDictTerminator` (`__slots__`, no instance `__dict__`), which is the +/// AttributeError signal `object_setattr` raises on. /// /// `dont_look_inside` makes this a residual-call boundary for the JIT /// CodeWriter: `setdictvalue` is JIT-reachable via STORE_ATTR, but the node @@ -627,12 +629,7 @@ pub unsafe fn instance_node_setdictvalue( ensure_mapdict_initialized(obj); let inst = &mut *(obj as *mut pyre_object::W_ObjectObject); let map = inst._get_mapdict_map(); - let flag = node_write(map, inst, name, DICT, value); - debug_assert!( - flag, - "node_write returned false for a DICT attribute on a hasdict instance" - ); - flag + node_write(map, inst, name, DICT, value) } /// Whether `map` is rooted at a `DevolvedDictTerminator` (mapdict.py:382), @@ -4452,9 +4449,12 @@ impl pyre_object::dictmultiobject::DictStrategy for MapDictStrategy { } /// mapdict.py:1172-1175 `setitem_str` — `flag = w_obj.setdictvalue(...); - /// assert flag`. `instance_node_setdictvalue` debug_asserts the flag itself. + /// assert flag`. The receiver is the instance behind a materialised + /// `__dict__` view, so its terminator has a dict and the write cannot fail. unsafe fn setitem_str(&self, w_dict: PyObjectRef, key: &str, w_value: PyObjectRef) { - instance_node_setdictvalue(mapdict_strategy_unerase(w_dict), Wtf8::new(key), w_value); + let flag = + instance_node_setdictvalue(mapdict_strategy_unerase(w_dict), Wtf8::new(key), w_value); + debug_assert!(flag, "mapdict.py:1174 assert flag"); } /// mapdict.py:1185-1196 `setdefault`. diff --git a/pyre/pyre-interpreter/src/pyframe.rs b/pyre/pyre-interpreter/src/pyframe.rs index 7b1d286c3b1..7eccedebeda 100644 --- a/pyre/pyre-interpreter/src/pyframe.rs +++ b/pyre/pyre-interpreter/src/pyframe.rs @@ -3736,6 +3736,23 @@ impl PyFrame { Ok(()) } + /// Whether a modelled `fast2locals` can reproduce this code object's + /// locals mapping from the fastlocals alone. + /// + /// Only the plain-locals half of `fast2locals` is modelled: the cell / + /// freevar half (pyframe.py:576-598) reads through `w_cell_get` per slot + /// and the `CO_FAST_HIDDEN` slots are skipped, so a code object carrying + /// either is refused and keeps the residual path. A non-OPTIMIZED + /// (module / class / exec) frame is refused too: it hands back its LIVE + /// namespace rather than the independent copy + /// [`PyFrame::frame_locals_snapshot`] builds for a function frame. + pub fn code_locals_are_plain_fastlocals(code: &CodeObject) -> bool { + code.flags.contains(CodeFlags::OPTIMIZED) + && code.cellvars.is_empty() + && code.freevars.is_empty() + && !(0..code.varnames.len()).any(|i| hidden_local(code, i)) + } + /// pyframe.py:208-218 `make_arguments` — build Arguments from the value /// stack. `methodcall` and `w_function` are stored on the resulting /// Arguments for diagnostic use (better error messages on argument @@ -4396,6 +4413,60 @@ fn delitem_str_object(w_obj: PyObjectRef, name: &str) -> Result<(), crate::PyErr } } +/// `pyframe.py:557 self.space.newdict(instance=True)` — the mapping half of +/// `fast2locals`, for a trace that models the fastlocals reads instead of +/// residualizing `interp_inspect.py:7-11 locals`. +/// +/// Takes no `PyFrame`: the modelled expansion feeds the slot values in as +/// ordinary Ref operands, so nothing reachable from here can call +/// [`crate::executioncontext::force_frame`]. That is the whole point of the +/// split — a helper that touched the frame would re-arm the escape this +/// modelling exists to remove. The frame's own `w_locals` cache is +/// deliberately NOT populated: an OPTIMIZED frame hands out an independent +/// copy per read (`frame_locals_snapshot`), so the cache is never the object +/// application code sees, and a later `f_locals` rebuilds it from the +/// fastlocals anyway. +pub extern "C" fn jit_locals_dict_new() -> i64 { + unsafe { pyre_object::w_dict_new() as i64 } +} + +/// `pyframe.py:566-568 fast2locals` for ONE visible fastlocal slot: bind +/// `code.varnames[index]` to `value` in `dict`. +/// +/// Unbound slots are not routed here at all — the modelled expansion emits a +/// `guard_isnull` for them and skips the store, which is `fast2locals`' +/// `delitem` arm applied to a mapping that never held the key. +/// +/// Returns `dict` so the unrolled slot chain threads the (possibly forwarded) +/// mapping from one store to the next instead of holding a raw address across +/// an allocating call. +/// +/// # Safety +/// `dict` must be a live dict, `code` a live `CodeObject` with +/// `index < varnames.len()`, and `value` a live non-null `W_Root`. +pub extern "C" fn jit_locals_dict_setitem_local( + dict: i64, + code: i64, + index: i64, + value: i64, +) -> i64 { + let _roots = pyre_object::gc_roots::push_roots(); + let dict_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(dict as PyObjectRef); + let value_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(value as PyObjectRef); + let code = unsafe { &*(code as usize as *const CodeObject) }; + let name: &str = &code.varnames[index as usize]; + unsafe { + pyre_object::dictmultiobject::w_dict_setitem_str( + pyre_object::gc_roots::shadow_stack_get(dict_slot), + name, + pyre_object::gc_roots::shadow_stack_get(value_slot), + ); + } + pyre_object::gc_roots::shadow_stack_get(dict_slot) as i64 +} + #[cfg(test)] mod tests { use super::load_const_from_code; diff --git a/pyre/pyre-interpreter/src/type_methods.rs b/pyre/pyre-interpreter/src/type_methods.rs index 6a28b45cd0f..bdac374c718 100644 --- a/pyre/pyre-interpreter/src/type_methods.rs +++ b/pyre/pyre-interpreter/src/type_methods.rs @@ -704,7 +704,6 @@ pub fn list_method_reverse(args: &[PyObjectRef]) -> Result Result { require_list_receiver(args, "sort", true)?; - let list = args[0]; // Keep the argument decoding shared with `sorted()` before changing the // receiver's visible storage. let (positional, kwargs) = crate::builtins::split_builtin_kwargs(args); @@ -719,16 +718,26 @@ pub fn list_method_sort(args: &[PyObjectRef]) -> Result &'static mut JitDriverPair { let ptr = (callbacks::get().driver_pair)(); unsafe { &mut *(ptr as *mut JitDriverPair) } } + +/// Get the JIT driver pair, or `None` when no callback table is installed. +/// +/// A skeleton walk drives `jitcode_dispatch` directly with no pyre-jit eval +/// behind it, so there is no driver to reach. Cold bookkeeping paths that only +/// annotate an outcome take this spelling and skip themselves, rather than +/// panicking in `callbacks::get`. +#[inline] +pub fn try_driver_pair() -> Option<&'static mut JitDriverPair> { + let ptr = (callbacks::try_get()?.driver_pair)(); + Some(unsafe { &mut *(ptr as *mut JitDriverPair) }) +} diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs index 5976819c5d2..e7a325e10f6 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -3225,6 +3225,7 @@ pub(crate) fn try_execute_residual_call_via_executor( { (cb.drain_backend_jit_exc)(); } + crate::state::note_vable_escape_abort(); return Err(DispatchError::VableEscapedDuringResidualCall { pc: op_pc }); } } @@ -4970,6 +4971,23 @@ pub(crate) fn dispatch_residual_call_iRd_kind( return Ok((DispatchOutcome::Continue, op.next_pc)); } + // Zero-argument `locals()` / `vars()` / `dir()` on the walk's own portal + // frame: model `fast2locals`' fastlocals reads as `getarrayitem_vable_r` + // plus a non-forcing dict-build chain — the shape the meta-tracer produces + // upstream, where `pyframe.py:539 fast2locals` is `@jit.unroll_safe` and + // therefore looked into. This arm runs BEFORE + // `try_execute_residual_call_via_executor` arms the vable token protocol, + // which is the point: the opaque residual is what turns the locals-read + // barrier into `VableEscapedDuringResidualCall`. Any non-matching shape + // falls through to the generic residual (SAFE). + if ctx.is_authoritative_executor + && dst_bank == 'r' + && ei.pyre_helper == majit_ir::PyreHelperKind::CallFn + && try_walker_specialize_builtin_locals(ctx, code, op, &r_args, dst)?.is_some() + { + return Ok((DispatchOutcome::Continue, op.next_pc)); + } + // `math.sqrt(x)` / `float(x)` on an exact numeric argument: inline the // domain-guarded pure `CALL_F(sqrt_nonneg_jit)` (ll_math.rs) resp. the // `CastIntToFloat` / identity conversion instead of the opaque diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs index 792c7a0d17d..d5b139ae91f 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs @@ -6558,6 +6558,356 @@ pub(crate) fn try_walker_specialize_builtin_range( Ok(Some(())) } +/// Unrolling bound for [`try_walker_specialize_builtin_locals`]. +/// +/// The modelled expansion is a straight-line unroll of `fast2locals`' slot +/// loop, one guard plus at most one store per fastlocal. Upstream bounds the +/// same unroll by `@jit.unroll_safe` on a loop whose trip count is the code +/// object's `numlocals`; the explicit ceiling here keeps a pathologically +/// wide frame from turning one `locals()` into hundreds of trace ops. Over +/// the bound the fold declines and the generic residual runs (SAFE). +const MAX_MODELLED_FASTLOCALS: usize = 32; + +/// Which builtin [`try_walker_specialize_builtin_locals`] is standing in for. +/// +/// All three resolve their frame through the same `topframe_for_locals` and +/// read the same fastlocals, so one modelled expansion serves them; they +/// differ only in what they make of the resulting mapping. +#[derive(Clone, Copy, PartialEq, Eq)] +enum FrameLocalsBuiltin { + /// `locals()` / `vars()` — the mapping itself is the result. + Mapping, + /// `dir()` — the mapping's sorted key set is the result. + SortedNames, +} + +/// Zero-argument `locals()` / `vars()` / `dir()` on the walk's own portal +/// frame: model +/// `pyframe.py:539-583 fast2locals` in the trace instead of residualizing +/// `interp_inspect.py:7-11 locals` → `pyframe.py:525-529 getdictscope`. +/// +/// `fast2locals` is `@jit.unroll_safe`, and `policy.py:60-67` cancels +/// `contains_loop` for unroll_safe graphs, so upstream LOOKS INSIDE it: each +/// `self.locals_cells_stack_w[i]` lowers to `getarrayitem_vable_r` +/// (`jtransform.py:1877 do_fixed_list_getitem`), answered from +/// `metainterp.virtualizable_boxes`, and `jtransform.py:2164-2172 +/// rewrite_op_jit_force_virtualizable` returns `[]` for a read the tracer is +/// inside. There is no residual and no virtualizable force anywhere on the +/// upstream locals-read path. +/// +/// Pyre residualizes the same read as one opaque `bh_call_fn(locals, PY_NULL)` +/// `CallMayForce`, which arms `virtualizable.py:281-291 +/// force_virtualizable_if_necessary` for the whole call; the read barrier +/// `force_frame_before_locals_read` then clears `TOKEN_TRACING_RESCALL` and +/// `tracing_after_residual_call` reads that clear as an escape +/// (`VableEscapedDuringResidualCall`), losing the loop. The deviation is the +/// residual BOUNDARY, not the barrier — `rvirtualizable.py:49-53` injects the +/// same hook on reads upstream and `pyjitpl.py:3373-3390` aborts +/// unconditionally on a detected force — so this removes the boundary and +/// leaves the barrier live for every shape it declines. +/// +/// Emitted shape, mirroring `pyframe.py:551-568` slot by slot: +/// `guard_value(callable)`, one `getarrayitem_vable_r(frame, ConstInt(i))` per +/// fastlocal (the same lowering `emit_load_fast_ref!` already emits for +/// LOAD_FAST), a `guard_isnull` / `guard_nonnull` pinning the slot's bound-ness, +/// and a plain non-forcing `Call` chain `newdict` → `setitem_str` per bound +/// slot. None of those ops can reach `force_frame`, so nothing arms the vable +/// protocol. +/// +/// `dir()` appends one further non-forcing `Call` to +/// `jit_dir_names_from_locals`, the split-out tail of `builtin_dir`'s +/// no-argument path, which turns that mapping into its sorted key set. It +/// takes the mapping and not the frame, so it too cannot reach `force_frame`. +/// +/// Returns `None` (fall through to the generic residual, SAFE — exactly +/// today's behaviour) for every other shape: a rebound `locals` / `vars` / +/// `dir` name, +/// a bound receiver, any argument, an inline sub-walk, a frame that is not the +/// standard virtualizable the boxes describe, a hidden top frame, a +/// non-OPTIMIZED (module / class / exec) frame, cellvars / freevars / +/// `CO_FAST_HIDDEN` slots, a slot the shadow cannot answer with a Ref, and a +/// frame wider than [`MAX_MODELLED_FASTLOCALS`]. +pub(crate) fn try_walker_specialize_builtin_locals( + ctx: &mut WalkContext<'_, '_, Sym>, + code: &[u8], + op: &DecodedOp, + r_args: &[OpRef], + dst: usize, +) -> Result, DispatchError> { + // Plain zero-argument `bh_call_fn(callable, PY_NULL)` shape only. + if r_args.len() != 2 { + return Ok(None); + } + let arg_concretes = read_ref_var_list_concrete(code, op, 1, ctx); + let (ConcreteValue::Ref(concrete_callable), ConcreteValue::Ref(null_or_self)) = + (arg_concretes[0], arg_concretes[1]) + else { + return Ok(None); + }; + // A non-null `null_or_self` is a bound receiver `bh_call_fn_impl` prepends + // as arg0 — not a plain `locals()` call. + if concrete_callable.is_null() || !null_or_self.is_null() { + return Ok(None); + } + // `vars()` with no argument delegates straight to `builtin_locals` + // (`app_inspect.py:21-24`), so both names share the fold; `vars(obj)` and + // `dir(obj)` carry an extra operand and are already excluded by the arity + // gate. + let fold = if pyre_interpreter::builtins::is_builtin_locals_function(concrete_callable) + || pyre_interpreter::builtins::is_builtin_vars_function(concrete_callable) + { + FrameLocalsBuiltin::Mapping + } else if pyre_interpreter::builtins::is_builtin_dir_function(concrete_callable) { + FrameLocalsBuiltin::SortedNames + } else { + return Ok(None); + }; + // The modelled reads answer from `virtualizable_boxes`, which describe the + // PORTAL frame only. An inline sub-walk publishes a different concrete + // frame whose locals live in the callee shadow, not in those boxes. + if ctx.fbw_mode.inline_subwalk || current_inline_concrete_frame() != 0 { + return Ok(None); + } + let (Some(vable_op), Some(vable_ptr)) = ( + ctx.trace_ctx.standard_virtualizable_box(), + ctx.trace_ctx.standard_virtualizable_ptr(), + ) else { + return Ok(None); + }; + // The frame `locals()` reports on is `ec.gettopframe_nohidden()` + // (`interp_inspect.py:7-11`). Resolve it the same way and require it to BE + // the standard virtualizable: a hidden portal frame, or any deeper frame + // handed out through the backref chain, resolves elsewhere and declines. + let ec = pyre_interpreter::call::getexecutioncontext(); + if ec.is_null() { + return Ok(None); + } + let frame = unsafe { (*ec).gettopframe_nohidden() }; + if frame.is_null() || frame as usize != vable_ptr { + return Ok(None); + } + let frame_ref = unsafe { &*frame }; + let code_ptr = unsafe { pyre_interpreter::pyframe::pyframe_get_pycode(frame_ref) }; + let code_obj = unsafe { &*code_ptr }; + if !pyre_interpreter::PyFrame::code_locals_are_plain_fastlocals(code_obj) { + return Ok(None); + } + let numlocals = code_obj.varnames.len(); + if numlocals > MAX_MODELLED_FASTLOCALS { + return Ok(None); + } + // `locals_cells_stack_w` is PyFrame's only virtualizable array + // (`virtualizable_gen.rs arrays`), so array index 0 names it. + let Some(info) = ctx.trace_ctx.virtualizable_info().cloned() else { + return Ok(None); + }; + let Some(lengths) = ctx + .trace_ctx + .virtualizable_array_lengths() + .map(<[usize]>::to_vec) + else { + return Ok(None); + }; + if info.num_arrays() != 1 || lengths.first().copied().unwrap_or(0) < numlocals { + return Ok(None); + } + let (Some(fdescr), Some(adescr)) = ( + info.array_field_descrs().first().cloned(), + info.array_descrs.first().cloned(), + ) else { + return Ok(None); + }; + // Resolve every slot's shadow entry BEFORE emitting anything, so a slot the + // shadow cannot answer declines from a clean trace position. The read is + // the standard-virtualizable arm of `_opimpl_getarrayitem_vable` + // (`virtualizable_boxes[index]`), which records no op — the emit pass below + // re-runs it through the real entry point. + let mut slots: Vec = Vec::with_capacity(numlocals); + for i in 0..numlocals { + let flat = info.get_index_in_array(0, i, &lengths); + let Some((slot_op, entry_value)) = ctx.trace_ctx.virtualizable_entry_at(flat) else { + return Ok(None); + }; + // The value comes from the SHADOW, never from `frame.locals_w()`. An + // unsynchronized virtualizable's heap array holds whatever the frame + // last wrote out — measured one FOR_ITER iteration behind on the loop + // variable — which is exactly the staleness the read barrier's + // `force_now` repairs before the residual reads it. The shadow already + // holds the repaired value, so sourcing from it reproduces the forced + // residual's answer without the force, and it is what upstream's + // traced-in `fast2locals` reads (`getarrayitem_vable_r` answered from + // `virtualizable_boxes`). + // + // Prefer the OpRef's own concrete over the `virtualizable_values` copy: + // the op table is the GC-forwarded channel, so a Ref that moved across + // an earlier residual is current there. + let value = match ctx + .trace_ctx + .concrete_of_opref(slot_op) + .filter(|v| matches!(v, majit_ir::Value::Ref(_))) + .unwrap_or(entry_value) + { + majit_ir::Value::Ref(gcref) => gcref.as_usize() as pyre_object::PyObjectRef, + _ => return Ok(None), + }; + slots.push(value); + } + + // Authentic mapping, built on the plain eval loop exactly as the skipped + // residual would — through the SAME helpers the emitted calls invoke, so + // the recording-time value and the compiled loop's value cannot diverge. + let (concrete_dict, concrete_result) = { + let _roots = pyre_object::gc_roots::push_roots(); + let value_roots: Vec = slots + .iter() + .map(|&value| { + let slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(value); + slot + }) + .collect(); + let dict_root = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root( + pyre_interpreter::pyframe::jit_locals_dict_new() as pyre_object::PyObjectRef + ); + for (i, &value_root) in value_roots.iter().enumerate() { + let value = pyre_object::gc_roots::shadow_stack_get(value_root); + if value.is_null() { + continue; + } + pyre_interpreter::pyframe::jit_locals_dict_setitem_local( + pyre_object::gc_roots::shadow_stack_get(dict_root) as i64, + code_ptr as i64, + i as i64, + value as i64, + ); + } + // `dir()`'s tail runs here too, so the recorded result is produced by + // the very helper the emitted call names. It allocates, so the + // mapping is re-read from its pinned slot afterwards. + let result = match fold { + FrameLocalsBuiltin::Mapping => pyre_object::gc_roots::shadow_stack_get(dict_root), + FrameLocalsBuiltin::SortedNames => { + pyre_interpreter::builtins::jit_dir_names_from_locals( + pyre_object::gc_roots::shadow_stack_get(dict_root) as i64, + ) as pyre_object::PyObjectRef + } + }; + (pyre_object::gc_roots::shadow_stack_get(dict_root), result) + }; + // The tail reports a failure as PY_NULL instead of publishing it; nothing + // has been emitted yet, so decline and let the residual raise. + if concrete_result.is_null() { + return Ok(None); + } + + // --- emit the specialized IR (walker-native) --- + // Pin the callable identity (LOAD_GLOBAL `locals` is usually already a + // constant via the namespace cell fold). + let callable_op = r_args[0]; + if !callable_op.is_constant() { + let expected = ctx.trace_ctx.const_ref(concrete_callable as i64); + ctx.trace_ctx + .record_guard(OpCode::GuardValue, &[callable_op, expected], 0); + walker_capture_snapshot_for_last_guard(ctx, op.pc)?; + ctx.trace_ctx + .heap_cache_mut() + .replace_box(callable_op, expected); + } + let concrete_dict_value = majit_ir::Value::Ref(majit_ir::GcRef(concrete_dict as usize)); + // The code object is the jitdriver green this trace is keyed on + // (`interp_jit.py:23 greens = ['next_instr', 'is_being_profiled', + // 'pycode']`), so its address is a constant for the compiled loop and + // carries no guard of its own. + let code_const = ctx.trace_ctx.const_int(code_ptr as i64); + let mut dict_op = ctx.trace_ctx.call_ref_typed_with_effect( + pyre_interpreter::pyframe::jit_locals_dict_new as *const (), + &[], + &[], + majit_ir::EffectInfo::new( + majit_ir::ExtraEffect::CannotRaise, + majit_ir::OopSpecIndex::None, + ), + ); + ctx.trace_ctx + .set_opref_concrete(dict_op, concrete_dict_value); + for (i, &value) in slots.iter().enumerate() { + // `self.locals_cells_stack_w[i]` — `jtransform.py:1877 + // do_fixed_list_getitem`, the identical lowering `emit_load_fast_ref!` + // emits for LOAD_FAST. On the standard virtualizable this resolves to + // `virtualizable_boxes[index]` and records no op. + let index_const = ctx.trace_ctx.const_int(i as i64); + let (slot_op, _) = ctx.trace_ctx.vable_getarrayitem_ref_indexed( + op.pc, + vable_op, + index_const, + i as i64, + fdescr.clone(), + adescr.clone(), + ); + // `pyframe.py:566-571` branches on the slot being bound; pin the + // direction so a slot that changes bound-ness side-exits instead of + // publishing a mapping with the wrong key set. A slot the trace + // already holds as a constant needs no guard. + let bound = !value.is_null(); + if !slot_op.is_constant() { + let opcode = if bound { + OpCode::GuardNonnull + } else { + OpCode::GuardIsnull + }; + walker_emit_fold_guard_with_snapshot(ctx, op.pc, opcode, &[slot_op])?; + } + if !bound { + continue; + } + dict_op = ctx.trace_ctx.call_ref_typed_with_effect( + pyre_interpreter::pyframe::jit_locals_dict_setitem_local as *const (), + &[dict_op, code_const, index_const, slot_op], + &[ + majit_ir::Type::Ref, + majit_ir::Type::Int, + majit_ir::Type::Int, + majit_ir::Type::Ref, + ], + majit_ir::EffectInfo::new( + majit_ir::ExtraEffect::CannotRaise, + majit_ir::OopSpecIndex::None, + ), + ); + // Every link of the chain names the SAME mapping, so the post-build + // address is the live one for all of them. + ctx.trace_ctx + .set_opref_concrete(dict_op, concrete_dict_value); + } + let result_op = match fold { + FrameLocalsBuiltin::Mapping => dict_op, + // `builtin_dir`'s no-argument tail, split out so the trace and the + // eval loop enumerate and sort one key set through one implementation. + FrameLocalsBuiltin::SortedNames => { + let names_op = ctx.trace_ctx.call_ref_typed_with_effect( + pyre_interpreter::builtins::jit_dir_names_from_locals as *const (), + &[dict_op], + &[majit_ir::Type::Ref], + majit_ir::EffectInfo::new( + majit_ir::ExtraEffect::CannotRaise, + majit_ir::OopSpecIndex::None, + ), + ); + ctx.trace_ctx.set_opref_concrete( + names_op, + majit_ir::Value::Ref(majit_ir::GcRef(concrete_result as usize)), + ); + // PY_NULL is the tail's unpublished-error report; side-exit on it + // so the residual `dir()` re-runs and raises from the eval loop. + walker_emit_fold_guard_with_snapshot(ctx, op.pc, OpCode::GuardNonnull, &[names_op])?; + names_op + } + }; + write_residual_call_result_to_dst(ctx, op.pc, dst, 'r', result_op)?; + Ok(Some(())) +} + /// `math.sqrt(x)` on an exact int/float argument: inline the domain-guarded /// pure `CALL_F(sqrt_nonneg_jit)` (ll_math.rs `ll_math_sqrt` → `sqrt_nonneg`, /// EF_ELIDABLE_CANNOT_RAISE) instead of the opaque diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index e00d69374f4..2bad450819d 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -3198,15 +3198,44 @@ pub(crate) fn note_root_trace_too_long( } } +/// Stage `reason` as the abort the walker is returning, so the single +/// `aborted_tracing` that follows counts it under that counter instead of the +/// `Generic` catch-all. +/// +/// Naming the reason is pure accounting: the abort itself travels in the +/// `DispatchError` the caller returns and does not consult this slot. A +/// skeleton walk has no driver to stage onto, so it skips the note instead of +/// panicking in `callbacks::get`. +fn stage_walker_abort_reason(reason: i32) { + let Some((driver, _)) = crate::driver::try_driver_pair() else { + return; + }; + driver.meta_interp_mut().stage_abort_reason(reason); +} + /// Name `Counters.ABORT_FORCE_QUASIIMMUT` as the reason for the abort the /// walker is returning (`pyjitpl.py:1116`), so the single `aborted_tracing` /// that follows counts it under `profiler.abort_force_quasiimmut` instead of /// the `Generic` catch-all. pub(crate) fn note_force_quasi_immut_abort() { - let (driver, _) = crate::driver::driver_pair(); - driver - .meta_interp_mut() - .stage_abort_reason(majit_metainterp::counters::ABORT_FORCE_QUASIIMMUT); + stage_walker_abort_reason(majit_metainterp::counters::ABORT_FORCE_QUASIIMMUT); +} + +/// Name `Counters.ABORT_ESCAPE` as the reason for the abort the walker returns +/// when `tracing_after_residual_call` reports the virtualizable was forced +/// (`pyjitpl.py:3389-3390` raises `SwitchToBlackhole(Counters.ABORT_ESCAPE)` +/// there). Without this the reason ladder falls through to +/// `AbortReason::Generic`, whose `as_int()` is `ABORT_BRIDGE` — so the escape +/// lands in the bucket `jitprof.rs ABORT_COUNTER_KINDS` labels +/// `bridge_or_generic`, and the tally reads as bridge activity that never +/// happened. +/// +/// `raising_exception` does not travel this channel: pyre computes it from the +/// residual's own `exec_result` (`residual_call.rs`), where the escape's `Err` +/// arm already sets it, so upstream's hardcoded `True` needs no counterpart +/// here. +pub(crate) fn note_vable_escape_abort() { + stage_walker_abort_reason(majit_metainterp::counters::ABORT_ESCAPE); } pub(crate) fn wrapfloat(ctx: &mut TraceCtx, value: OpRef) -> OpRef { diff --git a/pyre/pyre-jit/tests/gc_stress.rs b/pyre/pyre-jit/tests/gc_stress.rs index bfb284ab9bd..6532bc5d555 100644 --- a/pyre/pyre-jit/tests/gc_stress.rs +++ b/pyre/pyre-jit/tests/gc_stress.rs @@ -1046,6 +1046,48 @@ while i < 40: ); } +/// `list.sort` decodes `reverse=` by calling the object's `__bool__`, which runs +/// Python and can collect. The receiver and the `key=` callable are read out of +/// the builtin's native argument copy, which the collector does not update, so +/// both have to be pinned across that call rather than after it. +#[test] +fn list_sort_receiver_and_key_survive_the_reverse_bool_callback() { + run_on_worker( + r#" +import gc + +class Flag: + def __init__(self, v): + self.v = v + def __bool__(self): + gc.collect() + return self.v + +def keyf(o): + gc.collect() + return -o + +i = 0 +while i < 40: + items = [5, 3, 9, 1, 7] + items.sort(reverse=Flag(True)) + assert items == [9, 7, 5, 3, 1], items + + other = [5, 3, 9, 1, 7] + other.sort(key=keyf, reverse=Flag(False)) + assert other == [9, 7, 5, 3, 1], other + + plain = [5, 3, 9, 1, 7] + plain.sort(key=None, reverse=Flag(False)) + assert plain == [1, 3, 5, 7, 9], plain + i += 1 +"#, + "", + "list.sort reverse-callback root checks", + "list.sort reverse callback GC rooting program failed", + ); +} + /// The reflected binary-operator helper holds both operands and the reflected /// implementation in Rust locals while `L.__add__` / `L.__sub__` run Python /// and collect before `R.__radd__` / `R.__rsub__` use those values. diff --git a/pyre/pyre-object/src/listobject.rs b/pyre/pyre-object/src/listobject.rs index f1b23a925cc..364992ec786 100644 --- a/pyre/pyre-object/src/listobject.rs +++ b/pyre/pyre-object/src/listobject.rs @@ -1702,6 +1702,14 @@ pub unsafe fn w_list_init_items(obj: PyObjectRef, items: Vec) { let strategy = list_strategy_for(&items); let mut storage = build_list_storage(&items, strategy); let obj = crate::gc_roots::shadow_stack_get(obj_slot); + // `w_list_clear` is the twin destructive re-installation and holds the + // stripe lock across its `drop_object_items`; a concurrent reader must not + // see the half-installed state this one writes either. Take the lock after + // the allocating `build_list_storage` so the acquire cannot deadlock behind + // a collection, and reload `obj` behind it because a contended acquire + // blocks through `before_external_block`. + let _list_guard = w_list_lock(obj); + let obj = crate::gc_roots::shadow_stack_get(obj_slot); let list = &mut *(obj as *mut W_ListObject); // `drop_object_items`' `try_gc_owns_object` query is a safepoint and the // fresh blocks have no heap edge until the stores below, so close their pin