diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c1a17dced1..650dd6c49c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -186,6 +186,19 @@ jobs: python3 scripts/addr_class_inventory.py --self-test python3 scripts/addr_class_inventory.py + # #7341 layer 3. A RuntimeHandleScope gives an object liveness; it does + # nothing for a raw pointer already read out of the slot. Every rooting bug + # in the quarantine sweep had rooting ALREADY -- what was missing was + # ordering the re-read against the collection point. + # `RuntimeHandle::across_{mut,const,nanbox}` expresses that ordering and + # never binds the pre-call address. This counts the sites that still don't, + # and only lets the number fall. Baseline: scripts/raw_handle_debt_baseline.txt. + - name: Raw-handle debt ratchet + if: ${{ !cancelled() }} + run: | + python3 scripts/raw_handle_debt.py --self-test + python3 scripts/raw_handle_debt.py + # The gap-suite ratchet decides whether conformance-smoke goes red, so # its own logic is unit-checked on the cheap job rather than only being # exercised 8 shards deep. diff --git a/changelog.d/7389-raw-handle-across.md b/changelog.d/7389-raw-handle-across.md new file mode 100644 index 0000000000..c61bb5811b --- /dev/null +++ b/changelog.d/7389-raw-handle-across.md @@ -0,0 +1,29 @@ +**Added** `RuntimeHandle::across_{mut,const,nanbox}` and a raw-handle debt +ratchet, the layer-3 half of the rooting-by-construction argument in +`docs/src/internals/rfc-rooting-by-construction.md`. + +A `RuntimeHandleScope` gives an object *liveness*: the collector marks it and +rewrites the slot. It does nothing for a raw pointer already read out of that +slot — that copy is invisible to the collector, and if the object moves it names +from-space. Every rooting bug fixed in the #7341 quarantine sweep (#7373, #7374, +#7375, #7376, #7381, #7383, #7385) had rooting **already**; what was missing in +each was ordering the re-read against the collection point. + +`across_*` runs the allocating call and returns the object's post-collection +address in one step, so the pre-call pointer is never bound and cannot be reached +for by mistake. The `.size` arm of `js_object_get_field_by_name` is converted as +the worked example. + +This is a debt counter, **not** a soundness proof, and the script says so. Rust +has no effect system to mark "this call may allocate", so no signature can reject +holding a stale copy across one; a `&mut Heap` token cannot be threaded through +`extern "C"` boundaries either. What the ratchet does is make the count of +unconverted sites visible and monotonically decreasing — 1006 across 110 files +today, wired into `test.yml` beside the address-classification audit. + +Both the combinator tests and the ratchet were checked against their own negative +controls: sabotaging `across_mut` to return the pre-call pointer fails the test +with the intended message, and the ratchet fails on a rise and refuses to raise +its own baseline. The script's `--self-test` asserts the matcher still fires on +the shapes it exists to count and still ignores `across_*`, so a silently-broken +matcher cannot report zero and pass forever. diff --git a/crates/perry-runtime/src/gc/roots/runtime_handles.rs b/crates/perry-runtime/src/gc/roots/runtime_handles.rs index 49061588f6..209814a0d4 100644 --- a/crates/perry-runtime/src/gc/roots/runtime_handles.rs +++ b/crates/perry-runtime/src/gc/roots/runtime_handles.rs @@ -178,6 +178,68 @@ impl<'scope> RuntimeHandle<'scope> { }) } + /// Run `f` — which may allocate, and therefore may MOVE the object this + /// handle roots — and return its result together with the object's + /// **post-collection** address. + /// + /// # Why this exists + /// + /// `docs/src/internals/gc-rooting-invariant.md` states the rule, and the + /// second half is the half that keeps getting dropped: + /// + /// > A value read out of a root and held in a register across a call is not + /// > rooted. It is a copy, and the collector cannot see copies. + /// + /// A `RuntimeHandleScope` gives an object *liveness*: the collector marks it + /// and rewrites the slot. It does nothing about a raw pointer already read + /// out of that slot. Every bug in the #7341 quarantine sweep that was fixed + /// by rooting had rooting **already** — what was missing was ordering the + /// re-read relative to the collection point: + /// + /// ```ignore + /// let obj = obj_h.get_raw_mut_ptr::(); + /// let found = class_instance_has_member(class_id, "size"); // ALLOCATES + /// (*obj).field_count // from-space + /// ``` + /// + /// The defect is not a missing root. It is that `obj` is still *nameable* + /// after the call. This combinator removes that: the pre-call address is + /// never bound, so there is nothing stale to reach for. + /// + /// ```ignore + /// let (found, obj) = obj_h.across_mut::( + /// || class_instance_has_member(class_id, "size"), + /// ); + /// (*obj).field_count // post-collection + /// ``` + /// + /// # What it does NOT do + /// + /// It is not a proof. It cannot stop you reading the pointer *before* the + /// call and holding that copy yourself — Rust has no effect system to mark + /// "this call may allocate", so no signature can reject that. What it does + /// is make the correct shape shorter than the incorrect one and give the + /// ratchet in `scripts/raw_handle_debt.py` something to count down. + #[inline] + pub fn across_mut(&self, f: impl FnOnce() -> R) -> (R, *mut T) { + let result = f(); + (result, self.get_raw_mut_ptr::()) + } + + /// `across_mut` for a `*const` receiver. See its docs. + #[inline] + pub fn across_const(&self, f: impl FnOnce() -> R) -> (R, *const T) { + let result = f(); + (result, self.get_raw_const_ptr::()) + } + + /// `across_mut` for a NaN-boxed value. + #[inline] + pub fn across_nanbox(&self, f: impl FnOnce() -> R) -> (R, f64) { + let result = f(); + (result, self.get_nanbox_f64()) + } + pub fn get_nanbox_f64(&self) -> f64 { f64::from_bits(self.get_nanbox_u64()) } diff --git a/crates/perry-runtime/src/gc/tests/copying.rs b/crates/perry-runtime/src/gc/tests/copying.rs index c35ed39488..e4461ae196 100644 --- a/crates/perry-runtime/src/gc/tests/copying.rs +++ b/crates/perry-runtime/src/gc/tests/copying.rs @@ -1033,6 +1033,65 @@ fn root_source_runtime_handle_rewrite_is_attributed_to_runtime_handles() { assert!(trace.root_sources.runtime_handles.rewritten_slots > 0); } +#[test] +fn across_mut_hands_back_the_post_collection_address() { + // #7341 layer 3: the whole point of `across_mut` is that the pre-call + // address is never nameable. This asserts it is not decoration -- the + // pointer it returns must differ from the one read BEFORE the closure ran, + // because the closure evacuated the object. + let _guard = CopyingNurseryTestGuard::new(0); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + gc_register_mutable_root_scanner_with_source( + scan_runtime_handle_roots_mut, + MutableRootScannerSource::RuntimeHandles, + ); + let child = young_leaf(); + let scope = RuntimeHandleScope::new(); + let handle = scope.root_raw_mut_ptr(child as *mut u8); + + let stale = handle.get_raw_mut_ptr::() as usize; + assert_eq!( + stale, child, + "pre-collection read should be the original address" + ); + + let (trace, fresh) = handle.across_mut::(|| collect_minor_trace(GcTriggerKind::Direct)); + + assert_copied_minor_trace(&trace, true, CopiedMinorFallbackReason::None, false); + assert_ne!( + fresh as usize, stale, + "across_mut returned the stale address -- the combinator is not re-reading" + ); + assert_eq!( + fresh as usize, + handle.get_raw_mut_ptr::() as usize, + "across_mut must agree with a fresh read of the same handle" + ); +} + +#[test] +fn across_nanbox_hands_back_the_post_collection_value() { + let _guard = CopyingNurseryTestGuard::new(0); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + gc_register_mutable_root_scanner_with_source( + scan_runtime_handle_roots_mut, + MutableRootScannerSource::RuntimeHandles, + ); + let child = young_leaf(); + let scope = RuntimeHandleScope::new(); + let handle = scope.root_nanbox_u64(ptr_bits(child)); + + let stale = handle.get_nanbox_u64(); + let (trace, fresh) = handle.across_nanbox(|| collect_minor_trace(GcTriggerKind::Direct)); + + assert_copied_minor_trace(&trace, true, CopiedMinorFallbackReason::None, false); + assert_ne!( + fresh.to_bits(), + stale, + "across_nanbox returned the stale value -- the combinator is not re-reading" + ); +} + #[test] fn root_source_runtime_and_ffi_mutable_scanners_are_attributed_separately() { let _guard = CopyingNurseryTestGuard::new(0); diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs index 01ae4453f7..1d0d70c8f5 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs @@ -253,8 +253,14 @@ pub extern "C" fn js_object_get_field_by_name( if is_size_key { let size_arm_scope = crate::gc::RuntimeHandleScope::new(); let size_arm_obj = size_arm_scope.root_raw_const_ptr(obj as *const u8); - let has_own_size = super::super::own_key_present(obj as *mut ObjectHeader, key); - obj = size_arm_obj.get_raw_const_ptr::(); + // #7341 layer 3: `across_const` runs the allocating call and hands + // back the POST-collection address in one step, so the pre-call + // pointer is never bound and cannot be reached for by mistake. + // This is the shape every fix in the sweep converged on. + let (has_own_size, fresh) = size_arm_obj.across_const::(|| { + super::super::own_key_present(obj as *mut ObjectHeader, key) + }); + obj = fresh; if !has_own_size { // A subclass may also OVERRIDE `size` on its prototype // (`class M extends Map { get size() { return 42 } }`). Such an @@ -293,11 +299,12 @@ pub extern "C" fn js_object_get_field_by_name( None => {} } } - // #7341: republish before falling through — the helpers - // above may have moved it, and every arm below - // dereferences `obj`. - obj = obj_h.get_raw_const_ptr::(); } + // #7341: republish before falling through — the helpers above + // may have moved it, and every arm below dereferences `obj`. + // One republish at the end of the arm covers both branches; + // #7385 also wrote one inside the `if`, which this supersedes + // (rustc flagged it as assigned-never-read). obj = size_arm_obj.get_raw_const_ptr::(); } } diff --git a/scripts/raw_handle_debt.py b/scripts/raw_handle_debt.py new file mode 100755 index 0000000000..7bd2783781 --- /dev/null +++ b/scripts/raw_handle_debt.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python3 +"""Ratchet the number of bare raw-pointer reads out of GC root handles. + +A `RuntimeHandleScope` gives an object liveness -- the collector marks it and +rewrites the slot. It does nothing for a raw pointer already read out of that +slot: that copy is invisible to the collector, and if the object moves it names +from-space. Every rooting bug fixed in the #7341 quarantine sweep had rooting +ALREADY; what was missing was ordering the re-read against the collection point. + +`RuntimeHandle::across_{mut,const,nanbox}` expresses that ordering in one call +and never binds the pre-call address. Each bare `get_raw_*_ptr` is a site where +that ordering is a review question instead of a shape. + +This is a DEBT COUNTER, not a soundness proof. Rust has no effect system to mark +"this call may allocate", so no signature can reject holding a stale copy. Not +every bare read is a bug -- many are the final read in a scope with nothing +after them. The number is meaningful because it can only be paid down. + +Usage: + scripts/raw_handle_debt.py # report, fail if above the baseline + scripts/raw_handle_debt.py --update # rewrite the baseline (must go DOWN) +""" +import re, sys, pathlib + +ROOT = pathlib.Path(__file__).resolve().parent.parent +SRC = ROOT / "crates" / "perry-runtime" / "src" +BASELINE = ROOT / "scripts" / "raw_handle_debt_baseline.txt" +PAT = re.compile(r"\.get_raw_(?:mut|const)_ptr\b") + +# The accessors and the `across_*` combinators are DEFINED here and call each +# other; counting this file would make the ratchet count its own implementation +# and rise every time a combinator is added. Exclude it. +EXCLUDE = {"crates/perry-runtime/src/gc/roots/runtime_handles.rs"} + +def count(): + total, per_file = 0, {} + for f in sorted(SRC.rglob("*.rs")): + rel = str(f.relative_to(ROOT)) + if rel in EXCLUDE: + continue + n = len(PAT.findall(f.read_text(encoding="utf-8", errors="replace"))) + if n: + per_file[rel] = n + total += n + return total, per_file + +def self_test(): + """Guard the gate against its own regressions. + + A ratchet whose matcher silently stops matching reports 0 and passes + forever. Assert the pattern still fires on the shapes it exists to count, + and still ignores the combinator that replaces them. + """ + must_match = [ + "let obj = obj_h.get_raw_mut_ptr::();", + "src_h.get_raw_const_ptr::()", + ] + must_not_match = [ + "let (found, obj) = h.across_mut::(|| f());", + "h.across_const::(|| g())", + "h.get_nanbox_f64()", + ] + for line in must_match: + if not PAT.search(line): + print(f"self-test FAILED: pattern no longer matches: {line}") + return 1 + for line in must_not_match: + if PAT.search(line): + print(f"self-test FAILED: pattern wrongly matches: {line}") + return 1 + if not SRC.is_dir(): + print(f"self-test FAILED: source tree missing at {SRC}") + return 1 + total, per_file = count() + if total == 0 or not per_file: + print("self-test FAILED: counted zero sites -- the walk is broken") + return 1 + print(f"self-test ok ({total} sites across {len(per_file)} files)") + return 0 + +def main(): + if "--self-test" in sys.argv: + return self_test() + total, per_file = count() + if "--update" in sys.argv: + prev = int(BASELINE.read_text().split()[0]) if BASELINE.exists() else None + if prev is not None and total > prev: + print(f"refusing to raise the baseline: {prev} -> {total}") + print("the ratchet only goes down; convert sites to across_* instead") + return 1 + BASELINE.write_text(f"{total}\n") + print(f"baseline set to {total}" + (f" (was {prev})" if prev is not None else "")) + return 0 + if not BASELINE.exists(): + print(f"no baseline; run --update. current={total}") + return 1 + prev = int(BASELINE.read_text().split()[0]) + print(f"bare raw-handle reads: {total} (baseline {prev})") + if total > prev: + print(f"::error::raw-handle debt rose {prev} -> {total}") + print("Use RuntimeHandle::across_{mut,const,nanbox} -- it runs the") + print("allocating call and returns the post-collection address, so the") + print("stale pointer is never bound. See #7341.") + for path, n in sorted(per_file.items(), key=lambda kv: -kv[1])[:10]: + print(f" {n:4d} {path}") + return 1 + if total < prev: + print(f"debt fell by {prev - total}; run --update to lock it in") + return 0 + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/raw_handle_debt_baseline.txt b/scripts/raw_handle_debt_baseline.txt new file mode 100644 index 0000000000..9540e56f97 --- /dev/null +++ b/scripts/raw_handle_debt_baseline.txt @@ -0,0 +1 @@ +1006