Fix recursion-depth gaps vs. OpenSCAD: tail-call optimization + hard abort on detected recursion (#83) - #102
Conversation
…ement Issue #83: kMaxCallDepth was a conservative guess with only a vague "800-1000" citation behind it. Re-measured by driving unguarded recursion inside a 1 MiB-stack thread (matching the documented MSVC-stack constraint) until it overflows, for both a light numeric-recursion shape (~1180) and the heavier recursive-list-building idiom the issue calls out (~680) — the new cap keeps >2x margin under the worse of the two while allowing meaningfully deeper legitimate recursion than before.
Raising kMaxCallDepth alone couldn't close #83: fetched the actual OpenSCAD test files it was filed against (recursion-test-function, tail-recursion-tests, issue3118-recur-limit — not present in this repo) and ran them against a real OpenSCAD 2021.01 oracle plus ChiselCAD's scad_dump tool. tail-recursion-tests.scad expects tail-recursive functions to reach depths of 2,000-50,000, which no native-stack cap could ever survive — real OpenSCAD passes this via genuine tail-call optimization (confirmed by reading FunctionCall::evaluate's trampoline loop directly in openscad/openscad's src/core/Expression.cc), not a generous recursion limit. Interpreter::evalFunctionBody() mirrors that design: a named function's body is evaluated via a loop that unwraps ternary branches and let bindings in place and, on hitting a call in tail position (self or mutual), rebinds params and loops into the callee's body directly instead of recursing — no new native stack frame per tail hop, bounded only by kMaxTailHops (1,000,000, matching upstream's own cap) rather than kMaxCallDepth. Non-tail recursion (e.g. `a + f(a-1)`) is untouched, still going through ordinary evaluate() and kMaxCallDepth. Verified against the real corpus file: every echo() line in tail-recursion-tests.scad (minus ftail_mixed, which uses assert()/ echo()-as-expression forms ChiselCAD's grammar doesn't parse at all — a separate, unrelated gap) now matches real OpenSCAD's output exactly. Also fixes a distinct bug surfaced while validating this: every real evaluation entry point (CsgEvaluator::evaluate's convenience overload, HeadlessBuild.cpp, and the scad_dump/scad_to_stl test tools) called loadAssignments() before loadFunctions(), so a top-level assignment calling a user-defined function (e.g. `y = f(3);`) always evaluated to undef — a pre-existing quirk a test comment had already flagged but left unfixed. loadFunctions() only registers non-owning pointers with no evaluation, so swapping the order is strictly safe.
The two remaining upstream corpus files from #83 (recursion-test- function.scad, issue3118-recur-limit.scad) both expect an unconditionally- recursive function to abort the entire script with an error, not degrade to undef and keep going — confirmed against a live OpenSCAD 2021.01 binary: earlier echo()s still print, but the statement whose argument triggers the recursion prints nothing, and every later statement is skipped. CsgEvaluator already has exactly this "abort the rest of the script" mechanism (m_aborted), used today for a failed top-level assert(). Gave Interpreter its own recursionAborted() flag, set at the two existing recursion guards (kMaxCallDepth and evalFunctionBody's kMaxTailHops) instead of just silently returning undef, and made evaluate() itself short-circuit once set. CsgEvaluator::checkRecursionAbort() folds that into its own m_aborted the same way a failed assert() already does: called at evalNode()'s top (covers every subsequent statement generically) and right after echo()/assert()'s own argument evaluation (needed since those two produce a visible side effect in the same statement that would otherwise fire using the aborted undef result). Verified against the real corpus files: both now match real OpenSCAD's behavior (no output for the triggering statement, error diagnostic, empty geometry, earlier statements unaffected). Diagnostic wording is intentionally not byte-exact (missing OpenSCAD's "in file X, line Y" and TRACE lines) — filed as #101, same category as #85's existing diagnostic- wording-parity scope, not attempted here.
particlesector
left a comment
There was a problem hiding this comment.
Reviewed. The TCO trampoline, raised depth cap, and load-order fix all look correct and well-tested. One gap noted inline: the recursion-abort check isn't wired into the general primitive/transform/module argument-evaluation paths, only echo()/assert(), so a recursing call as e.g. a cube() argument still leaks a bogus geometry node into the scene before the abort is caught on the next statement.
Generated by Claude Code
Review feedback on PR #102: checkRecursionAbort() was only called at evalNode()'s top and inline in echo()/assert()'s own argument loops, so a recursion abort discovered while evaluating a *primitive's* own parameter (e.g. `cube(crash())`) wasn't caught until the next evalNode() call — the degenerate leaf built from the resulting undef param still made it into the scene. echo()/assert() needed their own inline checks regardless (they push a side effect during evalModuleCall itself, before evalNode's dispatch even returns), but nothing else funneling through evalNode's std::visit dispatch (primitives, transforms, booleans, module calls, ...) had an equivalent guard. Added one more check right after that dispatch returns: if evaluating this node just tripped the interpreter's recursion guard, discard whatever result was built and return nullptr instead of letting it become part of the scene — matches real OpenSCAD's exception-based unwind, which never produces geometry for the statement that itself triggered the exception.
particlesector
left a comment
There was a problem hiding this comment.
Re-reviewed. Commit 6a168c0 addresses the earlier feedback correctly: checkRecursionAbort() now runs right after evalNode()'s std::visit dispatch returns, before the mods (highlight/root/background) handling, so a recursion abort discovered anywhere during a node's own evaluation — not just child statements — discards that node's result instead of letting a degenerate leaf leak into the scene. Verified the placement is before the mods branches (so an aborted node also can't get pushed into m_rootOnlyNodes/backgroundRoots), and the new regression test (cube(crash()); cube(5); → empty roots, one diagnostic) matches the fix.
No further issues found from this pass.
Generated by Claude Code
Summary
Closes #83. Also files (but does not implement) #101 as an intentionally-deferred follow-up.
kMaxCallDepthfrom 200 to 300 for genuinely non-tail recursion, backed by an empirical stack-overflow measurement (1 MiB-stack thread, GCC 13-O3) rather than the previous vague "800-1000" citation.Interpreter::evalFunctionBody()): a named function's body is evaluated via a trampoline that unwraps ternary branches andletbindings in place and, on hitting a call in tail position (self or mutual recursion), loops into the callee directly instead of recursing — no new native stack frame per hop, bounded by a separate 1,000,000-hop cap (kMaxTailHops) matching real OpenSCAD's own cap exactly. This is what actually closes the gap: raisingkMaxCallDepthalone can never satisfy OpenSCAD's owntail-recursion-tests.scad, which expects tail-recursive functions to reach depths of 2,000-50,000.CsgEvaluator's existingm_abortedmechanism (previously only used for a failed top-levelassert()) so a tripped recursion guard now halts the rest of the script with an Error diagnostic, matching real OpenSCAD's fatal "Recursion detected calling function 'X'" behavior instead of silently degrading toundefand continuing.loadAssignments()beforeloadFunctions(), so a top-level assignment calling a user-defined function (e.g.y = f(3);) always evaluated toundef. A test comment had already flagged this as a known quirk but left it unfixed. The fix is a pure reordering —loadFunctions()only registers non-owning pointers, no evaluation — so it's provably safe.Verification
None of the OpenSCAD test files this issue cites (
recursion-test-function.scad,tail-recursion-tests.scad,issue3118-recur-limit.scad) are checked into this repo. Fetched them directly fromopenscad/openscadand ran them against both a real OpenSCAD 2021.01 binary (apt install openscad) and ChiselCAD'sscad_dumptool for a byte-level diff, rather than guessing:tail-recursion-tests.scad: everyecho()line (minusftail_mixed, which usesassert()/echo()as expression forms ChiselCAD's grammar doesn't parse at all — a separate, unrelated gap) now matches real OpenSCAD's output exactly, including the 50,000-deep cases.recursion-test-function.scad/issue3118-recur-limit.scad: both now match real OpenSCAD's behavior (no output for the triggering statement, error diagnostic, empty geometry, earlier statements' output preserved) — diagnostic wording isn't byte-exact (missing thein file X, line Ysuffix andTRACE:lines), tracked separately as [Low] Recursion-detected hard abort doesn't match OpenSCAD's exact diagnostic wording (ERROR/TRACE lines) #101 since it's cosmetic-only, same category the project already deprioritized in [Low] Missing arity-mismatch / file-not-found diagnostic wording parity with OpenSCAD (cosmetic) #85.Also confirmed the tail-call optimization mirrors real OpenSCAD's actual mechanism, not a guess — read
FunctionCall::evaluate's trampoline loop directly inopenscad/openscad'ssrc/core/Expression.cc.Testing
This environment can't do the full Manifold/vcpkg CMake build, so validation used a standalone build of the language/CSG-evaluator subset (self-contained, no Manifold dependency) via
apt'sopenscad/libglm-dev/catch2packages:CsgEvaluator, and the load-order fix.Test plan
Generated by Claude Code