From cb90bae30bcbf52775a8f9f87f659192e563d915 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Wed, 29 Jul 2026 23:29:40 -0700 Subject: [PATCH] Fix O(N^2) tail-call trampoline teardown under GCC/MSVC (fixes #50) Both trampolines (evalFunctionBodyTrampoline, user_calls.cpp; and runCompiledFunctionTrampoline/runCompiledFunctionFromBoundTrampoline, bytecode_vm.cpp) keep every hop's own EvalContext alive in a `chain` vector for the whole run ($-vars stay dynamically scoped through even an isolated call, so a later hop may need to walk back through an earlier one's dyn level). Tearing `chain` down was left to its own std::vector destructor -- whose element-destruction order is unspecified by the C++ standard. libc++ (Clang) destroys back-to-front, matching ScopeTrailStorage::popLevel's "scan from the back" optimization (its own doc comment assumes the level being popped is usually the most recently pushed one); libstdc++ (GCC) 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). Confirmed via a minimal repro (std::vector::clear() literally prints destruction order in opposite directions under clang++ vs g++-16 for identical source) and an N-scaling experiment (Clang: clean O(N); GCC: time roughly quadruples when N doubles, at every step). The two previously-skipped tests (see #50) went from 25-50+ minutes each on GCC to under 1 second, matching Clang throughout -- verified locally with a real Homebrew GCC 16 build (same machine/OS/allocator as the Clang baseline) and against the full 1292-test suite on both compilers. Fix: an RAII guard tears `chain` down back-to-front explicitly, on every exit path (normal return OR exception unwind -- the recursion- guard error these tests hit is exactly the case a trailing-statement teardown would miss). Re-enables the two tests previously skipped on non-Apple CI, plus a new timing-bounded regression tripwire. Co-Authored-By: Claude Sonnet 5 --- pyproject.toml | 2 +- src/bytecode_vm.cpp | 29 +++++++++++++++++++++++++ src/user_calls.cpp | 26 ++++++++++++++++++++++ tests/test_tail_calls.cpp | 45 +++++++++++++++++++++++++-------------- 4 files changed, 85 insertions(+), 17 deletions(-) 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;