diff --git a/include/openscad_cpp_evaluator/bytecode_compiler.hpp b/include/openscad_cpp_evaluator/bytecode_compiler.hpp index 8d50cb8..fd805fa 100644 --- a/include/openscad_cpp_evaluator/bytecode_compiler.hpp +++ b/include/openscad_cpp_evaluator/bytecode_compiler.hpp @@ -99,4 +99,28 @@ std::optional tryCompileAssignmentBlock(const std::vector tryCompileModuleBody(const oscad::ModuleDeclaration& decl); +// Same compilation (assignment/if/for/resolved-module-call get real +// bytecode, everything else a native passthrough), but for an ARBITRARY +// statement list, not just a ModuleDeclaration's own `children` -- the +// general form behind Evaluator::tryRunCompiledChildren, which every +// evalChildren() call now tries first (stmt_eval.cpp), including a +// builtin module's own children (translate()/union()/etc.'s internal +// evalChildren call, src/builtins/*.cpp) and a for-loop's/let-block's +// body. This is what actually reaches the "native passthrough wraps a +// recursive module call" case Op::NativeStatement alone can't -- see this +// project's own session notes on the Stage 2 "NativeStatement gap" for +// why a builtin call itself (translate, not just what's declared inside +// this project's own module bodies) still can't become a SINGLE flat +// instruction stream with its caller this way (its own resolve function, +// e.g. resolveTransform, computes params AND evaluates children as one +// opaque native call -- decoupling that is a separate, larger project), +// but this DOES mean the children of any wrapping builtin get their own, +// independent chance to compile, cutting the number of native stack +// frames needed per recursive "wrap-then-recurse" level rather than +// eliminating them entirely. `scope`: the list's own lexical scope for +// resolving a call site's callee -- callers pass `children.front()-> +// scope()`, mirroring tryRunCompiledAssignmentBlock's own convention. +std::optional tryCompileChildrenList(const std::vector& children, + const oscad::Scope* scope); + } // namespace oscadeval diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index fbd781b..6521ec3 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -520,6 +520,18 @@ class Evaluator { // "silently equivalent to interpreting it" contract, just for a whole // block instead of one leaf expression. bool tryRunCompiledAssignmentBlock(const std::vector& assignments, EvalContext& ctx); + // Same contract as tryRunCompiledAssignmentBlock, above, but for + // evalChildren's own FULL `children` list (not just its leading + // assignment run) -- see tryCompileChildrenList's own doc comment + // (bytecode_compiler.hpp) for why every evalChildren() call site + // tries this FIRST now, including a builtin module's own children + // (translate()/union()/etc.) and a for-loop's/let-block's body, not + // just module/top-level bodies. `ctx` is used directly (not a scoped + // child), matching runCompiledModuleBody's own contract -- a + // children-list chunk's own writes (assignments, $-vars) must persist + // into the caller's scope exactly the way the native per-statement + // loop's own writes already do. + bool tryRunCompiledChildren(const std::vector& children, EvalContext& ctx); void evalModularCall(const oscad::ModularCall& node, EvalContext& ctx); void evalFor(const oscad::ModularFor& node, EvalContext& ctx); void evalLetBlock(const oscad::ModularLet& node, EvalContext& ctx); @@ -923,6 +935,25 @@ class Evaluator { // comment above for the full reasoning. std::unordered_map> assignBlockChunkCache_; + // Whole-CHILDREN-LIST chunks (see tryCompileChildrenList's own doc + // comment, bytecode_compiler.hpp, and tryRunCompiledChildren's, + // below) -- every evalChildren() call now tries the FULL list it was + // given as one chunk first (not just its own assignments sub-list), + // including a builtin module's own children (translate()/union()/ + // etc.'s internal evalChildren call) and a for-loop's/let-block's + // body -- so a resolvable module call anywhere in that list gets + // Op::CallModule bytecode, and any if/for control flow around it gets + // real Jump-based bytecode too, instead of falling through to the + // native per-statement loop. Keyed by the list's own FIRST element, + // same convention as assignBlockChunkCache_ (stable and unique per + // list: a given evalChildren() call site is always handed the same + // leading element across repeated evaluations). Same nullopt-means- + // tried-and-failed convention, same dangling-pointer hazard, same + // two-part fix (cleared alongside stmtExprChunkCache_ at the top of + // every resolveTreeImpl call, only read/written while inResolvePass_) + // as stmtExprChunkCache_ itself. + std::unordered_map> childrenListChunkCache_; + // See stmtExprChunkCache_'s own doc comment, immediately above, for why // this exists. Not a reentrancy guard (resolveTreeImpl is never called // while another one is already active), just an on/off switch for @@ -1231,6 +1262,20 @@ class Evaluator { // ordering constraint the inline computation already has. int callStackTopIndex() const { return callStack_.empty() ? -1 : static_cast(callStack_.size()) - 1; } + // The innermost active call's own declaration node, for error()'s TRACE + // walk -- needed by driveVm's own driveVmNativeDepth_ guard (see that + // field's doc comment), which has no single reliable ASTNode of its own + // to report against (the top VmFrame's chunk may be a children-list + // chunk with a null selfDecl -- see tryCompileChildrenList's own doc + // comment, bytecode_compiler.cpp). callStack_ is guaranteed non-empty + // by the time this guard can fire (it only trips past + // kMaxDriveVmNativeDepth levels of nesting, and every level pushes a + // CallStackFrame first via enterUserCall). Same "free function, no + // friend declaration" reasoning as callStackTopIndex, just above. + const oscad::ASTNode* currentCallDeclNode() const { + return callStack_.empty() ? nullptr : callStack_.back().declNode; + } + // The explicit, heap-allocated VM call stack that replaced native C++ // recursion for a call between two compiled chunks -- see this // project's own session notes ("iterate over the code, don't use the @@ -1269,6 +1314,37 @@ class Evaluator { // recalibrating for real. static constexpr size_t kMaxVmCallStackDepth = 1'000'000; + // Counts native C++ nesting of driveVm itself -- NOT logical call + // depth (that's vmCallStack_.size()/callStack_.size(), both of which + // grow just as much for a pure VM-internal Op::CallModule hop, which + // costs zero native stack). A hop entirely serviced by driveVm's own + // while loop (Op::CallModule/CallFn/CallFnTail/CallDynamic/ + // CallDynamicTail) never increments this. It DOES increment when a + // compiled body statement isn't one of the specially-compiled forms + // (e.g. `translate(v) recur(n-1);` -- a builtin-with-children falls to + // Op::NativeStatement) and that native evalStatement call itself + // re-enters the VM (evalChildren -> tryRunCompiledChildren/ + // tryRunCompiledAssignmentBlock/evalExprMaybeCompiled -> a fresh + // driveVm call, nested on the native stack, unlike Op::CallModule's own + // push). This is a REAL native recursive call each time, same hazard + // kMaxUserCallDepth guards against -- caught for real: `module + // recur(n) { translate([0,0,n]) recur2(n); } module recur2(n) { if + // (n>0) recur(n-1); else cube(1); }` segfaulted (exit 139) around + // n=3000 with no guard at all, since kMaxCsgTreeDepth only catches this + // AFTER the (already-crashed) native descent would have unwound. Public + // for the same reason vmCallStack_ is: driveVm (bytecode_vm.cpp) is a + // free function, not a member, so it needs direct access with no + // friend declaration in play. + size_t driveVmNativeDepth_ = 0; + // Same figure as kMaxUserCallDepth -- each native re-entry above costs + // a comparable handful of native frames (evalStatement, evalModularCall, + // a builtin resolve function, evalChildren, tryRunCompiledChildren, + // runCompiledModuleBody, driveVm), so the same Windows-CI-calibrated + // ceiling applies. Deliberately its own named constant (not a reuse of + // kMaxUserCallDepth in place) so the two can be recalibrated + // independently if profiling ever shows they should diverge. + static constexpr size_t kMaxDriveVmNativeDepth = 30; + // Bracketed public in place: Op::CallModule's own runtime handler and // driveVm's module-frame completion branch (bytecode_vm.cpp, a // separate translation unit) push/pop this directly -- exactly the diff --git a/pyproject.toml b/pyproject.toml index 7eadbfe..cd58756 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.11.0" +version = "0.12.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/bytecode_compiler.cpp b/src/bytecode_compiler.cpp index b8285fe..caad082 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -1127,10 +1127,24 @@ class Compiler { // every statement -- assignment or not -- collapse to the same simple // "compile once, in the right position" treatment. void compileStatementList(const std::vector>& children, std::vector& out) { + std::vector raw; + raw.reserve(children.size()); + for (const auto& c : children) raw.push_back(c.get()); + compileStatementList(raw, out); + } + + // Same, for a list of raw (non-owning) pointers -- Evaluator:: + // evalChildren's own primary overload already receives its `children` + // this shape (a caller-owned list, e.g. a builtin module's own + // node.children, or a for-loop's freshly-built bodyNodes vector, not + // necessarily one this Compiler's own declaration owns) -- see + // tryCompileChildrenList's own doc comment, below, for the entry point + // that needs this shape directly. + void compileStatementList(const std::vector& children, std::vector& out) { std::vector assignments; std::vector others; - for (const auto& c : children) { - (c->kind() == oscad::NodeKind::Assignment ? assignments : others).push_back(c.get()); + for (const oscad::ASTNode* c : children) { + (c->kind() == oscad::NodeKind::Assignment ? assignments : others).push_back(c); } for (const oscad::ASTNode* stmt : assignments) compileOneStatement(*stmt, out); for (const oscad::ASTNode* stmt : others) compileOneStatement(*stmt, out); @@ -1228,6 +1242,21 @@ class Compiler { std::vector topIdx(numDims); std::vector forIterNextIdx(numDims); for (size_t d = 0; d < numDims; ++d) { + // Every dimension but the outermost needs its own IterList + // index reset to 0 each time it's (re-)entered from the + // ENCLOSING dimension's own successful bind -- NativeIterMaterialize + // only resets it once, up front, before ANY iteration runs; by + // the second (and every later) outer-dimension value, this + // dimension's own index is still sitting at "exhausted" from + // the PREVIOUS pass, and would immediately look exhausted + // again without this. Placed strictly BEFORE this dimension's + // own ForIterNext/topIdx (not at it) so this dimension's OWN + // "try the next value" jump (its own ForIterEnd, below) lands + // AT ForIterNext directly and skips the reset -- only the + // fall-through from the enclosing dimension's bind passes + // through it. Harmless (a no-op) the very first time, since + // the index is already 0 from NativeIterMaterialize then. + if (d > 0) out.push_back({Op::IterReset, iterListIds[d], 0, nullptr}); topIdx[d] = out.size(); forIterNextIdx[d] = out.size(); Instruction ins; @@ -1391,4 +1420,26 @@ std::optional tryCompileModuleBody(const oscad::ModuleDeclaration return chunk; } +std::optional tryCompileChildrenList(const std::vector& children, + const oscad::Scope* scope) { + CompiledChunk chunk; + // Same completion semantics as a module chunk (no return value, its + // whole effect is the side effect of what lands in treeStack_) -- + // reuses runCompiledModuleBody's own bare-frame entry point as-is + // (bytecode_vm.cpp) to run it, no new runtime machinery needed. + // selfDecl stays null -- this list has no single declaration identity + // of its own the way a ModuleDeclaration's body does (no upvalues are + // ever resolved against it either way; module bodies don't create + // escaping closures). + chunk.isModule = true; + Compiler compiler(chunk, scope, nullptr, {}); + try { + compiler.compileStatementList(children, chunk.bodyCode); + } catch (const NotCompilable&) { + return std::nullopt; + } + chunk.numIterLists = compiler.nextIterList(); + return chunk; +} + } // namespace oscadeval diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index a0fa7be..9259c2d 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -340,7 +340,22 @@ void teardownVmCallStackDownTo(Evaluator& ev, size_t floor) { // returns; only the floor frame's own result escapes to the caller. Value driveVm(Evaluator& ev, size_t floor) { Value finalResult; + // Native-stack-safety guard, distinct from vmCallStack_'s own + // heap-bounded kMaxVmCallStackDepth check -- see driveVmNativeDepth_'s + // own doc comment (evaluator.hpp) for exactly what this catches (a + // NativeStatement-triggered re-entry into the VM, still a genuine + // native C++ call unlike an ordinary Op::CallModule/CallFn hop). + struct NativeDepthGuard { + Evaluator& ev; + explicit NativeDepthGuard(Evaluator& e) : ev(e) { ++ev.driveVmNativeDepth_; } + ~NativeDepthGuard() { --ev.driveVmNativeDepth_; } + } nativeDepthGuard(ev); try { + if (ev.driveVmNativeDepth_ > Evaluator::kMaxDriveVmNativeDepth) { + const oscad::ASTNode* node = ev.currentCallDeclNode(); + if (!node) node = ev.vmCallStack_.back()->chunk->selfDecl; + ev.error("Recursion too deep (native call stack)", *node); + } while (ev.vmCallStack_.size() > floor) { VmFrame& f = *ev.vmCallStack_.back(); if (f.pc >= f.code->size()) { diff --git a/src/csg_resolve.cpp b/src/csg_resolve.cpp index 76b17c8..b462625 100644 --- a/src/csg_resolve.cpp +++ b/src/csg_resolve.cpp @@ -208,8 +208,13 @@ std::vector> Evaluator::resolveTreeImpl(const NodeList& // "re-render after an edit" pattern) never risks a stale entry from a // PRIOR pass whose own AST nodes may since have been freed. See // stmtExprChunkCache_'s own doc comment (evaluator.hpp) for the full - // hazard this guards against. + // hazard this guards against. assignBlockChunkCache_'s own doc + // comment already claimed this -- it just wasn't actually done; a + // real, pre-existing gap, fixed here alongside childrenListChunkCache_ + // getting the same treatment from day one. stmtExprChunkCache_.clear(); + assignBlockChunkCache_.clear(); + childrenListChunkCache_.clear(); ResolvePassGuard resolvePassGuard(inResolvePass_); evalChildren(nodes, ctx); std::vector> tree = std::move(treeStack_.back()); diff --git a/src/stmt_eval.cpp b/src/stmt_eval.cpp index 66a149a..17d1c34 100644 --- a/src/stmt_eval.cpp +++ b/src/stmt_eval.cpp @@ -221,6 +221,22 @@ void Evaluator::evalStatement(const oscad::ASTNode& node, EvalContext& ctx) { } void Evaluator::evalChildren(const std::vector& children, EvalContext& ctx) { + // Whole-LIST compile first (see tryRunCompiledChildren's own doc + // comment, evaluator.hpp) -- tries the FULL `children` as one chunk + // before falling back to assignments-then-others below. This is what + // lets a resolvable module call anywhere in ANY children list -- + // not just a module/top-level body, but a for-loop's/let-block's own + // body, or a builtin's own children (translate()/union()/etc., whose + // own resolve function calls this SAME evalChildren internally) -- + // get real Op::CallModule bytecode instead of always falling through + // to the native per-statement loop below. Unconditionally correct + // either way since nothing about these statements' observable + // behavior differs based on which path ran them (same reasoning as + // tryRunCompiledAssignmentBlock's own doc comment, just for the WHOLE + // list instead of its own leading assignment run). + lastCtx_ = &ctx; + if (tryRunCompiledChildren(children, ctx)) return; + // OpenSCAD executes all assignments in a scope before anything else, // each group preserving source order -- mirrors evaluate()/ // _eval_children's `assignments + others` partition. diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 1a5b1b2..5a67b05 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -82,6 +82,19 @@ bool Evaluator::tryRunCompiledAssignmentBlock(const std::vector& children, EvalContext& ctx) { + if (children.empty() || !useBytecodeVm() || !inResolvePass_) return false; + const oscad::ASTNode* first = children.front(); + auto it = childrenListChunkCache_.find(first); + if (it == childrenListChunkCache_.end()) { + it = childrenListChunkCache_.emplace(first, tryCompileChildrenList(children, first->scope())).first; + if (it->second) flattenNestedLiterals(*it->second); + } + if (!it->second || !chunkEligibleNow(*it->second)) return false; + runCompiledModuleBody(*this, *it->second, ctx); + return true; +} + const Value* Evaluator::findUpvalue(const oscad::ASTNode* targetDecl, int slot) const { // Walks upvalueParent links, NOT a blind scan of callStack_ -- see // that field's own doc comment for the real bug this distinction diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index c275b23..af19d25 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -572,23 +572,31 @@ TEST(BytecodeCompiler, MakeClosureLetsContainerCompileDespiteEscapingClosure) { // compiled or not -- confirmed empirically, not assumed, by diffing // this test's own result against the pre-Op::MakeClosure compiler. // - // 3, not 6: BOTH top-level statements' own expressions now compile too - // (Phase 1, module/top-level compilation). `stored = outer(5);` is - // ALSO its own compiled assignment BLOCK now (evalChildren's own - // leading assignment-run, a single-assignment block here -- see - // tryRunCompiledAssignmentBlock, evaluator.hpp), which bypasses - // evalChildren's own per-STATEMENT checkDebug entirely on top of - // `outer(5)`'s own call-site checkpoint already being lost (same - // reasoning as evalExprMaybeCompiled's doc comment) -- 2 stops lost - // there, not 1. `echo(stored(3));` (doEcho, not a batched assignment) - // still only loses its own call-site checkpoint the ordinary way -- 1 - // stop. 6 - 2 - 1 = 3. + // 4, not 6: BOTH top-level statements' own expressions now compile too + // (Phase 1, module/top-level compilation). Confirmed directly via a + // standalone debug-hook trace, not derived by hand: (1) `stored = + // outer(5);`'s own STATEMENT-level checkpoint (line 2, depth 0) -- + // now the evalChildren top-level list compiles as ONE chunk + // (Evaluator::tryRunCompiledChildren, the "NativeStatement gap" fix), + // this assignment runs as an ordinary Op::NativeStatement entry, + // which DOES fire its own statement-level checkDebug -- unlike the + // narrower, single-purpose tryRunCompiledAssignmentBlock path this + // used to take instead (a real, single-assignment "block"), which + // skips that checkpoint entirely. `outer(5)`'s own call-site + // checkpoint is still lost either way (compiled either path). (2) + // `outer`'s own body-entry (line 1, depth 1). (3) `echo(stored(3));`'s + // own statement-level checkpoint (line 3, depth 0) -- unchanged from + // before, echo was never part of the assignment-block compile. (4) + // `stored(3)`'s own body-entry (line 1, depth 1) -- `stored` is an + // ESCAPING closure (captures `n`), never compiles regardless (see + // this test's own title), so this is the interpreter's usual + // body-entry stop for a genuine call. const int stops = countDebugHookStops( "function outer(n) = n > 0 ? function(y) y + n : function(y) y - n;\n" "stored = outer(5);\n" "echo(stored(3));", std::unordered_map>{}); - EXPECT_EQ(stops, 3); + EXPECT_EQ(stops, 4); } TEST(BytecodeCompiler, MakeClosureCapturesFreshValuePerInvocation) { @@ -1297,6 +1305,52 @@ TEST(ModuleBodyCompiles, RecursiveModuleWithIfSucceedsWellPastTheOldNativeLimitC ASSERT_EQ(e.bodies.size(), 1u); } +// The "NativeStatement gap" pattern (Evaluator::tryRunCompiledChildren): a +// builtin-with-children statement (translate here) always falls to +// Op::NativeStatement, so a recursive call made ONLY from inside one is a +// genuine, repeated native C++ re-entry into the VM (evalChildren -> +// tryRunCompiledChildren -> runCompiledModuleBody -> driveVm) every single +// level -- unlike a bare `recur(n-1);` statement, which compiles straight +// to Op::CallModule and costs zero native stack. Before +// driveVmNativeDepth_/kMaxDriveVmNativeDepth existed, this pattern +// segfaulted (exit 139) around n=3000 instead of throwing; this proves it +// now fails the same controlled way kMaxUserCallDepth already does for the +// interpreted case. +TEST(ModuleBodyCompiles, NativeReentrantRecursionHitsAControlledErrorInsteadOfCrashing) { + ScopedVm vm(true); + try { + evalSrc("module recur(n) { translate([0,0,n]) recur2(n); }\n" + "module recur2(n) { if (n > 0) { recur(n - 1); } else { cube(1); } }\n" + "recur(3000);"); + FAIL() << "expected EvalError"; + } catch (const EvalError& e) { + const std::string what = e.what(); + EXPECT_NE(what.find("Recursion too deep"), std::string::npos) << what; + } +} + +// 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 +// only a non-recursive LEAF statement per level is builtin-wrapped. Each +// such native re-entry completes immediately (no further recursion inside +// it), so driveVmNativeDepth_ never nests past 1-2 regardless of how deep +// `n` goes -- proving driveVmNativeDepth_'s new guard doesn't wrongly cap +// this well past kMaxUserCallDepth=30/kMaxDriveVmNativeDepth=30. +TEST(ModuleBodyCompiles, DeepPureVmRecursionWithNonRecursingBuiltinWrappedLeafSucceeds) { + ScopedVm vm(true); + Evaluated e = evalSrc("module recur(n) {" + " if (n > 0) {" + " translate([0,0,n]) cube(1);" + " recur(n - 1);" + " } else {" + " cube(1);" + " }" + "}" + "recur(100);"); + ASSERT_EQ(e.bodies.size(), 101u); +} + TEST(ModuleBodyCompiles, RecursiveModuleThrowingPartwayUnwindsCleanlyAndVmFrameRunsAgain) { ScopedVm vm(true); Evaluator ev;