Skip to content

Extend Op::PushCsgWrap to intersection_for -- the true last native-reentry gap #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
revarbat merged 2 commits into from
Aug 1, 2026
Merged

Extend Op::PushCsgWrap to intersection_for -- the true last native-reentry gap #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f542265
Extend Op::PushCsgWrap to intersection_for -- the true last gap
revarbat Aug 1, 2026
adb20e8
Merge remote-tracking branch 'origin/main' into intersection-for-csg-…
revarbat Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 91 additions & 60 deletions include/openscad_cpp_evaluator/bytecode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ enum class Op {

// -- CSG-wrap compilation (closes the LAST native-reentry source in ----
// -- the original NativeStatement gap: union()/difference()/ -
// -- intersection()) ----------------------------------------------------
// -- intersection()/intersection_for) -----------------------------------
// union()/difference()/intersection() weren't covered by the original
// Op::PushBuiltinWrap (see that op's own doc comment) because they need
// bespoke bookkeeping PushBuiltinWrap's single all-children bracket
Expand All @@ -362,24 +362,30 @@ enum class Op {
//
// a = index into CompiledChunk::csgWrapSites. Runtime handler: captures
// ev.randsCallCount() BEFORE argument resolution (same rands-in-args
// taint reasoning as PushBuiltinWrap), resolves the (rare, $-only)
// arguments via resolveCallArgs exactly like resolveCsg itself does
// (discarding the positional/named result -- union/difference/
// intersection take no real parameters -- keeping only the possibly-
// $-scoped child ctx), pushes that ctx onto f.ctxChain unconditionally
// (mirrors Transform/Color's own unconditional push -- a bare
// `union() {...}` with no `$fn=...` override still pushes a ctx that's
// merely a copy, cheap and uniform rather than a special-cased branch),
// pushes a fresh ev.treeStack_ frame (every child statement's own
// CSGNode(s), across every group, land flat in this ONE frame --
// mirrors Evaluator::buildTreeNode/evalModularCall's own single
// treeStack_.emplace_back() around the whole call, not one per group),
// and stashes {op, randsBefore, siteIdx, empty groupSizes} onto
// VmFrame::csgWrapStack (a real per-frame LIFO, same reasoning as
// builtinWrapStack: nested/sequenced CSG wraps within one frame's own
// instruction stream, e.g. `union() { difference() {...} }`). The
// compiler always emits a plain Op::CheckDebugStatement immediately
// before this (see emitCsgWrap, bytecode_compiler.cpp), mirroring
// taint reasoning as PushBuiltinWrap); for a `hasArgs` site (union/
// difference/intersection) resolves the (rare, $-only) arguments via
// resolveCallArgs exactly like resolveCsg itself does (discarding the
// positional/named result -- these take no real parameters -- keeping
// only the possibly-$-scoped child ctx) and pushes that ctx onto
// f.ctxChain unconditionally (mirrors Transform/Color's own
// unconditional push -- a bare `union() {...}` with no `$fn=...`
// override still pushes a ctx that's merely a copy, cheap and uniform
// rather than a special-cased branch); intersection_for (`hasArgs`
// false -- it isn't a call, has no `.arguments`) skips both entirely,
// matching resolveIntersectionFor's own use of its caller's ctx
// unchanged (its per-ITERATION child ctxs are a completely separate
// concern, handled by the compiled cartesian loop's own Op::
// ForIterNext, not by this bracket). Either way: pushes a fresh
// ev.treeStack_ frame (every child statement's/iteration's own
// CSGNode(s) land flat in this ONE frame -- mirrors Evaluator::
// buildTreeNode/evalModularCall's own single treeStack_.emplace_back()
// around the whole call, not one per group), and stashes {op,
// randsBefore, siteIdx, empty groupSizes} onto VmFrame::csgWrapStack (a
// real per-frame LIFO, same reasoning as builtinWrapStack: nested/
// sequenced CSG wraps within one frame's own instruction stream, e.g.
// `union() { difference() {...} }`). The compiler always emits a plain
// Op::CheckDebugStatement immediately before this (see emitCsgWrap/
// compileIntersectionForLoop, bytecode_compiler.cpp), mirroring
// emitBuiltinWrap's own pattern -- this is a genuine statement doing
// real work here, not a call transferring control to a declaration.
PushCsgWrap,
Expand Down Expand Up @@ -413,14 +419,19 @@ enum class Op {
// VmFrame::csgWrapStack's own top entry FIRST (before anything that can
// itself throw, e.g. setTreeDepthOrThrow below -- same "exception-
// teardown's own pending count must already be right" reasoning as
// PopBuiltinWrap), pops the ctx PushCsgWrap unconditionally pushed,
// PopBuiltinWrap), pops the ctx Push pushed (only when `hasArgs`),
// pops ev.treeStack_ to retrieve every group's own CSGNode(s) (flat,
// exactly like resolveCsg's own `children` result -- group boundaries
// live only in group_sizes, never in the CSGNode list's own shape), and
// builds the tagged CSGNode exactly like Evaluator::buildTreeNode's own
// post-resolveBody() half does, with params = {"op": site.op,
// "group_sizes": ValueList(pending.groupSizes)} -- byte-for-byte what
// native resolveCsg returns -- pushing the result onto the new top of
// post-resolveBody() half does, with params = {"group_sizes":
// ValueList(pending.groupSizes)} plus, only when `includeOpParam`,
// "op": site.op -- byte-for-byte what native resolveCsg returns for
// union/difference/intersection (generateCsg needs "op" to
// disambiguate the ONE function it shares across all 3) and what
// native resolveIntersectionFor returns (no "op" key at all --
// generateIntersectionFor is its own dedicated, separately-registered
// function, never needs one) -- pushing the result onto the new top of
// treeStack_. a = index into CompiledChunk::csgWrapSites (same site
// Push used).
//
Expand Down Expand Up @@ -474,32 +485,32 @@ enum class Op {
// no separate site table needed).
CallChildren,

// A single "native passthrough" statement -- intersection_for, every
// OTHER builtin module call not covered by Op::PushBuiltinWrap/
// Op::PushCsgWrap (see those ops' own doc comments for exactly which
// builtins ARE covered -- cube/sphere/import/text/surface/etc., the
// true LEAVES that never take a `children` block at all, so wrapping a
// recursive call inside one isn't even syntactically possible), the `*`
// modifier's own no-op case, or a user-module call that didn't
// resolve at compile time (shadowed, forward-declared, or otherwise
// not statically known) -- anything compileStatementList doesn't give
// its own real bytecode. (Assignment/ModularEcho/ModularAssert/
// ModularLet used to fall here too; they now have their own real
// bytecode -- Op::StoreModuleVar/Op::Echo/Op::AssertStatement/
// Op::OpenLetScope+StoreLetVar -- purely a throughput change, since
// none of these were ever the recursion-depth risk this compiler
// targets. `#`/`%`/`!` modifiers and translate/rotate/scale/mirror/
// multmatrix/resize/color used to fall here too, for the SAME
// throughput reasoning -- WRONG in that one specific case: a
// recursive module call wrapped in one of these is exactly the
// 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. 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. union()/difference()/intersection()
// fell here too, and were the last remaining REAL native-reentry risk
// (bespoke group_sizes bookkeeping meant they couldn't just reuse
// A single "native passthrough" statement -- every builtin module call
// not covered by Op::PushBuiltinWrap/Op::PushCsgWrap (see those ops'
// own doc comments for exactly which builtins ARE covered -- cube/
// sphere/import/text/surface/etc., the true LEAVES that never take a
// `children` block at all, so wrapping a recursive call inside one
// isn't even syntactically possible), the `*` modifier's own no-op
// case, or a user-module call that didn't resolve at compile time
// (shadowed, forward-declared, or otherwise not statically known) --
// anything compileStatementList doesn't give its own real bytecode.
// (Assignment/ModularEcho/ModularAssert/ModularLet used to fall here
// too; they now have their own real bytecode -- Op::StoreModuleVar/
// Op::Echo/Op::AssertStatement/Op::OpenLetScope+StoreLetVar -- purely
// a throughput change, since none of these were ever the recursion-
// depth risk this compiler targets. `#`/`%`/`!` modifiers and
// translate/rotate/scale/mirror/multmatrix/resize/color used to fall
// here too, for the SAME throughput reasoning -- WRONG in that one
// specific case: a recursive module call wrapped in one of these is
// exactly the 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. 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. union()/difference()/
// intersection() fell here too, and were the last remaining REAL
// native-reentry risk among ModularCall-shaped statements (bespoke
// group_sizes bookkeeping meant they couldn't just reuse
// PushBuiltinWrap's own bracket) -- they now have Op::PushCsgWrap,
// above. hull()/minkowski()/render()/linear_extrude()/rotate_extrude()/
// projection()/offset()/roof() fell here too, closing out every
Expand All @@ -508,11 +519,15 @@ enum class Op {
// extrude, etc.) -- they now share Op::PushBuiltinWrap's own bracket
// via 6 new Kind values (Passthrough/LinearExtrude/RotateExtrude/
// Projection/Offset/Roof), reusing the exact same mechanism rather
// than inventing another. Only intersection_for remains -- its
// group_sizes are keyed by RUNTIME loop iteration, not a static
// source-statement list the way union/difference/intersection's are,
// so it can't reuse either existing bracket shape without new loop-
// compilation machinery; still deliberately native as of this writing.)
// than inventing another. intersection_for -- NOT a ModularCall at
// all, its own NodeKind -- fell here too and was the true LAST
// native-reentry gap; it now shares Op::PushCsgWrap's own bracket
// (CsgWrapSite::hasArgs/includeOpParam both false for it) wrapped
// around a compiled cartesian-product loop reusing Op::ForIterNext/
// ForIterEnd/NativeIterMaterialize verbatim (see
// compileIntersectionForLoop, bytecode_compiler.cpp) -- one Op::
// CsgGroupStart/CsgGroupEnd pair per full iteration instead of per
// source statement. Every non-leaf builtin construct is covered now.)
// 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,
Expand Down Expand Up @@ -880,14 +895,30 @@ struct CompiledChunk {
// One Op::PushCsgWrap/PopCsgWrap site pair -- see those ops' own doc
// comments for the full "why union/difference/intersection need
// bespoke group_sizes bookkeeping instead of just reusing
// BuiltinWrapSite" rationale. `op` is always "union"/"difference"/
// "intersection" (the only 3 names resolveDispatch() maps to
// resolveCsg) -- always a genuine ModularCall (unlike BuiltinWrapSite's
// Modifier kind, this construct has no non-ModularCall variant), so
// `node` is typed precisely rather than a generic ASTNode*.
// BuiltinWrapSite" rationale. `op` is "union"/"difference"/
// "intersection" (the 3 names resolveDispatch() maps to resolveCsg,
// always a genuine ModularCall) OR "intersection_for" (NOT a
// ModularCall -- a distinct NodeKind/ModuleInstantiation subtype with
// its own `.assignments`/`.body`, see compileIntersectionForLoop,
// bytecode_compiler.cpp) -- `node` is therefore the generic ASTNode*
// BuiltinWrapSite's own Modifier kind already established this pattern
// for, down-cast to ModularCall only when `hasArgs` is true.
//
// `hasArgs`/`includeOpParam` are both true for union/difference/
// intersection, both false for intersection_for: it isn't a call at
// all (no `.arguments`, so Op::PushCsgWrap's handler must skip
// resolveCallArgs/the ctx push entirely -- matches native
// resolveIntersectionFor, which uses the SAME ctx its caller passed in
// unchanged, never a $-scoped child of its own), and its own
// CSGParams has no "op" key at all (generateIntersectionFor is a
// dedicated, separately-registered generate function, unlike
// generateCsg which needs "op" in params to disambiguate union/
// difference/intersection from ONE shared function).
struct CsgWrapSite {
std::string op;
const oscad::ModularCall* node = nullptr;
bool hasArgs = true;
bool includeOpParam = true;
const oscad::ASTNode* node = nullptr;
};

// One Op::AssertStatement site -- see that op's own doc comment for
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build"

[project]
name = "openscad_cpp_evaluator"
version = "0.13.6"
version = "0.13.7"
description = "C++ OpenSCAD evaluator with Python bindings"
readme = "README.md"
requires-python = ">=3.12"
Expand Down
82 changes: 77 additions & 5 deletions src/bytecode_compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,6 +1241,76 @@ class Compiler {
out.push_back({Op::PopCsgWrap, idx, 0, &call.position()});
}

// intersection_for(assignments...) { body } -- the cartesian-product
// analog of emitCsgWrap: same bracket (CsgWrapSite::hasArgs=false,
// includeOpParam=false -- see that struct's own doc comment,
// bytecode.hpp), but each "group" is one full loop ITERATION's own
// contribution instead of one top-level source STATEMENT's, mirroring
// resolveIntersectionFor's own two-level shape exactly (control.cpp):
// a cartesian-product loop over `node.assignments`, and at the
// innermost base case, ONE currentTreeFrameSize()-delta group per full
// iteration. Reuses compileForLoop's own cartesian-loop scaffold
// verbatim (Op::NativeIterMaterialize/ForIterNext/ForIterEnd/
// IterReset) -- Op::ForIterNext's own ctx construction,
// `ctx.childCtx(nullptr, std::nullopt, ctx.childrenNodes,
// ctx.childrenCallerCtx)`, is exactly what resolveIntersectionFor's
// own recurse lambda does too (`parentCtx.childCtx(nullptr,
// std::nullopt, ctx.childrenNodes, ctx.childrenCallerCtx)`), since
// childrenNodes/childrenCallerCtx propagate unchanged through every
// nested childCtx either way -- just with Op::CsgGroupStart/
// CsgGroupEnd wrapped around the body instead of a bare
// compileStatementList. The leading checkDebug is only emitted when
// node.body isn't empty, mirroring resolveIntersectionFor's own `if
// (!bodyNodes.empty())` guard exactly (unlike compileForLoop's own
// ModularFor sibling, which always emits one even for an empty body,
// against the FOR node itself as a fallback marker -- intersection_for
// has no such native fallback, so neither does this) -- but
// CsgGroupStart/CsgGroupEnd themselves are UNCONDITIONAL, since native
// measures a (possibly zero-size) group regardless of body emptiness.
void compileIntersectionForLoop(const oscad::ModularIntersectionFor& n, std::vector<Instruction>& out) {
out.push_back({Op::CheckDebugStatement, internNativeStatement(&n), 0, nullptr});
CompiledChunk::CsgWrapSite site;
site.op = "intersection_for";
site.hasArgs = false;
site.includeOpParam = false;
site.node = &n;
chunk_.csgWrapSites.push_back(std::move(site));
const int idx = static_cast<int>(chunk_.csgWrapSites.size()) - 1;
out.push_back({Op::PushCsgWrap, idx, 0, &n.position()});

const size_t numDims = n.assignments.size();
std::vector<int> iterListIds(numDims);
for (size_t d = 0; d < numDims; ++d) {
iterListIds[d] = nextIterList_++;
out.push_back({Op::NativeIterMaterialize, internNativeExpr(n.assignments[d]->expr.get()), iterListIds[d],
&n.assignments[d]->position()});
}
std::vector<size_t> topIdx(numDims);
std::vector<size_t> forIterNextIdx(numDims);
for (size_t d = 0; d < numDims; ++d) {
if (d > 0) out.push_back({Op::IterReset, iterListIds[d], 0, nullptr});
topIdx[d] = out.size();
forIterNextIdx[d] = out.size();
Instruction ins;
ins.op = Op::ForIterNext;
ins.a = internName(n.assignments[d]->name->name);
ins.b = iterListIds[d];
ins.node = n.assignments[d].get();
out.push_back(ins);
}
if (!n.body.empty()) {
out.push_back({Op::NativeCheckDebugExprLevel, internNativeStatement(n.body.front().get()), 0, nullptr});
}
out.push_back({Op::CsgGroupStart, 0, 0, nullptr});
compileStatementList(n.body, out);
out.push_back({Op::CsgGroupEnd, 0, 0, nullptr});
for (size_t i = numDims; i-- > 0;) {
out.push_back({Op::ForIterEnd, static_cast<int>(topIdx[i]), 0, nullptr});
out[forIterNextIdx[i]].c = static_cast<int>(out.size());
}
out.push_back({Op::PopCsgWrap, idx, 0, &n.position()});
}

void compileOneStatement(const oscad::ASTNode& stmt, std::vector<Instruction>& out) {
using oscad::NodeKind;
trackSpan(stmt);
Expand Down Expand Up @@ -1550,18 +1620,20 @@ class Compiler {
compileForLoop(static_cast<const oscad::ModularFor&>(stmt), out);
return;
}
case NodeKind::ModularIntersectionFor: {
compileIntersectionForLoop(static_cast<const oscad::ModularIntersectionFor&>(stmt), out);
return;
}
default:
// ModularModifierDisable (`*`) -- deliberately native: its
// child never evaluates at all (see evalStatement's own
// ModularModifierDisable case, stmt_eval.cpp), so there's
// no recursion to eliminate here in the first place.
// intersection_for -- deliberately native (bespoke
// per-statement grouping, same reasoning as union/
// difference/intersection; not covered by
// Op::PushBuiltinWrap, see that op's own doc comment).
// `#`/`%`/`!` and translate/rotate/scale/mirror/multmatrix/
// resize/color have their own real bytecode cases now,
// above; echo/assert/assignment/let-block already did.
// above; echo/assert/assignment/let-block already did;
// intersection_for has its own case now too (above),
// reusing Op::PushCsgWrap via compileIntersectionForLoop.
out.push_back({Op::NativeStatement, internNativeStatement(&stmt), 0, &stmt.position()});
return;
}
Expand Down
Loading