diff --git a/majit/majit-ir/src/effectinfo.rs b/majit/majit-ir/src/effectinfo.rs index fbb8346c397..3dcbd28a3ec 100644 --- a/majit/majit-ir/src/effectinfo.rs +++ b/majit/majit-ir/src/effectinfo.rs @@ -901,6 +901,14 @@ pub enum PyreHelperKind { /// helper (`pyopcode.py:866-870`). Same standing as /// [`PyreHelperKind::LoadLocals`]. LoadBuildClass, + /// `bh_load_import_fn(frame)` — the builtin lookup half of IMPORT_NAME. + /// The following invocation is emitted through [`PyreHelperKind::CallFn`] + /// so gateway builtins retain their ordinary meta-traceable call shape. + LoadImport, + /// `bh_load_import_locals_fn(frame)` — IMPORT_NAME's locals argument + /// (`pyopcode.py:1119-1125`). Infallible, same standing as + /// [`PyreHelperKind::LoadLocals`]. + LoadImportLocals, /// `bh_call_fn_N(callable, null_or_self, args...)` — the CALL-family /// Python-call helper. `null_or_self` (arg index 1) is a sentinel /// the helper checks before use (a non-null receiver is prepended as diff --git a/majit/majit-translate/src/front/mir.rs b/majit/majit-translate/src/front/mir.rs index 060e4f35c63..cbb49f94cfb 100644 --- a/majit/majit-translate/src/front/mir.rs +++ b/majit/majit-translate/src/front/mir.rs @@ -10347,9 +10347,12 @@ impl<'a> Lowering<'a> { /// indexing. RPython represents the same operation directly as /// `getitem` / `setitem`; the Rust trait shim is opaque in Charon and must /// not survive as a residual call. Range implementations share the same - /// name, so accept only the literal `usize` second argument; RangeFrom / - /// RangeTo continue through `front::slice_index`'s bounded getslice - /// rewrites. + /// name, so accept only an integer-bank second argument. `usize` is the + /// ordinary spelling, but Charon may preserve an integer alias instead of + /// the literal atom; [`vec_index_type_is_scalar`] performs the same + /// representation test used by `Vec::index`. RangeFrom / RangeTo remain + /// Ref-bank values and continue through `front::slice_index`'s bounded + /// getslice rewrites. fn is_slice_scalar_index_call(&self, reg: &RegularCall, index_ty: Option<&TyRef>) -> bool { let CallKind::Fun(FunId::Regular { id }) = ®.kind else { return false; @@ -10360,7 +10363,16 @@ impl<'a> Lowering<'a> { "core::slice::index::::index" | "core::slice::index::::index_mut" ) }); - is_index && index_ty.and_then(|ty| self.tyref_literal_uint_atom(ty)) == Some("Usize") + let callsite_index_is_scalar = index_ty + .is_some_and(|ty| vec_index_type_is_scalar(ty, self.llbc)) + || reg + .generics + .get("types") + .and_then(serde_json::Value::as_array) + .and_then(|types| types.get(1)) + .and_then(|ty| serde_json::from_value::(ty.clone()).ok()) + .is_some_and(|ty| vec_index_type_is_scalar(&ty, self.llbc)); + is_index && callsite_index_is_scalar } fn is_slice_scalar_index_mut_call(&self, reg: &RegularCall) -> bool { @@ -27306,4 +27318,48 @@ mod tests { "From for usize should use RPython's cast_bool_to_uint path" ); } + + /// `split_builtin_kwargs` returns `&args[..args.len() - 1]` after proving + /// the slice non-empty. MIR carries both the stop and receiver through + /// block-link aliases, so the RangeTo proof must resolve those aliases + /// before recognizing the orthodox `getslice_minusone` shape. + #[test] + #[ignore] + fn split_builtin_kwargs_rangeto_aliases_lower_to_getslice() { + use crate::model::{CallTarget, OpKind}; + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../../build/llbc/pyre-interpreter.ullbc" + ); + let llbc = Llbc::load(path).expect("load real LLBC"); + let graph = super::lower_function(&llbc, "split_builtin_kwargs") + .expect("lower split_builtin_kwargs"); + let calls_path = |want: &[&str]| -> usize { + let want: Vec = want.iter().map(|part| part.to_string()).collect(); + graph + .blocks + .iter() + .flat_map(|block| &block.operations) + .filter(|op| { + matches!( + &op.kind, + OpKind::Call { + target: CallTarget::FunctionPath { segments }, + .. + } if segments == &want + ) + }) + .count() + }; + assert_eq!( + calls_path(&["core", "slice", "index", "", "index"]), + 0, + "the proven RangeTo index must not remain a core residual" + ); + assert_eq!( + calls_path(&["__getslice_minusone"]), + 1, + "the marker strip must use the len-minus-one slice helper" + ); + } } diff --git a/majit/majit-translate/src/front/slice_index.rs b/majit/majit-translate/src/front/slice_index.rs index f1b647a019c..edf39fb4061 100644 --- a/majit/majit-translate/src/front/slice_index.rs +++ b/majit/majit-translate/src/front/slice_index.rs @@ -980,6 +980,12 @@ fn rangeto_static_length_bound_matches( /// required wraparound semantics. `ArrayLen` and plain `sub` are the measured /// post-lowering forms (`front/mir.rs`). fn minus_one_end_matches(graph: &FunctionGraph, end: &Variable, slice: &Variable) -> bool { + let Some(end) = resolve_block_alias(graph, end) else { + return false; + }; + let Some(slice) = resolve_block_alias(graph, slice) else { + return false; + }; let Some((lhs, rhs)) = graph .blocks .iter() @@ -993,15 +999,18 @@ fn minus_one_end_matches(graph: &FunctionGraph, end: &Variable, slice: &Variable rhs, result_ty: ValueType::Unsigned, }, - ) if result == end && op == "sub" => Some((lhs.clone(), rhs.clone())), + ) if result == &end && op == "sub" => Some((lhs.clone(), rhs.clone())), _ => None, }) else { return false; }; + let lhs = resolve_block_alias(graph, &lhs).unwrap_or(lhs); + let rhs = resolve_block_alias(graph, &rhs).unwrap_or(rhs); let has_len = graph.blocks.iter().flat_map(|b| &b.operations).any(|op| { op.result.as_ref() == Some(&lhs) - && matches!(&op.kind, OpKind::ArrayLen { base, .. } if base == slice) + && matches!(&op.kind, OpKind::ArrayLen { base, .. } + if resolve_block_alias(graph, base).as_ref() == Some(&slice)) }); let has_one = graph .blocks diff --git a/majit/majit-translate/tests/test_unroll_safe_inventory.rs b/majit/majit-translate/tests/test_unroll_safe_inventory.rs index 537cd7c8eb3..7ef17200685 100644 --- a/majit/majit-translate/tests/test_unroll_safe_inventory.rs +++ b/majit/majit-translate/tests/test_unroll_safe_inventory.rs @@ -49,6 +49,10 @@ const REVIEWED_UNROLL_SAFE: &[(&str, &str)] = &[ // No upstream counterpart by name; the loop is a bounded scan of a // fixed-size argument slice. ("leading_non_null_count", "flat builtin-keyword ABI scan"), + // `argument.py:172` carries `@jit.unroll_safe` on `_match_signature`, the + // keyword-binding loop this one mirrors; both are bounded by a signature + // fixed at the callee rather than by the call's arguments. + ("bind_builtin_kwargs", "argument.py _match_signature"), // `executioncontext.py` marks both `f_back`-chain walks `@jit.unroll_safe`. // // Evidence for the descent-scope change: neither graph is reachable from diff --git a/pyre/bench/synth/inline_freevar_after_mayforce.py b/pyre/bench/synth/inline_freevar_after_mayforce.py index b2e446a50f2..16c82d0a06a 100644 --- a/pyre/bench/synth/inline_freevar_after_mayforce.py +++ b/pyre/bench/synth/inline_freevar_after_mayforce.py @@ -1,20 +1,46 @@ # pyre-check: max-pypy-ratio=86 -# pyre-check: jitstats-band=guard_failures=8 -# `guard_failures` here is carried by two things this fixture is not about. -# One is the host: one tree read 1003 dynasm / 1008 cranelift on macOS and -# ubuntu and 1004 / 1009 on windows in a single CI run (`1d212895c6b`), with the -# loop and bridge counts agreeing everywhere. The other is the collection -# schedule -- one binary swept across nursery sizes read 1034 / 1014 / 1007 / -# 1007 at 2 / 4 / 6 / 8 MB, 27 counts, while `loops_compiled` and -# `bridges_compiled` did not move. Suppressing the whole trace-time fold table -# moved it by one count and suppressing the folds this branch adds by none, so -# it is not reading those either. +# pyre-check: jitstats-band=guard_failures=23,loops_compiled=1 +# Both bands cover one measured effect: this fixture's loop count is decided by +# a warm-up race, not by whether its arm compiles. +# +# `forward` calls `adjust` exactly once per iteration, so the two green keys +# tick their function-entry counters in lockstep and which of them crosses the +# threshold first is emergent. When `forward` wins, it traces `adjust` inlined +# and `adjust` never reaches the entry door again -- seven loops. When `adjust` +# wins, it also earns its own 36-op entry trace -- eight loops -- and `forward` +# still inlines it, its trace going 69 ops to 70 with a `GuardNotInvalidated` +# as the extra op. Both outcomes are live in one tree at once: windows reads +# seven while macos and ubuntu read eight, byte-identical to each other. +# +# The race is sensitive to how much unrelated work runs before it. Registering +# one more GC type shifts it -- three added descrs at startup were enough to +# flip the order -- and so does `PYTHONDONTWRITEBYTECODE`: one binary read +# seven loops with bytecode writes suppressed and eight with them enabled, at +# every N from 48000 through 128000. None of it reaches the arm. The arm's +# trace was read directly in all four combinations and is identical op for op, +# same call targets modulo the ASLR slide, differing only in the order of an +# unordered `SetfieldGc` write-back set. +# +# So width 1 on `loops_compiled` admits exactly the second outcome, and +# `guard_failures` has to admit what that outcome costs: 1005 -> 1018 dynasm +# and 1009 -> 1024 cranelift on both failing runners, 15 counts at the widest. +# The remaining 8 is what this directive already carried, for two things this +# fixture is also not about. One is the host: one tree read 1003 dynasm / 1008 +# cranelift on macOS and ubuntu and 1004 / 1009 on windows in a single CI run +# (`1d212895c6b`), with the loop and bridge counts agreeing everywhere. The +# other is the collection schedule -- one binary swept across nursery sizes +# read 1034 / 1014 / 1007 / 1007 at 2 / 4 / 6 / 8 MB, 27 counts, while +# `loops_compiled` and `bridges_compiled` did not move. Suppressing the whole +# trace-time fold table moved it by one count and suppressing the folds this +# branch adds by none, so it is not reading those either. +# +# `bridges_compiled` stays gated exactly at 5 and held at 5 in every +# configuration measured above, and the regression floor still gates +# `loops_aborted` at 0, so the dead-bridge and abort classes this suite exists +# to catch are untouched by either band. What is given up is reading a +# one-count fall in `loops_compiled` as "a hot loop went back to interpreted", +# which this fixture cannot support anyway while the race decides that count. # -# Width 8 covers both: the one-count host split, and the several counts a tree -# that allocates differently picks up on top of it -- this branch read 1011 on -# all three runners against a baseline of 1008. Anything wider than a tree's own -# allocation behaviour still gates, and `loops_compiled` and `bridges_compiled` -# stay gated exactly. Only the loop count answers whether the arm compiles. # The ceiling is a function of N, so raising N refits it. pypy's execution here # is almost all fixed cost -- doubling N moved it 0.035s to 0.039s -- while this # backend pays roughly 27us per iteration, so the ratio tracks N nearly one for @@ -65,9 +91,10 @@ # loop and no guard failure. # # The committed baselines are seven loops, five bridges, and 1005 dynasm / 1009 -# cranelift / 1004 wasm. Only the loop count answers whether the arm compiles, -# so treat a one-count guard-failure move as the unattributed remainder rather -# than as this fixture's subject. +# cranelift / 1004 wasm. Neither count answers whether the arm compiles -- the +# arm's trace reads identically under every configuration that moved them, see +# the band note above -- so treat a move in either as the unattributed +# remainder rather than as this fixture's subject. N = 64000 diff --git a/pyre/design.md b/pyre/design.md index bef12dacd9f..f879633a5a4 100644 --- a/pyre/design.md +++ b/pyre/design.md @@ -32,7 +32,8 @@ Rust, and a PyPy-equivalent (pyre) on top of that.** |---|---|---| | RPython the language | **Rust** | The host language is no longer a Python subset; it is a real language with a real type system. See §3.1. | | RPython translator (flowspace → annotator → rtyper) | **majit-translate** (`front/ast` → `flowspace/` → `annotator/` → `rtyper/`) over **Charon LLBC** artifacts | Same pipeline, same module names, run at `cargo build` time over extracted `.ullbc` instead of live bytecode. | -| `jtransform`/codewriter → JitCode | **codewriter/** → JitCode | Identical role. | +| `jtransform`/codewriter → JitCode | **majit-translate `codewriter/`** → JitCode | Same role, at `cargo build` time. pyre additionally runs a *second*, hand-written codewriter over user `CodeObject`s at runtime; see §3.7. | +| `warmspot` — translation-time portal generator | **split**: build-time derivation in majit-translate, hand-written warm entry in pyre-jit | Not a port. `apply_jit` is unwired and `warmspot.rs` is a `pub use` namespace; see §3.7. | | metainterp, optimizer, resume, blackhole | **majit-metainterp / majit-trace** | Line-by-line port of the *tracing* JIT (pyjitpl5 lineage), not the 2007 PE JIT. | | x86/ARM/… hand-written backends (~300k LOC) | **majit-backend-dynasm / -cranelift / -wasm** | Three thin backends behind one trait, current primary dynasm; see §3.4. | | incminimark GC | **majit-gc** (nursery + oldgen + incremental + card marking) | Port of the winner, not of Boehm/refcount/mark-sweep. | @@ -300,6 +301,97 @@ configuration. pyre keeps: --- +### 3.7 The portal boundary: warmspot split in two + +Upstream mints the portal at translation time. `warmspot.apply_jit` — the body +of the translator task literally named "JIT compiler generation" +(`task_pyjitpl_lltype`) — derives each driver's green/red specification +(`make_args_specification`), rewrites the `jit_merge_point` and `can_enter_jit` +markers into calls (`rewrite_jit_merge_point`, `rewrite_can_enter_jits`), and +fills the fields `JitDriverStaticData` declares but never computes. Upstream's +`jitdriver.py` is an attribute container with two executable statements +precisely because warmspot writes the rest. pyre splits that pipeline across two +layers, unevenly: + +- **The derivation and the marker erasure do run at build time**, over Charon + LLBC, in majit-translate's `jtransform` and `CallControl::setup_jitdriver`, + driven from `pyre-jit-trace/build.rs`. The derived green/red layout is + asserted against the real MIR operands and a mismatch fails the build. This + half is at the right layer and is not debt. +- **`apply_jit` itself is unported.** `task_pyjitpl_lltype` assembles every + upstream-shaped argument and then returns `TaskError`, because majit-translate + does not depend on majit-metainterp; `warmspot.rs` is a `pub use` namespace, + not an implementation. Seven `missing_task_leaf` sites exist across that + driver, so the stub is not unique — it is named here because the fields it + would fill are instead written by hand from consumer source. +- **A second codewriter runs at runtime.** majit-translate's + `transform_graph_to_jitcode` consumes a `FunctionGraph` once per build; + pyre-jit's `transform_graph_to_jitcode` consumes a user `CodeObject`, is + fallible, and runs unboundedly. Upstream has one, over the interpreter's own + graphs. This is the A1 debt in this area — it is written, it carries Python + opcode semantics, and it has already produced a wrong answer of exactly the + class N3 names: in a chained blackhole resume `portal_frame_reg` aliased the + caller frame, so an inlined callee's `LOAD_GLOBAL` indexed the caller's + `names` table. A1 is **not** weakened to accommodate it; it stands as a + tracked generation defect whose convergence target is majit-translate's + codewriter. + +**Measured cost, 2026-08-22.** Installing `sys.setprofile`, `sys.settrace` or +`cProfile` costs **1168–2836×** on a hot loop where PyPy 7.3.20 pays +**1.1–4.6×** and stays compiled. It is a total outage, not a reuse failure: +warming *under* the profiler never compiles at all. Event counts match CPython +exactly, so this is a cliff and not a wrong answer. Stated plainly: **a profile +taken on pyre measures the interpreter, not the JIT**, and pdb and coverage.py +are in the same position. + +**The root is a missing bracket, not the folded green.** Upstream brackets the +portal itself: `PyFrame.execute_frame` wraps `dispatch` — the function that +carries the merge point and nothing else — in `ExecutionContext.enter`, +`call_trace`, then `return_trace` and `leave` in `finally` clauses. pyre put +that bracket *inside* the plain dispatch body (`eval_frame_plain_with_resume`) +and left the JIT dispatch body bare: `eval_with_jit_inner` substitutes +`install_current_frame`, which performs only `enter`'s topframeref/f_backref +half, and `CurrentFrameGuard`'s drop, which performs only `leave`'s +topframeref half. Neither emits an event, and `pyre-jit` contains no +`call_trace` or `return_trace` call at all. + +A JIT-activated frame therefore emits no `call` and no `return` event, and the +only thing hiding that is the refusal itself — `frame_tracing_active` sends +every traced or profiled frame down `execute_frame_plain`, which is the +bracketed path. `run_with_jit` states the dependency in the affirmative: it +routes non-JIT-eligible frames through `execute_frame` "so `call_trace` / +`return_trace` frame events still fire". **The gate is not a performance +concession; it is the whole implementation of frame events for JIT-eligible +frames**, and the measured event parity above is produced by it. Restoring the +bracket above the portal is a prerequisite for touching the gate, and it needs +no green. + +**What upstream does not do.** It does not fold the tracing state away. +`ExecutionContext` declares `_immutable_fields_` with `profilefunc?` and +`w_tracefunc?`, yet the recorded traces read both as ordinary fields and guard +them, and the comment directly above that declaration says so: the fields +"should be known to a constant … but they're not". They are cheap because +they sit on the entry bridge, once per frame activation — not because they +disappear. Nor would the declaration help here: `quasi_immut_descr` requires a +constant struct operand, and pyre's `ec` is a portal red (`PYPYJIT_RED_VARS`), +so it is never one. The per-opcode half is a different mechanism again — +`dispatch_bytecode`'s explicit `we_are_jitted()` arm tests the *per-frame* +`w_f_trace` through the virtualizable `debugdata`, not the global tracefunc. + +Where the green does pay is the profiled-call dispatch: `call_valuestack` and +its keyword/ex siblings branch on `get_is_being_profiled()` before +`call_args_and_c_profile`, and a real green folds those branches to nothing in +the unprofiled trace while giving the profiled state its own cell, counter and +procedure token. That is the last step of the repair, not the first. + +**Falsification.** Restoring the activation bracket should leave event counts +unchanged with the gate still in place, and should let the gate's +`profilefunc`/global-tracefunc disjuncts be dropped without losing events. If +events go missing once the bracket is above the portal, the bracket is not what +the gate was standing in for and this entry is wrong. + +--- + ## 4. Norms (operating rules) **N1 — Layering.** majit never depends on pyre. pyre-interpreter stays diff --git a/pyre/extra_tests/parity_tests/import_name_cached_name_and_rebind_jit.py b/pyre/extra_tests/parity_tests/import_name_cached_name_and_rebind_jit.py new file mode 100644 index 00000000000..c0a1d3c28c8 --- /dev/null +++ b/pyre/extra_tests/parity_tests/import_name_cached_name_and_rebind_jit.py @@ -0,0 +1,61 @@ +# CPython-suite gap: import-hook tests do not combine co_names identity with a +# hot loop that rebinds builtins.__import__ after IMPORT_NAME has been traced. +# parity-tests reason: IMPORT_NAME must keep PyPy's co_names_w object and live +# builtin lookup when its call is exposed to the meta-tracer. + +import builtins + + +old_import = builtins.__import__ +os_module = old_import("os") +calls = [0, 0] +N = 40000 +SWITCH = N // 2 + + +def run(): + names = run.__code__.co_names + expected_name = names[names.index("os")] + expected_globals = globals() + + def first(name, globals_arg, locals_arg, fromlist, level): + assert name is expected_name + assert globals_arg is expected_globals + assert locals_arg is None + # `import os` compiles to IMPORT_NAME with no fromlist and an absolute + # level. Both hooks return os_module regardless, so a wrong value + # reaches nothing that would fail unless it is asserted here. + assert fromlist is None + assert level == 0 + calls[0] += 1 + return os_module + + def second(name, globals_arg, locals_arg, fromlist, level): + assert name is expected_name + assert globals_arg is expected_globals + assert locals_arg is None + # `import os` compiles to IMPORT_NAME with no fromlist and an absolute + # level. Both hooks return os_module regardless, so a wrong value + # reaches nothing that would fail unless it is asserted here. + assert fromlist is None + assert level == 0 + calls[1] += 1 + return os_module + + builtins.__import__ = first + try: + i = 0 + while i < N: + import os + + assert os is os_module + if i == SWITCH: + builtins.__import__ = second + i += 1 + finally: + builtins.__import__ = old_import + + +run() +assert calls == [SWITCH + 1, N - SWITCH - 1], calls +print("OK") diff --git a/pyre/extra_tests/parity_tests/import_name_class_body_locals_jit.py b/pyre/extra_tests/parity_tests/import_name_class_body_locals_jit.py new file mode 100644 index 00000000000..806d64fa102 --- /dev/null +++ b/pyre/extra_tests/parity_tests/import_name_class_body_locals_jit.py @@ -0,0 +1,38 @@ +# CPython-suite gap: the import-hook tests never put IMPORT_NAME inside a class +# body whose loop runs hot enough to compile, so nothing covers the one frame +# kind whose locals mapping is a real dict rather than None. +# parity-tests reason: `pyopcode.py:1119-1125` reads the frame's debug locals +# and substitutes None only when the frame has none. A class body has one, so +# a traced IMPORT_NAME that bakes None is visible to any custom __import__. + +import builtins + +old_import = builtins.__import__ +os_module = old_import("os") +seen = [] + + +def hook(name, globals_arg, locals_arg, fromlist, level): + seen.append(locals_arg is None) + return os_module + + +N = 40000 +builtins.__import__ = hook +try: + + class C: + i = 0 + while i < N: + import os + + i += 1 + +finally: + builtins.__import__ = old_import + +assert len(seen) == N, len(seen) +# A baked None shows up only once the loop compiles, so report the iteration +# it starts at rather than the whole list. +assert not any(seen), seen.index(True) +print("OK") diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index beb94f5842e..027bf6f7d54 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -3715,7 +3715,8 @@ pub fn install_default_builtins(ns: PyObjectRef) { // `moduledef.py:78-87 startup` — "Copy our __import__ to builtins". // `baseobjspace.py:730` keeps that same object as // `space.w_default_importlib_import`. - let w_import = make_module_builtin_function("__import__", builtin_dunder_import); + let w_import = + make_module_builtin_function("__import__", __pyre_wrap_builtin_dunder_import); crate::importing::set_default_importlib_import(w_import); w_import }); @@ -5040,6 +5041,11 @@ pub(crate) fn clinic_arity( /// keywords by parameter name without a per-function `Signature`; the /// `#[pyre_function]` wrapper supplies the name/required tables it knows /// at expansion time. +// PyPy: `Arguments._match_signature` (`pypy/interpreter/argument.py`) +// is `@jit.unroll_safe`. The loops below are bounded by the builtin's static +// signature and argument count in the same way; without the hint the JIT +// policy residualizes this gateway step and cannot descend into the builtin. +#[majit_macros::unroll_safe] pub(crate) fn bind_builtin_kwargs( args: &[PyObjectRef], names: &[&str], @@ -5052,12 +5058,27 @@ pub(crate) fn bind_builtin_kwargs( // leaves an omitted one `PY_NULL`; a positional-only registration hands // the body just the arguments the call made. Reading a null slot as an // argument that was not passed makes the two registrations bind alike. - let supplied = positional.iter().filter(|v| !v.is_null()).count(); + let mut supplied = 0; + let mut positional_index = 0; + while positional_index < positional.len() { + if !positional[positional_index].is_null() { + supplied += 1; + } + positional_index += 1; + } + let mut required_count = 0; + let mut required_index = 0; + while required_index < required.len() { + if required[required_index] { + required_count += 1; + } + required_index += 1; + } clinic_arity( fn_name, supplied, real_kwarg_count(kwargs), - required.iter().filter(|r| **r).count(), + required_count, names.len(), 0, )?; @@ -5066,18 +5087,47 @@ pub(crate) fn bind_builtin_kwargs( // `argument.py` keys the message off `space.text_w(keyword_names_w[i])`, // the keyword's own storage, so a name carrying a lone surrogate reaches // `e.args[0]` intact. Keep the WTF-8 rather than a lossy `String`. - let mut unknown: Option = None; - for (i, &v) in positional.iter().enumerate() { - scope[i] = v; - filled[i] = !v.is_null(); - } - if let Some(dict) = kwargs { - let entries = unsafe { pyre_object::w_dict_str_entries_wtf8(dict) }; - for (key, val) in entries.iter() { - if key.as_str() == Ok("__pyre_kw__") { + let keyword_entries = kwargs.map(|dict| unsafe { pyre_object::w_dict_str_entries_wtf8(dict) }); + let mut unknown: Option = None; + // PyPy `_match_signature` copies positional values with + // `take = min(num_args, co_argcount - upfront)` and `for i in range(take)`, + // so the constant signature bounds let the JIT unroll ordinary indexed + // reads. Keep that storage shape instead of Rust iterator adapters, whose + // `Filter`/`Enumerate` state has no RPython counterpart — and keep the + // `min`: `scope` and `filled` are `names.len()` long, while `clinic_arity` + // bounds the count of non-null entries rather than the slice itself, so a + // null-padded `positional` longer than the signature reaches here. + let take = positional.len().min(names.len()); + let mut positional_index = 0; + while positional_index < take { + let value = positional[positional_index]; + scope[positional_index] = value; + filled[positional_index] = !value.is_null(); + positional_index += 1; + } + if let Some(entries) = keyword_entries.as_ref() { + let mut entry_index = 0; + while entry_index < entries.len() { + let (key, val) = &entries[entry_index]; + let key_str = if unsafe { pyre_object::dictmultiobject::wtf8_key_is_utf8(key) } { + Some(unsafe { pyre_object::dictmultiobject::wtf8_key_as_str_unchecked(key) }) + } else { + None + }; + if key_str == Some("__pyre_kw__") { + entry_index += 1; continue; } - match names.iter().position(|n| key.as_str() == Ok(*n)) { + let mut matched_index = None; + let mut name_index = 0; + while name_index < names.len() { + if key_str == Some(names[name_index]) { + matched_index = Some(name_index); + break; + } + name_index += 1; + } + match matched_index { Some(idx) => { if filled[idx] { return Err(crate::PyError::type_error(format!( @@ -5093,29 +5143,49 @@ pub(crate) fn bind_builtin_kwargs( // so a call that misses a required argument is reported // against that argument even when it also passed a keyword // the function does not know. - None => unknown = Some(key.to_wtf8_buf()), + None => unknown = Some(entry_index), } + entry_index += 1; } } - for i in 0..names.len() { - if !filled[i] && required[i] { + let mut name_index = 0; + while name_index < names.len() { + if !filled[name_index] && required[name_index] { return Err(crate::PyError::type_error(format!( "{fn_name}() missing required argument '{}' (pos {})", - names[i], - i + 1, + names[name_index], + name_index + 1, ))); } + name_index += 1; } - if let Some(key) = unknown { - let mut msg = - Wtf8Buf::from_string(format!("{fn_name}() got an unexpected keyword argument '")); - msg.push_wtf8(&key); - msg.push_str("'"); - return Err(crate::PyError::type_error(msg)); + if let Some(entry_index) = unknown { + let entries = keyword_entries + .as_ref() + .expect("an unknown keyword index requires keyword entries"); + return builtin_unexpected_keyword_failure(fn_name, &entries[entry_index].0); } Ok(scope) } +/// Cold `Arguments._match_signature` unexpected-keyword formatter. +/// +/// PyPy retains an `ArgErrUnknownKwds` until the gateway converts it to the +/// final `TypeError`, so accepted calls never trace WTF-8 string assembly. +/// Keep the same boundary here; the hot binder carries only the offending +/// entry index and reaches this residual helper after missing-required checks. +#[cold] +#[majit_macros::dont_look_inside] +pub(crate) fn builtin_unexpected_keyword_failure( + fn_name: &str, + key: &rustpython_wtf8::Wtf8, +) -> Result, crate::PyError> { + let mut msg = Wtf8Buf::from_string(format!("{fn_name}() got an unexpected keyword argument '")); + msg.push_wtf8(key); + msg.push_str("'"); + Err(crate::PyError::type_error(msg)) +} + /// Resolve a builtin with a single required positional-or-keyword parameter /// through the gateway `parse_into_scope`, so the argument binds by name and /// the trailing `__pyre_kw__` marker dict never leaks as a value. Mirrors an @@ -19216,14 +19286,13 @@ fn builtin_dunder_import(args: &[PyObjectRef]) -> Result() - } else { - unsafe { (*frame).execution_context } - } - }); + // PyPy's `interp___import__` receives `space` and any slow native-import + // fallback reaches the execution context through + // `space.getexecutioncontext()`. Do not recover it from pyre's + // portal-level CURRENT_FRAME TLS: an inlined callee has its own red frame, + // while that anchor can still name the caller. The established + // object-space analogue owns the shared execution context directly. + let exec_ctx = crate::call::getexecutioncontext(); // The native importer keys every lookup by `&str`, so a name that has no // such spelling goes straight to the app-level bootstrap. Re-read the // name through its root: `space_index_w` above may have moved it. @@ -19236,6 +19305,30 @@ fn builtin_dunder_import(args: &[PyObjectRef]) -> Result Result { + builtin_dunder_import(args) +} + +#[cfg(not(target_arch = "wasm32"))] +#[linkme::distributed_slice(crate::gateway::BUILTIN_WRAPPER_DESCRIPTORS)] +#[allow(non_upper_case_globals)] +static __pyre_wrap_builtin_dunder_import_target: crate::gateway::BuiltinWrapperDescriptor = + crate::gateway::BuiltinWrapperDescriptor { + path: concat!(module_path!(), "::", "__pyre_wrap_builtin_dunder_import"), + func: __pyre_wrap_builtin_dunder_import, + }; + #[cfg(test)] mod tests { use super::*; diff --git a/pyre/pyre-interpreter/src/eval.rs b/pyre/pyre-interpreter/src/eval.rs index e21bfc8c065..7b67f12e243 100644 --- a/pyre/pyre-interpreter/src/eval.rs +++ b/pyre/pyre-interpreter/src/eval.rs @@ -4085,11 +4085,15 @@ impl OpcodeStepExecutor for PyFrame { Self::push_anchored(&anchor, result) } - fn import_name(&mut self, name: &str) -> Result<(), PyError> { + fn import_name(&mut self, name: &str, nameindex: usize) -> Result<(), PyError> { let w_fromlist = self.pop(); let w_flag = self.pop(); let anchor = FrameAnchor::new(self); - let w_obj = crate::importing::import_name(self, name, w_fromlist, w_flag)?; + // PyPy pyopcode.py `w_modulename = self.getname_w(nameindex)`. + let w_modulename = unsafe { + crate::pycode::w_code_getname_w_or_new(self.pycode as PyObjectRef, nameindex, name) + }; + let w_obj = crate::importing::import_name(self, w_modulename, w_fromlist, w_flag)?; Self::push_anchored(&anchor, w_obj) } diff --git a/pyre/pyre-interpreter/src/importing.rs b/pyre/pyre-interpreter/src/importing.rs index a0830fae3be..e126cdd7bfc 100644 --- a/pyre/pyre-interpreter/src/importing.rs +++ b/pyre/pyre-interpreter/src/importing.rs @@ -2738,13 +2738,11 @@ pub fn add_sys_path_0() { // ── check_sys_modules ──────────────────────────────────────────────── // PyPy equivalent: importing.py `check_sys_modules(space, w_modulename)` -/// Reads the process-owned `SYS_MODULES` registry (and the runtime-stamped -/// `sys.modules` dict through `sys_modules_dict`), neither a build-time -/// constant, so the JIT residualizes the call rather than folding a stale -/// `sys.modules` snapshot (`@dont_look_inside`, the `sys_modules_dict` / -/// `lookup_exc_class` shape). The `Option` return fits one word -/// and the `&str` argument matches `lookup_exc_class`. -#[majit_macros::dont_look_inside] +/// PyPy `importing.py:check_sys_modules` is an ordinary traceable lookup. +/// The mutable `sys.modules` dictionary supplies the invalidation boundary; +/// hiding this whole function behind `dont_look_inside` turns the cached +/// import fast path into an `EF_RANDOM_EFFECTS` residual and prevents the +/// optimizer from seeing the dictionary read at all. pub(crate) fn check_sys_modules(name: &str) -> Option { // Once installed, the Python-visible dict is the sole semantic module // cache. PyPy's `check_sys_modules` reads `space.sys.get('modules')` and @@ -4568,12 +4566,12 @@ fn absolute_import( // ── IMPORT_NAME ────────────────────────────────────────────────────── /// PyPy equivalent: pyopcode.py `IMPORT_NAME`. -pub fn import_name( - frame: &mut PyFrame, - name: &str, - w_fromlist: PyObjectRef, - w_flag: PyObjectRef, -) -> Result { +/// `pyopcode.py` `IMPORT_NAME`'s `self.get_builtin().getdictvalue(space, +/// '__import__')`, on its own so the interpreter and the JIT's `LoadImport` +/// residual read the importer the same way. The `is_module` test has no +/// upstream counterpart: `get_builtin` can hand back a plain mapping, which +/// carries no module dict to look the name up in. +pub fn lookup_dunder_import(frame: &PyFrame) -> Result { let w_builtin = frame.get_builtin(); let w_import = if !w_builtin.is_null() && unsafe { is_module(w_builtin) } { let w_dict = unsafe { pyre_object::w_module_get_w_dict(w_builtin) }; @@ -4584,16 +4582,33 @@ pub fn import_name( } } else { None - } - .ok_or_else(|| crate::PyError::new(crate::PyErrorKind::ImportError, "__import__ not found"))?; + }; + w_import + .ok_or_else(|| crate::PyError::new(crate::PyErrorKind::ImportError, "__import__ not found")) +} - let w_locals = match frame.getdebug() { +/// IMPORT_NAME's third argument (`pyopcode.py:1119-1125`). `getdebug()` +/// peeks: a frame that never materialized its locals mapping passes `None` +/// instead of creating one, which is what separates this from LOAD_LOCALS' +/// `getorcreatedebug()`. Split out so the JIT's `LoadImportLocals` residual +/// and the interpreter answer from the same code. +pub fn import_locals(frame: &PyFrame) -> PyObjectRef { + match frame.getdebug() { Some(d) if !d.w_locals.is_null() => d.w_locals, _ => pyre_object::w_none(), - }; - let w_globals = frame.get_w_globals(); - let w_modulename = pyre_object::w_str_new(name); + } +} + +pub fn import_name( + frame: &mut PyFrame, + w_modulename: PyObjectRef, + w_fromlist: PyObjectRef, + w_flag: PyObjectRef, +) -> Result { + let w_import = lookup_dunder_import(frame)?; + let w_locals = import_locals(frame); + let w_globals = frame.get_w_globals(); crate::call::call_callable( frame, w_import, diff --git a/pyre/pyre-interpreter/src/jit_fnaddr.rs b/pyre/pyre-interpreter/src/jit_fnaddr.rs index af4186e6434..2aaa3bc3ec2 100644 --- a/pyre/pyre-interpreter/src/jit_fnaddr.rs +++ b/pyre/pyre-interpreter/src/jit_fnaddr.rs @@ -395,6 +395,14 @@ pub fn jit_trace_fnaddrs() -> Vec<(&'static str, i64)> { "builtins::builtin_kwargs_marker_dict", crate::builtins::builtin_kwargs_marker_dict as *const (), ); + // `builtin_unexpected_keyword_failure` deliberately remains unpublished: + // its `&str` and `&Wtf8` arguments are two-word aggregates and its + // `Result, PyError>` return is multiword, neither of + // which the one-word residual-call ABI carries. `bind_builtin_kwargs` is + // `unroll_safe`, so the codewriter descends into it and reaches this + // `#[cold]` `#[dont_look_inside]` call as a residual; without an address + // it falls back to the symbolic hash instead of passing and returning the + // wrong number of words. // RPython annotator PBC parity for `BuiltinCode.func`: every generated // interp2app wrapper is a possible value of the indirect function-pointer diff --git a/pyre/pyre-interpreter/src/module/_ssl/mod.rs b/pyre/pyre-interpreter/src/module/_ssl/mod.rs index 0333f5fb19b..9aabfe019d8 100644 --- a/pyre/pyre-interpreter/src/module/_ssl/mod.rs +++ b/pyre/pyre-interpreter/src/module/_ssl/mod.rs @@ -216,15 +216,23 @@ fn tls_result(result: pyre_native::ssl::TlsResult) -> Result Result { +fn fs_path(obj: PyObjectRef) -> Result { if obj.is_null() || unsafe { is_none(obj) || is_bool(obj) } { return Err(crate::PyError::type_error( "path should be string, bytes, os.PathLike or integer, not NoneType", )); } - Ok(crate::gateway::fspath_buf(obj)? - .to_string_lossy() - .into_owned()) + // `PyUnicode_FSConverter`: a path is named by a `str`, by the filesystem + // `bytes` that `str` encodes to, or by an `os.PathLike`, and all three + // reach the host as those bytes. `fspath_buf` takes a `str` its caller has + // already established — reading a `bytes` through it decodes the payload + // as text and takes a length word out of the middle of the path. + // Those bytes stay a path all the way to the host call. Decoding them to + // text is lossy on a name the filesystem accepts but UTF-8 does not: two + // distinct files collapse onto one `U+FFFD` spelling, and re-encoding that + // spelling names neither of them. + let bytes = crate::gateway::fsencode_bytes_w(obj)?; + Ok(crate::gateway::os_string_from_fs_bytes(&bytes).into()) } fn password_bytes(obj: PyObjectRef) -> Result>, crate::PyError> { @@ -818,9 +826,9 @@ mod context_methods { let password = crate::builtins::bind_pos_or_kw(user, kwargs, 2, "password", "load_cert_chain", 3)?; crate::builtins::kwarg_reject_unknown(kwargs, KEYWORDS, "load_cert_chain")?; - let cert_path = path_string(cert)?; + let cert_path = fs_path(cert)?; let key_path = match key { - Some(value) if !unsafe { is_none(value) } => path_string(value)?, + Some(value) if !unsafe { is_none(value) } => fs_path(value)?, _ => cert_path.clone(), }; // OpenSSL asks its callback only after parsing discovers an @@ -891,14 +899,14 @@ mod context_methods { )); } if let Some(cafile) = cafile { - let path = path_string(cafile)?; + let path = fs_path(cafile)?; native_result(unsafe { pyre_native::ssl::context_load_verify_file(self.backend, &path) })?; } if let Some(capath) = capath { - let path = path_string(capath)?; - if !std::path::Path::new(&path).is_dir() { + let path = fs_path(capath)?; + if !path.is_dir() { return Err(crate::PyError::os_error_with_errno( libc::ENOENT, "CA directory does not exist", @@ -948,14 +956,14 @@ mod context_methods { /// the first and `SSL_CERT_DIR` only the second, so neither variable /// may suppress the other's source. fn set_default_verify_paths(&mut self) -> Result<(), crate::PyError> { + // `SSL_CERT_FILE` and `SSL_CERT_DIR` name files, so their bytes + // reach the host as a path for the same reason `fs_path`'s do. let env_path = |name: &[u8]| { - crate::host_seam::getenv(name) - .ok() - .flatten() - .map(|value| String::from_utf8_lossy(&value).into_owned()) + crate::host_seam::getenv(name).ok().flatten().map(|value| { + std::path::PathBuf::from(crate::gateway::os_string_from_fs_bytes(&value)) + }) }; - let cert_file = - env_path(b"SSL_CERT_FILE").filter(|path| std::path::Path::new(path).is_file()); + let cert_file = env_path(b"SSL_CERT_FILE").filter(|path| path.is_file()); match cert_file { Some(path) => native_result(unsafe { pyre_native::ssl::context_load_verify_file(self.backend, &path) @@ -967,8 +975,8 @@ mod context_methods { .map(|_| ())?, } let (_, default_dir) = pyre_native::ssl::default_verify_paths(); - let cert_dir = env_path(b"SSL_CERT_DIR").unwrap_or(default_dir); - if std::path::Path::new(&cert_dir).is_dir() { + let cert_dir = env_path(b"SSL_CERT_DIR").unwrap_or_else(|| default_dir.into()); + if cert_dir.is_dir() { // OpenSSL defers hashed directory loading until chain lookup, // so it does not contribute to cert_store_stats here. unsafe { pyre_native::ssl::context_add_verify_dir(self.backend, &cert_dir) }; @@ -1074,7 +1082,7 @@ mod context_methods { "load_dh_params() missing required path argument", )); } - let path = path_string(path)?; + let path = fs_path(path)?; let data = std::fs::read(&path).map_err(|error| { crate::PyError::os_error_with_errno( error.raw_os_error().unwrap_or(libc::EIO), @@ -2586,7 +2594,7 @@ mod cert_store { } fn test_decode_cert(args: &[PyObjectRef]) -> Result { - let path = path_string(args[0])?; + let path = fs_path(args[0])?; let cert = native_result(pyre_native::ssl::certificate_decode_file(&path))?; Ok(decoded_certificate_dict(cert)) } diff --git a/pyre/pyre-interpreter/src/pyopcode.rs b/pyre/pyre-interpreter/src/pyopcode.rs index 1f3e00efde1..607141b1c35 100644 --- a/pyre/pyre-interpreter/src/pyopcode.rs +++ b/pyre/pyre-interpreter/src/pyopcode.rs @@ -1259,7 +1259,7 @@ pub trait OpcodeStepExecutor: SharedOpcodeHandler { } // ── Import ── - fn import_name(&mut self, _name: &str) -> Result<(), PyError> { + fn import_name(&mut self, _name: &str, _nameindex: usize) -> Result<(), PyError> { Err(crate::PyError::type_error("import_name not implemented")) } fn import_from(&mut self, _name: &str) -> Result<(), PyError> { @@ -3386,7 +3386,7 @@ pub fn execute_import_name( unreachable!() }; let name_idx = u32_as_usize(namei.get(op_arg)); - executor.import_name(code.names[name_idx].as_ref())?; + executor.import_name(code.names[name_idx].as_ref(), name_idx)?; Ok(StepResult::Continue) } diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index 5af43cf547c..1ac68bf678f 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -1276,7 +1276,7 @@ static W_BYTES_DESCR_GROUP: LazyLock = LazyLock::new(|| { ( "data", pyre_object::bytesobject::BYTES_DATA_OFFSET, - std::mem::size_of::<*const Vec>(), + std::mem::size_of::<*const pyre_object::bytesobject::BytesBlock>(), Type::Ref, false, true, diff --git a/pyre/pyre-jit/src/call_jit.rs b/pyre/pyre-jit/src/call_jit.rs index 7b13f9213b3..2ff49c5801a 100644 --- a/pyre/pyre-jit/src/call_jit.rs +++ b/pyre/pyre-jit/src/call_jit.rs @@ -5917,65 +5917,6 @@ pub extern "C" fn bh_delete_attr_fn(obj: i64, w_code_ptr: i64, name_idx: i64) -> 0 } -/// IMPORT_NAME residual (`import_name` HLOp → `residual_call_ir_r`). -/// Resolves the module name from the jitcode's own code object via -/// `name_idx` (same `co_names` invariant as `bh_load_attr_fn`), fetches -/// `__import__` from the threaded frame's builtins, and calls it with the -/// frame's globals and locals. Importing a module may run its -/// top-level Python (`MayForce`); on error the exception is published -/// through `BH_LAST_EXC_VALUE` for the trailing `GuardNoException` and the -/// call returns 0. `fromlist` and `level` are the two popped operands -/// (`eval.rs import_name`: `fromlist = pop()`, `level = pop()`). -pub extern "C" fn bh_import_name_fn( - fromlist: i64, - level: i64, - w_code_ptr: i64, - frame_ptr: i64, - name_idx: i64, -) -> i64 { - let w_code = w_code_ptr as pyre_object::PyObjectRef; - let code = unsafe { - &*(pyre_interpreter::w_code_get_ptr(w_code) as *const pyre_interpreter::CodeObject) - }; - let idx = name_idx as usize; - debug_assert!( - idx < code.names.len(), - "bh_import_name_fn name_idx {idx} out of range ({} names) — codegen invariant", - code.names.len() - ); - if idx >= code.names.len() { - return 0; - } - let name = code.names[idx].as_ref(); - let frame = frame_ptr as *mut PyFrame; - debug_assert!(!frame.is_null(), "IMPORT_NAME requires a live frame"); - if frame.is_null() { - // IMPORT_NAME produces a module or raises; it never yields a null - // result. A null frame cannot honour that, so fail closed by - // publishing an exception for the trailing `GuardNoException` - // instead of returning a bare 0 the guard would accept. - let mut err = pyre_interpreter::PyError::new( - pyre_interpreter::PyErrorKind::SystemError, - "IMPORT_NAME residual received a null frame", - ); - publish_residual_call_exception(err.to_exc_object() as i64); - return 0; - } - match pyre_interpreter::importing::import_name( - unsafe { &mut *frame }, - name, - fromlist as pyre_object::PyObjectRef, - level as pyre_object::PyObjectRef, - ) { - Ok(module) => module as i64, - Err(mut err) => { - let exc_obj = err.to_exc_object(); - publish_residual_call_exception(exc_obj as i64); - 0 - } - } -} - /// IMPORT_FROM residual (`import_from` HLOp → `residual_call_ir_r`). /// Resolves the attribute name from the jitcode's own code object via /// `name_idx` (same `co_names` invariant as `bh_load_attr_fn`) and runs @@ -6531,6 +6472,58 @@ pub extern "C" fn bh_load_build_class_fn(frame_ptr: i64) -> i64 { } } +/// IMPORT_NAME's builtin lookup, split from the subsequent Python call just +/// like PyPy's `pyopcode.py:IMPORT_NAME` (`get_builtin().__import__`, then +/// `space.call_function`). Keeping this lookup as a small residual lets the +/// ordinary `CallFn` path descend through a gateway `BuiltinCode.func` rather +/// than hiding the whole importer behind one opaque residual. +pub extern "C" fn bh_load_import_fn(frame_ptr: i64) -> i64 { + let frame = frame_ptr as *mut PyFrame; + debug_assert!( + !frame.is_null(), + "bh_load_import_fn requires a non-null PyFrame" + ); + if frame.is_null() { + let mut err = pyre_interpreter::PyError::new( + pyre_interpreter::PyErrorKind::SystemError, + "IMPORT_NAME received a null frame", + ); + publish_residual_call_exception(err.to_exc_object() as i64); + return 0; + } + match pyre_interpreter::importing::lookup_dunder_import(unsafe { &*frame }) { + Ok(w_import) => w_import as i64, + Err(mut err) => { + publish_residual_call_exception(err.to_exc_object() as i64); + 0 + } + } +} + +/// IMPORT_NAME's locals argument, split from the call for the same reason as +/// [`bh_load_import_fn`]. Infallible — it peeks the debug slot rather than +/// creating one — so like `bh_load_locals_fn` it has no exception-publishing +/// arm. +/// +/// The asymmetry with [`bh_load_import_fn`] is deliberate rather than an +/// oversight. That one resolves `__import__` and genuinely raises ImportError +/// when the name is absent, so it carries the publish-and-return-0 machinery. +/// This one reads a slot that is either populated or absent, and answers +/// `None` for absent, so it has no failure to report. A null frame is a wiring +/// bug in an emit site, not a runtime condition, and takes the same assert +/// `bh_load_locals_fn` uses for the same reason -- publishing an exception for +/// it would convert a miswired emit site into a `SystemError` raised at some +/// unrelated Python line. +pub extern "C" fn bh_load_import_locals_fn(frame_ptr: i64) -> i64 { + assert!( + frame_ptr != 0, + "bh_load_import_locals_fn requires a non-null PyFrame; every IMPORT_NAME \ + emit site must thread portal_frame_reg as its ref operand" + ); + let frame = unsafe { &*(frame_ptr as *mut PyFrame) }; + pyre_interpreter::importing::import_locals(frame) as i64 +} + /// DELETE_GLOBAL residual using the frame receiver and interned-name ABI. /// pyopcode.py DELETE_GLOBAL deletes directly from `w_globals`. pub extern "C" fn bh_delete_global_fn(frame_ptr: i64, w_name: i64) -> i64 { diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 1849272b5d5..054121803a7 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -4235,11 +4235,10 @@ fn build_gc() -> Box { pyre_object::gc_storage::storage_box_destructor::, pyre_object::celldict::set_module_dict_strategy_gc_type_id, ); - // bytes / bytearray `data` storage box (off-GC storage). A leaf - // `Vec` (no inner refs) shared by both types; `bytes_object_custom_trace` - // / `bytearray_object_custom_trace` grey it through the `data` field slot and - // the box tid's drop glue reclaims the buffer on sweep. Keep this id at the - // absolute registration tail. + // bytearray `data` storage box (off-GC storage). A leaf `Vec` (no inner + // refs); `bytearray_object_custom_trace` greys it through the `data` field + // slot and the box tid's drop glue reclaims the buffer on sweep. Keep this + // id at the absolute registration tail. register_leaf_storage_box::( &mut gc, pyre_object::gc_storage::storage_box_destructor::, @@ -4422,6 +4421,33 @@ fn build_gc() -> Box { gc.register_type(TypeInfo::with_gc_ptrs(size, offsets)) }); + // `bytes` `data` block — `rstr.py:1226-1228`'s `STR.chars`, an + // `Array(Char)`. A varsize GcArray of bytes with no inner refs, so it + // registers with the shape `get_array_token` reads off that one ARRAY and + // no destructor: the payload is inside the block, so the sweep reclaims it + // with the block and the collector sizes it from the block's own length + // header. `bytes_object_custom_trace` greys it through the `data` field + // slot, the same edge the storage box was reached by. + // + // Registered after every other type, synthetic structs included. A tid is a + // position in this chain, and the interpreter spells many of them as + // literals — `W_BYTES_GC_TYPE_ID` is 27, `W_LIST_GC_TYPE_ID` is 7 — so an + // insertion anywhere earlier renumbers every registration below it while the + // `debug_assert_eq!`s that pair each literal with its registration are + // compiled out of a release build. Allocations made before this line read a + // zero tid and take `alloc_bytes_block`'s plain-allocation arm; the trace + // skips those blocks on `try_gc_owns_object`, as it did for the storage box + // from its own later registration point. + let bytes_block_token = &pyre_object::bytesobject::BYTES_BLOCK_TOKEN; + let bytes_block_tid = gc.register_type(TypeInfo::varsize( + bytes_block_token.base_size, + bytes_block_token.item_size, + bytes_block_token.len_offset, + false, + Vec::new(), + )); + pyre_object::bytesobject::set_bytes_block_gc_type_id(bytes_block_tid); + // ── GC-root registration completeness oracle ───────────────────────── // Every `#[pyre_class]` type appends its descriptor to the whole-program // `PYRE_CLASS_DESCRIPTORS` slice. A type with inline managed children must @@ -7420,6 +7446,12 @@ fn for_iter_body_op_is_jit_safe(instr: pyre_interpreter::Instruction) -> bool { // here added no safety beyond the Layer 2 defense above. | I::CallFunctionEx | I::LoadGlobal { .. } + // IMPORT_NAME is the same Python-call boundary as CALL: it + // resolves builtins.__import__ and invokes it. The Layer 2 + // effect journal above is the replay-safety authority for both; + // rejecting only the opcode spelling kept otherwise identical + // `for` loops interpreted while PyPy traces them. + | I::ImportName { .. } | I::Resume { .. } // container builders: produce new heap objects but do not mutate // existing ones; walk-abort just drops the incomplete object @@ -14471,6 +14503,16 @@ mod tests { assert_eq!(unsupported_jit_shape_of(&code), UnsupportedJitShape::None); } + #[test] + fn for_iter_cached_import_body_is_jit_safe() { + use pyre_interpreter::compile_exec; + let module = compile_exec("def f(n):\n for _ in range(n):\n import os\n") + .expect("test code should compile"); + let code = function_code_from_module(&module, "f"); + assert!(function_entry_trace_is_jit_safe(&code)); + assert_eq!(unsupported_jit_shape_of(&code), UnsupportedJitShape::None); + } + #[test] fn for_iter_single_level_binaryop_mutation_body_is_jit_safe() { // single-level `s += t` (in-place list extend via BINARY_OP) recovers on diff --git a/pyre/pyre-jit/src/jit/codewriter.rs b/pyre/pyre-jit/src/jit/codewriter.rs index 279be681396..1b75ef10358 100644 --- a/pyre/pyre-jit/src/jit/codewriter.rs +++ b/pyre/pyre-jit/src/jit/codewriter.rs @@ -3075,32 +3075,6 @@ fn emit_frontend_is_op( ) } -fn emit_frontend_import_name( - graph: &mut super::flow::FunctionGraph, - block: &super::flow::BlockRef, - fromlist: super::flow::FlowValue, - level: super::flow::FlowValue, - code: super::flow::FlowValue, - frame: super::flow::FlowValue, - name_idx: super::flow::FlowValue, - offset: i64, -) -> super::flow::Variable { - emit_graph_op_with_result( - graph, - block, - "import_name", - vec![ - fromlist.into(), - level.into(), - code.into(), - frame.into(), - name_idx.into(), - ], - Kind::Ref, - offset, - ) -} - fn emit_frontend_import_from( graph: &mut super::flow::FunctionGraph, block: &super::flow::BlockRef, @@ -3575,7 +3549,6 @@ struct FnPtrIndices { format_with_spec_fn: HelperHandle, build_string_from_array_fn: HelperHandle, convert_value_fn: HelperHandle, - import_name_fn: HelperHandle, import_from_fn: HelperHandle, load_super_attr_fn: HelperHandle, super_attr_unwrap_fn: HelperHandle, @@ -3596,6 +3569,8 @@ struct FnPtrIndices { match_class_fn: HelperHandle, load_locals_fn: HelperHandle, load_build_class_fn: HelperHandle, + load_import_fn: HelperHandle, + load_import_locals_fn: HelperHandle, load_from_dict_or_globals_fn: HelperHandle, call_function_ex_fn: HelperHandle, unary_not_fn: HelperHandle, @@ -3952,13 +3927,6 @@ fn register_helper_fn_pointers( cpu.convert_value_fn as *const (), CallFlavor::MayForce, ); - // `bh_import_name_fn` runs `__import__` (module top-level Python may run) - // → `MayForce`. Appended last to preserve fn_ptr indices. - let import_name_fn = bind( - assembler, - cpu.import_name_fn as *const (), - CallFlavor::MayForce, - ); // `bh_import_from_fn` runs `importing::import_from` (a submodule-import // fallback may run module top-level Python) → `MayForce`. let import_from_fn = bind( @@ -4324,6 +4292,26 @@ fn register_helper_fn_pointers( cpu.load_build_class_fn as *const (), CallFlavor::Plain, ); + // IMPORT_NAME performs this builtin lookup and then uses the ordinary + // CallFn path for the actual invocation. Bind after the existing helpers + // so their pool indices remain stable. + let load_import_fn = bind( + assembler, + cpu.load_import_fn as *const (), + CallFlavor::Plain, + ); + // The locals half returns the frame's debug slot without an error path, + // so it binds `PlainCannotRaise`, the same pairing `load_locals_fn` above + // carries: the flavor bound here classifies the helper, while the residual + // this opcode emits is lowered `Plain` because the two fields it reads + // were never analyzed (see `lower_load_import_locals_hlop_to_insn`). It is + // not the no-heap flavor, which additionally asserts `can_collect=false` + // and an untouched heap. + let load_import_locals_fn = bind( + assembler, + cpu.load_import_locals_fn as *const (), + CallFlavor::PlainCannotRaise, + ); // The hand-written PUSH_EXC_INFO lowering must complete the interpreter's // caught-exception ownership transfer. Bind last so every existing // helper index remains stable. @@ -4394,7 +4382,6 @@ fn register_helper_fn_pointers( format_with_spec_fn, build_string_from_array_fn, convert_value_fn, - import_name_fn, import_from_fn, load_super_attr_fn, super_attr_unwrap_fn, @@ -4427,6 +4414,8 @@ fn register_helper_fn_pointers( match_class_fn, load_locals_fn, load_build_class_fn, + load_import_fn, + load_import_locals_fn, load_from_dict_or_globals_fn, call_function_ex_fn, call_kw_fn_0, @@ -6348,11 +6337,6 @@ impl CodeWriter { idx: convert_value_fn_idx, flavor: _convert_value_fn_flavor, }, - import_name_fn: - HelperHandle { - idx: import_name_fn_idx, - flavor: _import_name_fn_flavor, - }, import_from_fn: HelperHandle { idx: import_from_fn_idx, @@ -6443,6 +6427,16 @@ impl CodeWriter { idx: load_build_class_fn_idx, flavor: _load_build_class_fn_flavor, }, + load_import_fn: + HelperHandle { + idx: load_import_fn_idx, + flavor: _load_import_fn_flavor, + }, + load_import_locals_fn: + HelperHandle { + idx: load_import_locals_fn_idx, + flavor: _load_import_locals_fn_flavor, + }, load_from_dict_or_globals_fn: HelperHandle { idx: load_from_dict_or_globals_fn_idx, @@ -6699,7 +6693,6 @@ impl CodeWriter { format_with_spec_fn_idx, build_string_from_array_fn_idx, convert_value_fn_idx, - import_name_fn_idx, import_from_fn_idx, load_super_attr_fn_idx, super_attr_unwrap_fn_idx, @@ -6720,6 +6713,8 @@ impl CodeWriter { match_class_fn_idx, load_locals_fn_idx, load_build_class_fn_idx, + load_import_fn_idx, + load_import_locals_fn_idx, load_from_dict_or_globals_fn_idx, call_function_ex_fn_idx, unary_not_fn_idx, @@ -12305,37 +12300,83 @@ impl CodeWriter { emit_abort_permanent!(py_pc); } - // ImportName: pops 2 (fromlist=TOS, level=TOS1), pushes - // 1 module. Net: -1. `import_name(fromlist, level, code, - // name_idx)` HLOp → `residual_call_ir_r(import_name_fn, - // ListI[name_idx], ListR[fromlist, level, code])`. The - // jitcode's own PyCode travels as a post-rtype - // `Signed(ptr) + Kind::Ref` constant and the `co_names` - // index the helper resolves the module name with — the - // same surrogate-operand shape as the LoadAttr arm. - // `bh_import_name_fn` runs `__import__` through the - // TLS-pinned execution context (MayForce). + // PyPy pyopcode.py IMPORT_NAME: resolve + // `get_builtin().__import__`, then invoke it through + // the ordinary Python call path with + // `(name, globals, None, fromlist, level)`. Keeping + // the lookup and CallFn separate is load-bearing: the + // latter can descend through BuiltinCode.func and + // trace `_gcd_import`; one monolithic residual hid + // the entire importer. Instruction::ImportName { namei } => { let name_idx = namei.get(op_arg) as usize; - let code_const: super::flow::FlowValue = super::flow::Constant::new( - super::flow::ConstantValue::Signed(w_code as i64), - Some(Kind::Ref), - ) - .into(); - let name_idx_const: super::flow::FlowValue = - super::flow::Constant::signed(name_idx as i64).into(); let _ = emit_popvalue_ref!(current_depth, py_pc); let fromlist_value = pop_ref_or_fresh(&mut current_state, &mut graph); let _ = emit_popvalue_ref!(current_depth, py_pc); let level_value = pop_ref_or_fresh(&mut current_state, &mut graph); - let result_value = emit_frontend_import_name( + + let callable = emit_frontend_frame_only_ref( &mut graph, ¤t_block.block(), - fromlist_value, - level_value, - code_const, + "load_import", frame_var.into(), - name_idx_const, + py_pc as i64, + ); + + let name = code + .names + .get(name_idx) + .expect("IMPORT_NAME co_names index is validated by the compiler"); + // PyPy pyopcode.py `IMPORT_NAME`'s `getname_w` uses the exact interned + // object in this PyCode's `co_names_w` table. + let w_name = unsafe { + pyre_interpreter::pycode::w_code_getname_w_or_new( + w_code as pyre_object::PyObjectRef, + name_idx, + name.as_ref(), + ) + }; + let name_value = pyobject_const_ref_value(w_name); + // Every per-code jitcode now carries its own red + // frame. Read that frame's live w_globals exactly + // as PyPy's `self.get_w_globals()` does; an inlined + // callee must never inherit the caller's namespace + // or a code-wrapper constant in its place. + let globals_value: super::flow::FlowValue = emit_graph_op_with_result( + &mut graph, + ¤t_block.block(), + "getfield_vable_r", + vable_getfield_ref_graph_args( + frame_var.into(), + VABLE_NAMESPACE_FIELD_IDX, + ), + Kind::Ref, + py_pc as i64, + ) + .into(); + // `pyopcode.py:1119-1125` reads the frame's debug + // locals and substitutes `None` only when the frame + // has none. A baked `None` here would hide a + // materialized mapping from a custom `__import__`. + let locals_value = emit_frontend_frame_only_ref( + &mut graph, + ¤t_block.block(), + "load_import_locals", + frame_var.into(), + py_pc as i64, + ); + let result_value = emit_frontend_simple_call( + &mut graph, + ¤t_block.block(), + callable.into(), + super::flow::Constant::none().into(), + vec![ + name_value, + globals_value, + locals_value.into(), + fromlist_value, + level_value, + ], py_pc as i64, ); push_and_bump!(result_value.into(), py_pc); diff --git a/pyre/pyre-jit/src/jit/cpu.rs b/pyre/pyre-jit/src/jit/cpu.rs index c00a4d4e503..bfdaac4fb90 100644 --- a/pyre/pyre-jit/src/jit/cpu.rs +++ b/pyre/pyre-jit/src/jit/cpu.rs @@ -210,12 +210,6 @@ pub struct Cpu { /// `conv` is a `runtime_ops::convert_value_code`; user `__str__` / /// `__repr__` may run Python (fallible). pub convert_value_fn: extern "C" fn(i64, i64) -> i64, - /// `bh_import_name_fn(fromlist, level, code, frame, name_idx)` — - /// IMPORT_NAME `__import__` residual; resolves the module name from the - /// code object, reads `__name__`/`__package__` for relative imports from - /// the threaded `frame`, and imports through the TLS-pinned execution - /// context (may run module top-level Python → fallible). - pub import_name_fn: extern "C" fn(i64, i64, i64, i64, i64) -> i64, /// `bh_import_from_fn(module, code, name_idx)` — IMPORT_FROM residual; /// resolves the attribute name from the code object and runs /// `importing::import_from` on the peeked module (namespace lookup, then a @@ -345,6 +339,14 @@ pub struct Cpu { /// `LOAD_BUILD_CLASS` (`pyopcode.py`); reads `__build_class__` out of /// the frame's builtin mapping. pub load_build_class_fn: extern "C" fn(i64) -> i64, + /// Load `builtins.__import__` for IMPORT_NAME. Kept separate from the + /// call itself so the generated jitcode has the same ordinary Python + /// call boundary as PyPy's `IMPORT_NAME` implementation. + pub load_import_fn: extern "C" fn(i64) -> i64, + /// Load IMPORT_NAME's locals argument. Separate from `load_import_fn` + /// because `pyopcode.py:1119-1125` reads two independent things off the + /// frame before the call. + pub load_import_locals_fn: extern "C" fn(i64) -> i64, /// `newtuple(list_w)` (`objspace.py:332`) — (ref array) → new tuple. /// The array is the forced `popvalues` list; length travels inside /// the array, so any arity fits. @@ -507,7 +509,6 @@ impl Cpu { format_simple_fn: crate::call_jit::bh_format_simple_fn, format_with_spec_fn: crate::call_jit::bh_format_with_spec_fn, convert_value_fn: crate::call_jit::bh_convert_value_fn, - import_name_fn: crate::call_jit::bh_import_name_fn, import_from_fn: crate::call_jit::bh_import_from_fn, load_super_attr_fn: crate::call_jit::bh_load_super_attr_fn, super_attr_unwrap_fn: crate::call_jit::bh_super_attr_unwrap_fn, @@ -546,6 +547,8 @@ impl Cpu { delete_global_fn: crate::call_jit::bh_delete_global_fn, load_locals_fn: crate::call_jit::bh_load_locals_fn, load_build_class_fn: crate::call_jit::bh_load_build_class_fn, + load_import_fn: crate::call_jit::bh_load_import_fn, + load_import_locals_fn: crate::call_jit::bh_load_import_locals_fn, newtuple_from_array_fn: crate::call_jit::bh_newtuple_from_array, build_map_from_array_fn: crate::call_jit::bh_build_map_from_array, build_set_from_array_fn: crate::call_jit::bh_build_set_from_array, diff --git a/pyre/pyre-jit/src/jit/flatten.rs b/pyre/pyre-jit/src/jit/flatten.rs index de13fe952f3..40b3dbe18bc 100644 --- a/pyre/pyre-jit/src/jit/flatten.rs +++ b/pyre/pyre-jit/src/jit/flatten.rs @@ -2640,6 +2640,8 @@ pub fn graph_op_can_raise(op: &super::flow::SpaceOperation) -> bool { | "delete_name" | "delete_global" | "load_build_class" + | "load_import" + | "load_import_locals" | "simple_call" | "getattr" | "load_special" @@ -2666,7 +2668,6 @@ pub fn graph_op_can_raise(op: &super::flow::SpaceOperation) -> bool { | "match_keys" | "match_class" | "not_" - | "import_name" | "import_from" | "load_from_dict_or_globals" | "load_super_attr" @@ -3331,6 +3332,13 @@ pub struct LoweringContext { /// `load_build_class_fn` descrs-pool index. LOAD_BUILD_CLASS lowers to the /// same one-Ref shape as [`Self::load_locals_fn_idx`]. pub load_build_class_fn_idx: u16, + /// `load_import_fn` descrs-pool index. This is the builtin-lookup half of + /// IMPORT_NAME; the subsequent invocation uses the ordinary CallFn path. + pub load_import_fn_idx: u16, + /// `load_import_locals_fn` descrs-pool index. IMPORT_NAME's locals + /// argument lowers to the same one-Ref shape as + /// [`Self::load_locals_fn_idx`]. + pub load_import_locals_fn_idx: u16, /// `bind(assembler, cpu.newtuple_from_array_fn as *const (), /// CallFlavor::Plain)` descrs-pool index for the production /// source. BUILD_TUPLE records the rtyped `pyopcode.py` @@ -3499,16 +3507,6 @@ pub struct LoweringContext { /// `bh_convert_value_fn(value, conv)` runs str/repr/ascii (a user /// `__str__` / `__repr__` may force → `MayForce`). pub convert_value_fn_idx: u16, - /// `import_name_fn` descrs-pool index — see codewriter.rs - /// `register_helper_fn_pointers`. IMPORT_NAME records the - /// `import_name(fromlist, level, code, name_idx)` HLOp (code = the - /// jitcode's own PyCode as a `Signed(ptr) + Kind::Ref` constant, - /// name_idx = `co_names` index) lowered to `residual_call_ir_r( - /// ConstInt(fn_idx), ListI([name_idx]), ListR([fromlist, level, code]), - /// Descr) → reg` via [`lower_import_name_hlop_to_insn`]; - /// `bh_import_name_fn` runs `__import__` (module top-level Python may - /// run → `MayForce`). - pub import_name_fn_idx: u16, /// `import_from_fn` descrs-pool index. IMPORT_FROM records the /// `import_from(module, code, name_idx)` HLOp (code = the jitcode's own /// PyCode as a `Signed(ptr) + Kind::Ref` constant, name_idx = @@ -4487,6 +4485,84 @@ where ) } +/// Lower the builtin-lookup half of pyopcode.py IMPORT_NAME to a one-Ref +/// residual. Its result becomes the callable of a separate `simple_call`. +pub fn lower_load_import_hlop_to_insn( + op: &super::flow::SpaceOperation, + ctx: &LoweringContext, + get_register: &mut F, + lower_constant: &mut LC, +) -> Option +where + F: FnMut(super::flow::Variable) -> Register, + LC: FnMut(&Constant) -> Operand, +{ + if op.opname != "load_import" || op.args.len() != 1 { + return None; + } + let frame_operand = flatten_arg_with_lowering(&op.args[0], get_register, lower_constant); + let dst_reg = match &op.result { + Some(super::flow::FlowValue::Variable(var)) => get_register(*var), + _ => return None, + }; + // `get_builtin().getdictvalue('__import__')` is an analyzed, non-elidable + // lookup: it can raise ImportError but neither runs Python nor writes the + // GC heap. Use EF_CAN_RAISE with concrete-empty effect sets. The generic + // Plain flavor means “no graph was analyzed” and becomes RANDOM_EFFECTS / + // CALL_MAY_FORCE, contradicting PyPy's zero-forcing IMPORT_NAME trace. + let mut effect_info = majit_ir::EffectInfo::default(); + effect_info.pyre_helper = majit_ir::PyreHelperKind::LoadImport; + Some(build_residual_call_r_r_insn_with_effect_info( + ctx.load_import_fn_idx, + vec![frame_operand], + effect_info, + dst_reg, + )) +} + +/// Lower IMPORT_NAME's locals argument to a one-Ref residual call. +/// +/// `bh_load_import_locals_fn` dereferences `PyFrame.debugdata` and then +/// `FrameDebugData.w_locals`, and the optimizer caches both: `debugdata` is +/// virtualizable field 3, and the `f_locals` fold reads `w_locals` through +/// `frame_debug_data_w_locals_descr`, which is mutable because +/// `setdictscope` and `fast2locals` rebind it. Neither read is analyzed +/// here, so this takes the same `Plain` lowering as `load_locals`, the +/// sibling helper that reads the same two fields: no write set was computed, +/// which is `EF_RANDOM_EFFECTS`, and `OptHeap` routes that to `clean_caches` +/// instead of `force_from_effectinfo`. It also dispatches as +/// `CALL_MAY_FORCE` -- `RandomEffects` clears +/// `check_forces_virtual_or_virtualizable()` -- which is what syncs the +/// virtualizable the `debugdata` read goes through. +/// +/// A `PlainCannotRaise` lowering would instead advertise concrete-empty +/// readonly and write sets, so a lazy set pending on either field would not +/// be flushed before the helper reads it and `__import__` would receive a +/// stale mapping. Declaring the two descrs readonly does not recover that: +/// `compute_bitstrings` never runs on the `JitDriver` path, so every +/// `check_readonly_descr_field` lookup misses on the `u32::MAX` sentinel, +/// and the raw-set fallback in `force_from_effectinfo` covers only the write +/// side. The `EF_RANDOM_EFFECTS` boundary is what carries the reads. +pub fn lower_load_import_locals_hlop_to_insn( + op: &super::flow::SpaceOperation, + ctx: &LoweringContext, + get_register: &mut F, + lower_constant: &mut LC, +) -> Option +where + F: FnMut(super::flow::Variable) -> Register, + LC: FnMut(&Constant) -> Operand, +{ + lower_frame_only_ref_hlop_to_insn( + op, + "load_import_locals", + ctx.load_import_locals_fn_idx, + majit_ir::PyreHelperKind::LoadImportLocals, + get_register, + lower_constant, + ) +} + /// Lower pyopcode.py DELETE_GLOBAL to a void two-Ref residual call. pub fn lower_delete_global_hlop_to_insn( op: &super::flow::SpaceOperation, @@ -4873,7 +4949,6 @@ pub fn build_residual_call_r_r_insn_from_operands( pyre_helper: majit_ir::PyreHelperKind, dst_reg: Register, ) -> Insn { - let arg_kinds = vec![Kind::Ref; ref_operands.len()]; // `bh_call_fn_N` dispatches the callable supplied at runtime. Its target // is therefore the indirect-call top set, not the empty effect set of the // helper wrapper itself: user code can mutate any escaped heap location. @@ -4887,6 +4962,16 @@ pub fn build_residual_call_r_r_insn_from_operands( effect_info_for_call_flavor(flavor) }; effect_info.pyre_helper = pyre_helper; + build_residual_call_r_r_insn_with_effect_info(fn_idx, ref_operands, effect_info, dst_reg) +} + +fn build_residual_call_r_r_insn_with_effect_info( + fn_idx: u16, + ref_operands: Vec, + effect_info: majit_ir::EffectInfo, + dst_reg: Register, +) -> Insn { + let arg_kinds = vec![Kind::Ref; ref_operands.len()]; let descr_operand = Operand::descr(DescrOperand::CallDescrStub(CallDescrStub { effect_info, arg_kinds, @@ -5426,6 +5511,13 @@ where if let Some(insn) = lower_load_build_class_hlop_to_insn(op, ctx, get_register, lower_constant) { return Some(insn); } + if let Some(insn) = lower_load_import_hlop_to_insn(op, ctx, get_register, lower_constant) { + return Some(insn); + } + if let Some(insn) = lower_load_import_locals_hlop_to_insn(op, ctx, get_register, lower_constant) + { + return Some(insn); + } if let Some(insn) = lower_tuple_build_hlop_to_insn(op, ctx, get_register, lower_constant) { return Some(insn); } @@ -5478,9 +5570,6 @@ where if let Some(insn) = lower_convert_value_hlop_to_insn(op, ctx, get_register, lower_constant) { return Some(insn); } - if let Some(insn) = lower_import_name_hlop_to_insn(op, ctx, get_register, lower_constant) { - return Some(insn); - } if let Some(insn) = lower_import_from_hlop_to_insn(op, ctx, get_register, lower_constant) { return Some(insn); } @@ -6689,67 +6778,6 @@ where )) } -/// Lower the IMPORT_NAME pyre HLOp `import_name(fromlist, level, code, frame, -/// name_idx)` → `result: Ref` to `residual_call_ir_r(ConstInt( -/// import_name_fn_idx), ListI([name_idx]), ListR([fromlist, level, code, -/// frame]), Descr) → reg`, the four-Ref sibling of -/// [`lower_getattr_hlop_to_insn`] (same `(refs.., int) → ref` marshalling the -/// void STORE_ATTR residual already proves with `[obj, value, code]`). -/// `bh_import_name_fn` resolves the module name from the code object and runs -/// `__import__` (module top-level Python may force → `MayForce`); `frame` is -/// the live red frame the residual reads `__name__`/`__package__` from for -/// relative-import resolution, mirroring `bh_load_global_fn`'s threaded frame -/// pointer instead of `getexecutioncontext().gettopframe()`. -/// -/// Returns `None` for non-`import_name` opnames so the caller can fall -/// through to other lowering arms. -pub fn lower_import_name_hlop_to_insn( - op: &super::flow::SpaceOperation, - ctx: &LoweringContext, - get_register: &mut F, - lower_constant: &mut LC, -) -> Option -where - F: FnMut(super::flow::Variable) -> Register, - LC: FnMut(&Constant) -> Operand, -{ - if op.opname != "import_name" || op.args.len() != 5 { - return None; - } - let fromlist = operand_for_value_arg(&op.args[0], get_register, lower_constant)?; - let level = operand_for_value_arg(&op.args[1], get_register, lower_constant)?; - let code = operand_for_value_arg(&op.args[2], get_register, lower_constant)?; - let frame = operand_for_value_arg(&op.args[3], get_register, lower_constant)?; - let name_idx = const_int_for_value_arg(&op.args[4])?; - let dst_reg = match &op.result { - Some(super::flow::FlowValue::Variable(var)) => get_register(*var), - _ => return None, - }; - let effect_info = effect_info_for_call_flavor(CallFlavor::MayForce); - let descr_operand = Operand::descr(DescrOperand::CallDescrStub(CallDescrStub { - effect_info, - arg_kinds: vec![Kind::Ref, Kind::Ref, Kind::Ref, Kind::Ref, Kind::Int], - result_kind: Some(Kind::Ref), - void_word_abi: false, - })); - Some(Insn::op_with_result( - "residual_call_ir_r", - vec![ - Operand::ConstInt(ctx.import_name_fn_idx as i64), - Operand::ListOfKind(ListOfKind::new( - Kind::Int, - vec![Operand::ConstInt(name_idx)], - )), - Operand::ListOfKind(ListOfKind::new( - Kind::Ref, - vec![fromlist, level, code, frame], - )), - descr_operand, - ], - dst_reg, - )) -} - /// Lower the IMPORT_FROM pyre HLOp `import_from(module, code, name_idx)` → /// `result: Ref` to `residual_call_ir_r(ConstInt(import_from_fn_idx), /// ListI([name_idx]), ListR([module, code]), Descr) → reg` — the same @@ -11738,7 +11766,6 @@ mod tests { format_simple_fn_idx: 101, format_with_spec_fn_idx: 102, convert_value_fn_idx: 104, - import_name_fn_idx: 105, import_from_fn_idx: 117, load_super_attr_fn_idx: 106, super_attr_unwrap_fn_idx: 107, @@ -13553,112 +13580,156 @@ mod tests { } #[test] - fn lower_import_name_hlop_emits_import_name_fn_residual() { - // `import_name(fromlist, level, code, frame, name_idx)` → - // `residual_call_ir_r(ConstInt(import_name_fn_idx), ListI([name_idx]), - // ListR([fromlist, level, code, frame]), Descr) → reg` (MayForce — - // module top-level Python may run). Four Ref operands plus one Int; - // `frame` is the live red frame the residual reads - // `__name__`/`__package__` from for relative-import resolution. - let fromlist_var = Variable::new(VariableId(8), Kind::Ref); - let level_var = Variable::new(VariableId(10), Kind::Ref); - let frame_var = Variable::new(VariableId(11), Kind::Ref); - let result_var = Variable::new(VariableId(9), Kind::Ref); - let (ctx, code_const, name_idx_const) = load_attr_lowering_fixture(); - let op = super::super::flow::SpaceOperation::new( - "import_name", - vec![ - fromlist_var.into(), - level_var.into(), - code_const.into(), - frame_var.into(), - name_idx_const.into(), - ], - Some(result_var.into()), + fn lower_load_import_locals_hlop_keeps_the_unanalyzed_boundary() { + // `pyopcode.py:1119-1125` reads the frame's debug locals before the + // call, and the helper that answers it dereferences + // `PyFrame.debugdata` -- virtualizable field 3 -- and then + // `FrameDebugData.w_locals`, which the `f_locals` fold caches and + // which `setdictscope` rebinds. Neither read is analyzed here, so + // the residual keeps the one-Ref frame-receiver shape its sibling + // `load_locals` carries: `EF_RANDOM_EFFECTS`, dispatched as + // `CALL_MAY_FORCE` so the virtualizable is synced and `OptHeap` + // flushes the cached field before the helper reads either. + let frame = Variable::new(VariableId(8), Kind::Ref); + let result = Variable::new(VariableId(9), Kind::Ref); + let op = SpaceOperation::new( + "load_import_locals", + vec![frame.into()], + Some(result.into()), 0, ); - let mut get_register = |var: Variable| match var.id { - VariableId(8) => Register { - kind: Kind::Ref, - index: 101, - }, - VariableId(10) => Register { - kind: Kind::Ref, - index: 103, - }, - VariableId(11) => Register { - kind: Kind::Ref, - index: 104, - }, - VariableId(9) => Register { - kind: Kind::Ref, - index: 102, - }, - _ => panic!("unexpected var id {:?}", var.id), + let ctx = LoweringContext { + load_import_locals_fn_idx: 135, + ..Default::default() }; - let mut lower_constant = super::flatten_constant_operand_for_test; - let insn = super::lower_import_name_hlop_to_insn( + let mut get_register = identity_register_mapper(); + let mut lower_constant = test_constant_lowering(); + let insn = lower_load_import_locals_hlop_to_insn( &op, &ctx, &mut get_register, &mut lower_constant, ) - .expect("5-arg import_name lowering must succeed"); + .expect("load_import_locals lowering must succeed"); + match insn { Insn::Op { opname, args, - result, + result: Some(dst), } => { - assert_eq!(opname, "residual_call_ir_r"); - assert!( - matches!(args[0], Operand::ConstInt(105)), - "import_name_fn pool index, got {:?}", - args[0] - ); + assert_eq!(opname, "residual_call_r_r"); + assert!(matches!(args[0], Operand::ConstInt(135))); match &args[1] { Operand::ListOfKind(list) => { - assert_eq!(list.kind, Kind::Int); - assert!( - matches!(&list.content[..], [Operand::ConstInt(5)]), - "ListI = [name_idx], got {:?}", - list.content - ); + assert_eq!(list.kind, Kind::Ref); + assert!(matches!( + &list.content[..], + [Operand::Register(Register { + kind: Kind::Ref, + index: 8 + })] + )); } - other => panic!("expected ListI, got {other:?}"), + other => panic!("expected one-Ref ListR, got {other:?}"), } + assert_eq!(dst, Register::new(Kind::Ref, 9)); match &args[2] { + Operand::Descr(descr) => match &**descr { + DescrOperand::CallDescrStub(stub) => { + assert_eq!( + stub.effect_info.pyre_helper, + majit_ir::PyreHelperKind::LoadImportLocals + ); + // A `CannotRaise` shape here would advertise + // concrete-empty readonly and write sets the + // helper never earned, and `force_from_effectinfo` + // would then skip both fields: there is no + // raw-set fallback on the readonly side, and + // `compute_bitstrings` never runs on this path. + assert_eq!( + stub.effect_info.extraeffect, + majit_ir::ExtraEffect::RandomEffects + ); + assert_eq!( + dispatch_kind_for_effect_info(&stub.effect_info), + CallFlavor::MayForce + ); + assert!(stub.effect_info.has_random_effects()); + } + other => panic!("expected CallDescrStub, got {other:?}"), + }, + other => panic!("expected call descr, got {other:?}"), + } + } + other => panic!("expected Insn::Op, got {other:?}"), + } + } + + #[test] + fn lower_load_import_hlop_emits_builtin_lookup_residual() { + // PyPy IMPORT_NAME performs the builtin lookup separately from its + // ordinary Python call. The lookup therefore has the same one-frame + // analyzed EF_CAN_RAISE residual shape, while the following + // `simple_call` remains visible to the meta-tracer. + let frame = Variable::new(VariableId(8), Kind::Ref); + let result = Variable::new(VariableId(9), Kind::Ref); + let op = SpaceOperation::new("load_import", vec![frame.into()], Some(result.into()), 0); + let ctx = LoweringContext { + load_import_fn_idx: 134, + ..Default::default() + }; + let mut get_register = identity_register_mapper(); + let mut lower_constant = test_constant_lowering(); + let insn = + lower_load_import_hlop_to_insn(&op, &ctx, &mut get_register, &mut lower_constant) + .expect("load_import lowering must succeed"); + + match insn { + Insn::Op { + opname, + args, + result: Some(dst), + } => { + assert_eq!(opname, "residual_call_r_r"); + assert!(matches!(args[0], Operand::ConstInt(134))); + match &args[1] { Operand::ListOfKind(list) => { assert_eq!(list.kind, Kind::Ref); - match &list.content[..] { - [ - Operand::Register(fl), - Operand::Register(lv), - Operand::ConstRef(0x2000), - Operand::Register(fr), - ] => { - assert_eq!(fl.index, 101, "leading Ref operand must be fromlist"); - assert_eq!(lv.index, 103, "second Ref operand must be level"); - assert_eq!(fr.index, 104, "fourth Ref operand must be frame"); - } - other => { - panic!( - "ListR must be [fromlist, level, code, frame], got {other:?}" - ) - } - } + assert!(matches!( + &list.content[..], + [Operand::Register(Register { + kind: Kind::Ref, + index: 8 + })] + )); } - other => panic!("expected ListR, got {other:?}"), + other => panic!("expected one-Ref ListR, got {other:?}"), + } + assert_eq!(dst, Register::new(Kind::Ref, 9)); + match &args[2] { + Operand::Descr(descr) => match &**descr { + DescrOperand::CallDescrStub(stub) => { + assert_eq!( + stub.effect_info.pyre_helper, + majit_ir::PyreHelperKind::LoadImport + ); + assert_eq!( + stub.effect_info.extraeffect, + majit_ir::ExtraEffect::CanRaise + ); + assert_eq!( + dispatch_kind_for_effect_info(&stub.effect_info), + CallFlavor::Plain + ); + assert!(!stub.effect_info.has_random_effects()); + } + other => panic!("expected CallDescrStub, got {other:?}"), + }, + other => panic!("expected call descr, got {other:?}"), } - assert_eq!( - result, - Some(Register { - kind: Kind::Ref, - index: 102 - }), - ); } - _ => panic!("expected Insn::Op, got {insn:?}"), + other => panic!("expected residual_call_r_r, got {other:?}"), } } diff --git a/pyre/pyre-native/src/ssl.rs b/pyre/pyre-native/src/ssl.rs index efea179bd14..617ad9914b7 100644 --- a/pyre/pyre-native/src/ssl.rs +++ b/pyre/pyre-native/src/ssl.rs @@ -533,8 +533,8 @@ fn read_private_key(data: &[u8], password: Option<&[u8]>) -> NativeResult, ) -> NativeResult { ensure_provider(); @@ -588,7 +588,10 @@ fn parse_concatenated_der(mut data: &[u8]) -> NativeResult>> { /// # Safety /// `context` must point to a live [`Context`]. #[inline(never)] -pub unsafe fn context_load_verify_file(context: *mut Context, path: &str) -> NativeResult { +pub unsafe fn context_load_verify_file( + context: *mut Context, + path: &std::path::Path, +) -> NativeResult { let data = std::fs::read(path).map_err(io_error)?; let items = rustls_pemfile::read_all(&mut Cursor::new(data)) .collect::, _>>() @@ -627,9 +630,9 @@ pub unsafe fn context_load_verify_file(context: *mut Context, path: &str) -> Nat /// # Safety /// `context` must point to a live [`Context`]. #[inline(never)] -pub unsafe fn context_add_verify_dir(context: *mut Context, path: &str) { +pub unsafe fn context_add_verify_dir(context: *mut Context, path: &std::path::Path) { let context = unsafe { &mut *context }; - let path = std::path::PathBuf::from(path); + let path = path.to_path_buf(); if !context.capaths.iter().any(|known| known == &path) { context.capaths.push(path); *context @@ -988,7 +991,7 @@ pub fn certificate_decode_der(der: &[u8]) -> NativeResult<*mut DecodedCertificat } #[inline(never)] -pub fn certificate_decode_file(path: &str) -> NativeResult<*mut DecodedCertificate> { +pub fn certificate_decode_file(path: &std::path::Path) -> NativeResult<*mut DecodedCertificate> { let data = std::fs::read(path).map_err(io_error)?; let certs = read_pem_certificates(&data)?; certificate_decode_der(certs[0].as_ref()) diff --git a/pyre/pyre-object/src/bytearrayobject.rs b/pyre/pyre-object/src/bytearrayobject.rs index fce06f8c0e6..9e2dfb661cf 100644 --- a/pyre/pyre-object/src/bytearrayobject.rs +++ b/pyre/pyre-object/src/bytearrayobject.rs @@ -94,37 +94,27 @@ fn w_bytearray_alloc(buf: Vec) -> PyObjectRef { ob_type: &BYTEARRAY_TYPE as *const PyType, w_class: get_instantiate(&BYTEARRAY_TYPE), }; + let body = W_BytearrayObject { + ob_header: header, + data, + length, + alloc, + logical_offset: 0, + exports: 0, + w_dict: PY_NULL, + w_weakreflifeline: PY_NULL, + }; let raw = crate::gc_hook::try_gc_alloc_stable_raw(W_BYTEARRAY_GC_TYPE_ID, W_BYTEARRAY_OBJECT_SIZE); - if !raw.is_null() { + let w_bytearray = if !raw.is_null() { unsafe { - std::ptr::write( - raw as *mut W_BytearrayObject, - W_BytearrayObject { - ob_header: header, - data, - length, - alloc, - logical_offset: 0, - exports: 0, - w_dict: PY_NULL, - w_weakreflifeline: PY_NULL, - }, - ); + std::ptr::write(raw as *mut W_BytearrayObject, body); } raw as PyObjectRef } else { - crate::lltype::malloc_typed(W_BytearrayObject { - ob_header: header, - data, - length, - alloc, - logical_offset: 0, - exports: 0, - w_dict: PY_NULL, - w_weakreflifeline: PY_NULL, - }) as PyObjectRef - } + crate::lltype::malloc_typed(body) as PyObjectRef + }; + w_bytearray } /// Allocate a new bytearray filled with zeros. diff --git a/pyre/pyre-object/src/bytesobject.rs b/pyre/pyre-object/src/bytesobject.rs index 2708b98b779..58021a48cd3 100644 --- a/pyre/pyre-object/src/bytesobject.rs +++ b/pyre/pyre-object/src/bytesobject.rs @@ -9,10 +9,11 @@ use crate::pyobject::*; pub static BYTES_TYPE: PyType = crate::pyobject::new_pytype("bytes"); -/// GC-managed byte buffer shared by `bytes` and `bytearray` bodies. +/// GC-managed byte buffer behind a `bytearray` body. /// /// The `Vec` is a leaf (no inner `PyObjectRef`s); its GC box carries only -/// drop glue that reclaims the buffer on sweep. +/// drop glue that reclaims the buffer on sweep. `bytes` holds its payload +/// inline instead, in a [`BytesBlock`]. pub type BytesDataStorage = Vec; /// Runtime-assigned GC type id for [`BytesDataStorage`]. Like the set-items @@ -31,16 +32,140 @@ pub fn bytes_data_gc_type_id() -> u32 { BYTES_DATA_GC_TYPE_ID.load(std::sync::atomic::Ordering::Relaxed) } +/// `rstr.py:1226-1228` — `STR.become(GcStruct('rpy_string', ('hash', Signed), +/// ('chars', Array(Char, ...))))`: the byte payload is a varsize `GcArray` +/// inside the managed heap, not a pointer to memory the collector cannot see. +/// +/// A storage box holds a `Vec` whose bytes live in the Rust heap, so the +/// collector sizes the payload as the 24 bytes of the container and its +/// major-collection threshold never learns about the buffer. The bytes here sit +/// after the length header, so `encode_type_shape`'s varsize rule +/// (`gctypelayout.py`) sizes the block from its own contents and the +/// threshold moves by what was actually allocated. +/// +/// Same block shape as [`crate::object_array::TypedItemsBlock`], with `Char` +/// items instead of words. +#[repr(C)] +pub struct BytesBlock { + /// The GcArray length header — the collector's `ofstolength`. + pub length: usize, + /// `chars` inline after the header; size known only at allocation time. + chars: [u8; 0], +} + +/// Offset of `chars[0]` — the collector's `ofstovar`. +pub const BYTES_BLOCK_CHARS_OFFSET: usize = std::mem::offset_of!(BytesBlock, chars); + +/// Offset of the length header the collector reads as the GcArray length. +pub const BYTES_BLOCK_LEN_OFFSET: usize = std::mem::offset_of!(BytesBlock, length); + +/// `get_array_token(Array(Char))` — the one triple every consumer of this +/// block's shape reads, as `encode_type_shape` reads all three from the one +/// ARRAY. +pub const BYTES_BLOCK_TOKEN: crate::object_array::ArrayToken = crate::object_array::ArrayToken { + base_size: BYTES_BLOCK_CHARS_OFFSET, + item_size: std::mem::size_of::(), + len_offset: BYTES_BLOCK_LEN_OFFSET, +}; + +// The collector sizes a block as `base_size + item_size * length` read at +// `len_offset`, and `bytes_block_chars` reads the payload at `base_size`. A +// field added to `BytesBlock` moves both silently, so pin the shape here: the +// length header first, the chars one word in, and one byte per item. +const _: () = { + assert!(BYTES_BLOCK_LEN_OFFSET == 0); + assert!(BYTES_BLOCK_CHARS_OFFSET == std::mem::size_of::()); + assert!(BYTES_BLOCK_TOKEN.item_size == 1); +}; + +/// Runtime-assigned GC type id for [`BytesBlock`], published by +/// `pyre-jit::eval` with the other tail registrations. +static BYTES_BLOCK_GC_TYPE_ID: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0); + +/// Record the GC type id registered for [`BytesBlock`]. +pub fn set_bytes_block_gc_type_id(id: u32) { + BYTES_BLOCK_GC_TYPE_ID.store(id, std::sync::atomic::Ordering::Relaxed); +} + +/// Read the runtime-assigned GC type id for [`BytesBlock`]. +#[majit_macros::dont_look_inside] +pub fn bytes_block_gc_type_id() -> u32 { + BYTES_BLOCK_GC_TYPE_ID.load(std::sync::atomic::Ordering::Relaxed) +} + +/// Allocate a [`BytesBlock`] holding `bytes`. +/// +/// The tier is the stable (old-gen) one the storage box already used: a +/// caller holds the returned block on the unrooted Rust stack while it +/// allocates the object body, and `try_gc_alloc_stable_raw` is the hook whose +/// address survives both collection kinds. It also keeps +/// [`w_bytes_data`]'s `&'static [u8]` pointing at bytes that never move. +/// +/// Falls back to a plain allocation before the GC is up or in a unit test, +/// where the block is immortal, as the storage box's `malloc_raw` fallback is. +pub fn alloc_bytes_block(bytes: &[u8]) -> *mut BytesBlock { + let size = BYTES_BLOCK_CHARS_OFFSET + bytes.len(); + let tid = bytes_block_gc_type_id(); + let raw = if tid != 0 { + crate::gc_hook::try_gc_alloc_stable_raw(tid, size) + } else { + std::ptr::null_mut() + }; + let block = if raw.is_null() { + let layout = bytes_block_layout(bytes.len()); + // SAFETY: the layout is non-zero — the header alone occupies a word. + unsafe { std::alloc::alloc(layout) } + } else { + raw + }; + if block.is_null() { + std::alloc::handle_alloc_error(bytes_block_layout(bytes.len())); + } + // SAFETY: `block` names `size` writable bytes, which is the header + // followed by `bytes.len()` char slots. + unsafe { + let block = block as *mut BytesBlock; + (*block).length = bytes.len(); + std::ptr::copy_nonoverlapping( + bytes.as_ptr(), + std::ptr::addr_of_mut!((*block).chars) as *mut u8, + bytes.len(), + ); + block + } +} + +/// The `[header | chars]` layout of a block holding `len` bytes. +fn bytes_block_layout(len: usize) -> std::alloc::Layout { + std::alloc::Layout::from_size_align( + BYTES_BLOCK_CHARS_OFFSET + len, + std::mem::align_of::(), + ) + .expect("bytes block layout") +} + +/// The `chars` of a block, as `rstr.py`'s `ll_chars` reads them. +/// +/// # Safety +/// `block` must name a live [`BytesBlock`]. +pub unsafe fn bytes_block_chars(block: *const BytesBlock) -> &'static [u8] { + unsafe { + std::slice::from_raw_parts( + std::ptr::addr_of!((*block).chars) as *const u8, + (*block).length, + ) + } +} + /// Python bytes object — immutable byte sequence. /// -/// PyPy: W_BytesObject stores `_value` (RPython string). -/// pyre: stores a heap-allocated `Vec` in a GC-managed non-moving storage -/// box (off-GC storage), same layout as W_BytearrayObject but without -/// setitem/extend. +/// `W_BytesObject._value` is an RPython string, whose `chars` array +/// (`rstr.py:1226-1228`) lives in the managed heap; [`BytesBlock`] is that +/// array. Same layout as W_BytearrayObject but without setitem/extend. #[repr(C)] pub struct W_BytesObject { pub ob_header: PyObject, - pub data: *const Vec, + pub data: *const BytesBlock, pub len: usize, /// Strong references owned by ctypes `_objects` dictionaries. Pyre is a /// tracing-GC runtime, so it has no CPython `ob_refcnt`; this trailing @@ -56,7 +181,7 @@ pub struct W_BytesObject { pub w_weakreflifeline: PyObjectRef, } -/// `W_BytesObject.data` — the pointer to the heap-allocated byte buffer. +/// `W_BytesObject.data` — the pointer to the block holding the bytes. pub const BYTES_DATA_OFFSET: usize = std::mem::offset_of!(W_BytesObject, data); /// `W_BytesObject.len` — the byte count, the analogue of the `strlen` PyPy @@ -89,8 +214,8 @@ impl crate::lltype::GcType for W_BytesObject { /// Allocate a new bytes object from a byte slice. /// -/// The `data` buffer lives in a GC-managed non-moving storage box; the sweep -/// reclaims it through the box tid's drop glue. The `W_BytesObject` body is +/// The `data` block is a varsize GcArray in the managed heap, so the sweep +/// reclaims it with no drop glue to run. The `W_BytesObject` body is /// allocated in GC old-gen (`try_gc_alloc_stable_raw`) so the collector traces /// through it and greys the box, mirroring `w_list_new`/`w_set_new`. Falls back /// to `malloc_typed`/`malloc_raw` when no GC hook is installed (unit tests). @@ -100,37 +225,40 @@ impl crate::lltype::GcType for W_BytesObject { #[majit_macros::dont_look_inside] pub fn w_bytes_from_bytes(bytes: &[u8]) -> PyObjectRef { let len = bytes.len(); - let data = crate::gc_storage::gc_alloc_storage_box(bytes.to_vec(), bytes_data_gc_type_id()); - let header = PyObject { - ob_type: &BYTES_TYPE as *const PyType, - w_class: get_instantiate(&BYTES_TYPE), - }; + // `build_list_storage` (listobject.rs) states the rule the block obeys: + // old-gen is mark-sweep, so a block with no heap edge yet is sweepable + // rather than merely immobile, and it has to be rooted across every later + // GC operation. `try_gc_alloc_stable_raw` is one of them + // (`IntArray::pin_block`), and `get_instantiate` allocates as well, so both + // the block and the class travel on the shadow stack and are read back from + // their slots once the last allocation is behind them. + let _roots = crate::gc_roots::push_roots(); + let data_slot = crate::gc_roots::shadow_stack_len(); + let data = alloc_bytes_block(bytes); + let _ = crate::gc_roots::pin_root(data as PyObjectRef); + let class_slot = crate::gc_roots::shadow_stack_len(); + let _ = crate::gc_roots::pin_root(get_instantiate(&BYTES_TYPE)); let raw = crate::gc_hook::try_gc_alloc_stable_raw(W_BYTES_GC_TYPE_ID, W_BYTES_OBJECT_SIZE); - if !raw.is_null() { + let body = W_BytesObject { + ob_header: PyObject { + ob_type: &BYTES_TYPE as *const PyType, + w_class: crate::gc_roots::shadow_stack_get(class_slot), + }, + data: crate::gc_roots::shadow_stack_get(data_slot) as *const BytesBlock, + len, + ctypes_keepalive_refs: 0, + w_dict: PY_NULL, + w_weakreflifeline: PY_NULL, + }; + let w_bytes = if !raw.is_null() { unsafe { - std::ptr::write( - raw as *mut W_BytesObject, - W_BytesObject { - ob_header: header, - data, - len, - ctypes_keepalive_refs: 0, - w_dict: PY_NULL, - w_weakreflifeline: PY_NULL, - }, - ); + std::ptr::write(raw as *mut W_BytesObject, body); } raw as PyObjectRef } else { - crate::lltype::malloc_typed(W_BytesObject { - ob_header: header, - data, - len, - ctypes_keepalive_refs: 0, - w_dict: PY_NULL, - w_weakreflifeline: PY_NULL, - }) as PyObjectRef - } + crate::lltype::malloc_typed(body) as PyObjectRef + }; + w_bytes } /// Allocate a bytes-subclass instance in the managed heap. PyPy's @@ -141,6 +269,13 @@ pub fn w_bytes_subclass_from_bytes(bytes: &[u8], w_class: PyObjectRef) -> PyObje let _roots = crate::gc_roots::push_roots(); let root_base = crate::gc_roots::shadow_stack_len(); let _ = crate::gc_roots::pin_root(w_class); + // The block is allocated before the body and rooted across it, not built + // inside the struct literal: the literal is evaluated after + // `try_gc_alloc_stable_raw` has already produced `raw`, which leaves that + // fresh body unrooted across the block's own allocation. + let data_slot = crate::gc_roots::shadow_stack_len(); + let data = alloc_bytes_block(bytes); + let _ = crate::gc_roots::pin_root(data as PyObjectRef); let raw = crate::gc_hook::try_gc_alloc_stable_raw( ::type_id(), ::SIZE, @@ -150,7 +285,7 @@ pub fn w_bytes_subclass_from_bytes(bytes: &[u8], w_class: PyObjectRef) -> PyObje ob_type: &BYTES_TYPE as *const PyType, w_class: crate::gc_roots::shadow_stack_get(root_base), }, - data: crate::lltype::malloc_raw(bytes.to_vec()), + data: crate::gc_roots::shadow_stack_get(data_slot) as *const BytesBlock, len: bytes.len(), ctypes_keepalive_refs: 0, w_dict: PY_NULL, @@ -262,8 +397,7 @@ pub unsafe fn w_bytes_getitem(obj: PyObjectRef, index: usize) -> u8 { pub unsafe fn w_bytes_data(obj: PyObjectRef) -> &'static [u8] { unsafe { let b = obj as *const W_BytesObject; - let data_ref: &Vec = &*(*b).data; - data_ref.as_slice() + bytes_block_chars((*b).data) } }