From ca5edbd8ab5b52af2db9bce79309ea12dcf076f5 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 30 Jul 2026 17:17:02 -0700 Subject: [PATCH 1/5] Guard against native stack overflow in deep non-tail recursion 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 --- include/openscad_cpp_evaluator/evaluator.hpp | 24 ++++++++ pyproject.toml | 2 +- tests/test_tail_calls.cpp | 62 ++++++++++++++++++-- 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index f8584c7..b2f00b2 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -633,6 +633,30 @@ class Evaluator { Value evalUserFunctionCore(const std::string& name, const oscad::ASTNode& declNode, const oscad::Expression& bodyExpr, EvalContext& childCtx, const oscad::Position* callPos, int upvalueParent, F&& computeResult) { + // Guards against a genuinely non-tail-recursive user function -- + // each logical call HERE is a real, unavoidable C++ recursive call + // (a trampolined tail call never re-enters this function at all, + // see evalFunctionBodyTrampoline/runCompiledFunctionTrampoline) -- + // silently overflowing the native stack: a hard, unrecoverable + // process crash with no C++ exception to catch. Confirmed present + // (before this guard existed) for a plain, closure-free `function + // f(n) = n<=0 ? 0 : 1+f(n-1);`, compiled OR interpreted, a few + // thousand levels deep on an 8MB desktop main-thread stack. + // kMaxUserCallDepth is a conservative, heuristic cap, not a + // precisely-calibrated one -- deliberately well under that + // measured threshold to leave real margin for a smaller worker- + // thread stack (e.g. BelfrySCAD's own debug-session thread) and + // for whatever native stack this call's own caller has already + // consumed before reaching here. A real OpenSCAD/BOSL2 recursive + // function is overwhelmingly tail-recursive in practice (exactly + // why reduce()/accumulate()/while() and this evaluator's own + // trampolines exist) -- genuine non-tail recursion this deep is + // the rare, likely-runaway case a controlled error serves far + // better than a crash. + static constexpr size_t kMaxUserCallDepth = 1000; + if (callStack_.size() >= kMaxUserCallDepth) { + error("Recursion too deep while calling function '" + name + "'", declNode); + } std::optional prof = profileEnter("function", name, callPos, &declNode.position()); callStack_.push_back(CallStackFrame{CallStackFrame::Kind::Function, name, callPos, &declNode.position(), &declNode, nullptr, upvalueParent}); diff --git a/pyproject.toml b/pyproject.toml index 437aa4b..9110e8f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.9.1" +version = "0.9.2" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/tests/test_tail_calls.cpp b/tests/test_tail_calls.cpp index 612aca6..9a86c05 100644 --- a/tests/test_tail_calls.cpp +++ b/tests/test_tail_calls.cpp @@ -9,10 +9,15 @@ // Every test here explicitly forces the interpreter path via ScopedVm(false) // (not the OSCAD_BYTECODE_VM env var, which is cached forever after its // first read within a test binary -- see Evaluator:: -// setBytecodeVmEnabledForTesting's own doc comment): the bytecode VM's own -// call machinery (Phase B, not yet implemented as of this file) still does -// genuine C++ recursion for every call, so a deep-recursion test run under -// the default VM-on path would crash for the wrong reason. +// setBytecodeVmEnabledForTesting's own doc comment). Both the interpreter +// AND the bytecode VM's own call machinery (Phase B) do genuine C++ +// recursion for a NON-tail call (there is no other way to implement one -- +// see NonTailRecursionIsUnaffectedByTheTrampoline, below) -- a sufficiently +// deep one used to silently overflow the native stack and crash the whole +// process; evalUserFunctionCore's own depth guard (evaluator.hpp) now turns +// that into a controlled EvalError well before that point, shared by both +// paths, exercised by both compiled- and interpreted-path variants of the +// same test at the bottom of this file. #include "openscad_cpp_evaluator/eval_error.hpp" #include "openscad_cpp_evaluator/evaluator.hpp" @@ -267,3 +272,52 @@ TEST(TailCalls, DebugHookFiresPerHopInsideATailChain) { // single body-entry stop. EXPECT_EQ(bodyEntry, 501 + 500 + 1); } + +TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpreted) { + ScopedVm vm(false); + // `1 + f(n-1)` is NOT a tail position (the addition happens after the + // recursive call returns) -- there is no trampoline for this, ever + // (see NonTailRecursionIsUnaffectedByTheTrampoline, above): every + // logical call is a real, unavoidable C++ recursive call through + // evalUserFunctionCore. Before its own depth guard existed, a script + // shaped exactly like this one reliably segfaulted the whole process + // a few thousand levels deep -- confirmed present in the wild (a + // plain, closure-free FunctionDeclaration, nothing specific to + // closures/letrec/tail-call machinery at all). Interpreted-path + // variant; see the compiled-path one below. + Evaluator ev; + auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(50000);"); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + try { + ev.resolveTree(ast, ctx); + FAIL() << "expected EvalError"; + } catch (const EvalError& e) { + EXPECT_NE(std::string(e.what()).find("Recursion too deep"), std::string::npos); + EXPECT_NE(std::string(e.what()).find("'f'"), std::string::npos); + } +} + +TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompiled) { + ScopedVm vm(true); + // Same script, same guard (evalUserFunctionCore is shared by both the + // interpreter and the bytecode VM's own compiled call path) -- the + // compiled path's own real per-call native stack usage is actually + // larger (more C++ frames between one logical call and the next, see + // runChunk/runCompiledFunctionFromBound/runCompiledFunctionFromBoundTrampoline), + // so this is the MORE exposed of the two paths, not a lesser check. + Evaluator ev; + auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(50000);"); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); +} + +TEST(TailCalls, ShallowNonTailRecursionStillWorksUnderTheDepthGuard) { + ScopedVm vm(true); + // The guard must not fire for perfectly ordinary, shallow non-tail + // recursion -- the overwhelmingly common real-world shape (a handful + // to a few dozen levels, e.g. walking a small nested data structure). + const std::string script = "function fact(n) = n <= 1 ? 1 : n * fact(n - 1);\necho(fact(10));"; + EXPECT_EQ(runCapturingEcho(script), "ECHO: 3.6288e+6"); +} From 53975e39bdfd9cf0c62db15c75b53ed9db60db57 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 30 Jul 2026 17:30:04 -0700 Subject: [PATCH 2/5] Lower the recursion depth cap after Windows CI still crashed at 1000 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 --- include/openscad_cpp_evaluator/evaluator.hpp | 34 ++++++++++++-------- tests/test_tail_calls.cpp | 4 +-- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index b2f00b2..0e85e0a 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -640,20 +640,26 @@ class Evaluator { // silently overflowing the native stack: a hard, unrecoverable // process crash with no C++ exception to catch. Confirmed present // (before this guard existed) for a plain, closure-free `function - // f(n) = n<=0 ? 0 : 1+f(n-1);`, compiled OR interpreted, a few - // thousand levels deep on an 8MB desktop main-thread stack. - // kMaxUserCallDepth is a conservative, heuristic cap, not a - // precisely-calibrated one -- deliberately well under that - // measured threshold to leave real margin for a smaller worker- - // thread stack (e.g. BelfrySCAD's own debug-session thread) and - // for whatever native stack this call's own caller has already - // consumed before reaching here. A real OpenSCAD/BOSL2 recursive - // function is overwhelmingly tail-recursive in practice (exactly - // why reduce()/accumulate()/while() and this evaluator's own - // trampolines exist) -- genuine non-tail recursion this deep is - // the rare, likely-runaway case a controlled error serves far - // better than a crash. - static constexpr size_t kMaxUserCallDepth = 1000; + // f(n) = n<=0 ? 0 : 1+f(n-1);`, compiled OR interpreted. + // kMaxUserCallDepth is a conservative, heuristic cap, NOT a + // precisely-calibrated one, and deliberately erred low: an initial + // value of 1000 -- comfortably under the several-thousand-level + // threshold this evaluator's own C++ recursion measured safe at on + // an 8MB desktop macOS/Linux main-thread stack -- still crashed on + // CI's windows-latest runner, whose default thread stack is + // apparently far smaller and/or whose MSVC-compiled call frames + // are larger than Clang's. Without a reliable way to measure the + // real figure on every platform/thread this evaluator might run + // on (e.g. BelfrySCAD's own debug-session worker thread, likely + // smaller still than any CI runner's main thread), this cap trades + // away some legitimate deep-non-tail-recursion headroom for actual + // safety -- a real OpenSCAD/BOSL2 recursive function is + // overwhelmingly tail-recursive in practice anyway (exactly why + // reduce()/accumulate()/while() and this evaluator's own + // trampolines exist), so genuine non-tail recursion even + // approaching this depth is already the rare, likely-runaway case + // a controlled error serves far better than a crash. + static constexpr size_t kMaxUserCallDepth = 200; if (callStack_.size() >= kMaxUserCallDepth) { error("Recursion too deep while calling function '" + name + "'", declNode); } diff --git a/tests/test_tail_calls.cpp b/tests/test_tail_calls.cpp index 9a86c05..493cdc2 100644 --- a/tests/test_tail_calls.cpp +++ b/tests/test_tail_calls.cpp @@ -286,7 +286,7 @@ TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpr // closures/letrec/tail-call machinery at all). Interpreted-path // variant; see the compiled-path one below. Evaluator ev; - auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(50000);"); + auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(2000);"); auto scope = oscad::buildScopes(ast); EvalContext ctx = EvalContext::makeRoot(scope.get()); try { @@ -307,7 +307,7 @@ TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompile // runChunk/runCompiledFunctionFromBound/runCompiledFunctionFromBoundTrampoline), // so this is the MORE exposed of the two paths, not a lesser check. Evaluator ev; - auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(50000);"); + auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(2000);"); auto scope = oscad::buildScopes(ast); EvalContext ctx = EvalContext::makeRoot(scope.get()); EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); From 0f9968322ab0c0c6dab270875ac0865306f65754 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 30 Jul 2026 17:45:16 -0700 Subject: [PATCH 3/5] DIAGNOSTIC: probe windows-latest CI's actual safe recursion depth 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 --- include/openscad_cpp_evaluator/evaluator.hpp | 9 +++- tests/test_tail_calls.cpp | 51 ++++++++++++++++++-- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 0e85e0a..0e52d5d 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -659,7 +659,14 @@ class Evaluator { // trampolines exist), so genuine non-tail recursion even // approaching this depth is already the rare, likely-runaway case // a controlled error serves far better than a crash. - static constexpr size_t kMaxUserCallDepth = 200; + // TEMPORARY: bumped for one diagnostic CI round (see + // TailCalls.DiagnosticFindWindowsSafeNonTailRecursionDepth, + // test_tail_calls.cpp) to find exactly how deep windows-latest CI + // can actually go before crashing -- 200 itself still segfaulted + // there, with no way to reproduce or bisect locally (no Windows + // access). Will be set to a real, evidence-based value once that + // run's log is read. + static constexpr size_t kMaxUserCallDepth = 5000; if (callStack_.size() >= kMaxUserCallDepth) { error("Recursion too deep while calling function '" + name + "'", declNode); } diff --git a/tests/test_tail_calls.cpp b/tests/test_tail_calls.cpp index 493cdc2..9a2f17d 100644 --- a/tests/test_tail_calls.cpp +++ b/tests/test_tail_calls.cpp @@ -25,6 +25,7 @@ #include "test_helpers.hpp" #include +#include #include using namespace oscadeval; @@ -273,7 +274,16 @@ TEST(TailCalls, DebugHookFiresPerHopInsideATailChain) { EXPECT_EQ(bodyEntry, 501 + 500 + 1); } -TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpreted) { +TEST(TailCalls, DISABLED_DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpreted) { + // TEMPORARILY DISABLED for this one diagnostic CI round: kMaxUserCallDepth + // is temporarily 5000 (see its own comment, evaluator.hpp) specifically + // so DiagnosticFindWindowsSafeNonTailRecursionDepth (below) can probe + // past every depth measured safe so far -- which means this script's + // own f(6000) now exceeds even this evaluator's OWN measured-safe + // native depth on ordinary desktop hardware, crashing for real before + // the guard ever gets a chance to fire. Re-enable (and fix the depth + // back down) once kMaxUserCallDepth is set to a real, evidence-based + // value. ScopedVm vm(false); // `1 + f(n-1)` is NOT a tail position (the addition happens after the // recursive call returns) -- there is no trampoline for this, ever @@ -286,7 +296,7 @@ TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpr // closures/letrec/tail-call machinery at all). Interpreted-path // variant; see the compiled-path one below. Evaluator ev; - auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(2000);"); + auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(6000);"); // TEMPORARY: see kMaxUserCallDepth's own comment, evaluator.hpp auto scope = oscad::buildScopes(ast); EvalContext ctx = EvalContext::makeRoot(scope.get()); try { @@ -298,7 +308,7 @@ TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpr } } -TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompiled) { +TEST(TailCalls, DISABLED_DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompiled) { ScopedVm vm(true); // Same script, same guard (evalUserFunctionCore is shared by both the // interpreter and the bytecode VM's own compiled call path) -- the @@ -307,7 +317,7 @@ TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompile // runChunk/runCompiledFunctionFromBound/runCompiledFunctionFromBoundTrampoline), // so this is the MORE exposed of the two paths, not a lesser check. Evaluator ev; - auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(2000);"); + auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(6000);"); // TEMPORARY: see kMaxUserCallDepth's own comment, evaluator.hpp auto scope = oscad::buildScopes(ast); EvalContext ctx = EvalContext::makeRoot(scope.get()); EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); @@ -321,3 +331,36 @@ TEST(TailCalls, ShallowNonTailRecursionStillWorksUnderTheDepthGuard) { const std::string script = "function fact(n) = n <= 1 ? 1 : n * fact(n - 1);\necho(fact(10));"; EXPECT_EQ(runCapturingEcho(script), "ECHO: 3.6288e+6"); } + +// TEMPORARY diagnostic: kMaxUserCallDepth=200 (evaluator.hpp) still +// segfaulted on CI's windows-latest runner with no way to reproduce or +// bisect locally (no Windows access) -- this test's own log output (each +// depth prints only once it has actually SUCCEEDED, flushed immediately) +// pinpoints the real safe boundary on that specific runner directly from +// the CI log, wherever this run happens to crash. Deleted once +// kMaxUserCallDepth is set to a real, evidence-based value. +TEST(TailCalls, DiagnosticFindWindowsSafeNonTailRecursionDepth) { + ScopedVm vm(false); + for (int depth : {10, 20, 30, 50, 75, 100, 150, 200, 300, 500, 750, 1000, 1500, 2000, 3000, 4000, 4900}) { + std::string script = + "function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(" + std::to_string(depth) + ");"; + Evaluator ev; + auto ast = parseSrc(script); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + ev.resolveTree(ast, ctx); + std::cerr << "[DIAG] interpreted depth " << depth << " OK" << std::endl; + } + Evaluator::setBytecodeVmEnabledForTesting(true); + for (int depth : {10, 20, 30, 50, 75, 100, 150, 200, 300, 500, 750, 1000, 1500, 2000, 3000, 4000, 4900}) { + std::string script = + "function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(" + std::to_string(depth) + ");"; + Evaluator ev; + auto ast = parseSrc(script); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + ev.resolveTree(ast, ctx); + std::cerr << "[DIAG] compiled depth " << depth << " OK" << std::endl; + } + Evaluator::setBytecodeVmEnabledForTesting(std::nullopt); +} From d08cb89c335e765f475049a71183d5e43cf3bb02 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 30 Jul 2026 17:56:57 -0700 Subject: [PATCH 4/5] DIAGNOSTIC round 2: measure throw+unwind cost specifically on Windows 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 --- tests/test_tail_calls.cpp | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/tests/test_tail_calls.cpp b/tests/test_tail_calls.cpp index 9a2f17d..7e6d3b8 100644 --- a/tests/test_tail_calls.cpp +++ b/tests/test_tail_calls.cpp @@ -332,35 +332,38 @@ TEST(TailCalls, ShallowNonTailRecursionStillWorksUnderTheDepthGuard) { EXPECT_EQ(runCapturingEcho(script), "ECHO: 3.6288e+6"); } -// TEMPORARY diagnostic: kMaxUserCallDepth=200 (evaluator.hpp) still -// segfaulted on CI's windows-latest runner with no way to reproduce or -// bisect locally (no Windows access) -- this test's own log output (each -// depth prints only once it has actually SUCCEEDED, flushed immediately) -// pinpoints the real safe boundary on that specific runner directly from -// the CI log, wherever this run happens to crash. Deleted once -// kMaxUserCallDepth is set to a real, evidence-based value. -TEST(TailCalls, DiagnosticFindWindowsSafeNonTailRecursionDepth) { +// TEMPORARY diagnostic, round 2: round 1 (plain recursion, no throw) found +// depth 300 safe, depth 500 unreached on windows-latest CI -- but +// kMaxUserCallDepth=200 (a THROW from depth 200, unwinding back out +// through 200 nested evalUserFunctionCore try/catch frames) ALSO +// segfaulted there. Together this means throwing+unwinding through N +// nested frames costs meaningfully more native stack than simply BEING N +// frames deep without throwing -- this loop measures THAT specific cost +// directly (assert(false) at depth N throws/unwinds through the exact +// same nested-try/catch shape Evaluator::error() does), at a finer +// granularity around the last-known-safe-without-throwing point (300). +TEST(TailCalls, DiagnosticFindWindowsSafeThrowUnwindDepth) { ScopedVm vm(false); - for (int depth : {10, 20, 30, 50, 75, 100, 150, 200, 300, 500, 750, 1000, 1500, 2000, 3000, 4000, 4900}) { - std::string script = - "function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(" + std::to_string(depth) + ");"; + for (int depth : {20, 40, 60, 80, 100, 120, 140, 160, 180, 200, 250, 300}) { + std::string script = "function f(n) = n <= 0 ? assert(false, \"boom\") 0 : 1 + f(n - 1);\nresult = f(" + + std::to_string(depth) + ");"; Evaluator ev; auto ast = parseSrc(script); auto scope = oscad::buildScopes(ast); EvalContext ctx = EvalContext::makeRoot(scope.get()); - ev.resolveTree(ast, ctx); - std::cerr << "[DIAG] interpreted depth " << depth << " OK" << std::endl; + EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); + std::cerr << "[DIAG] interpreted throw-unwind depth " << depth << " OK" << std::endl; } Evaluator::setBytecodeVmEnabledForTesting(true); - for (int depth : {10, 20, 30, 50, 75, 100, 150, 200, 300, 500, 750, 1000, 1500, 2000, 3000, 4000, 4900}) { - std::string script = - "function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(" + std::to_string(depth) + ");"; + for (int depth : {20, 40, 60, 80, 100, 120, 140, 160, 180, 200, 250, 300}) { + std::string script = "function f(n) = n <= 0 ? assert(false, \"boom\") 0 : 1 + f(n - 1);\nresult = f(" + + std::to_string(depth) + ");"; Evaluator ev; auto ast = parseSrc(script); auto scope = oscad::buildScopes(ast); EvalContext ctx = EvalContext::makeRoot(scope.get()); - ev.resolveTree(ast, ctx); - std::cerr << "[DIAG] compiled depth " << depth << " OK" << std::endl; + EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); + std::cerr << "[DIAG] compiled throw-unwind depth " << depth << " OK" << std::endl; } Evaluator::setBytecodeVmEnabledForTesting(std::nullopt); } From e2c572f307199f02c7e24bc88a7263de92ecc14e Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 30 Jul 2026 18:09:40 -0700 Subject: [PATCH 5/5] Finalize the recursion depth cap at 50, remove diagnostics 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 include; restores the two "hits the guard" tests (no longer disabled) with depths appropriate to the final cap. Co-Authored-By: Claude Sonnet 5 --- include/openscad_cpp_evaluator/evaluator.hpp | 46 +++++++------- tests/test_tail_calls.cpp | 65 ++++---------------- 2 files changed, 36 insertions(+), 75 deletions(-) diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 0e52d5d..669aa7e 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -641,32 +641,36 @@ class Evaluator { // process crash with no C++ exception to catch. Confirmed present // (before this guard existed) for a plain, closure-free `function // f(n) = n<=0 ? 0 : 1+f(n-1);`, compiled OR interpreted. + // // kMaxUserCallDepth is a conservative, heuristic cap, NOT a - // precisely-calibrated one, and deliberately erred low: an initial - // value of 1000 -- comfortably under the several-thousand-level - // threshold this evaluator's own C++ recursion measured safe at on - // an 8MB desktop macOS/Linux main-thread stack -- still crashed on - // CI's windows-latest runner, whose default thread stack is - // apparently far smaller and/or whose MSVC-compiled call frames - // are larger than Clang's. Without a reliable way to measure the - // real figure on every platform/thread this evaluator might run - // on (e.g. BelfrySCAD's own debug-session worker thread, likely - // smaller still than any CI runner's main thread), this cap trades - // away some legitimate deep-non-tail-recursion headroom for actual - // safety -- a real OpenSCAD/BOSL2 recursive function is + // precisely-calibrated one -- and specifically calibrated against + // the WORST measured platform, not the best. Two rounds of CI + // diagnostics (temporary test builds that recursed to increasing + // depths, printing success after each one so the CI log itself + // would show exactly where a crash happened) found: (1) this + // evaluator's own C++ recursion is safe into the THOUSANDS on an + // ordinary macOS/Linux desktop main-thread stack, but only into + // the low HUNDREDS on CI's windows-latest runner (smaller default + // thread stack and/or larger MSVC-compiled call frames than + // Clang's); (2) actually THROWING from that depth and unwinding + // back out through that many nested try/catch frames (this guard + // fires from inside evalUserFunctionCore, which every level of + // the recursion has its own try/catch in) costs meaningfully MORE + // native stack than simply being that deep without throwing -- + // 300 survived on Windows without throwing, but throwing+ + // unwinding through as few as 100 nested frames did not (80 was + // the last depth that survived). 50 leaves real margin below that + // 80 figure for whatever's already on the stack from this call's + // own caller, and for a smaller stack still (e.g. BelfrySCAD's own + // debug-session worker thread, plausibly smaller than any CI + // runner's main thread) that this evaluator has no way to probe + // for directly. A real OpenSCAD/BOSL2 recursive function is // overwhelmingly tail-recursive in practice anyway (exactly why // reduce()/accumulate()/while() and this evaluator's own - // trampolines exist), so genuine non-tail recursion even + // trampolines exist) -- genuine non-tail recursion even // approaching this depth is already the rare, likely-runaway case // a controlled error serves far better than a crash. - // TEMPORARY: bumped for one diagnostic CI round (see - // TailCalls.DiagnosticFindWindowsSafeNonTailRecursionDepth, - // test_tail_calls.cpp) to find exactly how deep windows-latest CI - // can actually go before crashing -- 200 itself still segfaulted - // there, with no way to reproduce or bisect locally (no Windows - // access). Will be set to a real, evidence-based value once that - // run's log is read. - static constexpr size_t kMaxUserCallDepth = 5000; + static constexpr size_t kMaxUserCallDepth = 50; if (callStack_.size() >= kMaxUserCallDepth) { error("Recursion too deep while calling function '" + name + "'", declNode); } diff --git a/tests/test_tail_calls.cpp b/tests/test_tail_calls.cpp index 7e6d3b8..cb1c058 100644 --- a/tests/test_tail_calls.cpp +++ b/tests/test_tail_calls.cpp @@ -25,7 +25,6 @@ #include "test_helpers.hpp" #include -#include #include using namespace oscadeval; @@ -274,16 +273,7 @@ TEST(TailCalls, DebugHookFiresPerHopInsideATailChain) { EXPECT_EQ(bodyEntry, 501 + 500 + 1); } -TEST(TailCalls, DISABLED_DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpreted) { - // TEMPORARILY DISABLED for this one diagnostic CI round: kMaxUserCallDepth - // is temporarily 5000 (see its own comment, evaluator.hpp) specifically - // so DiagnosticFindWindowsSafeNonTailRecursionDepth (below) can probe - // past every depth measured safe so far -- which means this script's - // own f(6000) now exceeds even this evaluator's OWN measured-safe - // native depth on ordinary desktop hardware, crashing for real before - // the guard ever gets a chance to fire. Re-enable (and fix the depth - // back down) once kMaxUserCallDepth is set to a real, evidence-based - // value. +TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpreted) { ScopedVm vm(false); // `1 + f(n-1)` is NOT a tail position (the addition happens after the // recursive call returns) -- there is no trampoline for this, ever @@ -291,12 +281,15 @@ TEST(TailCalls, DISABLED_DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashi // logical call is a real, unavoidable C++ recursive call through // evalUserFunctionCore. Before its own depth guard existed, a script // shaped exactly like this one reliably segfaulted the whole process - // a few thousand levels deep -- confirmed present in the wild (a - // plain, closure-free FunctionDeclaration, nothing specific to - // closures/letrec/tail-call machinery at all). Interpreted-path - // variant; see the compiled-path one below. + // (confirmed present in the wild -- a plain, closure-free + // FunctionDeclaration, nothing specific to closures/letrec/tail-call + // machinery at all). Interpreted-path variant; see the compiled-path + // one below. f(500) is comfortably past kMaxUserCallDepth (50, see its + // own doc comment, evaluator.hpp) -- the actual native recursion never + // gets anywhere near 500, since the guard fires at 50 regardless of + // what depth the script asks for. Evaluator ev; - auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(6000);"); // TEMPORARY: see kMaxUserCallDepth's own comment, evaluator.hpp + auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(500);"); auto scope = oscad::buildScopes(ast); EvalContext ctx = EvalContext::makeRoot(scope.get()); try { @@ -308,7 +301,7 @@ TEST(TailCalls, DISABLED_DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashi } } -TEST(TailCalls, DISABLED_DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompiled) { +TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompiled) { ScopedVm vm(true); // Same script, same guard (evalUserFunctionCore is shared by both the // interpreter and the bytecode VM's own compiled call path) -- the @@ -317,7 +310,7 @@ TEST(TailCalls, DISABLED_DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashi // runChunk/runCompiledFunctionFromBound/runCompiledFunctionFromBoundTrampoline), // so this is the MORE exposed of the two paths, not a lesser check. Evaluator ev; - auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(6000);"); // TEMPORARY: see kMaxUserCallDepth's own comment, evaluator.hpp + auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(500);"); auto scope = oscad::buildScopes(ast); EvalContext ctx = EvalContext::makeRoot(scope.get()); EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); @@ -331,39 +324,3 @@ TEST(TailCalls, ShallowNonTailRecursionStillWorksUnderTheDepthGuard) { const std::string script = "function fact(n) = n <= 1 ? 1 : n * fact(n - 1);\necho(fact(10));"; EXPECT_EQ(runCapturingEcho(script), "ECHO: 3.6288e+6"); } - -// TEMPORARY diagnostic, round 2: round 1 (plain recursion, no throw) found -// depth 300 safe, depth 500 unreached on windows-latest CI -- but -// kMaxUserCallDepth=200 (a THROW from depth 200, unwinding back out -// through 200 nested evalUserFunctionCore try/catch frames) ALSO -// segfaulted there. Together this means throwing+unwinding through N -// nested frames costs meaningfully more native stack than simply BEING N -// frames deep without throwing -- this loop measures THAT specific cost -// directly (assert(false) at depth N throws/unwinds through the exact -// same nested-try/catch shape Evaluator::error() does), at a finer -// granularity around the last-known-safe-without-throwing point (300). -TEST(TailCalls, DiagnosticFindWindowsSafeThrowUnwindDepth) { - ScopedVm vm(false); - for (int depth : {20, 40, 60, 80, 100, 120, 140, 160, 180, 200, 250, 300}) { - std::string script = "function f(n) = n <= 0 ? assert(false, \"boom\") 0 : 1 + f(n - 1);\nresult = f(" + - std::to_string(depth) + ");"; - Evaluator ev; - auto ast = parseSrc(script); - auto scope = oscad::buildScopes(ast); - EvalContext ctx = EvalContext::makeRoot(scope.get()); - EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); - std::cerr << "[DIAG] interpreted throw-unwind depth " << depth << " OK" << std::endl; - } - Evaluator::setBytecodeVmEnabledForTesting(true); - for (int depth : {20, 40, 60, 80, 100, 120, 140, 160, 180, 200, 250, 300}) { - std::string script = "function f(n) = n <= 0 ? assert(false, \"boom\") 0 : 1 + f(n - 1);\nresult = f(" + - std::to_string(depth) + ");"; - Evaluator ev; - auto ast = parseSrc(script); - auto scope = oscad::buildScopes(ast); - EvalContext ctx = EvalContext::makeRoot(scope.get()); - EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); - std::cerr << "[DIAG] compiled throw-unwind depth " << depth << " OK" << std::endl; - } - Evaluator::setBytecodeVmEnabledForTesting(std::nullopt); -}