diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index c3d8a17..b56160a 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -1218,6 +1218,12 @@ class Evaluator { // and callStack_.back() is already gone by the time it runs. See // activeDeclRefcount_'s own doc comment. const oscad::ASTNode* declNode = nullptr; + // True only when THIS call incremented nativeUserCallDepth_ (i.e. + // skipDepthGuard was false) -- exitUserCall* needs it to decrement + // symmetrically, for the same "callStack_.back() is already gone" + // reason as `kind`/`declNode` above. See nativeUserCallDepth_'s + // own doc comment for why this can't just be "always decrement". + bool countedTowardNativeDepth = false; }; // `skipDepthGuard`: kMaxUserCallDepth=50 exists purely to keep the // NATIVE C++ stack from overflowing (see its own doc comment) -- true @@ -1427,6 +1433,37 @@ class Evaluator { // eval_error.hpp). std::vector callStack_; + // Count of callStack_ entries that ACTUALLY cost native C++ stack -- + // i.e. pushed with skipDepthGuard=false (enterUserCall's own + // interpreted-call sites: evalUserFunctionCore, evalUserModule). + // Deliberately NOT the same number as callStack_.size(): a compiled- + // to-compiled push (pushBracketedCallFrame/pushBracketedModuleFrame, + // bytecode_vm.cpp, skipDepthGuard=true) still grows callStack_ (for + // TRACE/closure-detection/$parent_modules bookkeeping) but costs ZERO + // native stack, serviced by driveVm's own heap-based loop instead. + // + // Checking callStack_.size() itself against kMaxUserCallDepth (the + // ORIGINAL design) conflates these two: a real BOSL2 script's own + // AMBIENT callStack_ depth, inflated by many cheap skip=true compiled + // module-call pushes sitting on callStack_ already, could push a + // LATER genuinely-native-recursive call (e.g. an interpreted-path + // function call like BOSL2's own `ident()`) over the threshold even + // though the REAL native C++ nesting at that point was nowhere near + // it -- caught for real: Anklet.scad's own ambient callStack_ depth + // (mostly cheap Op::CallModule pushes) reached the mid-30s, tripping + // kMaxUserCallDepth=30 for an `ident()` call that itself was nested + // only a couple of GENUINE native frames deep. Exactly the same class + // of "fixed-count guard conflates logical depth with native-stack + // cost" bug driveVmNativeDepth_ already exists to avoid for the + // OTHER guard site (bytecode_vm.cpp) -- this is that same fix, + // applied here. Incremented/decremented symmetrically by + // enterUserCall/exitUserCallSuccess/exitUserCallException, gated on + // UserCallHandle::countedTowardNativeDepth (skipDepthGuard is only + // available at enter time; the handle carries the decision forward + // to exit time, since callStack_.back() -- and thus skipDepthGuard's + // own original context -- is already gone by then). + size_t nativeUserCallDepth_ = 0; + // Running count of Module-kind frames currently on callStack_ -- // maintained incrementally by enterUserCall/exitUserCall* rather than // rescanned from callStack_ on every module call (buildModuleChildCtx's diff --git a/pyproject.toml b/pyproject.toml index 99e6444..d801edd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.13.2" +version = "0.13.3" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/user_calls.cpp b/src/user_calls.cpp index c9993dd..83d5f6d 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -682,13 +682,22 @@ Evaluator::UserCallHandle Evaluator::enterUserCall(const std::string& name, cons // entirely; for what's left, the margin check is the proven-UNSAFE // mechanism on Windows (confirmed via real CI), this fixed count the // proven-safe one. - if (!skipDepthGuard && callStack_.size() >= kMaxUserCallDepth) { + // + // Checked against nativeUserCallDepth_, NOT callStack_.size() -- see + // that field's own doc comment (evaluator.hpp) for why: callStack_ + // also grows from cheap, zero-native-cost skipDepthGuard=true pushes, + // which must not count toward this native-stack-safety ceiling. + if (!skipDepthGuard && nativeUserCallDepth_ >= kMaxUserCallDepth) { error("Recursion too deep while calling " + std::string(isModule ? "module" : "function") + " '" + name + "'", declNode); } UserCallHandle h; h.kind = kind; h.declNode = &declNode; + if (!skipDepthGuard) { + ++nativeUserCallDepth_; + h.countedTowardNativeDepth = true; + } h.prof = profileEnter(isModule ? "module" : "function", name, callPos, &declNode.position()); callStack_.push_back(CallStackFrame{kind, name, callPos, &declNode.position(), &declNode, nullptr, upvalueParent}); callStack_.back().bodyCtx = &childCtx; // per-frame locals for the debugger @@ -710,6 +719,7 @@ void Evaluator::exitUserCallSuccess(const std::string& name, const UserCallHandl if (fireReturnHook && debugHooks_.returnHook) debugHooks_.returnHook(name, result, static_cast(callStack_.size())); callStack_.pop_back(); if (handle.kind == CallStackFrame::Kind::Module) --moduleCallDepth_; + if (handle.countedTowardNativeDepth) --nativeUserCallDepth_; noteActiveDeclExit(handle.declNode); if (handle.prof) profileExit(*handle.prof); } @@ -717,6 +727,7 @@ void Evaluator::exitUserCallSuccess(const std::string& name, const UserCallHandl void Evaluator::exitUserCallException(const UserCallHandle& handle) { callStack_.pop_back(); if (handle.kind == CallStackFrame::Kind::Module) --moduleCallDepth_; + if (handle.countedTowardNativeDepth) --nativeUserCallDepth_; noteActiveDeclExit(handle.declNode); if (handle.prof) profileExit(*handle.prof); } diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index 1b0b42d..c9e3adf 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -1390,6 +1390,45 @@ TEST(ModuleBodyCompiles, UnionWrappedRecursionStillHitsTheNativeReentryGuardCont } } +// Regression test for a real bug this same investigation caught: +// enterUserCall's own depth guard used to check callStack_.size() +// directly -- but callStack_ also grows from cheap, zero-native-cost +// COMPILED module-call pushes (skipDepthGuard=true), which must not +// count toward the native-stack-safety ceiling a LATER genuinely +// interpreted call (skipDepthGuard=false) is guarded by. Caught for +// real: a live BOSL2 script's own ambient callStack_ depth (mostly +// compiled Op::CallModule pushes) reached the mid-30s, tripping +// kMaxUserCallDepth=30 for a plain function call even though the REAL +// native C++ nesting at that point was nowhere near it. Fixed via +// nativeUserCallDepth_ (evaluator.hpp), a separate counter incremented +// only for skipDepthGuard=false pushes. +// +// recur() recurses 100 levels deep via bare `recur(n-1);` (pure +// Op::CallModule, skipDepthGuard=true, zero native cost, well past the +// old kMaxUserCallDepth=30) before calling leaf() at the base case -- +// forced to run INTERPRETED (skipDepthGuard=false) via a fast-continue +// breakpoint on its own declaration line (mirrors countDebugHookStops' +// own "force one specific chunk native" trick, above this file's first +// TEST). Before the fix, this leaf() call -- genuinely nested only 1 +// real native frame deep -- would have falsely tripped the guard purely +// from callStack_'s own ambient depth. +TEST(ModuleBodyCompiles, DeepCompiledModuleRecursionDoesNotFalselyTripTheInterpretedFunctionGuard) { + ScopedVm vm(true); + DebugHooks hooks; + hooks.debugHook = [](int, int, bool, bool, const std::string&, const std::vector&, + const DebugFramesFn&) { return DebugAction{}; }; + Evaluator ev(EchoFn{}, nullptr, nullptr, hooks); + ev.setFastContinueBreakpoints(std::unordered_map>{{"", {2}}}); + auto ast = parseSrc("module recur(n) { if (n > 0) { recur(n - 1); } else { cube(leaf()); } }\n" + "function leaf() = 1;\n" + "recur(100);"); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + std::vector> tree = ev.resolveTree(ast, ctx); + ASSERT_EQ(tree.size(), 1u); + EXPECT_EQ(tree[0]->kind, "cube"); +} + // The realistic case the NativeStatement-gap fix actually targets: the // RECURSIVE call itself is a bare statement (pure Op::CallModule, zero // native stack, bounded only by the heap-sized kMaxVmCallStackDepth), and