diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index f8584c7..669aa7e 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -633,6 +633,47 @@ 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. + // + // kMaxUserCallDepth is a conservative, heuristic cap, NOT a + // 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) -- 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 = 50; + 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..cb1c058 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,55 @@ 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 + // (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(500);"); + 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(500);"); + 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"); +}