diff --git a/pyproject.toml b/pyproject.toml index f9a56ba..4c8b532 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.6.0" +version = "0.6.1" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index f2556c4..1f269cd 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -664,10 +664,38 @@ Value runCompiledFunctionFromBound(Evaluator& ev, const CompiledChunk& chunk, co // *stack* growth (what this trampoline exists to eliminate) for heap // growth instead -- O(iteration count) EvalContext objects, bounded by // available RAM rather than a ~few-MB thread stack. +namespace { +// Tears a trampoline's own `chain` down back-to-front on EVERY exit path -- +// normal return OR exception unwind (recordTailCallHop's own recursion-guard +// error is the exact case that matters: an infinite tail-recursive function +// with no base case throws out of the loop entirely, bypassing any teardown +// code placed after it). Relying on `chain` simply falling out of scope +// instead is NOT equivalent: std::vector's own element-destruction order +// is unspecified by the standard -- libc++ destroys back-to-front (matching +// ScopeTrailStorage::popLevel's own "scan from the back" doc comment, which +// assumes the level being popped is usually the most recently pushed one -- +// O(1) per pop that way), but libstdc++ destroys front-to-back, the +// adversarial order for that same scan -- each pop then walks almost the +// entire remaining vector, turning an intended O(N) teardown into a real, +// measured O(N^2) (see issue #50: an N-scaling experiment plus a minimal +// repro proving libc++/libstdc++ disagree on std::vector::clear()'s +// destruction order for identical source). Popping back-to-front explicitly, +// via a destructor that always runs before `chain`'s own, is correct AND +// O(N) on every standard library and every exit path, not just the ones +// that happen to agree with libc++ and return normally. +struct ChainTeardown { + std::vector& chain; + ~ChainTeardown() { + while (!chain.empty()) chain.pop_back(); + } +}; +} // namespace + Value runCompiledFunctionTrampoline(Evaluator& ev, const CompiledChunk& chunk, const std::vector>& arguments, EvalContext& callerCtx, EvalContext& childCtx) { std::vector chain{childCtx}; + ChainTeardown teardown{chain}; TailCallRequest req; Value result = runCompiledFunction(ev, chunk, arguments, callerCtx, chain.back(), &req); unsigned recursionGuard = 0; @@ -689,6 +717,7 @@ Value runCompiledFunctionTrampoline(Evaluator& ev, const CompiledChunk& chunk, Value runCompiledFunctionFromBoundTrampoline(Evaluator& ev, const CompiledChunk& chunk, const BoundArgs& bound, EvalContext& childCtx) { std::vector chain{childCtx}; + ChainTeardown teardown{chain}; TailCallRequest req; Value result = runCompiledFunctionFromBound(ev, chunk, bound, chain.back(), &req); unsigned recursionGuard = 0; diff --git a/src/user_calls.cpp b/src/user_calls.cpp index e304f39..e26e84f 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -443,6 +443,32 @@ Value Evaluator::evalFunctionBodyTrampoline(const oscad::Expression& bodyExpr, E // dropped there) if memory ever measurably matters more than this // simpler all-or-nothing approach. std::vector chain{ctx}; + // Tears `chain` down back-to-front on EVERY exit path -- normal return + // OR exception unwind (recordTailCallHop's own recursion-guard error, + // below, is the exact case that matters: an infinite tail-recursive + // function with no base case throws out of this loop entirely, + // bypassing any teardown code placed after it). Relying on `chain` + // simply falling out of scope instead is NOT equivalent: std::vector's + // own element-destruction order is unspecified by the standard -- + // libc++ destroys back-to-front (matching ScopeTrailStorage::popLevel's + // own "scan from the back" doc comment, which assumes the level being + // popped is usually the most recently pushed one -- O(1) per pop that + // way), but libstdc++ destroys front-to-back, the adversarial order for + // that same scan -- each pop then walks almost the entire remaining + // vector, turning an intended O(N) teardown into a real, measured + // O(N^2) (see issue #50: an N-scaling experiment plus a minimal repro + // proving libc++/libstdc++ disagree on std::vector::clear()'s + // destruction order for identical source). Popping back-to-front + // explicitly, via a destructor that always runs before `chain`'s own, + // is correct AND O(N) on every standard library and every exit path, + // not just the ones that happen to agree with libc++ and return + // normally. + struct ChainTeardown { + std::vector& chain; + ~ChainTeardown() { + while (!chain.empty()) chain.pop_back(); + } + } teardown{chain}; unsigned recursionGuard = 0; while (true) { auto result = simplifyTailStep(*expr, ctx); diff --git a/tests/test_tail_calls.cpp b/tests/test_tail_calls.cpp index 8e3e293..612aca6 100644 --- a/tests/test_tail_calls.cpp +++ b/tests/test_tail_calls.cpp @@ -19,6 +19,7 @@ #include "test_helpers.hpp" +#include #include using namespace oscadeval; @@ -126,17 +127,6 @@ TEST(TailCalls, NonTailRecursionIsUnaffectedByTheTrampoline) { } TEST(TailCalls, InfiniteTailRecursionHitsTheIterationCapInsteadOfHanging) { -#ifndef __APPLE__ - // Skipped on non-Apple platforms: this drives the interpreter trampoline - // to the full 1,000,000-iteration recursion-guard cap, which is O(N) - // under Clang/libc++ (~1s) but a confirmed O(N^2) under GCC/libstdc++ - // (and, empirically, MSVC's STL too) -- 25-50+ minutes instead of - // seconds. See https://github.com/BelfrySCAD/openscad_cpp_evaluator/issues/50 - // for the full investigation (peak RSS/CPU/instruction-count evidence - // and an N-scaling experiment proving the asymptotic-complexity gap). - // Re-enable once that's root-caused and fixed. - GTEST_SKIP() << "O(N^2) under this toolchain -- see issue #50"; -#endif ScopedVm vm(false); Evaluator ev; auto ast = parseSrc("function loop(n) = loop(n + 1);\nresult = loop(0);"); @@ -150,11 +140,6 @@ TEST(TailCalls, InfiniteTailRecursionHitsTheIterationCapInsteadOfHanging) { } TEST(TailCalls, InfiniteTailRecursionErrorMentionsTheFunctionName) { -#ifndef __APPLE__ - // See InfiniteTailRecursionHitsTheIterationCapInsteadOfHanging's own - // comment just above -- same O(N^2)-under-this-toolchain issue (#50). - GTEST_SKIP() << "O(N^2) under this toolchain -- see issue #50"; -#endif ScopedVm vm(false); Evaluator ev; auto ast = parseSrc("function loop(n) = loop(n + 1);\nresult = loop(0);"); @@ -169,6 +154,34 @@ TEST(TailCalls, InfiniteTailRecursionErrorMentionsTheFunctionName) { } } +TEST(TailCalls, InfiniteTailRecursionHitsTheCapInBoundedWallTime) { + // Regression tripwire for issue #50: evalFunctionBodyTrampoline's own + // `chain` (every hop's EvalContext, kept alive for $-var ancestry -- + // see its own doc comment) used to rely on std::vector's + // OWN destructor to tear itself down, whose element-destruction order + // is unspecified by the standard. libstdc++ (GCC) and, per CI evidence, + // MSVC destroy front-to-back -- the adversarial order for + // ScopeTrailStorage::popLevel's "scan from the back" optimization + // (its own doc comment), turning an intended O(N) teardown into a + // real, measured O(N^2): the exact same 1,000,000-iteration script + // below took 25-50+ minutes on GCC/MSVC CI runners before the fix + // (an explicit back-to-front teardown, immune to the underlying + // vector's own unspecified order), vs ~1s on Clang/libc++ throughout. + // ponytail: generous fixed ceiling, not a strict perf target -- trips + // only on a real regression (this exact O(N^2) reintroduced), not + // machine noise; raise if a slower CI runner ever needs it. + ScopedVm vm(false); + Evaluator ev; + auto ast = parseSrc("function loop(n) = loop(n + 1);\nresult = loop(0);"); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + const auto start = std::chrono::steady_clock::now(); + EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); + const double ms = std::chrono::duration(std::chrono::steady_clock::now() - start).count(); + constexpr double kCeilingMs = 30000.0; + EXPECT_LT(ms, kCeilingMs) << "took " << ms << "ms -- possible O(N^2) regression, see issue #50"; +} + TEST(TailCalls, ErrorThrownDeepInATailChainProducesABoundedTrace) { ScopedVm vm(false); Evaluator ev;