Guard against native stack overflow in deep non-tail recursion - #57
Merged
Conversation
Any genuinely non-tail-recursive user function -- closure or not, letrec or not, compiled or interpreted -- makes a real, unavoidable C++ recursive call per logical call (there is no trampoline for a call whose result still needs further work after it returns, e.g. `1 + f(n-1)`). With no depth guard at all, this silently overflowed the native stack and segfaulted the whole process a few thousand levels deep, with no exception to catch. Confirmed present for a plain, closure-free `function f(n) = n<=0 ? 0 : 1+f(n-1);`, on both paths -- discovered while verifying the previous PR's ternary-wrapped letrec fix at scale, but entirely unrelated to it: this test file's own header comment already flagged the compiled path's exposure here as a known gap when TCO Phase B first landed, just never closed. Adds a conservative depth check to evalUserFunctionCore, the one choke point every genuine (non-trampolined) function call already passes through on both paths -- throwing a controlled EvalError well before the point this evaluator's own C++ recursion has been measured unsafe at, rather than a precisely-calibrated limit (a real deployment may run this on a smaller-stack thread than the 8MB desktop main thread this was measured against, e.g. BelfrySCAD's own debug-session worker thread). Trampolined tail recursion (reduce()/accumulate()/ while(), the deep self/mutual-recursion tests from the last several releases) is entirely unaffected, confirmed by the existing 500,000-deep tail-recursion test passing unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kMaxUserCallDepth=1000 was calibrated against this evaluator's own measured-safe threshold on an 8MB macOS/Linux desktop main-thread stack (a few thousand levels) with a stated safety margin -- CI's windows-latest runner still segfaulted on the new tests at that depth, meaning its actual usable stack (and/or MSVC's own per-frame cost) is substantially smaller than assumed. Lowers the cap to 200 -- a real, precisely-calibrated-per-platform figure isn't available without probing actual remaining stack space at runtime (out of scope for this fix), so this trades away some deep-non-tail-recursion headroom for actual cross-platform safety, matching the reasoning already laid out in the guard's own doc comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Temporary commit: even a cap of 200 still segfaulted on CI's windows-latest runner, with no way to reproduce or bisect locally (no Windows access, and this evaluator's own measured-safe native recursion depth on ordinary macOS/Linux desktop hardware is in the thousands). Bumps kMaxUserCallDepth to 5000 (comfortably past every locally-measured-safe threshold) and adds a diagnostic test that tries increasing depths in a loop, printing (and flushing) success before each one -- so wherever this run actually crashes on windows-latest, the CI log itself shows the exact last-safe depth directly, without needing local reproduction. Temporarily disables the two tests that assert the guard actually fires, since kMaxUserCallDepth=5000 now exceeds even this evaluator's own measured-safe depth on ordinary hardware, crashing before the guard gets a chance to run at all. Follow-up commit sets kMaxUserCallDepth to a real value based on this run's own log and restores the disabled tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Round 1 found plain (non-throwing) recursion safe to depth 300 on windows-latest CI, but kMaxUserCallDepth=200 (a THROW from depth 200, unwinding back out through 200 nested evalUserFunctionCore try/catch frames) still segfaulted there -- meaning throwing+unwinding through N nested frames costs meaningfully more native stack than simply being N frames deep without throwing. This measures that cost directly (assert(false) throws/unwinds through the exact same nested- try/catch shape Evaluator::error() does) at finer granularity around the round-1 boundary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two rounds of CI diagnostics found the real numbers: plain (non- throwing) recursion is safe to depth 300 on windows-latest CI, but throwing+unwinding back out through that many nested evalUserFunctionCore try/catch frames -- what this guard actually does -- crashed by depth 100 (80 was the last depth that survived). 50 leaves real margin below that empirically-found figure for whatever's already on the stack from a call's own caller, and for a smaller stack still that this evaluator has no way to probe for directly (e.g. BelfrySCAD's own debug-session worker thread). Removes both temporary diagnostic tests and the now-unused <iostream> include; restores the two "hits the guard" tests (no longer disabled) with depths appropriate to the final cap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.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.
Summary
1 + f(n-1)). With no depth guard at all, this silently overflowed the native stack and segfaulted the whole process, with no exception to catch.function f(n) = n<=0 ? 0 : 1+f(n-1);, on both the interpreted and compiled paths — discovered while verifying Extend letrec pre-declare to ternary-wrapped self/mutual references #56's ternary-wrapped letrec fix at scale, but entirely unrelated to it (test_tail_calls.cpp's own header comment already flagged the compiled path's exposure here as a known gap when tail-call-optimization Phase B first landed; never actually closed until now).evalUserFunctionCore, the one choke point every genuine (non-trampolined) function call already passes through on both paths.windows-latestrunner — and, more surprisingly, actually throwing from depth N and unwinding back out through N nestedevalUserFunctionCoretry/catch frames costs meaningfully more native stack than simply being that deep without throwing (300 survived plain recursion on Windows; throwing+unwinding through as few as 100 nested frames did not — 80 was the last depth that survived). Final cap: 50, with real margin below that 80 figure for whatever's already on the stack from a caller, and for a smaller stack still that this evaluator can't probe for directly (e.g. BelfrySCAD's own debug-session worker thread).evalUserFunctionCore.Test plan
DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpreted/...Compiled,ShallowNonTailRecursionStillWorksUnderTheDepthGuard.ERROR: Recursion too deep...instead of a segfault) on both paths; legitimate shallow non-tail recursion (fact(10),fact(30)) still works correctly.🤖 Generated with Claude Code