From b86bff0cb373eb612e42481cd882f191c5a98756 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 1 Aug 2026 04:32:59 -0700 Subject: [PATCH 1/2] Fix childrenListChunkCache_ collision between children() and children(N) The cache was keyed by the list's first element alone -- a convention borrowed from assignBlockChunkCache_, where it IS sufficient (a leading- assignment run's first member uniquely determines the run). Here it wasn't: children()'s forwarding produces two DIFFERENT lists sharing a first element (bare children() forwards the caller's whole list; children(0) a single-element slice of it), and whichever form ran first poisoned the cache for the other. Concrete failure: `module m() { children(); children(0); }` called with two children made children(0) emit BOTH shapes; reversed statement order truncated bare children() instead. Re-keyed by (front, size). Every evalChildren list producer enumerated: fixed AST lists have unique fronts per call site, and the only same- front pair (bare vs indexed forwarding) always differs in size except when the lists are literally identical anyway. Two regression tests (both statement orders), each verified to fail before the re-key. Also splits the lookup half of tryRunCompiledChildren into lookupOrCompileChildrenListChunk, needed shortly by Op::CallChildren's own runtime handler (same list->chunk step, different run mechanism). Co-Authored-By: Claude Fable 5 --- include/openscad_cpp_evaluator/evaluator.hpp | 44 +++++++++++++++++--- src/user_calls.cpp | 24 ++++++++--- tests/test_bytecode_compiler.cpp | 29 +++++++++++++ 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index b56160a..5b1f2e9 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -532,6 +532,17 @@ class Evaluator { // 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); +public: + // The cache-lookup half of tryRunCompiledChildren, shared with + // Op::CallChildren's runtime handler (bytecode_vm.cpp, a free + // function -- public for the same no-friend-declaration reasoning as + // vmCallStack_/treeStack_). Returns the eligible compiled chunk for + // `children` or nullptr. Caller owns the useBytecodeVm()/ + // inResolvePass_ gate -- the pass gate is load-bearing + // (childrenListChunkCache_ is pass-scoped, see its own doc comment), + // not defensive. + const CompiledChunk* lookupOrCompileChildrenListChunk(const std::vector& children); +private: void evalModularCall(const oscad::ModularCall& node, EvalContext& ctx); void evalFor(const oscad::ModularFor& node, EvalContext& ctx); void evalLetBlock(const oscad::ModularLet& node, EvalContext& ctx); @@ -951,15 +962,38 @@ class Evaluator { // 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- + // native per-statement loop. + // + // Keyed by (FIRST element, list SIZE) -- NOT the first element alone, + // the original convention borrowed from assignBlockChunkCache_ + // (where it IS sufficient: a leading-assignment run's first member + // uniquely determines the run). Here it wasn't: children()'s + // forwarding produces two DIFFERENT lists sharing a first element -- + // bare `children()` forwards the caller's whole list, `children(0)` + // a single-element slice of it -- and keying by front alone made + // whichever form ran first poison the cache for the other (caught + // for real: `module m() { children(); children(0); }\n + // m() { cube(); sphere(); }` made children(0) emit BOTH shapes, + // or bare children() emit only one, depending on statement order). + // (front, size) fully disambiguates every real producer: fixed AST + // lists have unique fronts per call site, and the only same-front + // pair (bare vs indexed forwarding) always differs in size except + // when the lists are literally identical anyway. 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_; + struct ChildrenListKeyHash { + size_t operator()(const std::pair& k) const { + // Standard hash-combine (boost-style golden-ratio mix) -- + // either half alone collides by construction here. + const size_t h1 = std::hash{}(k.first); + const size_t h2 = std::hash{}(k.second); + return h1 ^ (h2 + 0x9e3779b97f4a7c15ULL + (h1 << 6) + (h1 >> 2)); + } + }; + std::unordered_map, std::optional, ChildrenListKeyHash> + childrenListChunkCache_; // See stmtExprChunkCache_'s own doc comment, immediately above, for why // this exists. Not a reentrancy guard (resolveTreeImpl is never called diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 83d5f6d..8a8c7a6 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -84,15 +84,29 @@ bool Evaluator::tryRunCompiledAssignmentBlock(const std::vector& children, EvalContext& ctx) { if (children.empty() || !useBytecodeVm() || !inResolvePass_) return false; + const CompiledChunk* chunk = lookupOrCompileChildrenListChunk(children); + if (!chunk) return false; + runCompiledModuleBody(*this, *chunk, ctx); + return true; +} + +// The cache-lookup half of tryRunCompiledChildren, shared with +// Op::CallChildren's own runtime handler (bytecode_vm.cpp) -- both need +// exactly this "list -> eligible chunk or nullptr" step, differing only +// in how they then RUN the chunk (native runCompiledModuleBody reentry +// here, a direct vmCallStack_ push there). Caller is responsible for the +// useBytecodeVm()/inResolvePass_ gate (see childrenListChunkCache_'s own +// doc comment for why the pass gate is load-bearing, not defensive). +const CompiledChunk* Evaluator::lookupOrCompileChildrenListChunk(const std::vector& children) { const oscad::ASTNode* first = children.front(); - auto it = childrenListChunkCache_.find(first); + const auto key = std::make_pair(first, children.size()); + auto it = childrenListChunkCache_.find(key); if (it == childrenListChunkCache_.end()) { - it = childrenListChunkCache_.emplace(first, tryCompileChildrenList(children, first->scope())).first; + it = childrenListChunkCache_.emplace(key, 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; + if (!it->second || !chunkEligibleNow(*it->second)) return nullptr; + return &*it->second; } const Value* Evaluator::findUpvalue(const oscad::ASTNode* targetDecl, int slot) const { diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index c9e3adf..0876559 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -1299,6 +1299,35 @@ TEST(ModuleBodyCompiles, ReassignmentInsideForLoopBodyDoesNotSpuriouslyWarn) { for (const std::string& w : warnings) EXPECT_EQ(w.find("overwritten"), std::string::npos) << w; } +// Regression test for a real cache-collision bug: childrenListChunkCache_ +// was keyed by the list's FIRST element alone (a convention borrowed from +// assignBlockChunkCache_, where it IS sufficient) -- but children()'s +// forwarding produces two DIFFERENT lists sharing a first element: bare +// `children()` forwards the caller's whole list, `children(0)` a single- +// element slice of it. Whichever form ran first poisoned the cache for +// the other: here, bare children() cached a two-statement chunk keyed by +// &cubeStmt, then children(0)'s single-element {&cubeStmt} lookup HIT +// that key and emitted BOTH shapes. Fixed by keying on (front, size). +TEST(ModuleBodyCompiles, BareAndIndexedChildrenForwardingDoNotShareACachedChunk) { + ScopedVm vm(true); + // m() emits: children() -> cube+sphere, then children(0) -> cube only. + // 3 bodies total; the collision bug produced 4 (children(0) emitting + // sphere too). + Evaluated e = evalSrc("module m() { children(); children(0); }\n" + "m() { cube(1); sphere(r=1, $fn=8); }"); + EXPECT_EQ(e.bodies.size(), 3u); +} + +// Same collision, opposite statement order -- children(0) caching its +// single-element chunk first used to TRUNCATE the later bare children() +// (2 bodies where 3 belong). +TEST(ModuleBodyCompiles, IndexedThenBareChildrenForwardingDoNotShareACachedChunk) { + ScopedVm vm(true); + Evaluated e = evalSrc("module m() { children(0); children(); }\n" + "m() { cube(1); sphere(r=1, $fn=8); }"); + EXPECT_EQ(e.bodies.size(), 3u); +} + TEST(ModuleBodyCompiles, RecursiveModuleWithIfSucceedsWellPastTheOldNativeLimitCompiled) { ScopedVm vm(true); Evaluated e = evalSrc("module recur(n) { if (n > 0) { recur(n - 1); } else { cube(1); } }\nrecur(10000);"); From 7605a5d5b9ab702b00de61121791324f3504b13a Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 1 Aug 2026 04:57:08 -0700 Subject: [PATCH 2/2] Add Op::CallChildren: eliminate children()'s native reentry; fix indexed-trail out-of-order pop children() was the last dominant source of native C++ stack reentry from compiled code (85 of 93 native-reentry hits in Anklet.scad's real run) -- its statement fell to Op::NativeStatement, whose handler re-entered the VM through six native frames per children() per recursion level, and BOSL2's attachable() calls it at nearly every wrapper level. Unlike Op::PushBuiltinWrap's constructs, the forwarded children are runtime-varying (ctx.childrenNodes/childrenCallerCtx, set by buildModuleChildCtx from the CALLER's call-site AST), so they can't compile inline. Op::CallChildren instead resolves the list at runtime, looks up/compiles its chunk (the same pass-scoped childrenListChunkCache_ tryRunCompiledChildren uses), and pushes it onto vmCallStack_ directly -- mirroring Op::CallModule's zero-native-call push, but with a third frame shape: splice-owning (ownsModuleSplice=true, matching evalModularCall's unconditional splice branch for "children") yet bracketless (children() never gets a callStack_/profiling entry natively either). The completion branch and exception teardown already handle that combination unchanged. The "which nodes, evaluated against what context" half of builtinChildren (the caller-ctx re-derivation, the $-forwarding loop, the children(N) index filtering) is factored into a shared prepareChildrenForward helper so the native and compiled paths cannot drift. Handler ordering is load-bearing: checkDebug against the scope-wrapped ctx (byte-for-byte what Op::NativeStatement did for this node), randsBefore before argument resolution (rands-in-args taint), treeStack_ pushed last (exception safety without a try/catch), and a not-eligible fallback that reuses the already-resolved args instead of re-resolving via evalStatement (which would double rands()/echo/assert side effects). Also fixes a REAL latent bug this surfaced: IndexedScopeTrailStorage:: popLevel did a blind pop_back() per dirty name -- the exact out-of-order- pop corruption its non-indexed twin's own doc comment describes and fixed long ago -- benign only while every dyn-trail view died LIFO. The new forwarding frame's evalCtx (opened after, dying after, the call's own effCtx) is the first real non-LIFO lifetime: popping effCtx's level silently ate the still-live forwarded entry on top (caught for real: children($fn=9) read back as the root default 0 inside the forwarded child). Now level-aware, same as the twin, with a storage-level regression test mirroring the exact level shape. Verification: all 721 tests green on both OSCAD_BYTECODE_VM states; new tests cover the recursive wrap/children() chain at depth 1500 (bare and indexed), $-forwarding parity (wrapper writes and named-$ args), the trail pop fix, and (previous commit) the (front,size) cache re-key. Anklet.scad now renders end-to-end with fully unmodified guards -- 315984 bytes, byte-identical to the disabled-guards reference -- and its peak native reentry depth measured 10 (was 55 originally, 45 after Op::PushBuiltinWrap), 4x inside the Windows-proven-safe ceiling of 40. Bumps version to 0.13.4. Co-Authored-By: Claude Fable 5 --- include/openscad_cpp_evaluator/bytecode.hpp | 48 +++++++- include/openscad_cpp_evaluator/evaluator.hpp | 30 +++++ .../openscad_cpp_evaluator/scope_trail.hpp | 20 +++- pyproject.toml | 2 +- src/bytecode_compiler.cpp | 25 +++- src/bytecode_vm.cpp | 108 ++++++++++++++++++ src/user_calls.cpp | 24 ++-- tests/test_bytecode_compiler.cpp | 53 +++++++++ tests/test_scope_trail.cpp | 30 +++++ 9 files changed, 323 insertions(+), 17 deletions(-) diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index fdedb01..a36b293 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/include/openscad_cpp_evaluator/bytecode.hpp @@ -325,8 +325,50 @@ enum class Op { // same seam, now a counter). PopBuiltinWrap, + // A `children()` / `children(N)` statement -- the runtime-varying + // sibling of Op::CallModule/Op::PushBuiltinWrap, closing the LAST + // dominant native-reentry source (BOSL2's attachable() calls + // children() at nearly every wrapper level; measured 85 of 93 native + // reentries in a real script). Unlike PushBuiltinWrap's constructs, + // the forwarded children aren't known at compile time (they're the + // CALLER's own call-site statements, carried on ctx.childrenNodes/ + // childrenCallerCtx -- see Evaluator::buildModuleChildCtx), so they + // can't compile inline; instead the handler resolves the list at + // RUNTIME, looks up/compiles its chunk (the same + // childrenListChunkCache_ tryRunCompiledChildren uses, via + // lookupOrCompileChildrenListChunk -- gated on useBytecodeVm() && + // inResolvePass_, the cache is pass-scoped), and pushes it onto + // vmCallStack_ directly, mirroring Op::CallModule's own zero-native- + // call push -- but with a THIRD frame shape: splice-owning like a + // module frame (ownsModuleSplice=true, mirroring evalModularCall's + // own unconditional splice branch for "children"), yet bracketless + // like a bare frame (children() never gets a callStack_/profiling + // entry natively either -- only enterUserCall pushes those, and + // resolveChildren/builtinChildren never call it). driveVm's existing + // completion branch and teardownVmCallStackDownTo both already + // handle that combination (their bracket and splice concerns are + // independent). + // + // Handler ordering is load-bearing: checkDebug fires in-handler + // against the scope-wrapped ctx (byte-for-byte what Op:: + // NativeStatement does -- NOT via an emitted Op::CheckDebugStatement, + // whose handler passes the un-wrapped ctx); randsBefore is captured + // BEFORE argument resolution (rands-in-args taint, same lesson + // PushBuiltinWrap already encodes); and treeStack_ is pushed LAST, + // immediately before the frame push / native fallback (an early-out + // or a throw during arg resolution then has nothing to clean up -- + // the native path's own catch{pop;throw} has no equivalent here, so + // the reorder IS the exception-safety mechanism). The not-eligible + // fallback reuses the ALREADY-resolved args (never evalStatement, + // which would re-resolve them: double rands(), double side effects) + // by inlining evalModularCall's own children branch around a native + // evalChildren call. a = index into CompiledChunk::nativeStatements + // (the ModularCall node -- arguments, error position, splice node; + // no separate site table needed). + CallChildren, + // A single "native passthrough" statement -- intersection_for, - // children(), union/difference/intersection and every other builtin + // union/difference/intersection and every other builtin // module call NOT covered by Op::PushBuiltinWrap (see that op's own // doc comment for exactly which builtins ARE covered), the `*` // modifier's own no-op case, or a user-module call that didn't @@ -344,7 +386,9 @@ enum class Op { // pattern that made this op's own native reentry a genuine Windows // crash risk in practice, not just a missed optimization. See // Op::PushBuiltinWrap's own doc comment for the real story and the - // fix.) + // fix. children() fell here too, and was the LAST and largest such + // reentry source once PushBuiltinWrap's own set was covered -- it + // now has Op::CallChildren, above.) // a = index into CompiledChunk::nativeStatements. Runtime just does // what Evaluator::evalChildren's own per-statement loop already does // for one node: derive childCtx via ctx.withScope(...), checkDebug, diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 5b1f2e9..09e5a16 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -271,6 +271,25 @@ class Evaluator { // _builtin_children/_eval_children_lazy. void builtinChildren(const CallArgs& args, EvalContext& ctx); + // The "which nodes, evaluated against what context" half of + // builtinChildren, shared with Op::CallChildren's runtime handler + // (bytecode_vm.cpp) so the native and compiled paths cannot drift on + // any of the subtle parts: the caller-ctx re-derivation, the + // $-forwarding loop (which must read the post-resolveCallArgs effCtx + // -- a children($fn=12) named-$ override lives only there), and the + // children(N) statement-index filtering/bounds-check. nullopt means + // "nothing to evaluate" (no forwarded children in scope, empty list, + // or index out of range) -- a silent no-op in every existing caller, + // exactly matching builtinChildren's own early returns. `ctx` must be + // the effCtx resolveCallArgs returned, same as builtinChildren's own + // parameter today. Public for the same free-function reasoning as + // builtinChildren itself. + struct ChildrenForward { + EvalContext evalCtx; + std::vector nodes; + }; + std::optional prepareChildrenForward(const CallArgs& args, EvalContext& ctx); + // "WARNING: {message}{locSuffix(position)}" via echoFn_, no-op if unset. // Public: builtins/import.cpp's not-manifold warning is emitted from a // free function, same reasoning as tagGenerated()/builtinChildren(). @@ -1149,6 +1168,17 @@ class Evaluator { return bytecodeVmEnabled() && (!debugHooks_.debugHook || fastContinueBreakpoints_.has_value()); } + // Read accessor for inResolvePass_ (private, below) -- Op::CallChildren's + // runtime handler (bytecode_vm.cpp, a free function) must gate its + // childrenListChunkCache_ access on it, exactly like + // tryRunCompiledChildren's own self-gate: the cache is pass-scoped + // (cleared per resolveTreeImpl, AST-address-reuse hazard -- see + // stmtExprChunkCache_'s own doc comment), and driveVm CAN run outside + // the resolve pass (a host calling evalChildren directly reaches + // module opcodes via lookupOrCompileModuleChunk, which is NOT + // pass-scoped). + bool inResolvePass() const { return inResolvePass_; } + private: // The fine-grained half of the check above: even when useBytecodeVm() // says compiling/using bytecode is on the table at all, a SPECIFIC diff --git a/include/openscad_cpp_evaluator/scope_trail.hpp b/include/openscad_cpp_evaluator/scope_trail.hpp index 1f7f36b..681daa9 100644 --- a/include/openscad_cpp_evaluator/scope_trail.hpp +++ b/include/openscad_cpp_evaluator/scope_trail.hpp @@ -368,12 +368,30 @@ class IndexedScopeTrailStorage { dirty_[level].push_back(id); } + // Level-aware, NOT a blind pop_back() -- the exact fix + // ScopeTrailStorage::popLevel (above) already carries, for the exact + // out-of-order-pop bug class its own doc comment describes; this + // indexed twin never got the same fix because nothing violated LIFO + // view destruction on the dyn trail until Op::CallChildren's + // forwarding frame (bytecode_vm.cpp): its evalCtx (a dyn level opened + // AFTER the call's own effCtx level) is moved into a VmFrame and + // OUTLIVES effCtx, so effCtx's pop runs while evalCtx's later entry + // is still physically on top of the same name's stack -- the blind + // pop_back() silently removed the still-live forwarded value instead + // of the one actually being popped (caught for real: a + // children($fn=9) named-$ override read back as the root default 0 + // inside the forwarded child). void popLevel(int level) { auto it = dirty_.find(level); if (it != dirty_.end()) { for (int id : it->second) { auto& stack = stacks_[static_cast(id)]; - if (!stack.empty()) stack.pop_back(); + for (auto rit = stack.rbegin(); rit != stack.rend(); ++rit) { + if (rit->level == level) { + stack.erase(std::next(rit).base()); + break; + } + } } dirty_.erase(it); } diff --git a/pyproject.toml b/pyproject.toml index d801edd..91aac63 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.3" +version = "0.13.4" 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 f5f103a..a82a338 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -1420,12 +1420,25 @@ class Compiler { emitBuiltinWrap(*wrapKind, call.name->name, call, children, out); return; } - // Every other builtin, children(), or a name that didn't - // resolve to a user module statically -- native - // passthrough, exactly like echo/assert/etc. below. Never - // the recursion-depth risk this compiler targets for - // THESE (see NativeStatement's own doc comment, - // bytecode.hpp). + // children() -- the runtime-varying forwarding builtin, + // detected by the same function-pointer-identity pattern + // as Transform/Color above. Deliberately NO preceding + // Op::CheckDebugStatement (unlike emitBuiltinWrap's own + // emission): CallChildren's handler fires checkDebug + // itself against the SCOPE-WRAPPED ctx, byte-for-byte + // matching Op::NativeStatement's own handler -- the + // CheckDebugStatement handler passes the un-wrapped ctx, + // which would be a subtle behavior change for this node. + // See Op::CallChildren's own doc comment (bytecode.hpp). + if (dispatchIt != dispatch.end() && dispatchIt->second == &resolveChildren) { + out.push_back({Op::CallChildren, internNativeStatement(&stmt), 0, &stmt.position()}); + return; + } + // Every other builtin, or a name that didn't resolve to a + // user module statically -- native passthrough, exactly + // like echo/assert/etc. below. Never the recursion-depth + // risk this compiler targets for THESE (see + // NativeStatement's own doc comment, bytecode.hpp). out.push_back({Op::NativeStatement, internNativeStatement(&stmt), 0, &stmt.position()}); return; } diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index d445300..9edeeaf 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -329,6 +329,49 @@ void pushBracketedModuleFrame(Evaluator& ev, const CompiledChunk& chunk, const o ev.vmCallBrackets_.push_back(std::move(handle)); } +// Op::CallChildren's own push -- a THIRD frame shape, distinct from both +// existing helpers: splice-owning like pushBracketedModuleFrame's +// (ownsModuleSplice=true, mirroring evalModularCall's own unconditional +// splice branch for "children", csg_resolve.cpp) but BRACKETLESS like +// pushBareFrame's (nullopt in vmCallBrackets_ -- children() never gets a +// callStack_/profiling entry natively either: only enterUserCall pushes +// those, and resolveChildren/builtinChildren never call it, so no TRACE +// frame, no profile site, no $parent_modules bump, exactly matching the +// native path). driveVm's completion branch and teardownVmCallStackDownTo +// both already handle this combination -- their bracket (`if (bracket)`) +// and splice (`if (ownsModuleSplice)`) concerns are independent at both +// sites. `evalCtx` is prepareChildrenForward's result (the caller-derived, +// $-forwarded context the children must run under -- see that helper's +// own doc comment, evaluator.hpp). hopEligible set explicitly false -- +// pushBracketedModuleFrame itself omits the reset (benign there only +// because module chunks never contain tail-call opcodes); don't inherit +// a pooled function frame's stale true here either. +void pushChildrenForwardFrame(Evaluator& ev, const CompiledChunk& chunk, EvalContext evalCtx, + std::uint64_t randsBefore, const oscad::ASTNode& callNode) { + if (ev.vmCallStack_.size() >= Evaluator::kMaxVmCallStackDepth) { + ev.error("Recursion too deep while forwarding children()", callNode); + } + auto frame = ev.acquireVmFrame(); + frame->chunk = &chunk; + frame->code = &chunk.bodyCode; + frame->pc = 0; + frame->slots.assign(static_cast(chunk.numSlots), Value{}); + frame->stack.clear(); + frame->bound.clear(); + frame->accumStack.clear(); + frame->iterLists.assign(static_cast(chunk.numIterLists), IterList{}); + frame->ctxChain.clear(); + frame->ctxChain.push_back(std::move(evalCtx)); + frame->tailHopGuard = 0; + frame->logicalName.clear(); + frame->hopEligible = false; + frame->ownsModuleSplice = true; + frame->moduleRandsBefore = randsBefore; + frame->moduleSpliceCallNode = &callNode; + ev.vmCallStack_.push_back(std::move(frame)); + ev.vmCallBrackets_.emplace_back(std::nullopt); +} + // Tears down every frame from vmCallStack_'s own top down to (but not // including) `floor`, on the exception path -- releases each VmFrame to // the pool, tears down its ctxChain back-to-front (never relying on @@ -933,6 +976,71 @@ Value driveVm(Evaluator& ev, size_t floor) { } break; } + case Op::CallChildren: { + // Ordering is load-bearing throughout -- see this op's + // own doc comment (bytecode.hpp): checkDebug against + // the SCOPE-WRAPPED ctx (byte-for-byte what Op:: + // NativeStatement does for this same node today); + // randsBefore BEFORE argument resolution (rands-in-args + // taint); treeStack_ pushed LAST, immediately before + // the frame push / native fallback, so an early-out or + // a throw during arg resolution has nothing to clean + // up (the native path's own catch{pop;throw} has no + // equivalent out here -- this reorder IS the + // exception-safety mechanism; arg resolution never + // appends CSG nodes, so pushing after it is + // unobservable). + const auto* callNode = + static_cast(f.chunk->nativeStatements[static_cast(ins.a)]); + EvalContext scopedCtx = ctx.withScope(callNode->scope() ? callNode->scope() : ctx.scope); + ev.checkDebug(*callNode, scopedCtx); + const std::uint64_t randsBefore = ev.randsCallCount(); + auto [args, effCtx] = resolveCallArgs(ev, callNode->arguments, scopedCtx); + std::optional fwd = ev.prepareChildrenForward(args, effCtx); + if (!fwd) { + // No forwarded children in scope / empty / index + // out of range -- a silent no-op, exactly matching + // builtinChildren's own early returns. + ++f.pc; + break; + } + // Pass gate is load-bearing, not defensive -- the + // chunk cache is pass-scoped; see inResolvePass()'s + // own doc comment (evaluator.hpp). + const CompiledChunk* chunk = (ev.useBytecodeVm() && ev.inResolvePass()) + ? ev.lookupOrCompileChildrenListChunk(fwd->nodes) + : nullptr; + if (chunk) { + ev.treeStack_.emplace_back(); + pushChildrenForwardFrame(ev, *chunk, std::move(fwd->evalCtx), randsBefore, *callNode); + // f.pc deliberately NOT advanced -- resumes when + // the pushed frame completes; driveVm's completion + // branch runs the splice (isModule && + // ownsModuleSplice), mirroring evalModularCall's + // own "children" splice branch exactly. + } else { + // Fallback reuses the ALREADY-resolved args -- + // never evalStatement, which would re-resolve them + // (double rands(), double echo/assert side + // effects, double expr-level debug stops). + // Inlines evalModularCall's own children branch: + // push accumulator, run natively, pop, splice. + // The native evalChildren still self-gates and + // retries tryRunCompiledChildren internally. + ev.treeStack_.emplace_back(); + try { + ev.evalChildren(fwd->nodes, fwd->evalCtx); + } catch (...) { + ev.treeStack_.pop_back(); + throw; + } + std::vector> children = std::move(ev.treeStack_.back()); + ev.treeStack_.pop_back(); + ev.spliceModuleChildren(std::move(children), randsBefore, *callNode); + ++f.pc; + } + break; + } case Op::PushBuiltinWrap: { const CompiledChunk::BuiltinWrapSite& site = f.chunk->builtinWrapSites[static_cast(ins.a)]; // Captured BEFORE any argument resolution -- mirrors diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 8a8c7a6..3b8c796 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -911,15 +911,20 @@ Value Evaluator::parentModuleName(int idx) const { return Value{modules[static_cast(revIdx)]}; } -void Evaluator::builtinChildren(const CallArgs& args, EvalContext& ctx) { +std::optional Evaluator::prepareChildrenForward(const CallArgs& args, EvalContext& ctx) { Value idxArg = getArg(args, 0, "index", Value{}); - if (!ctx.childrenNodes || ctx.childrenNodes->empty()) return; + if (!ctx.childrenNodes || ctx.childrenNodes->empty()) return std::nullopt; const EvalContext* callerCtx = ctx.childrenCallerCtx; - if (!callerCtx) return; + if (!callerCtx) return std::nullopt; // A children() forwarding chain's own dyn/let_/etc. must alias the // *caller's* (not this ctx's) -- see EvalContext::withScope's rationale. EvalContext evalCtx = callerCtx->childCtx(nullptr, std::nullopt, callerCtx->childrenNodes, callerCtx->childrenCallerCtx); + // `ctx` here is the post-resolveCallArgs effCtx, deliberately: a + // `children($fn=12)`-style named-$ override lives only at effCtx's own + // trail level, and TrailView::items() is ancestry-visible, so reading + // from effCtx forwards both the call's own overrides AND everything + // the wrapper module's body set (`$fn = 100; children();`). for (const auto& [k, v] : ctx.dyn->items()) { if (!k.empty() && k[0] == '$') evalCtx.dyn->set(k, v); } @@ -928,8 +933,7 @@ void Evaluator::builtinChildren(const CallArgs& args, EvalContext& ctx) { } if (std::holds_alternative(idxArg)) { - evalChildren(*ctx.childrenNodes, evalCtx); - return; + return ChildrenForward{std::move(evalCtx), *ctx.childrenNodes}; } // children(N) indexes child *statements*, not output bodies -- a @@ -944,8 +948,14 @@ void Evaluator::builtinChildren(const CallArgs& args, EvalContext& ctx) { geoNodes.push_back(c); } } - if (idx < 0 || static_cast(idx) >= geoNodes.size()) return; - evalChildren(std::vector{geoNodes[static_cast(idx)]}, evalCtx); + if (idx < 0 || static_cast(idx) >= geoNodes.size()) return std::nullopt; + return ChildrenForward{std::move(evalCtx), {geoNodes[static_cast(idx)]}}; +} + +void Evaluator::builtinChildren(const CallArgs& args, EvalContext& ctx) { + std::optional fwd = prepareChildrenForward(args, ctx); + if (!fwd) return; + evalChildren(fwd->nodes, fwd->evalCtx); } } // namespace oscadeval diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index 0876559..fadc25d 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -1419,6 +1419,59 @@ TEST(ModuleBodyCompiles, UnionWrappedRecursionStillHitsTheNativeReentryGuardCont } } +// The BOSL2 attachable() shape this whole effort targets: a wrapper module +// whose body is just `children();`, applied at every level of a recursive +// chain. children() used to fall to Op::NativeStatement -- one genuine +// native C++ reentry (evalStatement -> evalModularCall -> builtinChildren +// -> evalChildren -> runCompiledModuleBody -> a fresh nested driveVm) per +// level, measured as 85 of 93 native-reentry hits in a real BOSL2 script. +// Op::CallChildren resolves the forwarded list at runtime and pushes its +// chunk onto vmCallStack_ directly (zero native frames), so this now +// SUCCEEDS well past the old ~40-level Windows-safe ceiling. Depth 1500: +// same figure the PushBuiltinWrap per-construct tests use -- comfortably +// past the old ceiling, and tree depth stays flat regardless (children() +// and user-module calls both splice, no kMaxCsgTreeDepth interaction). +TEST(ModuleBodyCompiles, RecursiveChildrenForwardingChainSucceedsWellPastTheOldNativeReentryLimit) { + ScopedVm vm(true); + Evaluated e = evalSrc("module wrap() { children(); }\n" + "module recur(n) { if (n > 0) { wrap() recur(n - 1); } else { cube(1); } }\n" + "recur(1500);"); + ASSERT_EQ(e.bodies.size(), 1u); +} + +// Same chain through the INDEXED form -- children(0) shares Op:: +// CallChildren with the bare form (the index is resolved at runtime by +// the same prepareChildrenForward helper the native path uses), so it +// gets the same zero-native-reentry treatment, not just bare children(). +TEST(ModuleBodyCompiles, RecursiveIndexedChildrenForwardingChainSucceedsWellPastTheOldNativeReentryLimit) { + ScopedVm vm(true); + Evaluated e = evalSrc("module wrap() { children(0); }\n" + "module recur(n) { if (n > 0) { wrap() recur(n - 1); } else { cube(1); } }\n" + "recur(1500);"); + ASSERT_EQ(e.bodies.size(), 1u); +} + +// $-forwarding parity for the compiled children() path: a wrapper module's +// own $-writes (`$fn = 7; children();`) and a children($fn=9)-style +// named-$ override must both reach the forwarded child, exactly as the +// native builtinChildren path forwards them (prepareChildrenForward's own +// $-loop, shared by both paths precisely so they can't drift -- these +// tests are the proof it actually holds end-to-end through the compiled +// opcode, not just by construction). +TEST(ModuleBodyCompiles, ChildrenForwardingCarriesWrapperDollarWritesCompiled) { + ScopedVm vm(true); + EXPECT_EQ(runCapturingEcho("module w() { $fn = 7; children(); }\n" + "w() echo($fn);"), + "ECHO: 7"); +} + +TEST(ModuleBodyCompiles, ChildrenForwardingCarriesNamedDollarArgCompiled) { + ScopedVm vm(true); + EXPECT_EQ(runCapturingEcho("module w() { children($fn = 9); }\n" + "w() echo($fn);"), + "ECHO: 9"); +} + // 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 diff --git a/tests/test_scope_trail.cpp b/tests/test_scope_trail.cpp index ea9f9cf..b82ca69 100644 --- a/tests/test_scope_trail.cpp +++ b/tests/test_scope_trail.cpp @@ -75,6 +75,36 @@ TEST(IndexedScopeTrailStorage, SetAtSameLevelDoesNotAccumulateEntries) { EXPECT_EQ(*storage.lookup("$fn", level), 999); } +// popLevel must remove the entry belonging to THE LEVEL BEING POPPED, not +// blindly pop_back() whatever happens to be physically last -- the exact +// out-of-order-pop bug class ScopeTrailStorage::popLevel's own doc comment +// already describes (and fixed) for the non-indexed twin; this indexed +// variant kept the blind pop until Op::CallChildren's forwarding frame +// (bytecode_vm.cpp) became the first real caller to violate LIFO view +// destruction on the dyn trail. Level shape mirrors that real scenario +// exactly: E (a children() call's own effCtx, opened first) and L (the +// forwarding evalCtx, opened later, moved into a VmFrame) both set the +// same name; E pops FIRST while L is still live. The blind pop removed +// L's entry -- the still-live forwarded value -- leaving a lookup from L +// falling through to the root default (caught for real: children($fn=9) +// read back as 0 inside the forwarded child). +TEST(IndexedScopeTrailStorage, PopLevelRemovesItsOwnEntryNotThePhysicallyLastOne) { + auto intern = std::make_shared(); + IndexedScopeTrailStorage storage(intern); + const int root = storage.openLevel(0); + storage.set("$fn", 0, root); + const int e = storage.openLevel(root); // effCtx's own level + storage.set("$fn", 9, e); + const int l = storage.openLevel(root); // forwarding evalCtx's level, opened AFTER e + storage.set("$fn", 9, l); + storage.popLevel(e); // e dies while l is still live -- non-LIFO + ASSERT_NE(storage.lookup("$fn", l), nullptr); + EXPECT_EQ(*storage.lookup("$fn", l), 9); + storage.popLevel(l); + ASSERT_NE(storage.lookup("$fn", root), nullptr); + EXPECT_EQ(*storage.lookup("$fn", root), 0); +} + // A captured (closure) level's own ancestor must stay alive even after // EVERY other TrailView referencing that ancestor has gone out of scope -- // see TrailView::openChild's own doc comment (parentView_) for why. This