Extend depth-safe compiled path to modifiers; compile leaf statements; fix two crash bugs found by BOSL2 sweep - #61
Merged
Conversation
…; 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>
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>
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>
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
Item 1 — modifier-wrapped recursion:
evalModifiernow routes its wrapped child throughevalChildreninstead of a hand-rolledcheckDebug+evalStatementpair, so#/!-wrapped recursive module calls get the sameOp::CallModuledepth treatmenttranslate()/union()already have from the NativeStatement-gap fix (#60).intersection_for/let()turned out to already be covered.Item 2 — compile Assignment/echo/assert/let-block statements: these used to fall back to
Op::NativeStatement. This closed a real regression —compileOneStatementhad noAssignmentcase at all, so PR #60's whole-list compile was silently shadowing the older, more specializedtryRunCompiledAssignmentBlockpath. New opcodes:CheckDebugStatement,StoreModuleVar,AssertStatement(+AssertSite, named-arg + eager-evaluation support matchingevalAssertStatement's contract),OpenLetScope/StoreLetVar. Landing this surfaced and fixed two real bugs (both caught by the existing test suite): a$fnoverride leaking past its ownlet()scope into later statements, and function calls inside the new inline expressions resolving against the wrong lexical scope.Two further bugs found by a differential BOSL2 sweep (901 tests, VM on/off), unrelated to items 1/2, both confirmed present on the pre-existing PR #60 baseline before fixing:
kMaxDriveVmNativeDepth=30was too conservative for real BOSL2 usage — 3 scripts that used to render started failing 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.test_poly_rootssegfaulted after printing a fully correct error message. Root-caused via a targeted AddressSanitizer build toEvaluator::releaseVmFramenever resettingownsModuleSplice/moduleRandsBefore/moduleSpliceCallNodebefore returning aVmFrameto the pool — a frame released after owning a module's own splice, later reused (LIFO) for an unrelated non-tail recursive function call, keptownsModuleSplice=true; when that function call was torn down by an exception mid-recursion,teardownVmCallStackDownTopoppedtreeStack_once too many, corrupting it. Fixed by resetting all three fields inreleaseVmFrame.Version bumped 0.12.0 → 0.13.0 per repo convention.
Test plan
oscad_eval_testssuite (708 tests) green withOSCAD_BYTECODE_VMon (default) and offpoly_rootscrash is gone (clean matching-output exit, no ASan error)kMaxDriveVmNativeDepthcrash-safety guarantee still holds at the new threshold (always-wrapped recursive pattern still fails cleanly, not a crash, at n=3000/10000)🤖 Generated with Claude Code