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 b56160a..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(). @@ -532,6 +551,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 +981,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 @@ -1115,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 83d5f6d..3b8c796 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 { @@ -897,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); } @@ -914,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 @@ -930,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 c9e3adf..fadc25d 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);"); @@ -1390,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