revert: back out #8464's blanket C-unwind conversion (measured +20 crashes, gc-stress) - #8484
Merged
Merged
Conversation
added 2 commits
August 20, 2026 15:29
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe closure dispatch family reverts from ChangesClosure dispatch ABI revert
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized revert restores the prior unwind behavior, with no actionable merge-blocking risk remaining beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…e abort Two candidate fixes for #8479 (#8464's dispatch family, #8480's bun:ffi thunks) both left tier1_every_ffi_type_against_test_dylib aborting on Linux with 'panic in a function that cannot unwind', and the message does not name the frame that carries the nounwind guard. macOS does not reproduce it. Carried on this branch because it already touches the suite, so the same e2e-scoped run that measures the revert also captures the backtrace.
This was referenced Aug 20, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Aug 20, 2026
The runtime is built panic=abort, and a JS throw travels as a raw Itanium _Unwind_Exception that must step THROUGH runtime Rust frames untouched. crate::eh says so, and the workspace Cargo.toml states the requirement outright: 'no RFC-2945 abort-on-unwind guards (which panic=unwind plants in every extern "C" helper and which a JS throw crossing a helper frame would trip)'. Marking a pass-through frame extern "C-unwind" in a panic=abort crate does NOT make it unwind-capable — it makes rustc wrap the call in an abort-on-unwind landing pad, i.e. it INSTALLS exactly the guard the design forbids. That inverts the intent of the last three attempts: * #8416 converted js_closure_call1 + js_native_call_value and broke tier1_every_ffi_type_against_test_dylib (measured: the test passes at 55a5b5a, aborts from 526e0b5 onward); * #8480 converted bun_ffi's sym/close thunks — another guard on the very path the throw takes, so it could not help; * #8464 converted ~40 dispatch functions and cost +20 gap crashes plus a red gc-stress (reverted in #8484; the post-revert sweep measured 36 -> 14). Put all four pass-through frames back to extern "C". Throw ORIGINATORS (js_throw and friends, which diverge and where the raise begins) are left alone — different case, no evidence they are implicated. Local (macOS) bun_ffi_stage1 is 3/3, but macOS has never reproduced this abort under any variant, so the Linux e2e-scoped run of this suite is the verdict. It must report before this merges. Refs #8479
proggeramlug
pushed a commit
that referenced
this pull request
Aug 20, 2026
…wned_typed_views The native-ABI proof gate fails on this one workload: three loops now miss vectorization for 'control_flow', which is not in its allowed list, and compiler-output-regression is a required full-suite-gate job. Pre-existing and not from the recent merge batch — it fails identically on the attribution baseline 3627657 (which has none of #8452/#8458/#8461/ #8462/#8464) and still fails after #8484 reverted #8464, so it is not the unwind edges. Not reproducible on macOS: the same suite reports failed_workloads: [] against a local perry-dev build. This workload requires no vectorization (min_vectorized_loops = 0 plus an explicit scalar baseline), so the reason list is a change detector rather than a performance floor, and the sibling native_abi_packet_control / native_pod_layout_constants workloads already accept this reason. Widening it trades a precise detector for an unblocked release. #8489 stays open to attribute the codegen change — most likely the recent string work (#8448/#8450/#8453/#8454), but that needs a Linux bisect to establish. Refs #8489
proggeramlug
added a commit
that referenced
this pull request
Aug 20, 2026
…wned_typed_views (#8490) The native-ABI proof gate fails on this one workload: three loops now miss vectorization for 'control_flow', which is not in its allowed list, and compiler-output-regression is a required full-suite-gate job. Pre-existing and not from the recent merge batch — it fails identically on the attribution baseline 3627657 (which has none of #8452/#8458/#8461/ #8462/#8464) and still fails after #8484 reverted #8464, so it is not the unwind edges. Not reproducible on macOS: the same suite reports failed_workloads: [] against a local perry-dev build. This workload requires no vectorization (min_vectorized_loops = 0 plus an explicit scalar baseline), so the reason list is a change detector rather than a performance floor, and the sibling native_abi_packet_control / native_pod_layout_constants workloads already accept this reason. Widening it trades a precise detector for an unblocked release. #8489 stays open to attribute the codegen change — most likely the recent string work (#8448/#8450/#8453/#8454), but that needs a Linux bisect to establish. Refs #8489 Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 20, 2026
…#8479) (#8488) * fix(runtime): remove the C-unwind abort guards from the JS throw path The runtime is built panic=abort, and a JS throw travels as a raw Itanium _Unwind_Exception that must step THROUGH runtime Rust frames untouched. crate::eh says so, and the workspace Cargo.toml states the requirement outright: 'no RFC-2945 abort-on-unwind guards (which panic=unwind plants in every extern "C" helper and which a JS throw crossing a helper frame would trip)'. Marking a pass-through frame extern "C-unwind" in a panic=abort crate does NOT make it unwind-capable — it makes rustc wrap the call in an abort-on-unwind landing pad, i.e. it INSTALLS exactly the guard the design forbids. That inverts the intent of the last three attempts: * #8416 converted js_closure_call1 + js_native_call_value and broke tier1_every_ffi_type_against_test_dylib (measured: the test passes at 55a5b5a, aborts from 526e0b5 onward); * #8480 converted bun_ffi's sym/close thunks — another guard on the very path the throw takes, so it could not help; * #8464 converted ~40 dispatch functions and cost +20 gap crashes plus a red gc-stress (reverted in #8484; the post-revert sweep measured 36 -> 14). Put all four pass-through frames back to extern "C". Throw ORIGINATORS (js_throw and friends, which diverge and where the raise begins) are left alone — different case, no evidence they are implicated. Local (macOS) bun_ffi_stage1 is 3/3, but macOS has never reproduced this abort under any variant, so the Linux e2e-scoped run of this suite is the verdict. It must report before this merges. Refs #8479 * test(ffi): document the throw-transport invariant on the FFI canary Also puts this file back in the diff so e2e-scoped actually runs the suite: PR #8488's first run reported GREEN having skipped it entirely ('Run scoped integration suites: skipped') because the scope computation found no test file changed — a pass that proves nothing. * test: put the #8416 counterpart suite in scope for this change cargo-test on PR #8488 passed WITHOUT running this suite — the PR-tier job is scoped and the file was not in the diff (0 mentions in its log). Since this change undoes the mechanism #8416 used to fix exactly this test, it has to be verified on Linux too, not assumed. * fix(ci): link a panic=abort runtime in cargo-test and e2e-scoped panic is a PROFILE-level setting: only release/dist/perry-dev set panic="abort". Both jobs built the static runtime archives with a bare `cargo build`, i.e. DEBUG + panic="unwind" — and under panic="unwind" rustc plants an RFC-2945 abort guard in every extern "C" helper, which a JS throw crossing that helper trips. So the compiled child binaries linked a runtime whose unwind semantics are the OPPOSITE of the one Perry ships, and the two throw-transport canaries could not both pass in that environment. That is the whack-a-mole behind #8416 (extern "C-unwind" — right for the debug runtime, wrong for the shipped one), #8464 (the same generalised: +20 gap crashes, gc-stress) and #8480 (more of it). Measured: against a panic=abort runtime with those conversions removed, BOTH tier1_every_ffi_type_against_test_dylib and function_apply_with_runtime_args_defers_to_a_located_aot_error pass — the first also verified on Linux e2e-scoped. Refs #8479 * fix(test): link a panic=abort runtime in the function_apply eval-surface suite The test hardcoded target/debug as its runtime dir and passed it through PERRY_RUNTIME_DIR, so it always linked a DEBUG, panic="unwind" archive — overriding even the job-level PERRY_RUNTIME_DIR. panic is a profile-level setting; only release/dist/perry-dev set panic="abort", which is what Perry actually ships. Under panic="unwind" rustc plants an RFC-2945 abort guard in every extern "C" helper, and a JS throw crossing one aborts. So this suite was asserting unwind semantics that do not exist in production, while bun_ffi_stage1 (which sets no PERRY_RUNTIME_DIR and gets a release runtime) asserted the real ones — making the two mutually unsatisfiable and driving #8416 -> #8464 -> #8480, each repairing one by breaking the other. Build the -static wrappers with --release and point runtime_dir() at target/release. Verified locally: this suite passes with no env overrides (334s), and bun_ffi_stage1 passes too — both against panic=abort. Refs #8479 --------- 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.
Reverts #8464. I wrote that PR and recommended merging it; the measurement says it made
mainworse and did not fix what it targeted.Measurement
Three sweeps, same 3-shard fast-mode gap suite:
gc-stress526e0b5023627657c715a30d7f6Runs: 32328533570 · 32368807258 · 32353180323. The middle row is the control I dispatched specifically to separate my changes from the string work — it rules the string PRs out for these two symptoms. (
compiler-output-regressionandcargo-testare red in the control too, so those are not from my batch.)Why this change in particular
#8464 converted ~40 dispatch functions to
extern "C-unwind". That is not an annotation-only change: LLVM turnscallintoinvokewith unwind edges, and unwinding then genuinely runs through frames that were previously guaranteed never to unwind — frames holding mutexes, raw pointers and GC state, whose cleanup paths had never executed. That is a coherent mechanism for all three symptoms at once: +20 crashes concentrated in throw/validation tests, a newly-redgc-stress, andnative_owned_typed_viewsnewly reportingcontrol_flowas a missed-vectorization reason (unwind edges are control flow).And it bought nothing: #8479 still reproduces on Linux with #8464 in place, so the abort it was written for is elsewhere.
How this got merged
Worth recording so the same hole doesn't repeat. #8464's own
e2e-scopedjob — the Linux run ofbun_ffi_stage1, which the PR deliberately opted into by naming the test file — was cancelled by the merge itself (run 32340796570, job cancelled at 08:52). Merging a PR cancels its in-flight run, so the one check that would have shown the fix didn't work never reported. macOS passes this test with or without any of these changes, so there was no local signal either.Scope
Reverts only #8464. #8480 (the
bun:ffithunk conversion) stays for now: it also failed to fix #8479, but there is no measurement showing it harmful, and it is 17 small stubs rather than the whole dispatch path. If the post-revert sweep does not return to ~16, that is the next thing to look at.cargo test -p perry-runtime --lib closureis green on the reverted tree (64 passed). The real check is the next sweep's regression count.Summary by CodeRabbit
Bug Fixes
Tests