Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
coderabbitai[bot] marked this conversation as resolved.

# 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.
Expand Down
29 changes: 29 additions & 0 deletions changelog.d/7389-raw-handle-across.md
Original file line number Diff line number Diff line change
@@ -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.
62 changes: 62 additions & 0 deletions crates/perry-runtime/src/gc/roots/runtime_handles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<ObjectHeader>();
/// 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::<ObjectHeader, _>(
/// || 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<T, R>(&self, f: impl FnOnce() -> R) -> (R, *mut T) {
let result = f();
(result, self.get_raw_mut_ptr::<T>())
}

/// `across_mut` for a `*const` receiver. See its docs.
#[inline]
pub fn across_const<T, R>(&self, f: impl FnOnce() -> R) -> (R, *const T) {
let result = f();
(result, self.get_raw_const_ptr::<T>())
}

/// `across_mut` for a NaN-boxed value.
#[inline]
pub fn across_nanbox<R>(&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())
}
Expand Down
59 changes: 59 additions & 0 deletions crates/perry-runtime/src/gc/tests/copying.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<u8>() as usize;
assert_eq!(
stale, child,
"pre-collection read should be the original address"
);

let (trace, fresh) = handle.across_mut::<u8, _>(|| 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::<u8>() 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);
Expand Down
19 changes: 13 additions & 6 deletions crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<ObjectHeader>();
// #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::<ObjectHeader, _>(|| {
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
Expand Down Expand Up @@ -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::<ObjectHeader>();
}
// #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::<ObjectHeader>();
}
}
Expand Down
112 changes: 112 additions & 0 deletions scripts/raw_handle_debt.py
Original file line number Diff line number Diff line change
@@ -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::<ObjectHeader>();",
"src_h.get_raw_const_ptr::<u8>()",
]
must_not_match = [
"let (found, obj) = h.across_mut::<ObjectHeader, _>(|| f());",
"h.across_const::<ObjectHeader, _>(|| 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())
1 change: 1 addition & 0 deletions scripts/raw_handle_debt_baseline.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1006
Loading