diff --git a/changelog.d/8106-root-reload-note-slot-symbols.md b/changelog.d/8106-root-reload-note-slot-symbols.md new file mode 100644 index 0000000000..b27e073607 --- /dev/null +++ b/changelog.d/8106-root-reload-note-slot-symbols.md @@ -0,0 +1,30 @@ +**Fixed a symbol name in codegen's root-reload allowlist that has never matched +anything (#5094).** `root_reload.rs`'s `NON_COLLECTING` listed +`js_gc_layout_note_slot`; the runtime exports `js_gc_note_slot_layout` +(`gc/layout.rs:814`) and `js_gc_note_slot_layout_aware` (`:833`). No symbol by +the old spelling exists anywhere in the tree — `gc_call_effects.rs` and all +twelve tests that reference these helpers use the real names. + +The failure mode is why it survived. The file's own contract says a helper +missing from the list "is treated as collecting, which inserts a reload the +checker would not have demanded — a load, not a bug". So the typo was +safe-direction and silent: every emitted slot-layout note forced a root reload +instead of none, including the one call per guarded array element store +(`expr/index_set_guarded.rs`), which is the hot path #5094 exists for. + +Both real names are now listed, and the phantom is removed from +`scripts/gc_root_dominance_check.py` as well — that file carried it too, +harmlessly, because it also carries the correct spelling. `_aware` is added +there alongside: it is `js_gc_note_slot_layout` behind an early return taken +when neither the new nor the old bits are pointer-bearing, so it does strictly +less than the entry point the set already admits — the same "differ only by +doing less" argument the file already accepts for `declare` vs `init`. The +one-way containment invariant (`root_reload.rs`'s list is a subset of the +checker's) is preserved in both directions of the edit. + +Two regression tests in `root_reload_tests.rs`, both failing on the parent +commit: every `NON_COLLECTING` entry must be a name +`gc_call_effects::classify_direct_callee` answers `CannotCollect` for — which is +the "the two lists must agree" rule the checker's own comment states and nothing +enforced — and the two note helpers are pinned by name, because the bug was a +*missing* entry and a containment test alone is satisfied by an empty set. diff --git a/crates/perry-codegen/src/root_reload.rs b/crates/perry-codegen/src/root_reload.rs index ccccc1672c..9a329979e7 100644 --- a/crates/perry-codegen/src/root_reload.rs +++ b/crates/perry-codegen/src/root_reload.rs @@ -183,22 +183,39 @@ const NON_COLLECTING: &[&str] = &[ "js_gc_init_typed_shape_layout", "js_gc_declare_typed_shape_layout", "js_gc_forget_object_layout", - "js_gc_layout_note_slot", + // The two real slot-layout note exports. `js_gc_layout_note_slot` used to + // stand here, and no such symbol has ever existed in the tree: the runtime + // spells them `js_gc_note_slot_layout` (`gc/layout.rs:814`) and + // `js_gc_note_slot_layout_aware` (`:833`), which is how + // `gc_call_effects.rs` and every test names them. The typo was + // safe-direction — an absent helper is treated as collecting, which costs + // a reload rather than losing one — but it meant EVERY emitted note forced + // a reload, including the one per guarded array element store + // (`expr/index_set_guarded.rs`), which is #5094's hot path. + "js_gc_note_slot_layout", + // `_aware` is `js_gc_note_slot_layout` plus an early return when neither + // the new nor the old bits are pointer-bearing, so it does strictly LESS + // than the entry point above. Same reasoning the checker already accepts + // for `declare` vs `init`. + "js_gc_note_slot_layout_aware", "js_write_barrier", "js_write_barrier_root_nanbox", "js_write_barrier_slot", - "js_runtime_write_barrier_slot", "js_gc_register_global_root", - // pure value predicates / bit twiddling + // pure value predicates / bit twiddling. + // + // `js_value_is_object`, `js_value_is_string` and `js_typeof_tag` used to sit + // here, and — like `js_gc_layout_note_slot` above them — none is a symbol + // this tree exports. Six such names had accumulated (the three here plus + // `js_runtime_write_barrier_slot`, `js_typed_feedback_shape_guard` and + // `js_typed_feedback_note`). They cost nothing on their own, because a name + // that matches no callee never fires; what they cost is camouflage — they + // made a REAL transposition indistinguishable from an aspirational entry. + // `every_non_collecting_entry_is_a_real_runtime_export` now rejects both. "js_is_truthy", "js_nanbox_get_pointer", - "js_value_is_object", - "js_value_is_string", - "js_typeof_tag", // inline-cache guards: pure reads "js_typed_feedback_closure_direct_call_guard", - "js_typed_feedback_shape_guard", - "js_typed_feedback_note", // verified non-allocating bookkeeping stores/reads "js_closure_set_capture_bits", "js_closure_get_capture_bits", diff --git a/crates/perry-codegen/src/root_reload_tests.rs b/crates/perry-codegen/src/root_reload_tests.rs index 82f600ab43..b9ec74e65d 100644 --- a/crates/perry-codegen/src/root_reload_tests.rs +++ b/crates/perry-codegen/src/root_reload_tests.rs @@ -909,3 +909,103 @@ fn a_capture_set_to_a_different_index_does_not_suppress_the_reload() { "a set to a DIFFERENT capture index must not suppress this reload" ); } + +/// Every name in [`NON_COLLECTING`] must be a symbol the runtime actually +/// exports. +/// +/// The list is consulted by exact string match against an LLVM callee, so a +/// name that matches nothing is inert — which is precisely why seven of them +/// accumulated undetected. Six were aspirational (`js_value_is_object`, +/// `js_typeof_tag`, …); the seventh, `js_gc_layout_note_slot`, was a +/// transposition of the real, hot `js_gc_note_slot_layout`, and it cost a root +/// reload at every emitted slot-layout note — including the one per guarded +/// array element store. +/// +/// Nothing could distinguish the two cases, because the fallback for an +/// unrecognised helper is safe-direction (treat as collecting ⇒ insert a +/// reload), so the only symptom was a permanent, quiet pessimisation. This +/// makes a misspelling a test failure. +#[test] +fn every_non_collecting_entry_is_a_real_runtime_export() { + let mut sources = String::new(); + for crate_dir in ["perry-runtime", "perry-stdlib"] { + let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join(crate_dir) + .join("src"); + collect_rust_sources(&root, &mut sources); + } + assert!( + sources.len() > 1_000_000, + "runtime sources did not load (read {} bytes); the test would pass \ + vacuously", + sources.len() + ); + + let phantom: Vec<&str> = NON_COLLECTING + .iter() + .copied() + .filter(|name| !declares_extern_c_fn(&sources, name)) + .collect(); + assert!( + phantom.is_empty(), + "NON_COLLECTING names with no `extern \"C\" fn` definition in \ + perry-runtime/perry-stdlib — a name that matches no callee is inert, \ + so a typo here is a silent pessimisation rather than a failure: \ + {phantom:?}" + ); +} + +/// Append every `.rs` file under `dir` to `out`. +fn collect_rust_sources(dir: &std::path::Path, out: &mut String) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + collect_rust_sources(&path, out); + } else if path.extension().is_some_and(|e| e == "rs") { + if let Ok(text) = std::fs::read_to_string(&path) { + out.push_str(&text); + out.push('\n'); + } + } + } +} + +/// Is there an `extern "C" fn ` definition in `sources`? +/// +/// Deliberately matches the definition, not a mention: every phantom this +/// catches was *mentioned* — in this list and in the checker's twin of it. +fn declares_extern_c_fn(sources: &str, name: &str) -> bool { + sources + .match_indices("extern \"C\" fn ") + .any(|(at, marker)| { + let rest = &sources[at + marker.len()..]; + rest.strip_prefix(name) + .is_some_and(|tail| !tail.starts_with(|c: char| c.is_alphanumeric() || c == '_')) + }) +} + +/// The two real slot-layout note exports must be present, spelled the way the +/// runtime exports them (`gc/layout.rs`) and `gc_call_effects.rs` matches them. +/// +/// Pinned by name rather than left to the existence test above, because the bug +/// this replaces was a *missing* entry, and "no phantom members" is satisfied +/// by an empty set. +#[test] +fn the_slot_layout_note_helpers_are_non_collecting() { + for name in ["js_gc_note_slot_layout", "js_gc_note_slot_layout_aware"] { + assert!( + NON_COLLECTING.contains(&name), + "{name} is emitted per guarded element/field store; leaving it out \ + forces a root reload at every one of them" + ); + } + assert!( + !NON_COLLECTING.contains(&"js_gc_layout_note_slot"), + "js_gc_layout_note_slot is not a symbol in this tree — it was a \ + transposition of js_gc_note_slot_layout" + ); +} diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 91b6abfcc2..557b6b4cca 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -468,15 +468,12 @@ def build_cfg(f): # cannot -- side-table metadata writes through the system allocator, which # arms no Perry GC trigger. "js_gc_init_typed_shape_layout", "js_gc_declare_typed_shape_layout", - "js_gc_layout_note_slot", "js_write_barrier_root_nanbox", "js_write_barrier_slot", - "js_runtime_write_barrier_slot", "js_gc_register_global_root", + "js_gc_register_global_root", # pure value predicates / bit twiddling - "js_is_truthy", "js_nanbox_get_pointer", "js_value_is_object", - "js_value_is_string", "js_typeof_tag", + "js_is_truthy", "js_nanbox_get_pointer", # inline-cache guards: pure reads "js_typed_feedback_closure_direct_call_guard", - "js_typed_feedback_shape_guard", "js_typed_feedback_note", # ctor identity selection "js_ctor_return_override", "llvm.lifetime.start.p0", "llvm.lifetime.end.p0", @@ -505,7 +502,15 @@ def build_cfg(f): "js_closure_unbox_callee_checked", # object/this_binding.rs:160 -- a thread-local cell swap "js_implicit_this_set", "js_implicit_this_get", - "js_gc_note_slot_layout", "js_string_addref_if_heap_string", + # `js_gc_note_slot_layout` (gc/layout.rs:814) and its `_aware` sibling + # (:833). `_aware` is the same body behind an early return taken when + # neither the new nor the old bits are pointer-bearing, so it does strictly + # LESS than the entry point beside it -- the same "differ only by doing + # less" argument this file already accepts for `declare` vs `init` above. + # A phantom third spelling, `js_gc_layout_note_slot`, sat in this set (and + # in `root_reload.rs`) and matched no symbol in the tree. + "js_gc_note_slot_layout", "js_gc_note_slot_layout_aware", + "js_string_addref_if_heap_string", # `js_get_string_pointer_unified` is deliberately NOT here. Its SSO branch # calls `js_string_materialize_to_heap`, which allocates (value/nanbox.rs:268), # so it is a collection point by this file's one-sided rule even though the