Close the NativeStatement gap and guard its new native-reentry path - #60
Merged
Conversation
evalChildren() now tries compiling its whole statement list (not just its leading assignment run) before falling back to the native per-statement loop, via tryRunCompiledChildren/tryCompileChildrenList. This lets a resolvable module call inside a builtin's own children (e.g. `translate(v) recur(n-1);`) get real Op::CallModule bytecode instead of always falling through evalStatement/evalModularCall. That fix reintroduced a native C++ recursion path that PR #59's explicit frame-stack VM was supposed to have eliminated: a builtin statement still dispatches natively via Op::NativeStatement, and if its children re-enter the VM through the new shortcut, that's a genuine nested native call, not a cheap Op::CallModule hop. A recursive module wrapped in a builtin at every level segfaulted around depth 3000 with no guard at all, since the existing CSG tree-depth guard only fires after a full (already-crashed) descent unwinds. Added driveVmNativeDepth_/kMaxDriveVmNativeDepth in driveVm() to catch this the same way kMaxUserCallDepth already does for the interpreted path -- a clean, catchable error instead of a crash -- while leaving the common case (recursion is pure Op::CallModule, only occasional non-recursing leaves are builtin- wrapped) uncapped by anything but the existing heap-bounded kMaxVmCallStackDepth. Also fixed two pre-existing bugs surfaced by evalChildren's much broader compiled-path coverage: compileForLoop was missing Op::IterReset for inner cartesian dimensions (multi-variable for loops silently under-iterated after the first outer value), and assignBlockChunkCache_ was never actually cleared despite its own doc comment claiming it was. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5 tasks
revarbat
added a commit
that referenced
this pull request
Jul 31, 2026
Bisecting toward Windows' real safe ceiling for this native-reentry chain: 30 is known-safe (PR #60's own successful merge), 50 is now known-unsafe (segfaulted on the guard-safety regression test both at 100 and 50). Trying 40, the midpoint -- still functionally sufficient (all 3 real BOSL2 scripts this guard was regressing still render locally at this value), full suite green locally both VM states. Awaiting CI to confirm Windows safety before considering this settled. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
revarbat
added a commit
that referenced
this pull request
Jul 31, 2026
…; fix two crash bugs found by BOSL2 sweep (#61) * Extend depth-safe compiled path to modifiers; compile leaf statements; fix two real crash bugs found by BOSL2 sweep Item 1: evalModifier now routes its wrapped child through evalChildren instead of a hand-rolled checkDebug+evalStatement pair, so a `#`/`!`- wrapped recursive module call gets the same Op::CallModule depth treatment translate()/union() already have from the NativeStatement-gap fix. intersection_for/let() turned out to already be covered (both already call evalChildren internally). Item 2: Assignment/ModularEcho/ModularAssert/ModularLet now compile to real bytecode instead of falling back to Op::NativeStatement. This closes a real regression: compileOneStatement had no Assignment case at all, so the whole-list compile added for the NativeStatement gap was silently shadowing the older, more specialized tryRunCompiledAssignmentBlock path. New opcodes: CheckDebugStatement, StoreModuleVar, AssertStatement (+ AssertSite, named-arg + eager-evaluation support matching evalAssertStatement's contract, distinct from AssertOp's lazy-message one), OpenLetScope/StoreLetVar. Module chunks can now have nonzero numSlots (a nested let-expression inside one of these statements' own arguments needs real slots, unlike anything else in a module body). Landing item 2 surfaced and fixed two real bugs, both caught by the existing test suite: a `$fn` override leaking past its own let() scope into later statements (inlining sub-expressions bypassed the isolation runCompiledExprChunk used to provide -- fixed with Op::OpenExprScope/ CloseExprScope), and function calls inside these new inline expressions resolving against the wrong lexical scope (compileExpr's PrimaryCall case reads the Compiler's fixed scope_ member directly, not per-statement -- fixed with an RAII scope swap in compileOneStatement). A differential sweep of BOSL2's own test corpus (901 tests, VM on/off) then found two further bugs, unrelated to items 1/2, both confirmed present on the pre-existing PR #60 baseline via git stash before fixing: - kMaxDriveVmNativeDepth=30 was too conservative for real BOSL2 usage -- 3 scripts (test_ball_bearing, test_teardrop_corner_mask, test_rounding_hole_mask) that used to render now failed via BOSL2's own deep _translate()/_show_highlight() wrapper nesting. Empirically raised to 100 (binary-searched: 50 is the minimum that fixes all 3, doubled for headroom) -- re-verified the original crash-safety guarantee still holds at the new value. - A real memory-corruption crash: test_poly_roots segfaulted after printing a fully correct error message. Root-caused via a targeted AddressSanitizer build to Evaluator::releaseVmFrame never resetting ownsModuleSplice/moduleRandsBefore/moduleSpliceCallNode before returning a VmFrame to the pool -- a frame released after owning a module's own splice, later reused (LIFO) for an unrelated non-tail recursive function call, kept ownsModuleSplice=true; when that function call was torn down by an exception thrown mid-recursion, teardownVmCallStackDownTo popped treeStack_ once too many, corrupting it. Fixed by resetting all three fields in releaseVmFrame, matching the reset it already does for stack/slots/bound. Full 708-test suite green both OSCAD_BYTECODE_VM states; BOSL2 differential sweep clean (zero crashes across all 901 tests, down from 2 before this commit). Version bumped 0.12.0 -> 0.13.0 per repo convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Lower kMaxDriveVmNativeDepth 100->50: 100 segfaulted on Windows CI Windows CI's own crash-safety regression test (ModuleBodyCompiles.NativeReentrantRecursionHitsAControlledErrorInsteadOfCrashing) segfaulted at 100 -- confirms this native-reentry chain has a heavier real per-level frame cost than kMaxUserCallDepth's own (which needed the identical kind of comedown, an initial 50 down to 30, for a shallower chain). Set to exactly the locally-determined functional minimum (50, the smallest value that fixes the 3 real BOSL2 scripts this guard was regressing) with no headroom this time, since headroom is what crashed. Re-verified locally: full suite green both VM states, guard-safety test passes, and all 3 BOSL2 scripts still succeed at 50. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Lower kMaxDriveVmNativeDepth 50->40: 50 also segfaulted on Windows CI Bisecting toward Windows' real safe ceiling for this native-reentry chain: 30 is known-safe (PR #60's own successful merge), 50 is now known-unsafe (segfaulted on the guard-safety regression test both at 100 and 50). Trying 40, the midpoint -- still functionally sufficient (all 3 real BOSL2 scripts this guard was regressing still render locally at this value), full suite green locally both VM states. Awaiting CI to confirm Windows safety before considering this settled. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
3 tasks
revarbat
added a commit
that referenced
this pull request
Aug 1, 2026
…ls (#62) Real crash, found via BelfrySCAD's own CI: pausing a debug session deep inside a native (interpreter-forced, e.g. by an active breakpoint on its own line) function call, with a COMPILED caller above it on the call stack, segfaulted the instant anything tried to read that caller's own frame locals (Evaluator::buildDebugFrames, walked by DebugFramesFn/getFrame() -- exactly what a debugger's "step over a call, but still honor a breakpoint set inside it" scenario needs). Root cause: pushBracketedCallFrame/pushBracketedModuleFrame (the explicit-stack VM's own compiled-call push helpers, bytecode_vm.cpp) called enterUserCall with a reference to their OWN local `childCtx` variable. enterUserCall stores that reference as CallStackFrame::bodyCtx for later use ("per-frame locals for the debugger") -- but the real, long-lived copy of that context ends up living in the pushed VmFrame's own ctxChain (heap-allocated, owned by vmCallStack_), not at the address bodyCtx pointed to. The instant either push helper returned, its own local childCtx was destroyed, leaving bodyCtx dangling for the ENTIRE lifetime of that call -- never noticed until something actually reads a compiled caller's own frame locals while a DEEPER (possibly native) call is still active on the stack. Confirmed present since the explicit-stack VM redesign itself (PR #59) -- bisected by rebuilding at each of PR #59/#60/#61's own commits and reinstalling into BelfrySCAD's venv, all three reproduce it. Root- caused via a targeted AddressSanitizer build of the Python bindings (a plain synchronous C++ probe didn't reproduce it -- needed the real cross-thread blocking a GUI debug session's own hook does) plus a threaded Python repro mirroring BelfrySCAD's debugger.py exactly. Fix: reorder both push helpers to build the VmFrame FIRST (so its own ctxChain owns the context), then call enterUserCall with a reference to frame->ctxChain.back() -- stable for the VmFrame's whole lifetime, since VmFrame lives behind a unique_ptr and never moves even if vmCallStack_ itself reallocates. New regression test (DebugHooks. GetFrameDoesNotCrashWalkingACompiledCallerAboveAForcedInterpretedCallee) reproduces the exact shape and crashes the test process without this fix (verified via git stash before landing this commit). Full 709-test suite green both OSCAD_BYTECODE_VM states. Verified against BelfrySCAD's own previously-crashing test (both directly and via its full 306-test suite) using an editable install of this fix. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
evalChildren()now tries compiling its whole statement list (not just the leading assignment run) before falling back to the native per-statement loop, so a resolvable module call inside a builtin's own children (e.g.translate(v) recur(n-1);) gets realOp::CallModulebytecode instead of always dispatching natively — the "NativeStatement gap" from PR Rewrite the bytecode VM onto an explicit heap frame stack #59's own follow-up work.Op::NativeStatementstill dispatches natively, and if its children re-enter the VM through the new shortcut, that's a genuine nested native call. A recursive module wrapped in a builtin at every level segfaulted around depth 3000 with no guard at all (the existing CSG tree-depth guard only fires after an already-crashed descent would unwind). AddeddriveVmNativeDepth_/kMaxDriveVmNativeDepthindriveVm()to catch this the same waykMaxUserCallDepthalready does for the interpreted path — a clean, catchable error instead of a crash — while leaving the common case (recursion is pureOp::CallModule, only occasional non-recursing leaves are builtin-wrapped) uncapped by anything but the existing heap-boundedkMaxVmCallStackDepth.compileForLoopwas missingOp::IterResetfor inner cartesian dimensions (multi-variableforloops silently under-iterated after the first outer value), andassignBlockChunkCache_was never actually cleared despite its own doc comment claiming it was.Test plan
oscad_eval_testssuite (700 tests) green withOSCAD_BYTECODE_VMon (default)OSCAD_BYTECODE_VM=0EvalError("Recursion too deep") instead of segfaulting at depth 3000+Op::CallModulerecursion with an occasional non-recursing builtin-wrapped leaf still succeeds well past the old 30-frame native limit🤖 Generated with Claude Code