fix(codegen): keep every alloca in the function entry block - #10550
Closed
proggeramlug wants to merge 2 commits into
Closed
proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
Date setters, Date.UTC, concat/splice/toSpliced/unshift and
Array.prototype.{push,unshift,splice,concat}.call emitted their argument
buffer (or splice's i64 out-parameter) into whatever block was current. An
alloca outside the entry block is a runtime stack bump released only on
return, so each loop iteration consumed stack until the process died with
SIGSEGV (~2^19 iterations at 8 MB). The same pattern in the dynamic
import/require and i18n join slots, new Worker, the V8 interop argument
buffers, the fused push length slot and the namespace populator is fixed too:
all of them now allocate through alloca_entry / alloca_entry_array /
lower_js_args_array.
LlFunction::for_each_final_item, which both backends consume, now refuses any
alloca outside the entry block, so a new call site cannot reintroduce the
class.
proggeramlug
added
the
package-audit
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (18)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe codegen now routes affected allocations to function entry blocks, rejects non-entry allocas during final instruction streaming, and adds compiler and runtime regression coverage for long-running loops. ChangesEntry-block alloca fix
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The allocation migration and invariant enforcement have no identified merge-blocking regression. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.49% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 53 functions across 17 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
proggeramlug
pushed a commit
that referenced
this pull request
Sep 18, 2026
Contributor
Author
|
Landed via merge train #10578 (v0.5.1593). All source commits preserve authorship; merged main matches the validated train exactly. |
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.
Summary
Several expression lowerings emitted their argument buffer (
alloca [N x double]) or out-parameter (alloca i64) into whatever block was current instead of the function's entry block. LLVM lowers a non-entryallocato a runtime stack-pointer bump that is released only when the function returns. So a loop callingd.setTime(i),Date.UTC(...),arr.concat(x),arr.splice(...),arr.toSpliced(...),arr.unshift(x)orArray.prototype.{push,unshift,splice,concat}.call(...)consumed stack on every iteration. It died with SIGSEGV after about 2^19 iterations at the default 8 MB stack, at a point that moved withulimit -s. date-fnsaddMinutesin a loop crashed the same way, because the cross-module inliner copies itssetTimeinto the caller's loop.This PR moves every such site to the existing entry-block helpers and adds a check where function bodies are finalized, so the class cannot come back one call site at a time.
Root cause
#167 added
LlFunction::alloca_entry_arrayfor the native-method dispatch buffers, but sibling lowerings kept doingblk.emit_raw(format!("{} = alloca [{} x double]", …))orblk.alloca(I64)in the current block. Line numbers are at 7661bc0.Confirmed crashers (each one SIGSEGVs in a loop on the baseline):
crates/perry-codegen/src/expr/os_uri_dates.rs:51:lower_date_setter, used by everyDate.prototype.set*crates/perry-codegen/src/expr/misc_methods.rs:198:Date.UTCcrates/perry-codegen/src/expr/os_uri_dates.rs:362:toSplicedcrates/perry-codegen/src/lower_array_method.rs:313(concat),:910(unshift),:972+:989(spliceout-slot + items). The issue listed:910/:972/:989as unreached;holder.arr.unshift(x)/holder.arr.splice(0, 1, x)on a class field reach them and crash.crates/perry-codegen/src/expr/instance_misc1.rs:1431+:1450:Expr::ArraySpliceout-slot + itemscrates/perry-codegen/src/expr/logical_collections.rs:853+:888:Array.prototype.*.callSame pattern, also converted:
expr/dyn_extern_i18n.rs:106,:384,:686,:1164: multi-target dynamicrequire/import()result slots, i18n row and plural join slotsexpr/worker_new.rs:52,:78:new Workerexpr/v8_interop.rs:118,:221: V8-interop argument bufferslower_call/native/native_instance_branch.rs:361: fusedpushlength slotcodegen/helpers.rs:1395-1401: the namespace populator's four buffers (runs once per module init, converted for uniformity)Fix
LlFunction::alloca_entry/alloca_entry_array, or throughlower_js_args_array, which already hoists its buffer and returnsnull/0for no arguments. Each buffer is still filled completely immediately before its call, as before, so no stale value survives across iterations. None of these slots is a GC root; their store/load positions are unchanged.crates/perry-codegen/src/function/entry_allocas.rs):LlFunction::for_each_final_itemis the one funnel both the textual and the native backends consume, and it now refuses anyallocathat lands outside the entry block. That covers typedLlInst::Alloca, raw text, allocas inside multi-line raw payloads, and allocas after an inline invoke-EH label in block 0. The panic names the function, block and instruction and points atalloca_entry.LlBlock::allocastays legal only while block 0 is current, which the parameter prologues rely on; its doc comment now says so.Tests
test-files/test_gap_10463_entry_block_allocas.ts(+ helpertest-files/_helpers/add_minutes_10463.ts). It runs each construct in its own loop, each needing at least 19 MB of stack under the old lowering: Date setters,Date.UTC,toSpliced,concat, localsplice, fieldunshift/splice,Array.prototype.*.call, and an imported date-fns-shapedaddMinutes.PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter test_gap_10463). Run separately, each of the 8 sections SIGSEGVs on its own at the default 8 MB stack.expr::entry_block_alloca_tests(cargo-test visible): a HIR corpus of every confirmed construct inside a counted loop, compiled withcompile_module. Each case first asserts its runtime entry (js_date_apply_setter,js_date_utc,js_array_to_spliced,js_array_splice,js_array_concat_variadic,js_array_unshift_variadic,js_arraylike_{push,unshift,splice,concat}) is called from a non-entry block, so the case is not vacuous. It then reads the emitted IR back with its own scanner and asserts noallocais outside any function's entry block. A scanner self-test covers the pre-fix shape.function::entry_allocas::tests: five refusal cases (raw alloca in a loop body, typed alloca in a non-entry block, alloca in a multi-line raw payload, alloca after an inline label in block 0, refusal on the native item stream) and two controls (the helpers hoist out of a loop block; a prologue alloca in block 0 is accepted).lower_date_setterreverted,date_setters_keep_their_argument_buffer_in_the_entry_blockfails through the refusal. With the refusal also disabled, it fails through the independent scanner, and the fiveshould_panictests fail.perry compile --no-link --trace llvmover all 1659test-files/*.tswith both compilers.test_wasm_addtimed out once on this branch's arm while building the wasm host for the first time; a rerun gives rc=0.python3 scripts/check_test_registration.py: OK.Validation
cargo test --release -p perry-codegen --tests./scripts/run_lint_gates.sh(full, incl. compile tier)Public benchmark evidence freshness(public artifact benchmark inputs changed); the same gate prints the same error in the baseline 7661bc0 checkoutPERRY_SKIP_BUILD=1 ./scripts/run_gap_tests.sh)GAP_EXIT=0, snapshot OK: 814 pass / 6 output mismatches / 0 compile fail / 0 crashed (820 tests incl. the new one). The 6 mismatches are exactly the baseline run's 6 (2159_defineproperty_class_prototype,2514_settracesigint,json_lazy_defineproperty_index,perfhooks_3088_3008_3010_3011,prop_plan_cache_invalidation,v8_2_3680plus). No new failures. An earlier run was discarded: the lint gatescripts/regen_api_docs.shrunscargo build --release -p perryand replaced the binary mid-run with a build using different feature unification. 3 ext-wrapper tests then hit compile failures; all 3 pass individually on the canonical build and in the clean runPerformance (
perf stat -e instructions:u,task-clock, 3 runs each,PERRY_NO_AUTO_OPTIMIZE=1, both arms underulimit -s unlimitedso the baseline can finish)setTime+setUTCMinutes), 2M iterationsa.concat(i), 2MArray.prototype.push.call+splice.callon an array, 2Msplicemiddle insert + head replace, n=60000Instruction counts are identical across runs within ±0.01%. Removing the per-iteration stack growth saves the page faults it caused: the date-setter loop touched 64 MB of stack on the baseline, hence the task-clock drop.
Package check (informational)
date-fns 4.4.0
addMinutes/addDays/startOfDay/differenceInMinutesin a 2M-iteration loop compiles and matches Node on this branch. In the two shapes I tried, the real date-fnsaddMinuteswas not cross-module-inlined on the baseline either, so that program did not crash there. The date-fns-shaped helper in the gap test is inlined and does crash on the baseline.Not verified
import()/require, i18n,new Worker, V8-interop, fused-pushand namespace-populator sites were not crash-tested in a loop. Their conversion is checked by the refusal and by the corpus sweep: the dynamic-import and namespace tests had non-entry allocas on the baseline and have none now.Fixes #10463
Summary by CodeRabbit
Bug Fixes
concat,splice,push,unshift, andtoSpliced.Tests