Repository navigation
interpreter: opt-in fastcall depth guard (max_fast_call_depth) - #4220
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is a deliberate C++ ABI break touching the interpreter's hot call path, panic/recover handling across six catch points, and the module-cache/policy versioning, which warrants final human review despite no defects being found.
Review effort: Balanced
Findings: None
What changed in this PR
This PR adds an opt-in guard against unbounded fastcall recursion in the daslang interpreter. Because a fastcall function pushes no das stack frame, deep fastcall recursion runs past the native guard page and crashes the process with nothing a recover can catch (unlike a regular call, which fails cleanly at stack.push). The new CodeOfPolicies::max_fast_call_depth / options max_fast_call_depth caps fastcall nesting; the default (0) leaves the interpreter unchanged.
Changes:
- When the cap is set,
Function::makeSimNodeemits a parallelSimNode_FastCallCheckednode family (including fused one- and two-argument shapes under a new"FastCallChecked"fusion key) that increments/decrementsContext::fastCallDeptharound the body and panics past the cap; at cap 0 only the unchecked nodes are emitted, so the hot path is unaffected. - Every panic handler (
SimNode_TryCatch+ debugger twin,das_try_recover,jit_try_recover, and theevalWithCatch/runWithCatchfamily) now saves/restores the counter, andrestartzeroes it, so a recovered overflow leaves no drift. - ABI/version bookkeeping:
Contextgains two trailing members,CodeOfPoliciesgains a trailing/*option*/field,DAS_POLICIES_VERSION→2 (tail-padding stamp), module-cache streamgetVersion→221; docs, architecture notes, and an unrelatedARCHITECTURE_EMIT.md→ARCHITECTURE_SHADER.mdsplit are included.
| File | Description |
|---|---|
| include/daScript/simulate/simulate.h | Adds fastCallDepth/maxFastCallDepth members, enterCheckedFastCall, and zeroes the counter in restart() |
| include/daScript/simulate/simulate_nodes.h | New SimNode_FastCallChecked<N> templated + variadic node family mirroring SimNode_FastCall |
| src/simulate/simulate_fusion_call1.cpp / call2.cpp | Checked fused op1/op2 node families + "FastCallChecked" fusion registration |
| src/simulate/simulate_visit.cpp | V_OP(FastCallChecked) visit for the new Any base |
| src/simulate/simulate_exceptions.cpp | Counter save/restore at all interpreter/AOT catch points |
| src/builtin/jit_runtime.cpp | Counter save/restore in jit_try_recover |
| src/ast/ast_simulate.cpp | Selects checked nodes when maxFastCallDepth != 0; mirrors option into the context |
| src/runtime/context.cpp | Reads the option in setup; copies maxFastCallDepth into clones |
| include/daScript/simulate/code_of_policies.h | New /*option*/ max_fast_call_depth tail field; DAS_POLICIES_VERSION→2 |
| include/daScript/ast/ast_serializer.h | getVersion()→221 for the policy-stream change |
| src/builtin/module_builtin_ast_serialize.cpp | Adds max_fast_call_depth to the policy X-macro cache-key stream |
| src/builtin/module_builtin_rtti.cpp | Binds the new policy field for rtti |
| tests/language/fast_call_depth.das, _fast_call_depth_recursers.das | End-to-end coverage: all node shapes, no-drift, host-policy/host-catch |
| tests-cpp/small/test_fast_call_depth_context.cpp | Verifies setup/clone/restart counter semantics |
| tests/module_cache/* | Verifies a cap keys a distinct cold cache record |
| docs, ARCHITECTURE*.md, CHANGELIST.md, daslib/shader_lingua_franca.das | Option/policy docs, architecture section, and the EMIT→SHADER doc split |
I verified the core logic in depth and found no objective defects: the checked node family and fused shapes mirror the unchecked paths exactly (added only the increment/decrement), fusion struct names are scoped so there is no redefinition clash, the counter is saved/restored symmetrically at all six catch points, maxFastCallDepth is set before makeSimNode reads it, the clone copies the cap while resetting the counter (matching the C++ test), the printf format matches its args under DAS_FORMAT_PRINT_ATTRIBUTE, version bumps are consistent, and the architecture-doc split leaves no dangling [arch] citations.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f0b6a33 to
8e92d94
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is a declared C++ ABI break touching the interpreter's core call path, exception recovery across six handlers, and the module-cache stream version, which warrants final human verification even though the implementation and tests reviewed as correct.
Review effort: Balanced
Findings: None
8e92d94 to
de21fc2
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is a deliberate C++ ABI break touching the core interpreter call/fusion path, exception/recover handlers, JIT/AOT runtime, and the module-cache serialization version, which warrants final human review despite no defects being found.
Review effort: Balanced
Findings: None
|
Control for the The three failing rows (Arcanoid, Boulder Dash, River Run, wasm) fail to cross-compile on the deployed |
de21fc2 to
4f870a7
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is a deliberate C++ ABI break touching core interpreter call/fusion paths, every exception/recover handler, and the module-cache serialization stream, which warrants final human review despite the implementation appearing correct and well-validated.
Review effort: Balanced
Findings: 1
Open (1)
…unbounded fastcall recursion into a recoverable panic A fastcall function pushes no das stack frame, so a chain of fastcall calls grows only the native stack: a regular call fails cleanly at stack.push, unbounded fastcall recursion runs to the native guard page and the process dies with no panic to recover. The guard is a policy, CodeOfPolicies::max_fast_call_depth (also options max_fast_call_depth), zero by default. When it is set, Function::makeSimNode emits a second node family, SimNode_FastCallChecked, that counts Context::fastCallDepth around the body and panics past the cap naming the option; the fused one- and two-argument shapes get their own FastCallChecked fusion set, so a checked call keeps its superinstructions. The unchecked FastCall family is untouched, so a program with the cap at zero emits only the unchecked nodes. A panic is a longjmp that unwinds no frame, so every handler that restores abiArg after a caught panic restores the counter too (the AOT das_try_recover before its recover body runs), and restart zeroes it. The policy field sits last in the struct because cached JIT DLLs bind the earlier fields by offset, and it lands in the struct's tail padding, so DAS_POLICIES_VERSION moves to 2 for the stamp to refuse a stale host; the module-cache stream version moves to 221. Context gains two trailing members (C++ ABI: external modules rebuild). The test's recursers live in a no_aot module, so the file runs under the interpreter, the JIT and test_aot and each sweep exercises its own recover handler; a tests-cpp case covers setup, clone and restart, and the module-cache suite keys a record on the new policy. daslib/ARCHITECTURE_EMIT.md records the das_try_recover pair and, at the 300-line cap, hands its shader sections to ARCHITECTURE_SHADER.md. AOT TTable::moveT (aot.h) now moves tombstones with the rest of the table header. It was the one field the move skipped, and das_move is a raw copy, so a table returned by value out of to_table_move carried whatever the stack held there into the rehash test of TableHash::reserve; the extra local the counter save puts in runWithCatch changed those bytes, and tests/language/resize_locked.das began rehashing mid-iteration under AOT on darwin, leaving a stale array lock for delete to refuse. tests-cpp/small/test_aot_table_move_tombstones.cpp fails on master and passes with the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4f870a7 to
1b0510c
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is a C++ ABI break touching core interpreter execution, all panic/recover paths, and the module-cache/policy stamp, which warrants final human review despite no defects being found.
Review effort: Balanced
Findings: None

C++ ABI break:
Contextgains two trailing members andCodeOfPoliciesa trailing field (DAS_POLICIES_VERSION2) - external modules and embedding hosts must rebuild.Why. A fastcall function pushes no das stack frame, so unbounded fastcall recursion runs past the native guard page and kills the process; a regular call fails cleanly at
stack.push, and nothing arecovercan see fires on the fastcall path.What changes.
CodeOfPolicies::max_fast_call_depth, alsooptions max_fast_call_depth, caps fastcall nesting; zero, the default, leaves the interpreter unchanged.Function::makeSimNodeemits a second node family,SimNode_FastCallChecked, that countsContext::fastCallDeptharound the body and panics past the cap; the fused one- and two-argument shapes get their ownFastCallCheckedfusion set.abiArgafter a caught panic restores the counter too, andrestartzeroes it, so a recovered overflow leaves no drift; the AOTdas_try_recoverrestores it before the recover body runs.DAS_POLICIES_VERSIONmoves to 2: the new field sits in the struct's tail padding, sosizeofcannot tell a stale host apart.TTable::moveT(aot.h) now movestombstoneswith the rest of the header. It was the one field the move skipped, anddas_moveis a raw copy, so a table returned by value out ofto_table_movecarried whatever the stack held there intoTableHash::reserve's rehash test; one extra local inrunWithCatchchanged those bytes andtests/language/resize_locked.dasstarted rehashing mid-iteration under AOT on darwin, leaving a stale array lock fordeleteto refuse.daslib/ARCHITECTURE_EMIT.mdrecords thedas_try_recoverpair beside thedas_finallyone, which the daslib checklist requires; that bullet takes the file past the 300-line gate, so its shader sections (7, 28, 29) move whole into the newdaslib/ARCHITECTURE_SHADER.md, with the two[arch]citations inshader_lingua_franca.dasrepointed anddaslib/ARCHITECTURE.md's routing line naming both companions.Observable behavior.
stack overflow, max_fast_call_depth <cap> exceeded while calling <fn>, recoverable.max_fast_call_depthis another record (stream version 221).Where to look.
Context::enterCheckedFastCallinsimulate.h, the checked family insimulate_nodes.h, the six catch points insimulate_exceptions.cppandjit_runtime.cpp, and the architecture sectioninclude/daScript/simulate/ARCHITECTURE.md#fastcall-depth-guard.Validation, claims, ledger
Validation
src/andinclude/, the policy and Context layouts, and the module-cache stream.tests-interpandtests-jitonly on the per-file time budget oftest_watchdog.dasandjit_lib.das, the standing Windows reds on this box (both pass alone);utils-testson MSBuild reading daspkg's deliberateerror:refusal lines as errors while every suite's summary is green (a Windows-only lane defect, CI runs the lane on Linux). The fix batch was validated with the targeted gates on the final tip: review-md, lint, ast-verify, docs (all eight cells, sphinx included), tests-cpp, tests-aot (the full suite) all pass; utils-tests reds on the MSBuild parse above with every suite green.tests/language/fast_call_depth.daspasses under the interpreter,-jitand the fulltest_aotbinary (11 arms each). Its recursers live in ano_aotmodule, so each sweep exercises its own recover handler. Thetests/languagesweep is 1860/1860 andtests/module_cache79/79.dastest --cov-path, the debugger) every body is rewritten, nothing is fastcall and the guard counts nothing, so the exactness arms skip there with that reason; the nightly'sextended_checks (linux, all)coverage cell caught the first version failing all six arms on the das-stack overflow of the coverage hook.resize_locked.dasunder AOT on the first rebased tip while master passed it; reproduced 3 of 3 on an M5 worktree of the branch, 0 of 2 on a master worktree beside it, bisected to the handler file by reverting it, then to stack layout (restoring the save in another order or dropping it fromdas_try_recoveralone changed nothing), and the panic textcan't delete locked arrayled to the uninitializedtombstones.tests-cpp/small/test_aot_table_move_tombstones.cppfails on master and passes with the fix.tests-cpp-smallpasses in the lane, with the newtest_fast_call_depth_context.cpp. Run by hand on a module cache minted by the previous binary,dasbind: a registrar served from the module cachefails until the cache is cleared, andjit abi check: a drift is one stderr warningfails identically on a September master binary - neither is this change.--jit-check-abi(a cross-compiled bundle's first launch on the target) was not run: this box has no wasm target. Both new members are appended last, so no bound offset ofContext,CodeOfPoliciesorProgrammoves and the cross-target picture is what it was.nightly_daspkg_index.ymldispatch is yours to arm.// include/daScript/simulate/ARCHITECTURE.md#fastcall-depth-guardpointers (simulate.hmembers,simulate_nodes.hfamily,ast_simulate.cppnode choice) were audited against the section - it matches the code.options max_fast_call_depth(checked by the dastest run oftests/language/fast_call_depth.das),CodeOfPolicies.max_fast_call_depth(checked by the rtti binding and the C++ test),recover(an existing construct, used by the same test).Claims - stated, not tested
SimNodeDebug_TryCatchrestores the counter, butoptions debuggerdisables fastcall, so no capped program reaches it; a break would show only in a build that emits debug try nodes for a fastcall program.LLVM_JIT_CODEGEN_VERSIONis not bumped: every boundProgramfield sits at or beforepolicies, so no offset a cached JIT DLL baked moves.options log_nodes), not by a test that inspects nodes; a lost registration would fall back to the unrolled checked node and count the same.Not done
SimNode_FastCall<N>/SimNode_FastCallChecked<N>as oneSimNode_FastCallT<int, bool CHECKED>withif constexpr(precedentSimNode_AtT), and the fused sets defined once withDAS_FASTCALL_ENTER_##OPNAMEhooks (the unchecked family preprocesses to identical tokens). Declined here because the two-family design was the explicit call; your ruling.Context::callOrFastcall, the entry AOT and JIT code use to call back into an interpreted function, stays uncounted; native code has no guard of its own either.tests/REVIEW.md,include/daScript/ast/REVIEW.md,include/daScript/simulate/REVIEW.md,src/ast/REVIEW.md,src/builtin/REVIEW.md,doc/REVIEW.md,doc/source/stdlib/handmade/REVIEW.md,daslib/REVIEW.md,skills/comment_style_hygiene.md) are held for a rule-doc PR.