Fix O(N^2) tail-call trampoline teardown under GCC/MSVC (fixes #50)

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"

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<T>'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<T>::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<EvalContext>& chain;
+    ~ChainTeardown() {
+        while (!chain.empty()) chain.pop_back();
+    }
+};
+} // namespace
+
 Value runCompiledFunctionTrampoline(Evaluator& ev, const CompiledChunk& chunk,
                                      const std::vector<std::unique_ptr<oscad::Argument>>& arguments,
                                      EvalContext& callerCtx, EvalContext& childCtx) {
     std::vector<EvalContext> 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<EvalContext> chain{childCtx};
+    ChainTeardown teardown{chain};
     TailCallRequest req;
     Value result = runCompiledFunctionFromBound(ev, chunk, bound, chain.back(), &req);
     unsigned recursionGuard = 0;

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<EvalContext> 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<T>'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<T>::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<EvalContext>& chain;
+        ~ChainTeardown() {
+            while (!chain.empty()) chain.pop_back();
+        }
+    } teardown{chain};
     unsigned recursionGuard = 0;
     while (true) {
         auto result = simplifyTailStep(*expr, ctx);

tests/test_tail_calls.cpp

@@ -19,6 +19,7 @@
 
 #include "test_helpers.hpp"
 
+#include <chrono>
 #include <gtest/gtest.h>
 
 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<EvalContext>'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<double, std::milli>(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;