fix(compiler): reach the nesting limit instead of overflowing on the way to it - #1051
Open
Guikingone wants to merge 5 commits into
Open
Guikingone wants to merge 5 commits into
Guikingone wants to merge 5 commits into
Conversation
Greptile SummaryThis PR prevents valid, deeply nested PHP source from overflowing the compiler stack before reaching the existing nesting limit.
Confidence Score: 5/5The PR appears safe to merge; the previously reported stack-safety and regression-coverage gaps are resolved without a new actionable issue. The compiler now supplies sufficient stack at both driver and recursive-phase boundaries, and the focused regressions cover binary compilation, the complete in-process pipeline, standalone phase invocation, and deep-value lifetime behavior. All previous findings were either fully fixed and conceded or manually resolved, and no outstanding merge-safety failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PHP source] --> B[Parser nesting-limit check]
B -->|Above 1024| C[Compiler depth diagnostic]
B -->|Within limit| D[Phase entry]
D --> E{Enough stack headroom?}
E -->|Yes| F[Reuse current compiler stack]
E -->|No| G[Reserve compiler stack mapping]
F --> H[Recursive compiler phase]
G --> H
H --> I[Next phase or compiled output]
Reviews (6): Last reviewed commit: "docs(compiler): correct the stack sizes ..." | Re-trigger Greptile |
…way to it
A deeply nested array literal aborted the compiler process:
$ python3 -c "d=200; print('<?php'); print('\\$a = ' + '['*d + '1' + ']'*d + ';')" > deep.php
$ elephc deep.php
thread 'main' has overflowed its stack
fatal runtime error: stack overflow, aborting
No diagnostic, no file, no position -- on source PHP itself compiles and runs.
## The limit already existed
`reject_excessive_nesting` caps bracket depth at `MAX_COMPILER_NESTING = 1024` and
reports "maximum compiler nesting depth exceeded". The cap was unreachable: every
recursive AST pass costs one frame per level -- the parser, `fold_expr`, the
magic-constant walker, `Checker::infer_type`, the optimizer's rewriters, EIR lowering
-- and the default 8 MiB main stack ran out around 140.
So the guard written to make hostile nesting a diagnostic could only ever fire for
input the compiler had already survived. Measured before:
depth 130 compiles
depth 200 STACK OVERFLOW <- abort
depth 1000 STACK OVERFLOW <- abort
depth 1100 "maximum compiler nesting depth exceeded"
## The fix is the stack, not the limit
Lowering the cap to what the stack survives would reject valid PHP: php-src compiles
1000 levels without complaint, and 130 is not a defensible line. The compiler now
runs on a thread sized against its OWN limit -- `COMPILER_STACK_BYTES`, 256 MiB,
reserved rather than committed, so an ordinary compile faults in only the pages its
depth actually touches.
A thread rather than a guard per pass because the passes are many and the list grows:
one place to size, and a pass added later inherits it.
Measured after:
depth 200 compiles, prints 1
depth 1000 compiles, prints 1
depth 1024 compiles, prints 1
depth 1100 "maximum compiler nesting depth exceeded"
Both depths were run and produce host PHP 8.5.10's answer.
## The in-process path needs its own reach
An embedder calling the crate -- and the test harness -- gets whatever stack its own
thread has, not the compiler binary's. The three passes the overflow actually landed
in therefore also grow their own stack through `parser::grow_stack_for_recursion`,
which is the parser's existing `stacker::maybe_grow` budget lifted into a shared
helper so both sides use one number.
Each pass was found by backtrace rather than guessed: `fold_expr` at 200, the
magic-constant `walk_expr` at 500, and `Checker::infer_type` behind them.
## Verified
`nesting_below_the_limit_compiles_and_runs` compiles a 1000-level literal through the
BINARY and runs it, asserting both that stderr mentions no stack overflow and that the
program prints `1`. The existing `deeply_nested_source_reports_compiler_depth_limit`
still gets its diagnostic at 20_000 levels, so the two ends of the limit are pinned
together.
`test_deeply_nested_literal_type_checks_in_process` type-checks 1024 levels IN
PROCESS, which is what pins the per-pass guards. It was confirmed load-bearing by
removing them: the test process aborts with `SIGABRT` rather than failing an
assertion, which is precisely the failure mode worth a regression test.
`cargo test --test codegen_tests -- codegen::arrays codegen::control_flow` passes, as
do 1680 lib tests, 1535 error tests, 398 parser tests and all 6 compiler security
limit tests.
## Docs
`docs/internals/the-parser.md` already documented the cap; it now also says why the
cap needed the stack to be reachable, and names both mechanisms. No `docs/php/` or
`examples/` change: nothing about what PHP code MEANS has changed, and an example
would have to be a 1024-level literal.
Fixes #686
Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
… gets
Review found the first cut short in both directions, and both were right.
## The fallback re-armed the bug
`run_on_compiler_stack` spawned a 256 MiB thread and, if the spawn failed, ran the
compiler inline on whatever stack the caller had. That is the exact configuration the
commit set out to eliminate, reached silently: a machine that cannot spawn the thread
gets the abort back with no indication why.
## The in-process claim was not tested
`test_deeply_nested_literal_type_checks_in_process` called `check_source`, which stops
after type checking. A full compile goes on through constant propagation, control-flow
pruning and normalization, dead-code elimination, reachability and EIR lowering, every
one of which recurses once per nesting level. The test could not have caught them.
Replacing it with a real `compile_and_run` proved it: the test process aborted with
`SIGABRT` inside `propagate_expr`.
## One budget, no thread
`compiler_stack::with_compiler_stack` replaces the worker thread with
`stacker::maybe_grow`: a memory mapping obtained on demand, reserved and not
committed, exactly as the thread stack was. The mapping is what makes the rest work.
* It has no spawn, so there is no failure path to fall back from. The review's
branch is gone rather than patched.
* It is generic over the return type and needs no `Send`, so the pipeline can run
inside it and hand back an AST, a `CheckResult`, a `Module`. A thread could not:
that is why the first cut could only wrap `main`, and why the in-process path had
to be left to per-pass guards.
* A panic unwinds through the caller normally instead of being caught and resumed.
* It is public, so it is the same budget on both sides rather than a number the
binary has and an embedder has to rediscover.
`main` wraps `main_inner` in it. The codegen harness wraps the driver that hand-rolls
the pipeline, because that function IS the embedder: it calls the crate's phases one
by one off a libtest worker thread, the way an external consumer would.
## Verified
`test_deeply_nested_literal_compiles_and_runs_in_process` compiles AND RUNS 1024
levels through the whole in-process pipeline and asserts the program prints `1`.
It passes with no `RUST_MIN_STACK` at all -- on the default libtest worker stack,
which is the small-stack embedder the review was worried about, and which the old
32 MiB-via-`RUST_MIN_STACK` arrangement was hiding.
Confirmed load-bearing by making `with_compiler_stack` call its body directly:
thread '...test_deeply_nested_literal_compiles_and_runs_in_process' has overflowed its stack
fatal runtime error: stack overflow, aborting
(signal: 6, SIGABRT)
`propagate_expr` -- the pass the review named -- grows its own stack too, beside the
guards already on the parser, the folder, the magic-constant walker, the checker and
`expr_invalidation`. That covers a single phase called on its own, outside any
wrapper; `with_compiler_stack` covers a whole run including the passes with no guard
of their own.
All 6 compiler security limit tests pass, including
`nesting_below_the_limit_compiles_and_runs`, which drives the BINARY at depth 1000 and
is now documented as the in-process test's twin: one pins the CLI, one pins the
embedding path.
## Docs
`docs/internals/the-parser.md` names `with_compiler_stack`, why it is a mapping rather
than a thread, and states that anything driving the pipeline in-process wraps itself
in it -- not just the binary.
Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
…an reach it
Review: the in-process regression runs through a harness that wraps the whole
pipeline in 256 MiB, so it passes whether or not the per-phase guards are still
there. Correct, and the test it asked for found more than a masked guard.
## What the isolated test showed
`tests/embedder_stack_tests.rs` calls one phase at a time on a 256 KiB thread, a
size it sets itself so the codegen suite's 32 MiB `RUST_MIN_STACK` cannot hide
anything. Written against the previous commit, three of its five fixtures aborted:
* `fold_constants` overflowed in `prelude_prune::usage::scan_expr`, which it
reaches through `superglobals::seed_cli_populated_superglobals` -- a recursive
walker inside the "guarded" phase, with no guard of its own;
* `types::check` and `propagate_constants` overflowed in
`conditional::exprs::rewrite_expr`, another phase entirely.
So the guards did not cover their own phases, and could not: a phase is not one
walker. Guarding every recursive `Expr` walker is not a fix either -- a scan finds
around 180 of them, the next pass added reintroduces the bug, and no test can prove
the absence.
## The budget moves up one level
Each recursive PHASE now wraps itself in `with_compiler_stack`, at the single
function its spellings funnel through: the parser, `conditional::apply`, the
magic-constant substitution, name resolution, constant folding,
`check_types_with_options` (which every `check*` reaches), constant propagation,
the three control-flow passes, reachability pruning and EIR program lowering.
An embedder calling one phase on a small worker thread is safe without knowing any
of this exists. The five per-walker guards and the shared
`parser::grow_stack_for_recursion` are gone: one mechanism, in one place per phase,
instead of a growing list of walkers to remember.
`COMPILER_STACK_HEADROOM` -- half the reservation -- is what keeps that from costing
anything on the normal path. `main` wraps the whole compile, and the phases inside
it then see plenty of headroom and use the run's own stack rather than reserving one
each.
## The driver still wraps, and now for a reason that is written down
Removing the harness's wrapper aborts the 1024-level fixture in `ExprKind::clone`.
That is not a phase: it is what a driver does BETWEEN the phases with an AST that
deep -- moving it, cloning it, dropping it -- recursing through derived `Clone` and
`Drop` code no guard can be placed inside. So `main` and the harness keep their
wrapper, and its doc comment now says that instead of claiming to be what makes the
phases survive.
## Verified
The five isolated fixtures pass at depth 1024 on a 256 KiB thread. Four of them
abort with `has overflowed its stack` when `with_compiler_stack` is reduced to
calling its body directly, which is the load-bearing check.
The fifth, the parser, passes either way at that size -- measured, with brackets and
with parentheses. That is consistent with the issue itself, whose aborts were always
in the passes below, so the fixture is documented as the guard it is rather than as
a reproduction.
`test_deeply_nested_literal_compiles_and_runs_in_process` still compiles and runs
1024 levels through the whole pipeline, and all 6 compiler security limit tests still
pass, including the binary-driven one at depth 1000.
`cargo test --test codegen_tests -- codegen::regressions codegen::types
codegen::optimizer` passes (1022), as do 1915 unit tests and 1534 error tests.
## Docs
`docs/internals/the-parser.md` now separates the two: every recursive phase carries
its own budget and names them, and a driver wraps itself for the AST handling between
phases, with the `ExprKind::clone` abort as the reason.
Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
…der stack
Review: `on_a_small_embedder_stack` moved each phase's result out through `join()`,
so a 1024-level `Program` was DROPPED on the parent test thread rather than the
256 KiB one. Recursive `Drop` is a walk like any other, and the fixtures were not
covering it.
The helper now takes a body that reports a small `String` summary, so every deep value
is built, used and destroyed inside the thread under test. Five fixtures, one
`summarize()` call each.
## What that measured
The drop completes. At the compiler's own nesting limit, destroying a whole `Program`
fits in 256 KiB -- the frames a derived `Drop` needs are far smaller than the frames a
pass needs, and smaller than the ones `Clone` needs.
That is the useful half of the answer, because it separates two cases a driver was
being asked to treat the same way:
* an embedder that merely HOLDS a phase's result and lets it fall out of scope is
fine on an ordinary worker thread;
* an embedder that CLONES it is the case `with_compiler_stack` exists for -- removing
the codegen harness's wrapper still aborts the 1024-level fixture in
`ExprKind::clone`.
`docs/internals/the-parser.md` records both, as a measurement rather than a caution.
## Verified
All five fixtures pass with the deep values now dying on the small thread, and four of
them still abort with `has overflowed its stack` when `with_compiler_stack` is reduced
to calling its body directly -- so the change strengthened the fixtures without
weakening what they pin. The fifth is the parser guard, which passes either way at this
size and is documented as such.
Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
… name
Review found three stale references left by the phase-level rewrite:
* `docs/internals/the-parser.md` and `tests/codegen/support/compiler.rs` both said
the standalone fixtures run on a 512 KiB thread; `EMBEDDER_STACK_BYTES` is 256 KiB,
lowered when the parser fixture turned out to survive the larger size either way.
* `tests/codegen/regressions/syntax_edges.rs` still named
`parser::grow_stack_for_recursion`, which this PR removed. It now describes what
actually carries the budget: the phase wrapper, plus the harness's own for the AST
handling between phases.
`tests/embedder_stack_tests.rs`'s preamble also grew past what AGENTS.md allows there --
it had carried earlier implementations and prior blind spots. Cut to the standard four
sections, with the mechanism left to the internals page that owns it.
No behaviour change: all five fixtures pass and `cargo fmt --check` is clean.
Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Guikingone
force-pushed
the
fix/686-deep-nesting-diagnostic
branch
from
September 16, 2026 21:00
2827474 to
a9c19e0
Compare
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
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 #686.
A deeply nested array literal aborted the compiler process:
No diagnostic, no file, no position -- on source PHP itself compiles and runs.
The limit already existed
reject_excessive_nestingcaps bracket depth atMAX_COMPILER_NESTING = 1024andreports "maximum compiler nesting depth exceeded". The cap was unreachable: every
recursive AST pass costs one frame per level -- the parser,
fold_expr, themagic-constant walker,
Checker::infer_type, the optimizer's rewriters, EIR lowering-- and the default 8 MiB main stack ran out around 140.
So the guard written to make hostile nesting a diagnostic could only ever fire for
input the compiler had already survived. Measured before:
The fix is the stack, not the limit
Lowering the cap to what the stack survives would reject valid PHP: php-src compiles
1000 levels without complaint, and 130 is not a defensible line. A compile now runs
inside
compiler_stack::with_compiler_stack, which gives itCOMPILER_STACK_BYTES(256 MiB) -- sized against the compiler's OWN limit, reserved rather than committed,
so an ordinary compile faults in only the pages its depth actually touches.
Measured after:
Both depths were run and produce host PHP 8.5.10's answer.
A mapping, not a thread
with_compiler_stackisstacker::maybe_grow: a memory mapping obtained on demand,with the same reserve-don't-commit behaviour a thread stack has. The first cut of this
PR spawned a 256 MiB thread instead, and review found two things wrong with that.
caller's small stack -- silently re-arming the abort this PR exists to remove. A
mapping has no spawn, so the failure path is gone rather than patched.
main. It needsSend + 'staticand gives backnothing, so the in-process path could not use it.
maybe_growis generic over thereturn type and needs no
Send, so the same wrapper works around anything.A panic also unwinds through the caller normally instead of having to be caught and
resumed.
The budget sits on the PHASE
The second review round found the in-process regression could not tell whether the
per-pass guards were still there: it ran through a harness that wrapped the whole
pipeline. The isolated test it asked for -- one phase at a time, on a 256 KiB
thread, a size the fixtures set themselves so
RUST_MIN_STACKcannot hide anything --found that they were not enough anyway:
fold_constantsoverflowed inprelude_prune::usage::scan_expr, reached throughsuperglobals::seed_cli_populated_superglobals-- a recursive walker inside the"guarded" phase, with no guard of its own;
types::checkandpropagate_constantsoverflowed inconditional::exprs::rewrite_expr, another phase entirely.A phase is not one walker, and guarding every recursive
Exprwalker is not the fix:a scan finds around 180 of them, the next pass added reintroduces the bug, and no test
can prove the absence.
So each recursive PHASE now wraps itself, at the single function its spellings funnel
through: the parser,
conditional::apply, magic-constant substitution, nameresolution, constant folding,
check_types_with_options(which everycheck*reaches), constant propagation, the three control-flow passes, reachability pruning
and EIR program lowering. An embedder calling one phase on a small worker thread is
safe without knowing this exists. The per-walker guards and the shared
parser::grow_stack_for_recursionare gone.COMPILER_STACK_HEADROOM-- half the reservation -- keeps that free on the normalpath:
mainwraps the whole compile, so the phases inside it see plenty of headroomand use the run's own stack instead of reserving one each.
A driver wraps too, for what it does BETWEEN phases
mainand the codegen harness keep their own wrapper, and the reason is now writtendown rather than assumed: removing the harness's aborts the 1024-level fixture in
ExprKind::clone. That is not a phase. It is what a driver does between phases withan AST that deep -- moving it, cloning it, dropping it -- recursing through derived
CloneandDropcode no guard can be placed inside.Verified
nesting_below_the_limit_compiles_and_runscompiles a 1000-level literal through theBINARY and runs it, asserting both that stderr mentions no stack overflow and that the
program prints
1. The existingdeeply_nested_source_reports_compiler_depth_limitstill gets its diagnostic at 20_000 levels, so the two ends of the limit are pinned
together.
test_deeply_nested_literal_compiles_and_runs_in_processcompiles AND RUNS 1024 levelsthrough the whole in-process pipeline -- not a type check, which is what the first cut
pinned and which stops before six recursive passes.
tests/embedder_stack_tests.rspins the phases one at a time on the small thread. Fourof its five fixtures abort with
has overflowed its stackwhenwith_compiler_stackisreduced to calling its body directly. The fifth, the parser, passes either way at that
size -- measured, with brackets and with parentheses -- which matches the issue, whose
aborts were always in the passes below; it is documented as the guard it is rather than
as a reproduction.
cargo test --test codegen_tests -- codegen::regressions codegen::types codegen::optimizerpasses (1022), as do 1915 unit tests, 1534 error tests and all 6compiler security limit tests.
Docs
docs/internals/the-parser.mdalready documented the cap; it now also says why the capneeded the stack to be reachable, that every recursive phase carries its own budget and
which ones they are, and that a driver wraps itself for the AST handling between phases
-- with the
ExprKind::cloneabort as the reason. Nodocs/php/orexamples/change:nothing about what PHP code MEANS has changed, and an example would have to be a
1024-level literal.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr