From c925796d5288281a4e1241d40c8bd9b5ec757401 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 12 Aug 2026 18:54:17 +0900 Subject: [PATCH 1/5] jit-trace: preserve semantic maps at non-live jitcode PCs --- pyre/pyre-jit-trace/src/state.rs | 139 +++++++++++++++++++------------ 1 file changed, 88 insertions(+), 51 deletions(-) diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 72c3be36ac7..18adca996fc 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -1919,8 +1919,7 @@ pub(crate) struct BridgeSemanticMaps { /// leg that fires first is ever named, so a run in which one leg never executed /// is indistinguishable from one in which it executed second. /// -/// Pure telemetry — with the variable unset every counter path is skipped and -/// the fallback's value is unchanged. +/// Pure telemetry — with the variable unset every counter path is skipped. struct EmptyTwinSite { name: &'static str, hits: std::sync::atomic::AtomicU64, @@ -1954,17 +1953,14 @@ static EMPTY_TWIN_MISS: EmptyTwinSite = EmptyTwinSite::new("twin_miss"); /// the carried offset does not decode as a `-live-` anchored startpoint. static EMPTY_TWIN_NON_DECODABLE: EmptyTwinSite = EmptyTwinSite::new("non_decodable"); -/// `jit_pc` / `twin` report the coordinate this leg declined on and what the -/// jitcode-keyed containing-depth twin would have answered there — the -/// measurement the "route the fallback through the twins instead of answering -/// 0" question needs. On the 399-file synth corpus all 20 executions answer -/// `jit_pc >= 0` with `twin = Some(3..=5)`, so that change is not inert. -/// -/// What this does NOT resolve: the counters are per leg, not per call site, so -/// a fire cannot be attributed to one of the callers. That matters, because -/// `setup_bridge_sym` is the only one whose `stack_depth_at_pc` read is -/// unconditional; at the others an empty `pcdep_entries` makes the depth inert -/// regardless of its value. +/// `jit_pc` / `twin` report the coordinate that reached the fallback and the +/// jitcode-keyed containing depth used there. Before that twin became the +/// production fallback, the 399-file synth corpus showed all 20 executions at +/// `jit_pc >= 0` with `twin = Some(3..=5)` while the function returned 0. +/// +/// The counters remain per leg, not per call site, so a fire cannot be +/// attributed to one of the callers. `setup_bridge_sym` is the consumer whose +/// `stack_depth_at_pc` read is unconditional. fn empty_twin_census( site: &EmptyTwinSite, rp: Option, @@ -2063,8 +2059,12 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // computed kept operand-stack temps are live; at the merge-target PC // they've been consumed and carry no pcdep entry. // - // A caller holding no Python coordinate answers 0. That is a decline, - // and a lossy one — not the best available answer. + // A caller holding no Python coordinate reads the floor-only static + // depth twin. This is independent of whether `jitcode_pc` itself is + // a `-live-`-anchored startpoint: the frame width and the per-slot + // color map are codewriter sidecars keyed by the containing operation, + // while `can_decode_live_vars` only decides whether the register-value + // stream can be enumerated directly at that byte. // // Jitcode-keyed spellings of this same static table exist: // `depth_containing_for_jitcode_pc` is built (`finalize_jitcode`, the @@ -2072,26 +2072,29 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // `depth_at_py_pc[containing_py]` for every offset, and // `depth_trivia_for_jitcode_pc` reads it at the trivia-skipped py. // `collect_outer_active_boxes`, the encode half this mirrors, already - // sources its own `stack_depth_at_pc` from those twins with no Python - // PC in hand. The census below measured what they would answer here: + // sources its own `stack_depth_at_pc` and pcdep from JitCode-keyed + // twins with no Python PC in hand. The census below measured what the + // containing-depth twin answers here: // over the 399-file synth corpus every one of the 20 executions of this // leg carried a non-negative offset at which the containing twin - // answers 3, 4 or 5 — so routing through it is a real change in the - // reconstructed frame's width, not a no-op, and it is left to its own - // measurement rather than folded into a coordinate repair. + // answers 3, 4 or 5. Returning 0 truncated the reconstructed frame's + // semantic prefix and dropped exactly those operand-stack slots. // // What this leg must not do, and did before the `Option`, is index the // Python-keyed table with the JitCode word. let via_py_pc = |rp: Option, site: &'static EmptyTwinSite| -> usize { - // Census-only: what the jitcode-keyed twin would answer at the - // coordinate this leg is declining on. Behind the census gate, so a - // production run does no extra work for it. - let twin = (crate::py_coord::emptytwin_census_enabled() && jitcode_pc >= 0) + // The JitCode-keyed static twin is the production fallback when no + // sanctioned Python coordinate is carried. It is built from the + // same `depth_at_py_pc` table as the raw read below and therefore + // preserves frame width without projecting a JitCode byte offset + // through a Python-keyed map. + let twin = (jitcode_pc >= 0) .then(|| payload.depth_containing_for_jitcode_pc(jitcode_pc as usize)) .flatten(); let Some(rp) = rp.and_then(|p| usize::try_from(p).ok()) else { - empty_twin_census(site, None, None, 0, jitcode_pc, twin); - return 0; + let depth = twin.unwrap_or(0); + empty_twin_census(site, None, None, depth, jitcode_pc, twin); + return depth as usize; }; if payload.code_ptr.is_null() { empty_twin_census(site, Some(rp), None, 0, jitcode_pc, twin); @@ -2102,29 +2105,30 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( empty_twin_census(site, Some(rp), Some(table.len()), depth, jitcode_pc, twin); depth as usize }; - let (stack_depth_at_pc, pcdep_entries) = if jitcode_pc >= 0 - && payload - .jitcode - .can_decode_live_vars(jitcode_pc as usize, sd.op_live) - { + let (stack_depth_at_pc, pcdep_entries) = if jitcode_pc >= 0 { let jp = jitcode_pc as usize; // Decode-identity: source depth/pcdep from the carried genuine - // `jitcode_pc` via the predecessor-keyed twins. Every real carried - // coordinate is an op-start or block-head offset of a colored - // jitcode, for which the codewriter seeds these twins; a twin miss - // degrades to the caller's own merge-target Python PC, keeping - // liveness and pcdep on one coordinate, and to 0 when the caller - // has none. - match ( - payload.depth_for_jitcode_pc_pred(jp), - payload.pcdep_for_jitcode_pc(jp), - ) { - (Some(depth), Some(pcdep)) => (depth as usize, pcdep), - _ => (via_py_pc(py_pc, &EMPTY_TWIN_MISS), Vec::new()), + // `jitcode_pc` via the predecessor-keyed twins. A `-live-` + // startpoint uses the walk-time depth twin, exactly as the encoder + // does. A non-decodable operation byte still owns semantic + // sidecars: retain its pcdep and obtain frame width from the static + // containing-depth twin instead of collapsing both to empty/zero. + // RPython's resume reader keeps these concerns separate too: + // `setup_resume_at_op(pc)` selects the frame position and + // `consume_boxes(get_current_position_info(), ...)` enumerates + // whatever register stream that position carries. + let pcdep = payload.pcdep_for_jitcode_pc(jp).unwrap_or_default(); + if payload.jitcode.can_decode_live_vars(jp, sd.op_live) { + let depth = payload + .depth_for_jitcode_pc_pred(jp) + .map(usize::from) + .unwrap_or_else(|| via_py_pc(py_pc, &EMPTY_TWIN_MISS)); + (depth, pcdep) + } else { + (via_py_pc(py_pc, &EMPTY_TWIN_NON_DECODABLE), pcdep) } } else { - // A non-decodable carried coordinate falls back to the merge-target - // PC so liveness and pcdep key the same point. + // A genuinely absent coordinate can only use a carried Python PC. (via_py_pc(py_pc, &EMPTY_TWIN_NON_DECODABLE), Vec::new()) }; BridgeSemanticMaps { @@ -2145,12 +2149,9 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( /// `recipe.jitcode_pc`, `residual_call.rs` `op_pc`, `resume_snapshot.rs` /// `callee_jitcode_pc`). The two `RebuiltFrame` callers (`state.rs` /// `reconstruct_inline_recipe` and `setup_bridge_sym`) do hold one — the -/// forward-carried `RebuiltFrame::py_pc`, which `py_coord.rs` names as a -/// sanctioned Python-coordinate source — and decline it anyway: they carried -/// `.pc` here before, and supplying `.py_pc` instead would widen -/// `semantic_prefix_len` off a coordinate the old code never read. Declining is -/// what keeps this a coordinate repair; what the declined leg leaves on the -/// table is recorded at `via_py_pc`. +/// forward-carried `RebuiltFrame::py_pc`, but the JitCode-keyed containing +/// depth and pcdep twins now carry the same semantic information without +/// routing the JitCode word through a Python-keyed map. pub(crate) fn bridge_semantic_maps_from_jitcode_pc( jitcode_index: i32, jitcode_pc: i32, @@ -13751,6 +13752,42 @@ mod tests { assert_eq!(sym.bridge_local_oprefs, Some(vec![OpRef::input_arg_ref(7)])); } + #[test] + fn bridge_semantic_maps_keep_sidecars_at_non_live_jitcode_offset() { + use pyre_interpreter::pyframe::PyFrame; + + ensure_test_callbacks(); + + let raw_code = compile_exec("x = 1").expect("test code should compile"); + let frame = PyFrame::new(raw_code); + let code_ref = frame.pycode as *const (); + + // A CALL/residual operation start can be a valid JitCode coordinate + // without being a `-live-`-anchored decode point. The codewriter still + // publishes containing-depth and pcdep sidecars for that coordinate; + // bridge reconstruction must not collapse them to 0/empty merely + // because the runtime JitCode body below has no live marker at pc=9. + let mut metadata = crate::PyJitCodeMetadata::degenerate(); + metadata.depth_containing_by_jit_pc = vec![(0, 4)]; + metadata.pcdep_by_jit_pc = vec![(0, vec![(1, 7, 3)])]; + metadata.has_color_map = true; + metadata.is_drained = true; + let pyjit = std::sync::Arc::new(crate::PyJitCode::from_parts( + std::sync::Arc::new(majit_metainterp::jitcode::JitCode::default()), + metadata, + std::ptr::null(), + false, + )); + let jitcode_index = METAINTERP_SD.with(|r| unsafe { + let ptr = r.borrow_mut().jitcode_for(code_ref, Some(pyjit)); + (*ptr).index + }); + + let maps = bridge_semantic_maps_from_jitcode_pc(jitcode_index, 9); + assert_eq!(maps.stack_depth_at_pc, 4); + assert_eq!(maps.pcdep_entries, vec![(1, 7, 3)]); + } + #[test] fn test_close_loop_args_preserves_ec_between_frame_and_virtualizable_header() { ensure_test_callbacks(); From 842be035c2bd142661c8579b1cdae52c4deb32a2 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 12 Aug 2026 19:47:11 +0900 Subject: [PATCH 2/5] mmap: bound find/rfind by the needle rather than clamping the subtraction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scan's upper bound is `span - len(needle)`, written as a saturating subtraction. When the needle is longer than the span that clamps to 0, which still leaves index 0 to try, and reading a needle-sized window there indexes past the end of the span: `mmap.find(b"ab")` on a one-byte map aborts the interpreter with "range end index 2 out of range for slice of length 1". Reject the case before the scan instead, in both find and rfind, and give the empty needle its own answer: it matches at the near end of the span, `start` for find and `end` for rfind, where the shared `start >= end || is_empty` guard used to fold it into -1 along with the inverted-span case. test_mmap stops aborting and reports its remaining failures normally (51 tests, 3 failures + 21 errors). test_mmap's own sweep already covers the oversized needle — `test_find_end` walks every start/end pair against `bytes.find` as the oracle, which is where the abort came from — but its pattern list has no empty needle, and that module is not in the suite gate. The parity fixture therefore carries the empty-needle half, keeps the oversized cases beside it so the two halves of one bound stay together, and pins values rather than the absence of the abort. It fails on the unpatched binary. Verified on dynasm; parity_tests green. The cranelift binary in this tree predates the change. Assisted-by: Claude --- .../mmap_find_span_shorter_than_needle.py | 77 +++++++++++++++++++ .../src/module/mmap/interp_mmap.rs | 20 ++++- 2 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 pyre/extra_tests/parity_tests/mmap_find_span_shorter_than_needle.py diff --git a/pyre/extra_tests/parity_tests/mmap_find_span_shorter_than_needle.py b/pyre/extra_tests/parity_tests/mmap_find_span_shorter_than_needle.py new file mode 100644 index 00000000000..3378d6e3fd9 --- /dev/null +++ b/pyre/extra_tests/parity_tests/mmap_find_span_shorter_than_needle.py @@ -0,0 +1,77 @@ +# CPython-suite gap: test_mmap's exhaustive find/rfind sweep (test_find_end, +# test_rfind) never uses an empty pattern — its list is +# [b"o", b"on", b"two", b"ones", b"s"] — so the near-end answer an empty needle +# owes is untested there. The oversized-needle cases below are covered by that +# sweep, and are kept only to hold the two halves of one bound together. +# parity-tests reason: test_mmap is not in the suite gate (still 3 failures and +# 21 errors), so nothing in the vendored suite protects either half today. + +"""`mmap.find`/`rfind` over a span that cannot hold the needle report -1. + +The scan's upper bound is `span - len(needle)`. Clamping that subtraction at +zero still leaves one index to try, and reading a needle-sized window there +runs off the end of a shorter span, so the interpreter aborts instead of +answering. + +The empty needle is the boundary in the other direction, and it is the half +with no oracle in the vendored suite: it matches at the near end of the span — +`start` for `find`, `end` for `rfind` — where a shared "empty or inverted" +guard would fold it into -1 along with the spans that really have no room. + +Every case pins the value rather than the absence of a panic, so the fixture +keeps its meaning once the abort is gone. +""" + +import mmap + +m = mmap.mmap(-1, 4) +m[:] = b"abca" + +# The empty needle matches at the near end of the span. +assert m.find(b"") == 0, m.find(b"") +assert m.rfind(b"") == 4, m.rfind(b"") +assert m.find(b"", 2) == 2, m.find(b"", 2) +assert m.rfind(b"", 0, 2) == 2, m.rfind(b"", 0, 2) +assert m.find(b"", 4) == 4, m.find(b"", 4) +assert m.rfind(b"", 4) == 4, m.rfind(b"", 4) +assert m.find(b"", -2) == 2, m.find(b"", -2) +assert m.rfind(b"", -2) == 4, m.rfind(b"", -2) + +# An inverted span holds nothing at all, not even the empty needle. +assert m.find(b"", 3, 1) == -1, m.find(b"", 3, 1) +assert m.rfind(b"", 3, 1) == -1, m.rfind(b"", 3, 1) + +# The needle is longer than the whole map. +assert m.find(b"abcab") == -1, m.find(b"abcab") +assert m.rfind(b"abcab") == -1, m.rfind(b"abcab") + +# The needle fits the map but not the requested span. +assert m.find(b"abc", 2) == -1, m.find(b"abc", 2) +assert m.rfind(b"abc", 2) == -1, m.rfind(b"abc", 2) +assert m.find(b"abc", 0, 2) == -1, m.find(b"abc", 0, 2) +assert m.rfind(b"abc", 0, 2) == -1, m.rfind(b"abc", 0, 2) + +# A span exactly the needle's length still has one candidate. +assert m.find(b"bc", 1, 3) == 1, m.find(b"bc", 1, 3) +assert m.rfind(b"bc", 1, 3) == 1, m.rfind(b"bc", 1, 3) + +# The ordinary answers, so a bound that returns -1 too eagerly is caught too. +assert m.find(b"a") == 0, m.find(b"a") +assert m.rfind(b"a") == 3, m.rfind(b"a") +assert m.find(b"ca") == 2, m.find(b"ca") +assert m.find(b"a", -1) == 3, m.find(b"a", -1) +assert m.find(b"abca", -10) == 0, m.find(b"abca", -10) + +m.close() + +# A one-byte map is the smallest span an oversized needle can overrun. +one = mmap.mmap(-1, 1) +one[:] = b"a" +assert one.find(b"ab") == -1, one.find(b"ab") +assert one.rfind(b"ab") == -1, one.rfind(b"ab") +assert one.find(b"") == 0, one.find(b"") +assert one.rfind(b"") == 1, one.rfind(b"") +assert one.find(b"a") == 0, one.find(b"a") +one.close() + +print("OK") diff --git a/pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs b/pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs index aeeb1244e60..ec65ed6c022 100644 --- a/pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs +++ b/pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs @@ -673,11 +673,17 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { } else { len }; - if start >= end || needle.is_empty() { + if start > end { + return Ok(pyre_object::w_int_new(-1)); + } + if needle.is_empty() { + return Ok(pyre_object::w_int_new(start as i64)); + } + if needle.len() > end - start { return Ok(pyre_object::w_int_new(-1)); } let hay = unsafe { std::slice::from_raw_parts(p.add(start), end - start) }; - let pos = (0..=hay.len().saturating_sub(needle.len())) + let pos = (0..=hay.len() - needle.len()) .find(|&i| &hay[i..i + needle.len()] == needle) .map(|i| (start + i) as i64) .unwrap_or(-1); @@ -727,11 +733,17 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { } else { len }; - if start >= end || needle.is_empty() { + if start > end { + return Ok(pyre_object::w_int_new(-1)); + } + if needle.is_empty() { + return Ok(pyre_object::w_int_new(end as i64)); + } + if needle.len() > end - start { return Ok(pyre_object::w_int_new(-1)); } let hay = unsafe { std::slice::from_raw_parts(p.add(start), end - start) }; - let pos = (0..=hay.len().saturating_sub(needle.len())) + let pos = (0..=hay.len() - needle.len()) .rev() .find(|&i| &hay[i..i + needle.len()] == needle) .map(|i| (start + i) as i64) From 0b885e3f6268e46d083af34f4e7365e6e87eb453 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Thu, 13 Aug 2026 09:50:35 +0900 Subject: [PATCH 3/5] mmap: implement the module on Windows The interpreter code was gated on `unix`, so `import mmap` on Windows gave an empty module. Compile it on Windows too: - A registry entry is now `MappedObj`, either a memmap2 mapping or a Win32 named one, because `mmap(..., tagname=...)` goes through CreateFileMappingW/MapViewOfFile. - Add the Windows constructor `mmap(fileno, length, tagname, access, offset)`: it duplicates the file handle, moves EOF when the view runs past it, and maps the handle, a named mapping, or anonymous memory. - close()/`__exit__` close the duplicated handle, size() reads GetFileSize, and resize() moves EOF and re-maps, rejecting a named mapping. - Errors carry the Win32 code in `.winerror`. - Windows registers only error, ACCESS_*, PAGESIZE and ALLOCATIONGRANULARITY; MAP_*/PROT_*/MADV_* and the madvise method stay POSIX-only. PAGESIZE and ALLOCATIONGRANULARITY now come from host_env rather than being both sysconf(_SC_PAGESIZE). - Bind the constructor arguments through a Signature so they accept keywords, `mmap.mmap(-1, 8, access=ACCESS_READ)` included. Also fix a stepped-slice cursor that wrapped: `m[1::sys.maxsize]` overflowed the i64 cursor negative and read outside the mapping. The cursor saturates and the slice length is derived in i128. --- pyre/pyre-interpreter/src/builtins.rs | 16 +- pyre/pyre-interpreter/src/jit_fnaddr.rs | 6 +- .../src/module/mmap/interp_mmap.rs | 1076 ++++++++++++----- pyre/pyre-interpreter/src/module/mmap/mod.rs | 8 +- pyre/pyre-interpreter/src/typedef.rs | 2 +- 5 files changed, 780 insertions(+), 328 deletions(-) diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index b0ea1cfd493..cec01fc2c95 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -78,7 +78,7 @@ pub(crate) unsafe fn backing_exports_incref(buffer: &pyre_object::buffer::Buffer // releasing the view it came from would let `close`/`resize` // unmap while this one still reads the mapping. let _ = w_obj; - #[cfg(all(unix, not(feature = "sandbox")))] + #[cfg(all(any(unix, windows), not(feature = "sandbox")))] if crate::module::mmap::interp_mmap::is_mmap(*w_obj) { crate::module::mmap::interp_mmap::mmap_exports_incref(*w_obj); } @@ -114,7 +114,7 @@ pub(crate) unsafe fn buffer_export_incref(obj: PyObjectRef) -> bool { pyre_object::memoryview::w_memoryview_exports_incref(obj); return true; } - #[cfg(all(unix, not(feature = "sandbox")))] + #[cfg(all(any(unix, windows), not(feature = "sandbox")))] if crate::module::mmap::interp_mmap::is_mmap(obj) { crate::module::mmap::interp_mmap::mmap_exports_incref(obj); return true; @@ -137,7 +137,7 @@ pub(crate) unsafe fn buffer_export_decref(obj: PyObjectRef) { } else if pyre_object::memoryview::is_w_memoryview(obj) { pyre_object::memoryview::w_memoryview_exports_decref(obj); } else { - #[cfg(all(unix, not(feature = "sandbox")))] + #[cfg(all(any(unix, windows), not(feature = "sandbox")))] crate::module::mmap::interp_mmap::mmap_exports_decref(obj); } } @@ -394,7 +394,7 @@ pub(crate) fn w_memoryview_new_simple_with_owner( /// Build the `W_MMap.readbuf_w`/`writebuf_w` view: one contiguous external /// byte window whose owner remains the mmap object. -#[cfg(all(unix, not(feature = "sandbox")))] +#[cfg(all(any(unix, windows), not(feature = "sandbox")))] unsafe fn w_memoryview_new_mmap( w_obj: PyObjectRef, address: usize, @@ -657,7 +657,7 @@ fn w_memoryview_new_with_flags_impl( // keeps its zero-copy window and derived geometry. return Ok(w_memoryview_new_derived(w_obj, |v| v.clone())); } - #[cfg(all(unix, not(feature = "sandbox")))] + #[cfg(all(any(unix, windows), not(feature = "sandbox")))] if let Some(view) = crate::module::mmap::interp_mmap::mmap_buffer_view(w_obj) { let (address, length, readonly) = view?; return Ok(w_memoryview_new_mmap(w_obj, address, length, readonly)); @@ -1781,7 +1781,7 @@ fn memoryview_repr(args: &[PyObjectRef]) -> Result /// Drop an mmap-backed view's export directly, bypassing any Python-callable /// release. Returns `true` when it handled an mmap backing. -#[cfg(all(unix, not(feature = "sandbox")))] +#[cfg(all(any(unix, windows), not(feature = "sandbox")))] unsafe fn release_external_backing(backing: PyObjectRef) -> bool { if crate::module::mmap::interp_mmap::is_mmap(backing) { unsafe { crate::module::mmap::interp_mmap::mmap_exports_decref(backing) }; @@ -1790,7 +1790,7 @@ unsafe fn release_external_backing(backing: PyObjectRef) -> bool { false } -#[cfg(not(all(unix, not(feature = "sandbox"))))] +#[cfg(not(all(any(unix, windows), not(feature = "sandbox"))))] unsafe fn release_external_backing(_backing: PyObjectRef) -> bool { false } @@ -15547,7 +15547,7 @@ unsafe fn fileio_writebuf( obj, )); } - #[cfg(all(unix, not(feature = "sandbox")))] + #[cfg(all(any(unix, windows), not(feature = "sandbox")))] if let Some(view) = crate::module::mmap::interp_mmap::mmap_buffer_view(obj) { let (address, length, readonly) = view?; if !readonly { diff --git a/pyre/pyre-interpreter/src/jit_fnaddr.rs b/pyre/pyre-interpreter/src/jit_fnaddr.rs index 4051649d08c..e48222f294e 100644 --- a/pyre/pyre-interpreter/src/jit_fnaddr.rs +++ b/pyre/pyre-interpreter/src/jit_fnaddr.rs @@ -1261,7 +1261,11 @@ pub fn jit_trace_fnaddrs() -> Vec<(&'static str, i64)> { // module itself is gated at `module/mod.rs:80`; the row has to carry // both or a sandbox build on Linux satisfies `unix` with the module // configured out. - #[cfg(all(unix, not(target_arch = "wasm32"), not(feature = "sandbox")))] + #[cfg(all( + any(unix, windows), + not(target_arch = "wasm32"), + not(feature = "sandbox") + ))] { let mmap_type: fn() -> pyre_object::PyObjectRef = crate::module::mmap::interp_mmap::mmap_type; diff --git a/pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs b/pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs index ec65ed6c022..2359e24d8e5 100644 --- a/pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs +++ b/pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs @@ -11,10 +11,12 @@ // `mmap.mmap(fileno, length, ...)` maps through `host_env::mmap` // (memmap2-based, cross-platform), not raw libc. Per-instance state // lives in the instance dict: `_ptr` (mapping pointer as i64), `_len` -// (i64), `_pos` (i64 cursor), `_access` (int), `_id` (registry key). -// The mapping is invalidated on close()/`__exit__` by dropping the -// registry entry (→ unmap); leaking it (e.g. GC drops the instance -// before close) is acceptable, matching CPython behaviour. +// (i64), `_pos` (i64 cursor), `_access` (int), `_id` (registry key), +// plus the descriptor the object owns — `_fd` on POSIX, `_handle` on +// Windows, where the constructor duplicates the file's handle +// (`rmmap.py:953-970`). The mapping is invalidated on close()/`__exit__` +// by dropping the registry entry (→ unmap); leaking it (e.g. GC drops the +// instance before close) is acceptable, matching CPython behaviour. // ────────────────────────────────────────────────────────────────────── // host_env's `MappedFile` is an RAII handle (memmap2) that unmaps on Drop, @@ -25,41 +27,91 @@ // stays a raw-pointer access. close()/`__exit__`/resize drop or replace the // entry; a map dropped by GC without close leaks its entry, exactly as the // previous raw-pointer code leaked the mapping. -#[cfg(unix)] +#[cfg(any(unix, windows))] use rustpython_host_env::mmap as host_mmap; -#[cfg(unix)] -static MMAP_REGISTRY: std::sync::Mutex> = +/// The live mapping one registry slot owns. A Windows `mmap(…, tagname=…)` +/// goes through `CreateFileMappingW`/`MapViewOfFile` (`rmmap.py:999-1004`) +/// rather than memmap2, so the two mapping flavours share one entry type. +#[cfg(any(unix, windows))] +enum MappedObj { + Mapped(host_mmap::MappedFile), + #[cfg(windows)] + Named(host_mmap::NamedMmap), +} + +#[cfg(any(unix, windows))] +impl MappedObj { + fn as_ptr(&self) -> *const u8 { + match self { + Self::Mapped(m) => m.as_ptr(), + #[cfg(windows)] + Self::Named(m) => m.as_slice().as_ptr(), + } + } + + fn len(&self) -> usize { + match self { + Self::Mapped(m) => m.as_slice().len(), + #[cfg(windows)] + Self::Named(m) => m.as_slice().len(), + } + } + + fn flush_range(&self, offset: usize, size: usize) -> std::io::Result<()> { + match self { + Self::Mapped(m) => m.flush_range(offset, size), + #[cfg(windows)] + Self::Named(m) => m.flush_range(offset, size), + } + } +} + +#[cfg(any(unix, windows))] +static MMAP_REGISTRY: std::sync::Mutex> = std::sync::Mutex::new(std::collections::BTreeMap::new()); -#[cfg(unix)] +#[cfg(any(unix, windows))] static MMAP_NEXT_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); -#[cfg(unix)] -fn mmap_registry_insert(m: host_mmap::MappedFile) -> (u64, *const u8, usize) { +#[cfg(any(unix, windows))] +fn mmap_registry_insert(m: MappedObj) -> (u64, *const u8, usize) { let ptr = m.as_ptr(); - let len = m.as_slice().len(); + let len = m.len(); let id = MMAP_NEXT_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed); MMAP_REGISTRY.lock().unwrap().insert(id, m); (id, ptr, len) } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn mmap_registry_remove(id: u64) { if id != 0 { MMAP_REGISTRY.lock().unwrap().remove(&id); } } -#[cfg(unix)] -fn mmap_registry_replace(id: u64, m: host_mmap::MappedFile) -> (*const u8, usize) { +#[cfg(any(target_os = "linux", target_os = "android", windows))] +fn mmap_registry_replace(id: u64, m: MappedObj) -> (*const u8, usize) { let ptr = m.as_ptr(); - let len = m.as_slice().len(); - // Inserting over the same key drops the previous MappedFile → unmaps it. + let len = m.len(); + // Inserting over the same key drops the previous mapping → unmaps it. MMAP_REGISTRY.lock().unwrap().insert(id, m); (ptr, len) } -#[cfg(unix)] +/// True when the mapping is a tagged one. `CreateFileMappingW` fixed such an +/// object's size when it was created and reopening it under the same name is +/// what shares it, so there is nothing to resize in place: `resize` rejects +/// them, where a real resize would only get as far as ERROR_USER_MAPPED_FILE +/// once a second mapping holds the name. +#[cfg(windows)] +fn mmap_registry_is_named(id: u64) -> bool { + matches!( + MMAP_REGISTRY.lock().unwrap().get(&id), + Some(MappedObj::Named(_)) + ) +} + +#[cfg(any(unix, windows))] fn mmap_registry_flush(id: u64, offset: usize, size: usize) -> std::io::Result<()> { match MMAP_REGISTRY.lock().unwrap().get(&id) { Some(m) => m.flush_range(offset, size), @@ -75,7 +127,7 @@ fn mmap_registry_madvise( advice: i32, ) -> std::io::Result<()> { match MMAP_REGISTRY.lock().unwrap().get(&id) { - Some(m) => m.madvise_range(start, length, advice), + Some(MappedObj::Mapped(m)) => m.madvise_range(start, length, advice), None => Ok(()), } } @@ -85,14 +137,27 @@ fn mmap_io_err(e: std::io::Error, ctx: &str) -> crate::PyError { crate::PyError::os_error_with_errno(e.raw_os_error().unwrap_or(0), ctx) } -#[cfg(unix)] +/// The host layer reaches the mapping through Win32, so the code an +/// `io::Error` carries here is a Win32 error: it belongs in `.winerror`, with +/// `.errno` and the OSError subclass derived from it (`rmmap.py:1010` +/// `lastSavedWindowsError`). +#[cfg(windows)] +fn mmap_io_err(e: std::io::Error, _ctx: &str) -> crate::PyError { + crate::PyError::os_error_win32_syscall2( + e.raw_os_error().unwrap_or(0), + pyre_object::PY_NULL, + pyre_object::PY_NULL, + ) +} + +#[cfg(any(unix, windows))] static MMAP_TYPE_OBJ: std::sync::OnceLock = std::sync::OnceLock::new(); /// Reads (and lazily installs) the runtime-assigned `mmap` type object, not a /// build-time constant, so the JIT residualizes the call instead of tracing /// into it (`@dont_look_inside`, the `gc_interp::enabled` shape). The /// `-> PyObjectRef` return fits a single word and it cannot raise. -#[cfg(unix)] +#[cfg(any(unix, windows))] #[majit_macros::dont_look_inside] pub(crate) fn mmap_type() -> pyre_object::PyObjectRef { *MMAP_TYPE_OBJ.get_or_init(|| { @@ -105,7 +170,7 @@ pub(crate) fn mmap_type() -> pyre_object::PyObjectRef { }) as pyre_object::PyObjectRef } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn mmap_get_attr_i64(obj: pyre_object::PyObjectRef, key: &str) -> i64 { let d = crate::baseobjspace::getdict_native(obj); if d.is_null() { @@ -118,7 +183,7 @@ fn mmap_get_attr_i64(obj: pyre_object::PyObjectRef, key: &str) -> i64 { 0 } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn mmap_set_attr(obj: pyre_object::PyObjectRef, key: &str, v: pyre_object::PyObjectRef) { let d = crate::baseobjspace::getdict_native(obj); if d.is_null() { @@ -129,7 +194,7 @@ fn mmap_set_attr(obj: pyre_object::PyObjectRef, key: &str, v: pyre_object::PyObj } } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn mmap_ptr(obj: pyre_object::PyObjectRef) -> Result<(*mut u8, usize), crate::PyError> { let p = mmap_get_attr_i64(obj, "_ptr") as usize as *mut u8; let len = mmap_get_attr_i64(obj, "_len") as usize; @@ -139,8 +204,84 @@ fn mmap_ptr(obj: pyre_object::PyObjectRef) -> Result<(*mut u8, usize), crate::Py Ok((p, len)) } -/// True when `obj` is an `mmap` instance. +/// `rmmap.py:387-405 MMap.close` — drop the mapping and the descriptor the +/// object owns. POSIX keeps the caller's own fd (nothing to release); +/// Windows holds a handle it duplicated at construction, and leaving it open +/// would keep the file locked after `close()`. +#[cfg(any(unix, windows))] +fn mmap_close(obj: pyre_object::PyObjectRef) -> Result<(), crate::PyError> { + if mmap_get_attr_i64(obj, "_ptr") == 0 { + return Ok(()); + } + mmap_check_exports(obj, "cannot close exported pointers exist")?; + mmap_registry_remove(mmap_get_attr_i64(obj, "_id") as u64); + #[cfg(windows)] + mmap_close_handle(obj); + mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(0)); + mmap_set_attr(obj, "_len", pyre_object::w_int_new(0)); + mmap_set_attr(obj, "_id", pyre_object::w_int_new(0)); + Ok(()) +} + +/// The file handle the object owns, or `None` for a mapping backed by no file +/// of its own — an anonymous one, and the pagefile-backed tagged mappings. +#[cfg(windows)] +fn mmap_handle(obj: pyre_object::PyObjectRef) -> Option { + let handle = mmap_get_attr_i64(obj, "_handle") as isize; + if handle == 0 || host_mmap::is_invalid_handle_value(handle) { + return None; + } + Some(handle as host_mmap::Handle) +} + +/// Close the duplicated file handle `_handle` names, and mark it invalid so a +/// second close is a no-op. +#[cfg(windows)] +fn mmap_close_handle(obj: pyre_object::PyObjectRef) { + if let Some(handle) = mmap_handle(obj) { + host_mmap::close_handle(handle); + } + mmap_set_attr( + obj, + "_handle", + pyre_object::w_int_new(host_mmap::INVALID_HANDLE as isize as i64), + ); +} + +/// `rmmap.py:509-524 MMap.file_size` — the backing file's current size, which +/// diverges from the mapped length after `resize()`. An anonymous map has no +/// file to stat, and fstat on the `-1` descriptor is the OSError that reports +/// it. #[cfg(unix)] +fn mmap_file_size(obj: pyre_object::PyObjectRef) -> Result { + let fd = mmap_get_attr_i64(obj, "_fd") as libc::c_int; + if fd < 0 { + return Err(crate::PyError::os_error( + "mmap: cannot find file size for anonymous map", + )); + } + let mut st: libc::stat = unsafe { core::mem::zeroed() }; + if unsafe { libc::fstat(fd, &mut st as *mut libc::stat) } != 0 { + return Err(crate::PyError::os_error_with_errno( + std::io::Error::last_os_error().raw_os_error().unwrap_or(0), + "mmap.size: fstat failed", + )); + } + Ok(st.st_size as i64) +} + +/// `rmmap.py:511-520` — `GetFileSize` on the handle the map owns. An +/// anonymous map has no handle and reports the mapped length instead. +#[cfg(windows)] +fn mmap_file_size(obj: pyre_object::PyObjectRef) -> Result { + match mmap_handle(obj) { + Some(handle) => host_mmap::get_file_len(handle).map_err(|e| mmap_io_err(e, "GetFileSize")), + None => Ok(mmap_get_attr_i64(obj, "_len")), + } +} + +/// True when `obj` is an `mmap` instance. +#[cfg(any(unix, windows))] pub(crate) fn is_mmap(obj: pyre_object::PyObjectRef) -> bool { match crate::typedef::r#type(obj) { Some(tp) => std::ptr::eq(tp.as_ptr(), mmap_type()), @@ -152,7 +293,7 @@ pub(crate) fn is_mmap(obj: pyre_object::PyObjectRef) -> bool { /// The mapping is raw foreign memory, so unmapping it under a live view is a /// use-after-free rather than a stale-but-owned read; `close` and `resize` /// refuse while this is non-zero. -#[cfg(unix)] +#[cfg(any(unix, windows))] pub(crate) fn mmap_exports_incref(obj: pyre_object::PyObjectRef) { let n = mmap_get_attr_i64(obj, "_exports"); mmap_set_attr(obj, "_exports", pyre_object::w_int_new(n + 1)); @@ -160,7 +301,7 @@ pub(crate) fn mmap_exports_incref(obj: pyre_object::PyObjectRef) { /// Paired with [`mmap_exports_incref`]; saturates at zero so a double release /// cannot wrap the count and strand the mapping. -#[cfg(unix)] +#[cfg(any(unix, windows))] pub(crate) unsafe fn mmap_exports_decref(obj: pyre_object::PyObjectRef) { if !is_mmap(obj) { return; @@ -169,8 +310,21 @@ pub(crate) unsafe fn mmap_exports_decref(obj: pyre_object::PyObjectRef) { mmap_set_attr(obj, "_exports", pyre_object::w_int_new((n - 1).max(0))); } +/// How many items `start:stop:step` selects. `sys.maxsize` is a legal step, +/// so the count is derived in `i128`, where `stop - start + step` cannot wrap +/// into a negative length. +#[cfg(any(unix, windows))] +fn mmap_slice_len(start: i64, stop: i64, step: i64) -> i64 { + let (span, stride) = if step > 0 { + ((stop as i128 - start as i128).max(0), step as i128) + } else { + ((start as i128 - stop as i128).max(0), -(step as i128)) + }; + ((span + stride - 1) / stride) as i64 +} + /// Reject unmapping while a view still points into the mapping. -#[cfg(unix)] +#[cfg(any(unix, windows))] fn mmap_check_exports(obj: pyre_object::PyObjectRef, message: &str) -> Result<(), crate::PyError> { if mmap_get_attr_i64(obj, "_exports") > 0 { return Err(crate::PyError::new(crate::PyErrorKind::BufferError, message)); @@ -181,7 +335,7 @@ fn mmap_check_exports(obj: pyre_object::PyObjectRef, message: &str) -> Result<() /// `W_MMap.readbuf_w` / `writebuf_w` — expose the live mapping to the /// object-space buffer protocol. `None` means the object is not an mmap; /// the inner error preserves the closed-mapping failure. -#[cfg(unix)] +#[cfg(any(unix, windows))] pub(crate) fn mmap_buffer_view( obj: pyre_object::PyObjectRef, ) -> Option> { @@ -195,7 +349,7 @@ pub(crate) fn mmap_buffer_view( })) } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn mmap_get_attr_obj(obj: pyre_object::PyObjectRef, key: &str) -> pyre_object::PyObjectRef { let d = crate::baseobjspace::getdict_native(obj); if d.is_null() { @@ -208,10 +362,10 @@ fn mmap_get_attr_obj(obj: pyre_object::PyObjectRef, key: &str) -> pyre_object::P // generator yielding the 1-byte slices `m[i:i+1]`. pyre models that // generator as a dedicated iterator object holding the source mmap, a // cursor, and a step (`+1` forwards, `-1` for `reversed`). -#[cfg(unix)] +#[cfg(any(unix, windows))] static MMAP_ITER_TYPE_OBJ: std::sync::OnceLock = std::sync::OnceLock::new(); -#[cfg(unix)] +#[cfg(any(unix, windows))] fn mmap_iterator_type() -> pyre_object::PyObjectRef { *MMAP_ITER_TYPE_OBJ.get_or_init(|| { let tp = crate::typedef::make_builtin_type("mmap_iterator", init_mmap_iterator_type); @@ -220,7 +374,7 @@ fn mmap_iterator_type() -> pyre_object::PyObjectRef { }) as pyre_object::PyObjectRef } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn make_mmap_iterator(m: pyre_object::PyObjectRef, start: i64, step: i64) -> pyre_object::PyObjectRef { let it = pyre_object::w_instance_new(mmap_iterator_type()); mmap_set_attr(it, "_m", m); @@ -229,7 +383,7 @@ fn make_mmap_iterator(m: pyre_object::PyObjectRef, start: i64, step: i64) -> pyr it } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn init_mmap_iterator_type(ns: pyre_object::PyObjectRef) { unsafe { pyre_object::dictmultiobject::w_dict_setitem_str_no_proxy( ns, @@ -263,22 +417,47 @@ fn init_mmap_iterator_type(ns: pyre_object::PyObjectRef) { ) }; } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn init_mmap_type(ns: pyre_object::PyObjectRef) { // `interp_mmap.py:341 __new__ = interp2app(mmap)` — the class call - // `mmap.mmap(fileno, length, ...)` lands here. args[0] is the - // type, the rest are the constructor positionals. + // `mmap.mmap(fileno, length, ...)` lands here. args[0] is the type, the + // rest are the constructor arguments; every one of them binds by keyword + // too, so the signature-aware carrier resolves them into fixed slots + // (`interp_mmap.py:333-335` / `:354-356` — the argument list is the one + // real difference between the two platforms' constructors). + // `Signature::new(argnames, varargname, kwargname, kwonlyargcount, + // posonlyargcount)`: nothing is keyword-only, and only `cls` is + // positional-only. + #[cfg(unix)] + let signature = crate::gateway::Signature::new( + vec!["cls", "fileno", "length", "flags", "prot", "access", "offset"], + None, + None, + 0, + 1, + ); + #[cfg(windows)] + let signature = crate::gateway::Signature::new( + vec!["cls", "fileno", "length", "tagname", "access", "offset"], + None, + None, + 0, + 1, + ); unsafe { pyre_object::dictmultiobject::w_dict_setitem_str_no_proxy( ns, "__new__", - crate::typedef::make_new_descr(|args| { - if args.is_empty() { - return Err(crate::PyError::type_error( - "mmap() requires fileno + length", - )); - } - mmap_construct(&args[1..]) - }), + crate::typedef::make_new_descr_with_signature( + |args| { + if args.is_empty() { + return Err(crate::PyError::type_error( + "mmap() requires fileno + length", + )); + } + mmap_construct(&args[1..]) + }, + signature, + ), ) }; // close() — munmap and zero the pointer. @@ -289,14 +468,7 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { "close", |args| { let obj = args.first().copied().unwrap_or(pyre_object::PY_NULL); - let p = mmap_get_attr_i64(obj, "_ptr") as usize; - if p != 0 { - mmap_check_exports(obj, "cannot close exported pointers exist")?; - mmap_registry_remove(mmap_get_attr_i64(obj, "_id") as u64); - mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(0)); - mmap_set_attr(obj, "_len", pyre_object::w_int_new(0)); - mmap_set_attr(obj, "_id", pyre_object::w_int_new(0)); - } + mmap_close(obj)?; Ok(pyre_object::w_none()) }, 1, @@ -326,10 +498,9 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { ), ) }; - // `interp_mmap.py:98-103 descr_size` returns `mmap.file_size()` — - // the underlying file's current size via fstat, not the mapped - // length. The two diverge after `resize()`, and an anonymous mmap - // (no fd) raises ValueError per rmmap.py:MMap.file_size. + // `interp_mmap.py:98-103 descr_size` returns `mmap.file_size()` — the + // underlying file's current size, not the mapped length. The two diverge + // after `resize()`. unsafe { pyre_object::dictmultiobject::w_dict_setitem_str_no_proxy( ns, "size", @@ -340,21 +511,7 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { if mmap_get_attr_i64(obj, "_ptr") == 0 { return Err(crate::PyError::value_error("mmap closed or invalid")); } - let fd = mmap_get_attr_i64(obj, "_fd") as libc::c_int; - if fd < 0 { - return Err(crate::PyError::os_error( - "mmap: cannot find file size for anonymous map", - )); - } - let mut st: libc::stat = unsafe { core::mem::zeroed() }; - let r = unsafe { libc::fstat(fd, &mut st as *mut libc::stat) }; - if r != 0 { - return Err(crate::PyError::os_error_with_errno( - std::io::Error::last_os_error().raw_os_error().unwrap_or(0), - "mmap.size: fstat failed", - )); - } - Ok(pyre_object::w_int_new(st.st_size as i64)) + Ok(pyre_object::w_int_new(mmap_file_size(obj)?)) }, 1, ), @@ -766,14 +923,7 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { "__exit__", crate::make_builtin_function("__exit__", |args| { if let Some(&obj) = args.first() { - let p = mmap_get_attr_i64(obj, "_ptr") as usize; - if p != 0 { - mmap_check_exports(obj, "cannot close exported pointers exist")?; - mmap_registry_remove(mmap_get_attr_i64(obj, "_id") as u64); - mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(0)); - mmap_set_attr(obj, "_len", pyre_object::w_int_new(0)); - mmap_set_attr(obj, "_id", pyre_object::w_int_new(0)); - } + mmap_close(obj)?; } Ok(pyre_object::w_bool_from(false)) }), @@ -823,7 +973,10 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { let mut i = start; while (step > 0 && i < stop) || (step < 0 && i > stop) { out.push(unsafe { *p.add(i as usize) }); - i += step; + // `sys.maxsize` is a legal step, and a cursor that + // wrapped past it would index the mapping from a + // negative offset. + i = i.saturating_add(step); } return Ok(pyre_object::bytesobject::w_bytes_from_bytes(&out)); } @@ -872,11 +1025,7 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { if unsafe { pyre_object::is_slice(index) } { let (start, stop, step) = unsafe { crate::baseobjspace::normalize_slice(index, len_i64)? }; - let length = if step > 0 { - ((stop - start).max(0) + step - 1) / step - } else { - ((start - stop).max(0) + (-step) - 1) / (-step) - }; + let length = mmap_slice_len(start, stop, step); if !unsafe { pyre_object::bytesobject::is_bytes_like(value) } { return Err(crate::PyError::type_error( "mmap slice assignment must be bytes-like", @@ -903,7 +1052,7 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { let mut k = 0usize; while (step > 0 && i < stop) || (step < 0 && i > stop) { unsafe { *p.add(i as usize) = buf[k] }; - i += step; + i = i.saturating_add(step); k += 1; } } @@ -970,8 +1119,13 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { ), ) }; + // `interp_mmap.py:372-374` — `madvise` is in `optional`, installed only + // where `rmmap.has_madvise`. Windows has no madvise(2), so the method is + // absent from the type there. + // // `interp_mmap.py:descr_madvise` — call madvise(addr+start, length, // advice). Defaults: start=0, length=remaining bytes. + #[cfg(all(unix, not(target_os = "redox")))] unsafe { pyre_object::dictmultiobject::w_dict_setitem_str_no_proxy( ns, "madvise", @@ -1004,20 +1158,8 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { )); } let _ = p; - #[cfg(all(unix, not(target_os = "redox")))] - { - mmap_registry_madvise( - mmap_get_attr_i64(obj, "_id") as u64, - start, - length, - option, - ) + mmap_registry_madvise(mmap_get_attr_i64(obj, "_id") as u64, start, length, option) .map_err(|e| mmap_io_err(e, "madvise"))?; - } - #[cfg(not(all(unix, not(target_os = "redox"))))] - { - let _ = (length, option); - } Ok(pyre_object::w_none()) }), ) }; @@ -1063,28 +1205,19 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { "source or destination out of range", )); } - #[cfg(unix)] + // `rmmap.py:587` uses `memmove`, so the ranges may overlap. unsafe { - libc::memmove( - (p + dest) as *mut libc::c_void, - (p + src) as *const libc::c_void, - count, - ); + std::ptr::copy((p + src) as *const u8, (p + dest) as *mut u8, count); } - #[cfg(not(unix))] - let _ = (p, dest, src, count); Ok(pyre_object::w_none()) }, 4, ), ) }; - // `interp_mmap.py:146 resize` → `rmmap.py:589-601`. POSIX path: - // ftruncate the backing fd (if any) to `offset + newsize`, then - // mremap(MREMAP_MAYMOVE). Platforms without mremap (e.g. macOS) - // raise SystemError to match PyPy's RValueError→SystemError - // translation at `interp_mmap.py:155-157`. Read-only / copy - // mappings reject with TypeError. + // `interp_mmap.py:146 resize` → `rmmap.py:589-651`. Read-only / copy + // mappings reject with TypeError; the remap itself is per-platform + // ([`mmap_resize_mapping`]). unsafe { pyre_object::dictmultiobject::w_dict_setitem_str_no_proxy( ns, "resize", @@ -1107,60 +1240,8 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { if newsize < 0 { return Err(crate::PyError::value_error("new_size must be positive")); } - let newsize = newsize as usize; - let fd = mmap_get_attr_i64(obj, "_fd") as libc::c_int; - let offset = mmap_get_attr_i64(obj, "_offset"); - - // host_env's `MappedFile` (memmap2) cannot mremap in place, so - // the Linux/Android resize re-creates the mapping at the new - // size and swaps the registry entry. A file-backed map is - // re-mapped from the (ftruncated) fd; an anonymous map is - // remade and the surviving bytes copied. The new mapping may - // land at a different address than an mremap would have, but - // that address is never exposed to Python, so the observable - // result is unchanged. Platforms without mremap keep raising - // SystemError, matching PyPy's RValueError→SystemError. - #[cfg(any(target_os = "linux", target_os = "android"))] - { - let id = mmap_get_attr_i64(obj, "_id") as u64; - let mapped = if fd >= 0 { - let r = unsafe { - libc::ftruncate(fd, (offset as libc::off_t) + newsize as libc::off_t) - }; - if r != 0 { - return Err(crate::PyError::os_error_with_errno( - std::io::Error::last_os_error().raw_os_error().unwrap_or(0), - "ftruncate", - )); - } - let borrowed = - unsafe { rustpython_host_env::crt_fd::Borrowed::borrow_raw(fd) }; - let (dup_fd, mapped) = - host_mmap::map_file(borrowed, offset, newsize, host_mmap::AccessMode::Write) - .map_err(|e| mmap_io_err(e, "mmap"))?; - drop(dup_fd); - mapped - } else { - let keep = old_len.min(newsize); - let old = unsafe { std::slice::from_raw_parts(p, keep) }.to_vec(); - let mut mapped = - host_mmap::map_anon(newsize).map_err(|e| mmap_io_err(e, "mmap"))?; - mapped.as_mut_slice()[..keep].copy_from_slice(&old); - mapped - }; - let (newptr, newlen) = mmap_registry_replace(id, mapped); - mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(newptr as usize as i64)); - mmap_set_attr(obj, "_len", pyre_object::w_int_new(newlen as i64)); - Ok(pyre_object::w_none()) - } - #[cfg(not(any(target_os = "linux", target_os = "android")))] - { - let _ = (p, old_len, fd, offset, newsize); - Err(crate::PyError::new( - crate::error::PyErrorKind::SystemError, - "mmap: resizing not available--no mremap()", - )) - } + mmap_resize_mapping(obj, p, old_len, newsize as usize)?; + Ok(pyre_object::w_none()) }, 2, ), @@ -1203,64 +1284,252 @@ fn init_mmap_type(ns: pyre_object::PyObjectRef) { ) }; } -#[cfg(unix)] +#[cfg(any(unix, windows))] const MMAP_ACCESS_DEFAULT: i64 = 0; -#[cfg(unix)] +#[cfg(any(unix, windows))] const MMAP_ACCESS_READ: i64 = 1; -#[cfg(unix)] +#[cfg(any(unix, windows))] const MMAP_ACCESS_WRITE: i64 = 2; -#[cfg(unix)] +#[cfg(any(unix, windows))] const MMAP_ACCESS_COPY: i64 = 3; -// `interp_mmap.py:55-130 mmap_new` — `args` carries the positional -// constructor arguments (fileno, length, flags, prot, access, offset) -// starting at index 0; the `__new__` typecall wrapper drops the class -// from args[0] before invoking this helper. -#[cfg(unix)] -fn mmap_construct( - args: &[pyre_object::PyObjectRef], -) -> Result { - if args.len() < 2 { - return Err(crate::PyError::type_error( - "mmap() requires fileno + length", +/// `rmmap.py:589-601` — ftruncate the backing fd (if any) to `offset + +/// newsize`, then remap. host_env's `MappedFile` (memmap2) cannot mremap in +/// place, so the mapping is re-created at the new size and the registry entry +/// swapped: a file-backed map is re-mapped from the (ftruncated) fd, an +/// anonymous map is remade and the surviving bytes copied. The new mapping +/// may land at a different address than an mremap would have, but that +/// address is never exposed to Python, so the observable result is unchanged. +#[cfg(any(target_os = "linux", target_os = "android"))] +fn mmap_resize_mapping( + obj: pyre_object::PyObjectRef, + p: *mut u8, + old_len: usize, + newsize: usize, +) -> Result<(), crate::PyError> { + let fd = mmap_get_attr_i64(obj, "_fd") as libc::c_int; + let offset = mmap_get_attr_i64(obj, "_offset"); + let mapped = if fd >= 0 { + let r = unsafe { libc::ftruncate(fd, (offset as libc::off_t) + newsize as libc::off_t) }; + if r != 0 { + return Err(crate::PyError::os_error_with_errno( + std::io::Error::last_os_error().raw_os_error().unwrap_or(0), + "ftruncate", + )); + } + let borrowed = unsafe { rustpython_host_env::crt_fd::Borrowed::borrow_raw(fd) }; + let (dup_fd, mapped) = + host_mmap::map_file(borrowed, offset, newsize, host_mmap::AccessMode::Write) + .map_err(|e| mmap_io_err(e, "mmap"))?; + drop(dup_fd); + mapped + } else { + mmap_remake_anon(p, old_len, newsize)? + }; + let (newptr, newlen) = mmap_registry_replace( + mmap_get_attr_i64(obj, "_id") as u64, + MappedObj::Mapped(mapped), + ); + mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(newptr as usize as i64)); + mmap_set_attr(obj, "_len", pyre_object::w_int_new(newlen as i64)); + Ok(()) +} + +/// `rmmap.py:602-651` — unmap the view and close the mapping object, move the +/// file's EOF to `offset + newsize`, then map the file again. Dropping the +/// old mapping first is required, not just tidy: `SetEndOfFile` fails with +/// ERROR_USER_MAPPED_FILE while any view of the file is open. An anonymous +/// mapping has no file to extend, so it is remade and the surviving bytes +/// copied, as on Linux. +#[cfg(windows)] +fn mmap_resize_mapping( + obj: pyre_object::PyObjectRef, + p: *mut u8, + old_len: usize, + newsize: usize, +) -> Result<(), crate::PyError> { + let id = mmap_get_attr_i64(obj, "_id") as u64; + if mmap_registry_is_named(id) { + return Err(crate::PyError::os_error( + "mmap: cannot resize a named memory mapping", )); } - for (idx, label) in [ - (0usize, "fileno"), - (1, "length"), - (2, "flags"), - (3, "prot"), - (4, "access"), - (5, "offset"), - ] { - if args.len() > idx && !unsafe { pyre_object::is_int(args[idx]) } { + let mapped = if let Some(handle) = mmap_handle(obj) { + let offset = mmap_get_attr_i64(obj, "_offset"); + mmap_registry_remove(id); + let mapped = host_mmap::extend_file(handle, offset + newsize as i64) + .and_then(|()| mmap_remap_handle(obj, handle, newsize)); + match mapped { + Ok(mapped) => mapped, + Err(e) => { + // The view is already gone, so `_ptr` would dangle; put the + // mapping back at its old size, and mark the object closed if + // even that fails. + match mmap_remap_handle(obj, handle, old_len) { + Ok(old) => { + mmap_store_mapping(obj, id, old); + } + Err(_) => { + mmap_close_handle(obj); + mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(0)); + mmap_set_attr(obj, "_len", pyre_object::w_int_new(0)); + mmap_set_attr(obj, "_id", pyre_object::w_int_new(0)); + } + } + return Err(mmap_io_err(e, "mmap")); + } + } + } else { + MappedObj::Mapped(mmap_remake_anon(p, old_len, newsize)?) + }; + mmap_store_mapping(obj, id, mapped); + Ok(()) +} + +/// `rmmap.py:640-645` — map the file the object's handle names, at the access +/// mode it was built with. +#[cfg(windows)] +fn mmap_remap_handle( + obj: pyre_object::PyObjectRef, + handle: host_mmap::Handle, + size: usize, +) -> std::io::Result { + let offset = mmap_get_attr_i64(obj, "_offset"); + let access = mmap_access_mode(mmap_get_attr_i64(obj, "_access")); + host_mmap::map_handle(handle, offset, size, access).map(MappedObj::Mapped) +} + +/// Publish a freshly created mapping as the object's live one. +#[cfg(windows)] +fn mmap_store_mapping(obj: pyre_object::PyObjectRef, id: u64, mapped: MappedObj) { + let (newptr, newlen) = mmap_registry_replace(id, mapped); + mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(newptr as usize as i64)); + mmap_set_attr(obj, "_len", pyre_object::w_int_new(newlen as i64)); +} + +/// A resized anonymous mapping is a new mapping holding the bytes that fit. +#[cfg(any(target_os = "linux", target_os = "android", windows))] +fn mmap_remake_anon( + p: *mut u8, + old_len: usize, + newsize: usize, +) -> Result { + let keep = old_len.min(newsize); + let old = unsafe { std::slice::from_raw_parts(p, keep) }.to_vec(); + let mut mapped = host_mmap::map_anon(newsize).map_err(|e| mmap_io_err(e, "mmap"))?; + mapped.as_mut_slice()[..keep].copy_from_slice(&old); + Ok(mapped) +} + +/// Platforms without mremap raise SystemError, matching PyPy's +/// RValueError→SystemError translation at `interp_mmap.py:155-157`. +#[cfg(not(any(target_os = "linux", target_os = "android", windows)))] +fn mmap_resize_mapping( + _obj: pyre_object::PyObjectRef, + _p: *mut u8, + _old_len: usize, + _newsize: usize, +) -> Result<(), crate::PyError> { + Err(crate::PyError::new( + crate::error::PyErrorKind::SystemError, + "mmap: resizing not available--no mremap()", + )) +} + +/// The mapping mode an `ACCESS_*` argument asks for. host_env expresses a +/// mapping as an `AccessMode` rather than the raw `flProtect` / +/// `dwDesiredAccess` pair `rmmap.py:904-914` derives. +#[cfg(windows)] +fn mmap_access_mode(access: i64) -> host_mmap::AccessMode { + match access { + x if x == MMAP_ACCESS_READ => host_mmap::AccessMode::Read, + x if x == MMAP_ACCESS_WRITE => host_mmap::AccessMode::Write, + x if x == MMAP_ACCESS_COPY => host_mmap::AccessMode::Copy, + _ => host_mmap::AccessMode::Default, + } +} + +/// A constructor argument that was actually supplied. The signature-aware +/// gateway pads the array out to the whole parameter list and leaves +/// `PY_NULL` in the slots the call omitted. +#[cfg(any(unix, windows))] +fn mmap_arg(args: &[pyre_object::PyObjectRef], idx: usize) -> Option { + args.get(idx).copied().filter(|a| !a.is_null()) +} + +/// `@unwrap_spec(fileno=int, length=int, …)` — every numeric constructor +/// argument is an int or the call is rejected before anything is mapped. +#[cfg(any(unix, windows))] +fn mmap_check_int_args( + args: &[pyre_object::PyObjectRef], + slots: &[(usize, &str)], +) -> Result<(), crate::PyError> { + for &(idx, label) in slots { + if let Some(a) = mmap_arg(args, idx) + && !unsafe { pyre_object::is_int(a) } + { return Err(crate::PyError::type_error(format!( "mmap() {label} must be an integer" ))); } } - let fd = (unsafe { pyre_object::w_int_get_value(args[0]) }) as libc::c_int; - let length = (unsafe { pyre_object::w_int_get_value(args[1]) }) as libc::size_t; - let flags_arg = if args.len() >= 3 { - (unsafe { pyre_object::w_int_get_value(args[2]) }) as libc::c_int - } else { - host_mmap::MAP_SHARED - }; - let prot_arg = if args.len() >= 4 { - (unsafe { pyre_object::w_int_get_value(args[3]) }) as libc::c_int - } else { - host_mmap::PROT_READ | host_mmap::PROT_WRITE - }; - let access = if args.len() >= 5 { - unsafe { pyre_object::w_int_get_value(args[4]) } - } else { - MMAP_ACCESS_DEFAULT - }; - let offset = if args.len() >= 6 { - (unsafe { pyre_object::w_int_get_value(args[5]) }) as libc::off_t - } else { - 0 + Ok(()) +} + +/// `interp_mmap.py:341-345 W_MMap.__init__` — park the mapping and record the +/// per-instance state every method reads back out of the instance dict. The +/// caller adds the descriptor it owns (`_fd` on POSIX, `_handle` on Windows). +#[cfg(any(unix, windows))] +fn mmap_new_object(mapped: MappedObj, access: i64, offset: i64) -> pyre_object::PyObjectRef { + let (id, ptr, len) = mmap_registry_insert(mapped); + let obj = pyre_object::w_instance_new(mmap_type()); + mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(ptr as usize as i64)); + mmap_set_attr(obj, "_len", pyre_object::w_int_new(len as i64)); + mmap_set_attr(obj, "_id", pyre_object::w_int_new(id as i64)); + mmap_set_attr(obj, "_pos", pyre_object::w_int_new(0)); + mmap_set_attr(obj, "_access", pyre_object::w_int_new(access)); + mmap_set_attr(obj, "_offset", pyre_object::w_int_new(offset)); + obj +} + +// `interp_mmap.py:333-350 mmap(fileno, length, flags, prot, access, offset)` +// — `args` carries the constructor arguments starting at index 0; the +// `__new__` typecall wrapper drops the class from args[0] before invoking +// this helper. +#[cfg(unix)] +fn mmap_construct( + args: &[pyre_object::PyObjectRef], +) -> Result { + let (Some(w_fileno), Some(w_length)) = (mmap_arg(args, 0), mmap_arg(args, 1)) else { + return Err(crate::PyError::type_error( + "mmap() requires fileno + length", + )); }; + mmap_check_int_args( + args, + &[ + (0, "fileno"), + (1, "length"), + (2, "flags"), + (3, "prot"), + (4, "access"), + (5, "offset"), + ], + )?; + let fd = (unsafe { pyre_object::w_int_get_value(w_fileno) }) as libc::c_int; + let length = (unsafe { pyre_object::w_int_get_value(w_length) }) as libc::size_t; + let flags_arg = mmap_arg(args, 2).map_or(host_mmap::MAP_SHARED, |a| { + (unsafe { pyre_object::w_int_get_value(a) }) as libc::c_int + }); + let prot_arg = mmap_arg(args, 3).map_or(host_mmap::PROT_READ | host_mmap::PROT_WRITE, |a| { + (unsafe { pyre_object::w_int_get_value(a) }) as libc::c_int + }); + let access = mmap_arg(args, 4).map_or(MMAP_ACCESS_DEFAULT, |a| unsafe { + pyre_object::w_int_get_value(a) + }); + let offset = mmap_arg(args, 5).map_or(0, |a| { + (unsafe { pyre_object::w_int_get_value(a) }) as libc::off_t + }); let (flags, prot) = match access { x if x == MMAP_ACCESS_READ => (host_mmap::MAP_SHARED, host_mmap::PROT_READ), x if x == MMAP_ACCESS_WRITE => { @@ -1301,109 +1570,302 @@ fn mmap_construct( drop(dup_fd); mapped }; - let (id, ptr, len) = mmap_registry_insert(mapped); - let obj = pyre_object::w_instance_new(mmap_type()); - mmap_set_attr(obj, "_ptr", pyre_object::w_int_new(ptr as usize as i64)); - mmap_set_attr(obj, "_len", pyre_object::w_int_new(len as i64)); - mmap_set_attr(obj, "_id", pyre_object::w_int_new(id as i64)); - mmap_set_attr(obj, "_pos", pyre_object::w_int_new(0)); - mmap_set_attr(obj, "_access", pyre_object::w_int_new(access)); + let obj = mmap_new_object(MappedObj::Mapped(mapped), access, offset as i64); mmap_set_attr(obj, "_fd", pyre_object::w_int_new(real_fd as i64)); - mmap_set_attr(obj, "_offset", pyre_object::w_int_new(offset as i64)); Ok(obj) } -pub fn register_module(ns: pyre_object::PyObjectRef) { - #[cfg(unix)] - { - // `interp_mmap.py:42 error = OSError` alias. - let w_os_error = crate::builtins::lookup_exc_class("OSError") - .expect("OSError must be installed before init_mmap"); - crate::module_ns_store(ns, "error", w_os_error); +// `interp_mmap.py:354-370 mmap(fileno, length, tagname, access, offset)` — +// the Windows constructor names a mapping object where POSIX passes +// flags/prot, and the mapping's protection comes from `access` alone +// (`rmmap.py:900-914`). +#[cfg(windows)] +fn mmap_construct( + args: &[pyre_object::PyObjectRef], +) -> Result { + let (Some(w_fileno), Some(w_length)) = (mmap_arg(args, 0), mmap_arg(args, 1)) else { + return Err(crate::PyError::type_error( + "mmap() requires fileno + length", + )); + }; + mmap_check_int_args(args, &[(0, "fileno"), (1, "length"), (3, "access"), (4, "offset")])?; + let fileno = (unsafe { pyre_object::w_int_get_value(w_fileno) }) as i32; + let length = unsafe { pyre_object::w_int_get_value(w_length) }; + // `rmmap.py:681-683 _check_map_size`. + if length < 0 { + return Err(crate::PyError::type_error( + "memory mapped size must be positive", + )); + } + let tagname = match mmap_arg(args, 2) { + Some(a) if !unsafe { pyre_object::is_none(a) } => { + if !unsafe { pyre_object::is_str(a) } { + return Err(crate::PyError::type_error(format!( + "expected str or None for 'tagname', not {}", + crate::type_methods::arg_type_name(a) + ))); + } + // The name reaches Win32 as UTF-16, which host_env builds from a + // `&str`, so a tag carrying a lone surrogate has no route through. + let Some(tag) = (unsafe { pyre_object::w_str_get_value_opt(a) }) else { + return Err(crate::PyError::value_error( + "mmap() tagname must not contain lone surrogates", + )); + }; + if tag.contains('\0') { + return Err(crate::PyError::value_error("embedded null character")); + } + tag + } + _ => "", + }; + let access = mmap_arg(args, 3).map_or(MMAP_ACCESS_DEFAULT, |a| unsafe { + pyre_object::w_int_get_value(a) + }); + if !(MMAP_ACCESS_DEFAULT..=MMAP_ACCESS_COPY).contains(&access) { + return Err(crate::PyError::value_error("mmap invalid access parameter.")); + } + let offset = mmap_arg(args, 4).map_or(0, |a| unsafe { pyre_object::w_int_get_value(a) }); + // `rmmap.py:897-898`. + if offset < 0 { + return Err(crate::PyError::value_error("negative offset")); + } + let mut map_size = length as usize; - // Constants. CPython exposes both POSIX MAP_/PROT_/MADV_ and the - // Python ACCESS_* aliases. The portable subset sources from - // host_env's re-exports; the platform-specific extras host_env does - // not re-export (MAP_FIXED, the Linux-only MAP_* flags, PROT_NONE) - // stay on libc. - crate::module_ns_store( - ns, - "MAP_SHARED", - pyre_object::w_int_new(host_mmap::MAP_SHARED as i64), - ); - crate::module_ns_store( - ns, - "MAP_PRIVATE", - pyre_object::w_int_new(host_mmap::MAP_PRIVATE as i64), - ); - crate::module_ns_store( - ns, - "MAP_ANON", - pyre_object::w_int_new(host_mmap::MAP_ANON as i64), + // `rmmap.py:916-918` — "assume -1 and 0 both mean invalid file descriptor + // to 'anonymously' map memory". The handle is duplicated so Python code + // may close the file it came from while the mapping lives on; the guard + // hands it back to the OS on every path that does not reach the object. + let mut file_handle = None; + if fileno != -1 && fileno != 0 { + let fh = rustpython_host_env::nt::handle_from_fd(fileno); + if host_mmap::is_invalid_handle_value(fh as isize) { + return Err(crate::PyError::os_error_with_errno( + libc::EBADF, + "mmap: bad file descriptor", + )); + } + let guard = MmapHandleGuard( + host_mmap::duplicate_handle(fh).map_err(|e| mmap_io_err(e, "DuplicateHandle"))?, ); + // `rmmap.py:925-928` trusts map_size when the size cannot be read + // (a non-seeking file); only a readable size constrains it. + if let Ok(file_len) = host_mmap::get_file_len(fh) { + if map_size == 0 { + if file_len == 0 { + return Err(crate::PyError::value_error("cannot mmap an empty file")); + } + if offset >= file_len { + return Err(crate::PyError::value_error( + "mmap offset is greater than file size", + )); + } + map_size = (file_len - offset) as usize; + } else { + // A view longer than the file grows the file, which is what + // `CreateFileMapping` itself does for a size beyond EOF. + let required = offset + .checked_add(map_size as i64) + .ok_or_else(|| crate::PyError::value_error("mmap length is too large"))?; + if required > file_len { + host_mmap::extend_file(guard.0, required) + .map_err(|e| mmap_io_err(e, "SetEndOfFile"))?; + } + } + } + file_handle = Some(guard); + } + + let handle = file_handle + .as_ref() + .map_or(host_mmap::INVALID_HANDLE, |guard| guard.0); + // A tag names a mapping object other processes can open by the same name, + // which memmap2 cannot express, so those go straight through + // `CreateFileMappingW`/`MapViewOfFile` (`rmmap.py:999-1004`). + let (mapped, owned_handle) = if !tagname.is_empty() { + let named = host_mmap::create_named_mapping( + handle, + tagname, + mmap_access_mode(access), + offset, + map_size, + ) + .map_err(|e| mmap_io_err(e, "CreateFileMapping"))?; + // The mapping object holds its own reference to the file, but the + // handle stays with the mmap so `size()` can still stat that file. + ( + MappedObj::Named(named), + file_handle.map_or(host_mmap::INVALID_HANDLE, |guard| guard.release()), + ) + } else if let Some(guard) = file_handle { + let handle = guard.release(); + let mapped = host_mmap::map_handle(handle, offset, map_size, mmap_access_mode(access)) + .map_err(|e| { + host_mmap::close_handle(handle); + mmap_io_err(e, "mmap") + })?; + (MappedObj::Mapped(mapped), handle) + } else { + let mapped = host_mmap::map_anon(map_size).map_err(|e| mmap_io_err(e, "mmap"))?; + (MappedObj::Mapped(mapped), host_mmap::INVALID_HANDLE) + }; + let obj = mmap_new_object(mapped, access, offset); + mmap_set_attr( + obj, + "_handle", + pyre_object::w_int_new(owned_handle as isize as i64), + ); + Ok(obj) +} + +/// Closes the duplicated file handle unless it is released into the finished +/// mmap object, so a constructor that fails after `DuplicateHandle` does not +/// keep the file open. +#[cfg(windows)] +struct MmapHandleGuard(host_mmap::Handle); + +#[cfg(windows)] +impl MmapHandleGuard { + fn release(mut self) -> host_mmap::Handle { + let handle = self.0; + self.0 = host_mmap::INVALID_HANDLE; + handle + } +} + +#[cfg(windows)] +impl Drop for MmapHandleGuard { + fn drop(&mut self) { + if !host_mmap::is_invalid_handle_value(self.0 as isize) { + host_mmap::close_handle(self.0); + } + } +} + +/// `rmmap.py:57-113` — the mapping flags and advice values a POSIX `mmap(2)` +/// takes. The portable subset sources from host_env's re-exports; the +/// platform-specific extras it does not re-export (MAP_FIXED, the Linux-only +/// MAP_* flags, PROT_NONE) stay on libc. Windows has none of them: the +/// mapping's protection comes from `access` alone there, and its module +/// carries only the ACCESS_* and page constants. +#[cfg(unix)] +fn register_posix_constants(ns: pyre_object::PyObjectRef) { + crate::module_ns_store( + ns, + "MAP_SHARED", + pyre_object::w_int_new(host_mmap::MAP_SHARED as i64), + ); + crate::module_ns_store( + ns, + "MAP_PRIVATE", + pyre_object::w_int_new(host_mmap::MAP_PRIVATE as i64), + ); + crate::module_ns_store( + ns, + "MAP_ANON", + pyre_object::w_int_new(host_mmap::MAP_ANON as i64), + ); + crate::module_ns_store( + ns, + "MAP_ANONYMOUS", + pyre_object::w_int_new(host_mmap::MAP_ANONYMOUS as i64), + ); + crate::module_ns_store( + ns, + "MAP_FIXED", + pyre_object::w_int_new(libc::MAP_FIXED as i64), + ); + #[cfg(any(target_os = "linux", target_os = "android"))] + { crate::module_ns_store( ns, - "MAP_ANONYMOUS", - pyre_object::w_int_new(host_mmap::MAP_ANONYMOUS as i64), + "MAP_POPULATE", + pyre_object::w_int_new(libc::MAP_POPULATE as i64), ); crate::module_ns_store( ns, - "MAP_FIXED", - pyre_object::w_int_new(libc::MAP_FIXED as i64), + "MAP_STACK", + pyre_object::w_int_new(libc::MAP_STACK as i64), ); - #[cfg(any(target_os = "linux", target_os = "android"))] - { - crate::module_ns_store( - ns, - "MAP_POPULATE", - pyre_object::w_int_new(libc::MAP_POPULATE as i64), - ); - crate::module_ns_store( - ns, - "MAP_STACK", - pyre_object::w_int_new(libc::MAP_STACK as i64), - ); - crate::module_ns_store( - ns, - "MAP_HUGETLB", - pyre_object::w_int_new(libc::MAP_HUGETLB as i64), - ); - crate::module_ns_store( - ns, - "MAP_NORESERVE", - pyre_object::w_int_new(libc::MAP_NORESERVE as i64), - ); - crate::module_ns_store( - ns, - "MAP_LOCKED", - pyre_object::w_int_new(libc::MAP_LOCKED as i64), - ); - crate::module_ns_store( - ns, - "MAP_NONBLOCK", - pyre_object::w_int_new(libc::MAP_NONBLOCK as i64), - ); - } crate::module_ns_store( ns, - "PROT_READ", - pyre_object::w_int_new(host_mmap::PROT_READ as i64), + "MAP_HUGETLB", + pyre_object::w_int_new(libc::MAP_HUGETLB as i64), ); crate::module_ns_store( ns, - "PROT_WRITE", - pyre_object::w_int_new(host_mmap::PROT_WRITE as i64), + "MAP_NORESERVE", + pyre_object::w_int_new(libc::MAP_NORESERVE as i64), ); crate::module_ns_store( ns, - "PROT_EXEC", - pyre_object::w_int_new(host_mmap::PROT_EXEC as i64), + "MAP_LOCKED", + pyre_object::w_int_new(libc::MAP_LOCKED as i64), ); crate::module_ns_store( ns, - "PROT_NONE", - pyre_object::w_int_new(libc::PROT_NONE as i64), + "MAP_NONBLOCK", + pyre_object::w_int_new(libc::MAP_NONBLOCK as i64), ); + } + crate::module_ns_store( + ns, + "PROT_READ", + pyre_object::w_int_new(host_mmap::PROT_READ as i64), + ); + crate::module_ns_store( + ns, + "PROT_WRITE", + pyre_object::w_int_new(host_mmap::PROT_WRITE as i64), + ); + crate::module_ns_store( + ns, + "PROT_EXEC", + pyre_object::w_int_new(host_mmap::PROT_EXEC as i64), + ); + crate::module_ns_store( + ns, + "PROT_NONE", + pyre_object::w_int_new(libc::PROT_NONE as i64), + ); + crate::module_ns_store( + ns, + "MADV_NORMAL", + pyre_object::w_int_new(host_mmap::MADV_NORMAL as i64), + ); + crate::module_ns_store( + ns, + "MADV_RANDOM", + pyre_object::w_int_new(host_mmap::MADV_RANDOM as i64), + ); + crate::module_ns_store( + ns, + "MADV_SEQUENTIAL", + pyre_object::w_int_new(host_mmap::MADV_SEQUENTIAL as i64), + ); + crate::module_ns_store( + ns, + "MADV_WILLNEED", + pyre_object::w_int_new(host_mmap::MADV_WILLNEED as i64), + ); + crate::module_ns_store( + ns, + "MADV_DONTNEED", + pyre_object::w_int_new(host_mmap::MADV_DONTNEED as i64), + ); +} + +pub fn register_module(ns: pyre_object::PyObjectRef) { + #[cfg(any(unix, windows))] + { + // `interp_mmap.py:42 error = OSError` alias. + let w_os_error = crate::builtins::lookup_exc_class("OSError") + .expect("OSError must be installed before init_mmap"); + crate::module_ns_store(ns, "error", w_os_error); + + #[cfg(unix)] + register_posix_constants(ns); + crate::module_ns_store( ns, "ACCESS_DEFAULT", @@ -1416,37 +1878,21 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { pyre_object::w_int_new(MMAP_ACCESS_WRITE), ); crate::module_ns_store(ns, "ACCESS_COPY", pyre_object::w_int_new(MMAP_ACCESS_COPY)); + + // `rmmap.py:204-206` / `:229-243` — POSIX has one allocation unit, the + // page size; Windows' mapping granularity is the coarser + // `SYSTEM_INFO.dwAllocationGranularity`. crate::module_ns_store( ns, - "MADV_NORMAL", - pyre_object::w_int_new(host_mmap::MADV_NORMAL as i64), - ); - crate::module_ns_store( - ns, - "MADV_RANDOM", - pyre_object::w_int_new(host_mmap::MADV_RANDOM as i64), - ); - crate::module_ns_store( - ns, - "MADV_SEQUENTIAL", - pyre_object::w_int_new(host_mmap::MADV_SEQUENTIAL as i64), - ); - crate::module_ns_store( - ns, - "MADV_WILLNEED", - pyre_object::w_int_new(host_mmap::MADV_WILLNEED as i64), + "PAGESIZE", + pyre_object::w_int_new(rustpython_host_env::os::page_size() as i64), ); crate::module_ns_store( ns, - "MADV_DONTNEED", - pyre_object::w_int_new(host_mmap::MADV_DONTNEED as i64), + "ALLOCATIONGRANULARITY", + pyre_object::w_int_new(rustpython_host_env::os::alloc_granularity() as i64), ); - // Page-related constants (sys.PAGESIZE in CPython mmap module). - let page = unsafe { libc::sysconf(libc::_SC_PAGESIZE) }; - crate::module_ns_store(ns, "PAGESIZE", pyre_object::w_int_new(page)); - crate::module_ns_store(ns, "ALLOCATIONGRANULARITY", pyre_object::w_int_new(page)); - // Register the type itself. crate::module_ns_store(ns, "mmap", mmap_type()); } diff --git a/pyre/pyre-interpreter/src/module/mmap/mod.rs b/pyre/pyre-interpreter/src/module/mmap/mod.rs index dee0d6f54e6..7b7781f4995 100644 --- a/pyre/pyre-interpreter/src/module/mmap/mod.rs +++ b/pyre/pyre-interpreter/src/module/mmap/mod.rs @@ -1,7 +1,9 @@ //! mmap module — PyPy: pypy/module/mmap/ //! -//! `mmap.mmap(fileno, length, ...)` wraps libc mmap(2) directly. Per-instance -//! state lives in the instance dict (`_ptr`/`_len`/`_pos`/`_access`); the -//! pointer is invalidated on close/`__exit__` via munmap. +//! `mmap.mmap(fileno, length, ...)` maps through `host_env::mmap`, so the +//! module works on POSIX and on Windows, where the constructor takes a +//! `tagname` instead of flags/prot. Per-instance state lives in the instance +//! dict (`_ptr`/`_len`/`_pos`/`_access`); the pointer is invalidated on +//! close/`__exit__`, which unmaps. crate::pyre_module_init!(interp_mmap); diff --git a/pyre/pyre-interpreter/src/typedef.rs b/pyre/pyre-interpreter/src/typedef.rs index aa2dbcbe613..ef8f2761aaa 100644 --- a/pyre/pyre-interpreter/src/typedef.rs +++ b/pyre/pyre-interpreter/src/typedef.rs @@ -21500,7 +21500,7 @@ pub(crate) fn buffer_as_bytes_like( // `W_MMap.readbuf_w` — the mapping is a bytes-like source in its own // right, so `bytes(m)` / `bytearray(m)` copy it here instead of falling // through to the iterable path. - #[cfg(all(unix, not(feature = "sandbox")))] + #[cfg(all(any(unix, windows), not(feature = "sandbox")))] if let Some(view) = crate::module::mmap::interp_mmap::mmap_buffer_view(obj) { let (address, length, _readonly) = view?; let data = unsafe { std::slice::from_raw_parts(address as *const u8, length) }; From f674b2a5f568a5c0ef217a04af93578ea1f5806e Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Thu, 13 Aug 2026 06:22:19 +0900 Subject: [PATCH 4/5] jit: split the dead-op removal test off the CSE purity predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `prune_dead_phis` asked `is_pure_op` whether an operation with no readers could be deleted. That predicate answers a different question: RPython keeps `LLOp.is_pure()` (`lloperation.py:82-93`) apart from `CanRemove` (`simplify.py:411-423`), and the two disagree in both directions — a mutable `getfield` is removable but not pure, and `debug_assert` is pure but retains its side effect. `getslice` is where that mattered. It is registered `pure=True` for flowspace folding (`operation.py:461`), which is what the arm cited, but it is absent from the `CanRemove` opname list and raising at `lloperation.py:578`, so `enum_ops_without_sideeffects()` — called with the default `raising_is_ok=False` — does not add it either. Nothing upstream authorises deleting it. Its lowering checks the bounds and allocates (`rlist.py:883-890`), and the sweep's raising-op guard does not cover it because pre-rtyped `GetSlice` is classified `RaiseClass::No`. Add `can_remove_op` for the removal role, as a restriction of `is_pure_op` so the switch cannot authorise a removal that did not already happen, and route the two `prune_dead_phis` sites through it. The `float_` prefix shared the `int_`/`uint_` suffix list, which accepted seven spellings with no lltype row — `float_floordiv`, `float_mod`, `float_lshift`, `float_rshift`, `float_and`, `float_or`, `float_xor`, the first of which `lloperation.py:260` explicitly leaves to `math.fmod`. Give float its own set from `lloperation.py:246-259`, which also admits `float_truediv` (`LLOp(canfold=True)`, so `enum_ops_without_sideeffects()` yields it). Two comments were wrong and are corrected: `newlist` subclasses `HLOperation` rather than `PureOperation`, so its authorisation is the `CanRemove` list alone; and the `instance_isinstance` lloperation row cited for `IsInstance` does not exist, the high-level `isinstance` entry being what authorises it. No existing test changes status, so the `getslice` removal was not reachable from any of them; whether it fires on the corpus is not settled here. Assisted-by: Claude --- majit/majit-translate/src/inline.rs | 120 ++++++++++++++++++---------- majit/majit-translate/src/model.rs | 40 +++++++++- 2 files changed, 115 insertions(+), 45 deletions(-) diff --git a/majit/majit-translate/src/inline.rs b/majit/majit-translate/src/inline.rs index 987d1c7090b..a0a332ec518 100644 --- a/majit/majit-translate/src/inline.rs +++ b/majit/majit-translate/src/inline.rs @@ -1127,33 +1127,12 @@ pub fn op_variable_refs(kind: &OpKind) -> Vec } } -/// `true` iff `kind` is side-effect-free and may be removed from the -/// graph when its result has no readers. Direct port of RPython -/// `simplify.py:405-417 CanRemove` set + the lltype-level -/// `lloperation.enum_ops_without_sideeffects()` extension that -/// `simplify.py:414-416` unions in. +/// `true` iff `kind` is side-effect-free for the existing folding, +/// CSE, and control-flow-shape consumers of this predicate. /// -/// Pure ops correspond to RPython: -/// - flowspace pure: `add sub mul div mod ... lt le eq ... bool len -/// hash getattr getitem`-family (`simplify.py:407-412`). -/// - lltype pure: `int_add int_lt ... getfield(_pure) -/// getarrayitem(_pure) getinteriorfield ... cast_*` from -/// `enum_ops_without_sideeffects`. -/// -/// Side-effecting ops correspond to RPython `setfield setarrayitem -/// setinteriorfield` (writes), `direct_call indirect_call` (calls -/// without elidable EI), every `*guard*` opname (control-flow guards), -/// and the JIT marker family (`jit_marker`, `debug_merge_point`, -/// `loop_header`, `live`, `record_known_result`, -/// `record_quasiimmut_field`, `jit_debug`). -/// -/// Used by `model::prune_dead_phis` to mirror PyPy -/// `simplify.py:441-445`'s split: pure ops route their args via -/// `dependencies[op.result] += op.args` (args become live only if -/// the result becomes live), while non-pure ops add their args -/// directly to `read_vars` (args always live). Without the split, -/// a phi feeding only a dead pure op would be kept alive via the -/// pure op's args even though both should die together. +/// Dead-operation removal uses [`can_remove_op`] instead. RPython keeps +/// `LLOp.is_pure()` (`lloperation.py:82-93`) and `CanRemove` +/// (`simplify.py:411-423`) as distinct predicates. pub fn is_pure_op(kind: &OpKind) -> bool { match kind { // `new` / `new_with_vtable` / `new_array_clear` heap-allocate fresh @@ -1215,19 +1194,18 @@ pub fn is_pure_op(kind: &OpKind) -> bool { | OpKind::VtableMethodPtr { .. } // `newtuple` is `PureOperation` (`operation.py:542-548`). | OpKind::NewTuple { .. } - // `newlist` is `PureOperation` — a fresh list allocation has no - // observable effect on existing state. + // `newlist` subclasses `HLOperation` (`operation.py:551-557`), not + // `PureOperation`; its DCE authorization comes from the high-level + // `simplify.py:411-418 CanRemove` list alone. | OpKind::NewList { .. } - // `getslice` is a `PureOperation` (`operation.py:461`, - // `pure=True`) — the slice copy reads the source and allocates a - // fresh list, with no observable effect on existing state. + // `getslice` is registered `pure=True` for flowspace folding/CSE + // (`operation.py:461`), but its possible exception excludes it from + // dead-op removal; see `can_remove_op`. | OpKind::GetSlice { .. } - // `isinstance` lowers to `int_between` over `obj.typeptr`'s - // subclass-range fields plus an optional null branch — all - // pure reads, classified `canfold=True` upstream - // (`lloperation.py instance_isinstance`). Keeping dead - // `IsInstance` results alive would block prune_dead_phis Step - // 5 even though the predicate is side-effect-free. + // The high-level `isinstance` entry in `simplify.py:411-418` + // `CanRemove` authorizes DCE. Its lowering reads subclass-range + // fields and emits `int_between`; there is no + // `instance_isinstance` lloperation row. | OpKind::IsInstance { .. } // `LoadStatic` reads a `static` declaration's compile-time // address — equivalent to `LOAD_GLOBAL` → Constant lookup, @@ -1309,6 +1287,22 @@ pub fn is_pure_op(kind: &OpKind) -> bool { } } +/// `true` iff `kind` may be removed when its result has no readers. +/// Models RPython `simplify.py:411-423 CanRemove`, including the default +/// `raising_is_ok=False` used by `enum_ops_without_sideeffects()`. +/// +/// This is deliberately a restriction of [`is_pure_op`], so switching the +/// dead-op pass to this predicate cannot authorize any new removal. +pub fn can_remove_op(kind: &OpKind) -> bool { + match kind { + // `getslice` is absent from `simplify.py:411-418 CanRemove` and is + // raising at `lloperation.py:578`, so + // `enum_ops_without_sideeffects()` does not add it either. + OpKind::GetSlice { .. } => false, + _ => is_pure_op(kind), + } +} + /// Whitelist of `OpKind::UnaryOp` opnames that are side-effect-free /// upstream — direct port of the unary entries in /// `simplify.CanRemove` (`rpython/translator/simplify.py:405-417`) @@ -1418,11 +1412,10 @@ fn is_pure_binop_opname(opname: &str) -> bool { // Pyre's BinOp arrives here pre-rtyper (frontend names like // `add`); the post-rtyper shape is also accepted so a future // post-rtyper DCE call site requires no further widening. - let prefix_match = opname + let integer_prefix_match = opname .strip_prefix("int_") - .or_else(|| opname.strip_prefix("uint_")) - .or_else(|| opname.strip_prefix("float_")); - if let Some(suffix) = prefix_match { + .or_else(|| opname.strip_prefix("uint_")); + if let Some(suffix) = integer_prefix_match { return matches!( suffix, "add" @@ -1443,6 +1436,15 @@ fn is_pure_binop_opname(opname: &str) -> bool { | "ge" ); } + if let Some(suffix) = opname.strip_prefix("float_") { + // `lloperation.py:246-261` defines only arithmetic through true + // division and the six comparisons, and explicitly leaves + // `float_mod` to `math.fmod`. + return matches!( + suffix, + "add" | "sub" | "mul" | "truediv" | "lt" | "le" | "eq" | "ne" | "gt" | "ge" + ); + } false } @@ -1559,6 +1561,42 @@ mod tests { use crate::model::{CallTarget, FunctionGraph, OpKind, ValueType}; use crate::parse::CallPath; + #[test] + fn dead_op_removal_and_float_purity_use_their_rpython_tables() { + let getslice = OpKind::GetSlice { args: vec![] }; + assert!(is_pure_op(&getslice)); + assert!(!can_remove_op(&getslice)); + + for name in [ + "float_add", + "float_sub", + "float_mul", + "float_truediv", + "float_lt", + "float_le", + "float_eq", + "float_ne", + "float_gt", + "float_ge", + ] { + assert!(is_pure_binop_opname(name), "{name} is an lltype float op"); + } + for name in [ + "float_floordiv", + "float_mod", + "float_lshift", + "float_rshift", + "float_and", + "float_or", + "float_xor", + ] { + assert!( + !is_pure_binop_opname(name), + "{name} is absent from the lltype float table" + ); + } + } + #[test] fn lowered_blackhole_op_purity_is_opname_aware() { let pure = |name: &str| { diff --git a/majit/majit-translate/src/model.rs b/majit/majit-translate/src/model.rs index 51d8e8005a7..4e1d6ef1a44 100644 --- a/majit/majit-translate/src/model.rs +++ b/majit/majit-translate/src/model.rs @@ -3894,7 +3894,7 @@ pub(crate) fn prune_dead_boxing_remnants(graph: &mut FunctionGraph) -> usize { /// `Block::canraise()` mirrors the upstream /// `block.exitswitch is c_last_exception` check; the raising /// op is the last entry in `block.operations`. -/// - If `is_pure_op(kind)` AND the op is not the raising_op +/// - If `can_remove_op(kind)` AND the op is not the raising_op /// AND `op.result.is_some()`: route operands via /// `dependencies[op.result] += operands` /// (`simplify.py:444-445 dependencies[op.result]. @@ -3979,7 +3979,7 @@ pub(crate) fn prune_dead_boxing_remnants(graph: &mut FunctionGraph) -> usize { reason = "Eq and Hash use immutable identity/value data; interior mutation is excluded, matching RPython identity-keyed dict semantics" )] pub fn prune_dead_phis(graph: &mut FunctionGraph) { - use crate::inline::is_pure_op; + use crate::inline::can_remove_op; use std::collections::HashMap; let start = graph.startblock; let return_block = graph.returnblock; @@ -4110,7 +4110,7 @@ pub fn prune_dead_phis(graph: &mut FunctionGraph) { // `simplify.py:441-445`: // if not canremove(op, block): read_vars.update(args) // else: dependencies[result] += args - let removable = is_pure_op(&op.kind) && Some(i) != raising_op_idx; + let removable = can_remove_op(&op.kind) && Some(i) != raising_op_idx; if let Some(result_var) = op.result.clone() && removable { @@ -4231,7 +4231,7 @@ pub fn prune_dead_phis(graph: &mut FunctionGraph) { Some(r) => !read_vars.contains(r), None => false, }; - if dead && is_pure_op(&op.kind) && Some(i) != raising_op_idx { + if dead && can_remove_op(&op.kind) && Some(i) != raising_op_idx { dead_op_positions.push((block.id, i)); } } @@ -6714,6 +6714,38 @@ mod tests { ); } + #[test] + fn prune_dead_phis_keeps_dead_raising_getslice() { + let mut graph = FunctionGraph::new("test"); + let entry = graph.startblock; + let list = graph + .push_op_var(entry, OpKind::ConstRefNull, true) + .unwrap(); + let start = graph.push_op_var(entry, OpKind::ConstInt(0), true).unwrap(); + let stop = graph.push_op_var(entry, OpKind::ConstInt(1), true).unwrap(); + let slice = graph + .push_op_var( + entry, + OpKind::GetSlice { + args: vec![list, start, stop], + }, + true, + ) + .unwrap(); + graph.set_return(entry, None); + + prune_dead_phis(&mut graph); + + assert!( + graph + .block(entry) + .operations + .iter() + .any(|op| op.result.as_ref() == Some(&slice)), + "dead getslice must remain because `simplify.CanRemove` excludes it" + ); + } + #[test] fn remove_dead_aggregates_drops_discarded_ctor_and_its_field_stores() { // entry: tmp = SyntheticTransparentCtor("Tuple"); From ebc7c586f933103d6bf44fe9344e9815984880f3 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Thu, 13 Aug 2026 16:46:27 +0900 Subject: [PATCH 5/5] jit-trace: drop pcdep at non-decodable resume positions Keep the containing frame width at non-live JitCode offsets while leaving the color map empty when the liveness stream cannot be decoded. Assisted-by: Claude --- pyre/pyre-jit-trace/src/state.rs | 67 +++++++++++++++----------------- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 18adca996fc..d7e71324586 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -2060,11 +2060,8 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // they've been consumed and carry no pcdep entry. // // A caller holding no Python coordinate reads the floor-only static - // depth twin. This is independent of whether `jitcode_pc` itself is - // a `-live-`-anchored startpoint: the frame width and the per-slot - // color map are codewriter sidecars keyed by the containing operation, - // while `can_decode_live_vars` only decides whether the register-value - // stream can be enumerated directly at that byte. + // depth twin. Frame width is a containing-operation property and does + // not require decoding the register-value stream at `jitcode_pc`. // // Jitcode-keyed spellings of this same static table exist: // `depth_containing_for_jitcode_pc` is built (`finalize_jitcode`, the @@ -2072,9 +2069,8 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( // `depth_at_py_pc[containing_py]` for every offset, and // `depth_trivia_for_jitcode_pc` reads it at the trivia-skipped py. // `collect_outer_active_boxes`, the encode half this mirrors, already - // sources its own `stack_depth_at_pc` and pcdep from JitCode-keyed - // twins with no Python PC in hand. The census below measured what the - // containing-depth twin answers here: + // sources its own `stack_depth_at_pc` from the JitCode-keyed depth twin + // with no Python PC in hand. The census below measured what it answers: // over the 399-file synth corpus every one of the 20 executions of this // leg carried a non-negative offset at which the containing twin // answers 3, 4 or 5. Returning 0 truncated the reconstructed frame's @@ -2105,30 +2101,30 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( empty_twin_census(site, Some(rp), Some(table.len()), depth, jitcode_pc, twin); depth as usize }; - let (stack_depth_at_pc, pcdep_entries) = if jitcode_pc >= 0 { + let (stack_depth_at_pc, pcdep_entries) = if jitcode_pc >= 0 + && payload + .jitcode + .can_decode_live_vars(jitcode_pc as usize, sd.op_live) + { let jp = jitcode_pc as usize; // Decode-identity: source depth/pcdep from the carried genuine - // `jitcode_pc` via the predecessor-keyed twins. A `-live-` - // startpoint uses the walk-time depth twin, exactly as the encoder - // does. A non-decodable operation byte still owns semantic - // sidecars: retain its pcdep and obtain frame width from the static - // containing-depth twin instead of collapsing both to empty/zero. - // RPython's resume reader keeps these concerns separate too: - // `setup_resume_at_op(pc)` selects the frame position and - // `consume_boxes(get_current_position_info(), ...)` enumerates - // whatever register stream that position carries. + // `jitcode_pc` via the predecessor-keyed twins. Once liveness is + // decodable, the two sidecars are independently optional: a miss + // in one must not discard the other. let pcdep = payload.pcdep_for_jitcode_pc(jp).unwrap_or_default(); - if payload.jitcode.can_decode_live_vars(jp, sd.op_live) { - let depth = payload - .depth_for_jitcode_pc_pred(jp) - .map(usize::from) - .unwrap_or_else(|| via_py_pc(py_pc, &EMPTY_TWIN_MISS)); - (depth, pcdep) - } else { - (via_py_pc(py_pc, &EMPTY_TWIN_NON_DECODABLE), pcdep) - } + let depth = payload + .depth_for_jitcode_pc_pred(jp) + .map(usize::from) + .unwrap_or_else(|| via_py_pc(py_pc, &EMPTY_TWIN_MISS)); + (depth, pcdep) } else { - // A genuinely absent coordinate can only use a carried Python PC. + // RPython resume.py rebuild_from_resumedata does + // setup_resume_at_op(pc) and then passes that frame's + // get_current_position_info() to consume_boxes. The latter calls + // get_live_vars_info(pc), so a position without a live-anchored + // register stream cannot supply a color-to-slot pcdep map. Keep + // the independently useful static frame width, but do not combine + // it with pcdep from a coordinate whose values cannot be decoded. (via_py_pc(py_pc, &EMPTY_TWIN_NON_DECODABLE), Vec::new()) }; BridgeSemanticMaps { @@ -2150,8 +2146,8 @@ pub(crate) fn bridge_semantic_maps_at_with_jitcode_pc( /// `callee_jitcode_pc`). The two `RebuiltFrame` callers (`state.rs` /// `reconstruct_inline_recipe` and `setup_bridge_sym`) do hold one — the /// forward-carried `RebuiltFrame::py_pc`, but the JitCode-keyed containing -/// depth and pcdep twins now carry the same semantic information without -/// routing the JitCode word through a Python-keyed map. +/// depth twin preserves their frame width without routing the JitCode word +/// through a Python-keyed map. pub(crate) fn bridge_semantic_maps_from_jitcode_pc( jitcode_index: i32, jitcode_pc: i32, @@ -13753,7 +13749,7 @@ mod tests { } #[test] - fn bridge_semantic_maps_keep_sidecars_at_non_live_jitcode_offset() { + fn bridge_semantic_maps_keep_width_but_drop_pcdep_at_non_live_jitcode_offset() { use pyre_interpreter::pyframe::PyFrame; ensure_test_callbacks(); @@ -13764,9 +13760,10 @@ mod tests { // A CALL/residual operation start can be a valid JitCode coordinate // without being a `-live-`-anchored decode point. The codewriter still - // publishes containing-depth and pcdep sidecars for that coordinate; - // bridge reconstruction must not collapse them to 0/empty merely - // because the runtime JitCode body below has no live marker at pc=9. + // publishes containing-depth and pcdep sidecars for that coordinate. + // The static width remains usable, but pcdep describes the register + // stream consumed by get_current_position_info; without a live marker + // at pc=9 that stream cannot be decoded and the map must be dropped. let mut metadata = crate::PyJitCodeMetadata::degenerate(); metadata.depth_containing_by_jit_pc = vec![(0, 4)]; metadata.pcdep_by_jit_pc = vec![(0, vec![(1, 7, 3)])]; @@ -13785,7 +13782,7 @@ mod tests { let maps = bridge_semantic_maps_from_jitcode_pc(jitcode_index, 9); assert_eq!(maps.stack_depth_at_pc, 4); - assert_eq!(maps.pcdep_entries, vec![(1, 7, 3)]); + assert!(maps.pcdep_entries.is_empty()); } #[test]