gc: teach gc-root-dominance the referent-with-no-name hole (#7616) - #7679
Conversation
b119c3d to
73d97f5
Compare
📝 WalkthroughWalkthroughThe checker adds transitive poll-reach analysis for allocating runtime symbols. CI runs the new audit, runtime classifications expand, self-tests cover analysis failures, and documentation records updated audit results and statepoint budgets. ChangesPoll-reach audit
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant audit_poll_reach
participant RuntimeSources
participant POLL_CAPABLE_RUNTIME
CI->>audit_poll_reach: run --audit-poll-reach
audit_poll_reach->>RuntimeSources: extract exported bodies and call edges
RuntimeSources-->>audit_poll_reach: return direct and transitive reachability
audit_poll_reach->>POLL_CAPABLE_RUNTIME: compare allocating reachable symbols
POLL_CAPABLE_RUNTIME-->>audit_poll_reach: return classifications
audit_poll_reach-->>CI: report gaps or pass
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/gc-root-dominance.yml (1)
356-367: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRecord the curated corpus re-measurement too, or state that it did not move.
The comment at Lines 364-367 records 86 -> 104 for the dependency-scale arm under the 77 new
POLL_CAPABLE_RUNTIMEsymbols. The curated arm at Lines 356-362 keeps--max-stale 39with no note. The widening applies to both corpora, so the curated number very likely moved as well.Add the measured curated value next to Line 362. If the curated count is now close to 39, the arm will flake on unrelated churn, and a reader has no record of the headroom.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/gc-root-dominance.yml around lines 356 - 367, Record the curated corpus stale-register count after widening POLL_CAPABLE_RUNTIME in the comment adjacent to the “Stale-register budget (curated)” command, or explicitly state that it did not change. If the measured value approaches the --max-stale 39 threshold, document the remaining headroom without changing the pinned budget.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 278-283: Update the audit example in the section discussing `#7453`
so the symbol name consistently uses js_url_coerce_string, matching the runtime
definition and ALLOC_RE classification; change only the line that currently
refers to url_coerce_string.
In `@scripts/gc_root_dominance_check.py`:
- Around line 815-825: Update the header comment above the safe-direction audit
to remove the claim that reachability uses direct calls only and that computing
a fixpoint requires a separate Rust call-graph tool. Align the prose with the
transitive behavior implemented by poll_reaching_runtime_symbols and described
by the fixpoint second-wave comment, while preserving the remaining
under-approximation and safe-direction explanation.
- Around line 844-884: Update runtime_symbol_bodies to call _strip_noncode once
on each file’s complete source before applying _EXTERN_C_FN_RE and
_balanced_body, preserving offsets while ensuring braces in comments and string
literals cannot affect matching. Remove the post-extraction stripping of
individual bodies, and extend _strip_noncode to replace character literals such
as '{' when needed for balanced brace detection.
---
Outside diff comments:
In @.github/workflows/gc-root-dominance.yml:
- Around line 356-367: Record the curated corpus stale-register count after
widening POLL_CAPABLE_RUNTIME in the comment adjacent to the “Stale-register
budget (curated)” command, or explicitly state that it did not change. If the
measured value approaches the --max-stale 39 threshold, document the remaining
headroom without changing the pinned budget.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ba02ccf-2665-46a3-8735-d50f8c7576e1
📒 Files selected for processing (4)
.github/workflows/gc-root-dominance.ymlchangelog.d/7679-poll-reach-audit.mddocs/src/internals/gc-rooting-invariant.mdscripts/gc_root_dominance_check.py
| 3. **A real symbol that was simply not in the set** (#7616 / #7453). The two | ||
| rounds above are both a NAME WITH NO REFERENT, and both audits look only in | ||
| that direction. `new URL(input, base)` held a raw `*mut StringHeader` from | ||
| `js_url_coerce_string` across the lowering of `base`; #7453's fix added | ||
| `url_coerce_string` to `ALLOC_RE` — its comment says *"that gap is why the | ||
| checker did not flag #7453"* — and stopped one list short, so the shape was |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the runtime symbol name.
crates/perry-runtime/src/url/mod.rs:108-125 defines js_url_coerce_string, and Lines [280-281] use that spelling. Line [282] drops the js_ prefix and documents url_coerce_string instead. Update the line so the audit example names the symbol classified by ALLOC_RE.
Proposed fix
- `url_coerce_string` to `ALLOC_RE`
+ `js_url_coerce_string` to `ALLOC_RE`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 3. **A real symbol that was simply not in the set** (#7616 / #7453). The two | |
| rounds above are both a NAME WITH NO REFERENT, and both audits look only in | |
| that direction. `new URL(input, base)` held a raw `*mut StringHeader` from | |
| `js_url_coerce_string` across the lowering of `base`; #7453's fix added | |
| `url_coerce_string` to `ALLOC_RE` — its comment says *"that gap is why the | |
| checker did not flag #7453"* — and stopped one list short, so the shape was | |
| 3. **A real symbol that was simply not in the set** (`#7616` / `#7453`). The two | |
| rounds above are both a NAME WITH NO REFERENT, and both audits look only in | |
| that direction. `new URL(input, base)` held a raw `*mut StringHeader` from | |
| `js_url_coerce_string` across the lowering of `base`; `#7453`'s fix added | |
| `js_url_coerce_string` to `ALLOC_RE` — its comment says *"that gap is why the | |
| checker did not flag `#7453`"* — and stopped one list short, so the shape was |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/src/internals/gc-rooting-invariant.md` around lines 278 - 283, Update
the audit example in the section discussing `#7453` so the symbol name
consistently uses js_url_coerce_string, matching the runtime definition and
ALLOC_RE classification; change only the line that currently refers to
url_coerce_string.
| # ## One-sided in the safe direction, twice | ||
| # | ||
| # * The reach relation is DIRECT calls only, not a fixpoint through the | ||
| # runtime's internal Rust functions. That under-approximates -- a wrapper two | ||
| # levels deep is missed -- so the audit can fail to report, never falsely | ||
| # report. Widening it to a fixpoint needs a Rust call graph, which is a | ||
| # different tool. | ||
| # * Line comments and string literals are stripped before the scan, because a | ||
| # premise extracted from a comment is a phantom in the same way a | ||
| # POLL_CAPABLE_RUNTIME entry naming nothing is. Measured: stripping removed | ||
| # exactly one false hit from the first run of this audit. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the "DIRECT calls only" claim; the shipped relation is a fixpoint.
Lines 817-821 state that the reach relation is direct calls only and that widening it to a fixpoint "needs a Rust call graph, which is a different tool". poll_reaching_runtime_symbols at Line 899 computes a transitive fixpoint, and its own docstring says so. The comment at Line 1251 also describes the fixpoint second wave. This file argues that stale prose becomes a false premise, so the header should match the code.
📝 Proposed wording change
-# * The reach relation is DIRECT calls only, not a fixpoint through the
-# runtime's internal Rust functions. That under-approximates -- a wrapper two
-# levels deep is missed -- so the audit can fail to report, never falsely
-# report. Widening it to a fixpoint needs a Rust call graph, which is a
-# different tool.
+# * The reach relation is a fixpoint over EXPORTED `js_*` symbols only. A hop
+# through a private Rust helper is still missed, so the audit can fail to
+# report, never falsely report.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ## One-sided in the safe direction, twice | |
| # | |
| # * The reach relation is DIRECT calls only, not a fixpoint through the | |
| # runtime's internal Rust functions. That under-approximates -- a wrapper two | |
| # levels deep is missed -- so the audit can fail to report, never falsely | |
| # report. Widening it to a fixpoint needs a Rust call graph, which is a | |
| # different tool. | |
| # * Line comments and string literals are stripped before the scan, because a | |
| # premise extracted from a comment is a phantom in the same way a | |
| # POLL_CAPABLE_RUNTIME entry naming nothing is. Measured: stripping removed | |
| # exactly one false hit from the first run of this audit. | |
| # ## One-sided in the safe direction, twice | |
| # | |
| # * The reach relation is a fixpoint over EXPORTED `js_*` symbols only. A hop | |
| # through a private Rust helper is still missed, so the audit can fail to | |
| # report, never falsely report. | |
| # * Line comments and string literals are stripped before the scan, because a | |
| # premise extracted from a comment is a phantom in the same way a | |
| # POLL_CAPABLE_RUNTIME entry naming nothing is. Measured: stripping removed | |
| # exactly one false hit from the first run of this audit. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_root_dominance_check.py` around lines 815 - 825, Update the header
comment above the safe-direction audit to remove the claim that reachability
uses direct calls only and that computing a fixpoint requires a separate Rust
call-graph tool. Align the prose with the transitive behavior implemented by
poll_reaching_runtime_symbols and described by the fixpoint second-wave comment,
while preserving the remaining under-approximation and safe-direction
explanation.
| def _balanced_body(src, start): | ||
| """The `{...}` block beginning at or after `start`, or None.""" | ||
| open_at = src.find("{", start) | ||
| if open_at < 0: | ||
| return None | ||
| depth = 0 | ||
| i = open_at | ||
| while i < len(src): | ||
| c = src[i] | ||
| if c == "{": | ||
| depth += 1 | ||
| elif c == "}": | ||
| depth -= 1 | ||
| if depth == 0: | ||
| return src[open_at:i + 1] | ||
| i += 1 | ||
| return None | ||
|
|
||
|
|
||
| def runtime_symbol_bodies(roots=SYMBOL_ROOTS): | ||
| """`js_* -> [body]` for every `extern "C" fn js_*` the runtime exports. | ||
|
|
||
| A symbol can appear more than once (per-platform `cfg` variants), so the | ||
| value is a list and every body is scanned. | ||
| """ | ||
| bodies = defaultdict(list) | ||
| for root in roots: | ||
| if not os.path.isdir(root): | ||
| continue | ||
| for dirpath, _dirs, files in os.walk(root): | ||
| for name in files: | ||
| if not name.endswith(".rs"): | ||
| continue | ||
| with open(os.path.join(dirpath, name), | ||
| encoding="utf-8", errors="replace") as fh: | ||
| src = fh.read() | ||
| for m in _EXTERN_C_FN_RE.finditer(src): | ||
| body = _balanced_body(src, m.end()) | ||
| if body is not None: | ||
| bodies[m.group(1)].append(_strip_noncode(body)) | ||
| return bodies |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strip comments and string literals before brace matching, not after.
_balanced_body counts braces on raw source. runtime_symbol_bodies applies _strip_noncode only at Line 883, after extraction. A brace inside a string literal or a line comment therefore unbalances the counter and truncates the body, or extends it past the function. Examples in real runtime code: panic!("unexpected }}"), // TODO: {, or '{'.
A truncated body drops call edges, so the audit silently under-reports the gap it exists to find. The with_edges < 100 floor only catches total extractor failure.
Strip the whole file once, then run _EXTERN_C_FN_RE and _balanced_body over the stripped text so offsets stay consistent.
🐛 Proposed fix
with open(os.path.join(dirpath, name),
encoding="utf-8", errors="replace") as fh:
- src = fh.read()
+ src = _strip_noncode(fh.read())
for m in _EXTERN_C_FN_RE.finditer(src):
body = _balanced_body(src, m.end())
if body is not None:
- bodies[m.group(1)].append(_strip_noncode(body))
+ bodies[m.group(1)].append(body)Note: _strip_noncode replaces a string literal with "", so the brace count stays correct after this change. Character literals such as '{' remain unhandled; consider adding them to the stripper.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _balanced_body(src, start): | |
| """The `{...}` block beginning at or after `start`, or None.""" | |
| open_at = src.find("{", start) | |
| if open_at < 0: | |
| return None | |
| depth = 0 | |
| i = open_at | |
| while i < len(src): | |
| c = src[i] | |
| if c == "{": | |
| depth += 1 | |
| elif c == "}": | |
| depth -= 1 | |
| if depth == 0: | |
| return src[open_at:i + 1] | |
| i += 1 | |
| return None | |
| def runtime_symbol_bodies(roots=SYMBOL_ROOTS): | |
| """`js_* -> [body]` for every `extern "C" fn js_*` the runtime exports. | |
| A symbol can appear more than once (per-platform `cfg` variants), so the | |
| value is a list and every body is scanned. | |
| """ | |
| bodies = defaultdict(list) | |
| for root in roots: | |
| if not os.path.isdir(root): | |
| continue | |
| for dirpath, _dirs, files in os.walk(root): | |
| for name in files: | |
| if not name.endswith(".rs"): | |
| continue | |
| with open(os.path.join(dirpath, name), | |
| encoding="utf-8", errors="replace") as fh: | |
| src = fh.read() | |
| for m in _EXTERN_C_FN_RE.finditer(src): | |
| body = _balanced_body(src, m.end()) | |
| if body is not None: | |
| bodies[m.group(1)].append(_strip_noncode(body)) | |
| return bodies | |
| def _balanced_body(src, start): | |
| """The `{...}` block beginning at or after `start`, or None.""" | |
| open_at = src.find("{", start) | |
| if open_at < 0: | |
| return None | |
| depth = 0 | |
| i = open_at | |
| while i < len(src): | |
| c = src[i] | |
| if c == "{": | |
| depth += 1 | |
| elif c == "}": | |
| depth -= 1 | |
| if depth == 0: | |
| return src[open_at:i + 1] | |
| i += 1 | |
| return None | |
| def runtime_symbol_bodies(roots=SYMBOL_ROOTS): | |
| """`js_* -> [body]` for every `extern "C" fn js_*` the runtime exports. | |
| A symbol can appear more than once (per-platform `cfg` variants), so the | |
| value is a list and every body is scanned. | |
| """ | |
| bodies = defaultdict(list) | |
| for root in roots: | |
| if not os.path.isdir(root): | |
| continue | |
| for dirpath, _dirs, files in os.walk(root): | |
| for name in files: | |
| if not name.endswith(".rs"): | |
| continue | |
| with open(os.path.join(dirpath, name), | |
| encoding="utf-8", errors="replace") as fh: | |
| src = _strip_noncode(fh.read()) | |
| for m in _EXTERN_C_FN_RE.finditer(src): | |
| body = _balanced_body(src, m.end()) | |
| if body is not None: | |
| bodies[m.group(1)].append(body) | |
| return bodies |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 876-877: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(dirpath, name),
encoding="utf-8", errors="replace")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_root_dominance_check.py` around lines 844 - 884, Update
runtime_symbol_bodies to call _strip_noncode once on each file’s complete source
before applying _EXTERN_C_FN_RE and _balanced_body, preserving offsets while
ensuring braces in comments and string literals cannot affect matching. Remove
the post-extraction stripping of individual bodies, and extend _strip_noncode to
replace character literals such as '{' when needed for balanced brace detection.
The checker reported the verbatim pre-#7453 code as clean in every GATED mode — including `--statepoints`, added in #7663 precisely because the other three could not read the lowering that ships. Re-planting #7453's exact code in `expr/url_main.rs` and running every mode over the 16 URL-lowering sources, both lowerings: mode clean sabotaged --moving-only (dominance) 0 0 --unrooted-allocas --moving-only 0 0 --stale-registers --moving-only 2 2 --statepoints --moving-only 2 2 --stale-registers (unfiltered) 24 35 --statepoints (unfiltered) 15 21 Every gated arm blind, both unfiltered arms not: `js_url_coerce_string` is in ALLOC_RE but not in POLL_CAPABLE_RUNTIME, so `--moving-only` drops the window. The one name takes the sabotaged arms to 13 and 8. `--audit-alloc-re` and `--audit-poll-capable` both hunt a NAME WITH NO REFERENT. The new `--audit-poll-reach` hunts a REFERENT WITH NO NAME: a symbol ALLOC_RE matches whose runtime body reaches a POLL_CAPABLE_RUNTIME symbol without being listed. Not "every poll-capable symbol must be listed" (297 call one directly — a coverage change with its own hit count), but "the checker's two lists must not disagree about the same symbol". 77 found, all listed. Shown able to fail: deleting js_url_coerce_string reddens the audit; neutering _strip_noncode reddens the decoy fixture; making the reach one-hop instead of a fixpoint reddens the transitive fixture. Both non-vacuity floors are asserted. Budgets: curated --stale-registers --moving-only 9 -> 13 (pinned 39, unchanged); native --statepoints --moving-only 7 -> 11, re-pinned with all four new hits named. No hit disappeared.
Audit — merging as v0.5.1391The finding is the deliverable, and it is a hard one to have gone looking for: the That table is the whole case: every gated arm is blind, both unfiltered arms are not. A gate that is blind precisely in the configuration it runs in is worse than no gate, because it is quoted as evidence.
|
73d97f5 to
e14bac9
Compare
1 participant
Closes #7616.
The measurement #7616 asked for
#7616's finding was that
gc_root_dominance_check.pyreports the verbatimpre-#7453 code as clean in all three of its original modes. #7663 has since
added a fourth,
--statepoints, which reads the native lowering — the onethat ships. The right close was a measurement, not an argument: plant the exact
reproducer, run
--statepoints --moving-only, record whether it is reported.It is not. The new mode is as blind as the three it was added to cover for.
The reproducer was re-planted verbatim in
crates/perry-codegen/src/expr/url_main.rsand the emitted IR confirmed register-for-register identical to what the issue
quotes:
Over the 16
test-files/sources that exercise the URL lowerings, bothlowerings, both corpora generated by the production pipeline (
--trace llvmplus
STATEPOINT_REWRITE_PASSESfor the native arm):--moving-only(dominance)--unrooted-allocas --moving-only--stale-registers --moving-only--statepoints --moving-only--stale-registers(unfiltered)--statepoints(unfiltered)Every gated arm blind; both unfiltered arms see it. The cause is the one
#7616 named:
js_url_coerce_stringis inALLOC_REbut not inPOLL_CAPABLE_RUNTIME, and--moving-only— which every gated arm runs, in allfour modes — drops any window it cannot classify as MOVING. Adding that one name
takes the sabotaged arms to 13 and 8 and leaves the clean arms at 2 and 2.
The generalisation
--audit-alloc-recatches a dead regex alternative.--audit-poll-capablecatches an entry naming no symbol. Both hunt a name with no referent.
Nothing hunted a referent with no name, and a missing entry suppresses
findings exactly as silently as a phantom one — #7453's fix comment says in as
many words "that gap is why the checker did not flag #7453", then stopped one
list short.
--audit-poll-reachis that third auditor. It deliberately does not assert"every poll-capable runtime symbol must be listed" — 297 exported symbols call
one directly, and deciding that is a coverage change with its own hit count
(the same reasoning
ALLOC_RE's deletedbigint_\w+_oprecords). It assertsonly that the checker's two lists must not disagree about the same symbol:
if
ALLOC_REsays a call's result is a heap value the checker must track, andthe runtime shows that same call invoking something
POLL_CAPABLE_RUNTIMEalready grants can re-enter JS, the premise for listing it is the premise the
set already granted its callee. There is no judgement left to make.
77 symbols,
js_url_coerce_stringamong them.POLL_CAPABLE_RUNTIMEgoes54 → 131.
Shown able to fail
Three plants, each verified to turn the gate red, and the clean run green:
js_url_coerce_stringfromPOLL_CAPABLE_RUNTIME--audit-poll-reachexits 2 and names it_strip_noncode--self-testreports the decoy fixture--self-testreports the transitive fixtureThe third is not hypothetical. The first version of this audit was one-hop:
it reported 52 names, they were added, and re-running found 10 more that reached
through them. An audit that has to be run in a loop until it stops finding
things reports an arbitrary prefix of its own answer, so the reach relation is a
fixpoint and the self-test pins it.
The decoy fixture's only mention of a poll-capable call is inside a
//commentand a string literal — and it carries the full
js_string_coerce(v)spelling,parentheses included, because a decoy that merely names the symbol is rejected
by the call regex rather than by the stripper and would pass with
_strip_noncodedeleted. (Measured: the first draft did.)Both non-vacuity floors are asserted rather than hoped for: an empty symbol scan
and a 600-symbol scan whose bodies are all empty must each be an error, not
a clean verdict.
Budgets — both corpora, which is the reason #7616 was a separate issue
--moving-only--unrooted-allocas --moving-only--stale-registers --moving-only--statepoints --moving-only--moving-only--unrooted-allocas --moving-only--stale-registers --moving-onlyOnly
--max-unrootedmoves. The four new hits are named one by one in theworkflow rather than absorbed into a number:
test_gap_array_splice_spread::main,unrooted:alloc— a fresharray held in a raw i64 across
js_array_like_to_arrayand consumed byjs_array_concat. fix(codegen): root the URL constructor's coerced string across base lowering (Layer 1) #7453's shape, in the spread lowering instead of the URLone, i.e. exactly the population this change exists to make visible.
test_gap_class_expr_dynamic_parent_ctor,unrooted:capture— thesame
js_closure_get_capture_bitsresidual the budget comment already names,newly visible because
js_new_function_constructis now classified as a mover.No hit disappeared. The 86 → 104 on the dependency corpus stays inside the
pinned 118 and the budget is deliberately not raised.
Validation
--self-test,--audit-alloc-re,--audit-poll-capable,--audit-poll-reach,--audit-immovable-sources: all green.lint-job commands, each with its own exit status checked: 0 failures.(
cargo fmt --all -- --checkincluded; no Rust changed.)scripts/gc_gate_wiring_check.py: 6 gates main-line-reachable and able to fail.unsabotaged before the clean measurements were taken.
Summary by CodeRabbit
New Features
CI
Documentation