fix(gc): the statepoint bridge must refuse an invoke it cannot root (#7327) - #7330
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe explicit statepoint bridge now detects ChangesInvoke statepoint handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
✨ 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 |
This was referenced Aug 3, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Aug 3, 2026
…echanism
Two corrections and one measurement.
The gap suite re-run against RS4GC in-process, two arms per test
(shadow-stack control + RS4GC), 479/479: 447 pass->pass, 19 pre-existing
diffs unchanged, 13 node_fail, ZERO new regressions, ZERO refusals, ZERO
compile failures. Zero refusals is the load-bearing number -- 128 of the
479 tests contain `try {}` and the bridge cannot compile any of them.
The earlier soak's "13 regressions, do not flip" was measured against
the bridge, before #7329/#7330, on a backend that structurally cannot
compile a quarter of the suite. It should not be carried forward.
And the x86-64 mechanism was wrong. The workflow comment claimed
_Unwind_GetGR(ctx, 7) "does not reliably return the stack pointer".
Measured on x86-64 Linux (glibc 2.39, gcc 13.3.0): it SEGFAULTS. RBX,
RBP and RIP return correctly; RAX and RSP both SIGSEGV, because libgcc
tracks only the columns CFI restores and RSP is derived from the CFA
rather than tracked. The fault is in the call itself, so no address
validation after it can help -- the previous wording pointed at the
wrong fix. Details and a reproducer in #7333.
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* fix(gc): route RS4GC through the in-process LLVM backend PERRY_RS4GC=1 died with `unterminated attribute group` on any stock toolchain: the pass ran under an external LLVM 22 `opt`, and Apple clang 21 then could not parse the IR it produced. That made RS4GC reachable only with PERRY_LLVM_CLANG pointed at a version-matched LLVM 22 -- and RS4GC is the only backend that can root an `invoke`, which since #7302 is every call inside a `try`. Run the pass in-process instead, where LLVM 22 is already pinned and no IR ever crosses a toolchain boundary. Two gaps had to be closed: - inprocess.rs ignored `-S`, so the statepoint backends' request for assembly silently produced an object. #7314's compact-map rewriter works on assembly text, so it needs the real thing. - the returned assembly was written straight to a `.o` with nothing assembling it (`ld: unknown file type`). Mirror the external path: write it to plan.asm_path, run compact_and_assemble, return the object. The assembler is resolved via find_clang() because plan.clang is the literal `(in-process)` placeholder here. * test(gc): cover the -S gap that made the in-process backend emit objects for assembly requests * ci(gc): assert RS4GC works with the PERRY_LLVM_* pinning removed Every existing RS4GC step pins PERRY_LLVM_OPT and PERRY_LLVM_CLANG to one brew install, which is the requirement the in-process route removes. An arm that keeps the pinning cannot observe that. This step is the only one that unsets both, and it runs probe 09 -- try-carrying, so every call in it is an invoke, which the explicit bridge refuses (#7330). It asserts four things, each of which has been a way a GC gate went green while measuring nothing: the map section exists, the compact rewrite ran, a copying minor actually copied, and RS4GC (not a per-function bail to the bridge) did the lowering. * docs: changelog fragment for #7339 * fix(codegen): give the in-process backend clang's default CPU, not LLVM's generic CI hit `LLVM ERROR: Cannot select: intrinsic %llvm.aarch64.fjcvtzs` and aborted the compile. Codegen decides whether to emit `fjcvtzs` (FEAT_JSCVT, ARMv8.3+, the single-instruction ECMAScript ToInt32) from the TRIPLE ALONE -- `set_jscvt_for_target` opts in for every Apple arm64 triple -- and that is sound for clang, whose default CPU for arm64-apple-* is apple-m1 (ARMv8.5). But `create_target_machine` with an empty CPU string selects LLVM's `generic`, which on aarch64 is ARMv8.0 and has no FEAT_JSCVT. So the two halves disagreed: one decided what to EMIT from the triple, the other what the target could EXECUTE, and only the clang path had them aligned. The in-process backend now derives the same default. Local build never hit it because a host build gets `-mcpu=native`, which takes the host-features branch. Reproduced exactly with `PERRY_TARGET_CPU=generic` (the CI path): fails before, compiles after, 9/9 probes compile, native-tuning path unchanged. Also fixes the doc-comment placement that had left `#[allow(clippy::type_complexity)]` attached to the wrong function. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* fix(gc): split the precise-root analysis from its lowering One knob answered two questions. "Which locals hold GC pointers, and where must each stay live" is the analysis and is backend-independent. "Is that answer represented as a heap-backed shadow frame or a native stack map" is the lowering, and LlFunction already chose it independently -- enable_shadow_frame_inner and reserve_shadow_slot both take the native path first. But the eight sites that build the slot map all gated on shadow_stack_enabled(), so PERRY_SHADOW_STACK=0 switched the ANALYSIS off and left the statepoint lowering with nothing to lower. The result was a binary with no precise frame roots at all: no __perry_gcmap section, same size as a plain shadow-off build, correct output. Nothing distinguished it from a good build until a collection freed a live object. #7332 made the pair a hard error as a stopgap. Route those eight sites through precise_root_analysis_enabled() instead and the pair becomes expressible, which is what the stopgap was standing in for. Measured on 01_nursery_churn: PERRY_STATEPOINTS=1 with and without PERRY_SHADOW_STACK=0 now emit an identical 885-byte root map and an identical __text. The knob keeps its own meaning on its own -- no gcmap, and still observable against the default build. A mode nobody can select is a mode nobody can measure, so this is the prerequisite for the shadow-stack lowering ever being removed rather than merely being switched off in one configuration nobody tests. * docs: changelog fragment for #7340 * docs(gc): record the full-suite RS4GC result and correct the x86-64 mechanism Two corrections and one measurement. The gap suite re-run against RS4GC in-process, two arms per test (shadow-stack control + RS4GC), 479/479: 447 pass->pass, 19 pre-existing diffs unchanged, 13 node_fail, ZERO new regressions, ZERO refusals, ZERO compile failures. Zero refusals is the load-bearing number -- 128 of the 479 tests contain `try {}` and the bridge cannot compile any of them. The earlier soak's "13 regressions, do not flip" was measured against the bridge, before #7329/#7330, on a backend that structurally cannot compile a quarter of the suite. It should not be carried forward. And the x86-64 mechanism was wrong. The workflow comment claimed _Unwind_GetGR(ctx, 7) "does not reliably return the stack pointer". Measured on x86-64 Linux (glibc 2.39, gcc 13.3.0): it SEGFAULTS. RBX, RBP and RIP return correctly; RAX and RSP both SIGSEGV, because libgcc tracks only the columns CFI restores and RSP is derived from the CFA rather than tracked. The fault is in the call itself, so no address validation after it can help -- the previous wording pointed at the wrong fix. Details and a reproducer in #7333. * docs(gc): measure the statepoint binary-size axis — it is root density, not metadata The plan asserted 'closing that axis needs fewer roots, not a tighter encoding' on the strength of one app measurement. Measured directly with two 2000-function programs: root-free functions +0 bytes (no map emitted, text identical) root-dense functions +4,330,592 B (97% __text, 21% gcmap) So statepoints carry NO fixed cost -- a function with nothing live across a safepoint pays nothing -- and the growth is the per-root relocation sequence, not the map. #7314's compact map fully answered the metadata objection, but metadata was never the dominant term at scale. Runtime on the same probes, quiet host, median of 5: statepoints 1-2% faster, every probe neutral or faster. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…7348) Perry carried two statepoint backends. The explicit bridge rewrote Perry's own IR text into gc.statepoint calls with hand-emitted relocations; RS4GC retypes root allocas and lets LLVM's RewriteStatepointsForGC insert every statepoint and relocation itself. They were never peers. RS4GC does strictly more: the bridge cannot root an invoke, so since #7330 it refused try-carrying functions and CI skipped 09_try_catch_roots on that arm. A mode that cannot compile what its sibling compiles, kept beside it with its own emitter, parser and knob, is the permanent hybrid this project keeps paying for. The bridge was also RS4GC's fallback — a bail in the recognizer silently downgraded the function to it. Measured first: 1,574 functions across test-drizzle-pg (1,543) and the probes (31) all lowered as rs4gc, none fell back. A fallback nothing takes is an untested configuration, so a bail is now a hard failure naming the function rather than a silent downgrade. Deleted with it, because only the bridge used them: the CFG root-liveness analysis (RS4GC gets liveness from LLVM's SSA form), the direct-call parser and statepoint emitter, PreciseRootBackend, and the PERRY_STATEPOINTS knob. PERRY_RS4GC=1 is the single switch; native_stack_roots_enabled() is now just rs4gc_enabled(). One fewer GC knob is one less kill-policy debt. Net -1,216 lines. The default shadow-stack path is untouched. Verified: 10/10 probes byte-match the oracle on the sole backend under forced evacuation with the verifying walker, 10/10 on the default arm, drizzle still builds, 593 codegen unit tests pass. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7327.
The defect
The bridge's line classifier matched only
call:An
invoketherefore skipped both the statepoint conversion and the fail-closedpanic!below it, and passed through as an ordinary line.Since #7302 moved exception lowering to
invoke/landingpad, that is every call inside atry. Measured on one program: 58 invokes, 0 carryinggc.statepoint, with allocating callees among them —js_object_alloc_class_inline_keys,js_array_push_f64,js_native_call_method_by_id. Those frames had no roots at all.The instrument was blind to it too
--statepoint-reportcounts only lines it recognises, so "0 parser fallbacks" said nothing about any call inside atry— including in #7314's headline census (23,301 safepoints, 0 fallbacks). Invokes are now counted, so the census stops overstating its own coverage.Why refuse rather than implement
Forming a statepoint from an invoke is real work: the statepoint must itself become an
invoke, withgc.resultand the relocates in the normal successor. RS4GC already does this correctly.Until the bridge does, refusing is the same fail-closed rule the plain stack-map fallback was deleted for in #7314 — "a loud compile failure beats silent heap corruption." The refusal is scoped: it fires only when the invoke actually has live roots across it and the callee is neither an
llvm.*intrinsic nor in the audited cannot-collect table.Verified
trystill compiles under the bridge — the refusal does not over-fire.precise_rootstests pass; file-size, GC store-site, addr-class gates green;cargo fmtclean.What this exposes — worth reading
There is currently no working statepoint path for try-carrying code on a default toolchain. RS4GC handles invokes, but on this machine it fails with Apple clang 21:
and needs
PERRY_LLVM_CLANGpointed at a version-matched LLVM 22. Verified pre-existing — it fails identically with an unmodified binary built before this change. The diagnostic says so rather than sending people to a backend that will not build for them.That gap matters for the shadow-stack removal plan: exceptions are not an edge case, so either the bridge learns invokes or RS4GC becomes reliably usable.
Not verified
Whether this clears any of the 13 gap-suite regressions the statepoint soak found — it converts them from silent wrong answers into compile refusals, which is an improvement in kind but not a pass. Measuring that needs the bridge to actually support invokes.
Summary by CodeRabbit
Bug Fixes
invokeinstructions during statepoint processing.Documentation