⏺ Confirmed clean: the only @plg_rt_metacall emission left is #24
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue23"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
#23
the complex-fallback line inside emit_metacall, which is
the sole emitter, wired to both metacall sites. The fix is
complete and verified.
#23 fixed — dispatch trampoline + depth guard
What changed (6 source files, 2 test files):
Runtime:
resolve_simple; wrapped call_goal in the depth guard (the
single chokepoint every metacall/findall/catch funnels
through).
wrapper, returns the compiled entry pointer for simple
predicates, 0 otherwise (builtins/control/errors → walker).
metacall_depth_limit (default 1000).
Codegen:
icmp eq 0 → branch → inttoptr+musttail (fast) /
plg_rt_metacall (fallback); program.rs declares the symbol;
both metacall sites (clause.rs, body.rs) now route through
it.
Verification:
now runs constant-stack to 5,000,000 — and the integration
test proves 100k-deep under a 512KB stack, exactly like
direct recursion.
control-construct-per-level recursion hits the guard →
resource_error(metacall_depth), exit 3; covers findall/3
and catch/3 re-entry; uncatchable (catch can't swallow it).
fallback (disjunction, call/2 partial application,
builtins, undefined→existence_error) all produce identical
results.
15 + v1_errors/control_arith/fact_table), fmt clean, clippy
clean. The v1_errors corpus — which pins ~200 v1 behaviors
byte-for-byte including call/findall/catch — passing is
the byte-identical evidence (the live-oracle diff-test is
the optional path).
integration (constant-stack call/1, graceful guard,
deep-findall graceful, uncatchable).
Net guarantee now: no metacall path can crash below its
bound — call/N tail recursion is constant-stack, and
everything else fails gracefully. That closes #23's "no
crash below the step limit" requirement and the
related-paths note.
Review — #23 fix: metacall trampoline + depth guard
This is the right shape for the bug. The diagnosis ("every
call(p(N))recursion lays down a Rust walker frame that musttail in the IR can't see") leads directly to a two-pronged fix, and each prong covers what the other can't:plg_rt_metacall_resolvepeels onecall/1wrapper, looks up the predicate, and returns its entry pointer; IR doesicmp 0 → inttoptr + musttail. This putscall(p(N))tail recursion on exactly the same constant-stack footing as a direct call.call1_recursion_runs_in_constant_c_stackat 100k under a 512KB ulimit pins this directly.call_goalincrementsmetacall_depth, raises uncatchableresource_error(metacall_depth)past the limit. Bounds everything the trampoline can't take — control constructs at every level,call/Nwith extras — so the worst case is exit 3, never exit 139.Other architectural calls worth calling out:
resolve_simplefactored as the shared chokepoint. Bothdispatch(Rust caller) andplg_rt_metacall_resolve(IR caller) go through it for lookup + arg-register marshalling. The doc explicitly names this discipline ("the two paths cannot drift"). This is exactly the "single source of truth" pattern that PRs #15/#17 introduced for the spans path; reusing it here is the right reflex.emit_metacallas the sole codegen emission point. Both metacall sites (body.rsvariable goal,clause.rscall/N) now route through one helper. The PR description's claim "the only@plg_rt_metacallemission left is the complex-fallback line insideemit_metacall" — confirmed by grep, and that's the right discipline. Future contributors can't accidentally inline a metacall without the trampoline.call_goalis the unified depth chokepoint. Every runtime-walked goal — queries,call/Nwalker fallback,findall/3body,catch/3recovery — funnels here. Bounding this gives a single ceiling for all paths, which is why thedeep_findall_goal_fails_gracefullytest passes without any findall-specific code.plg_rt_metacall_resolvesetsqbarrier = m.cps.len()exactly likeplg_rt_metacalldoes. The doc note pins this explicitly.catch/3shouldn't be able to trap a near-overflow and keep recursing.metacall_depth_error_is_uncatchablepins it.Issues below — none are blockers.
Real concerns
1. The depth-guard decrement is not panic-safe.
call_goal:If
call_goal_innerpanics or unwinds,metacall_depthleaks+1permanently. In a one-shot binary (panic = abort) this doesn't matter. But thewasmcommit on this branch addsRUST_EMBEDDING.md/WASM.mddesign docs that imply an embedding API is being designed — and in an embedding, a panickedcall_goalwhose Machine survives starts tightening the effective ceiling on every reuse until the user's calls fail spuriously. The fix is the sameDrop-based guard pattern PR #17 introduced forErrorSiteGuard:Then
call_goaldoeslet _g = enter_depth(m)?;or similar at the top, and the decrement is impossible to forget or to skip on a panic. With the embedding designs landing as part of this branch, the panic-safety case is near-term, not theoretical.2.
metacall_depth_limithas no out-of-binary configuration. It's a Machine field with a hardcoded 1000 default and no equivalent ofPLG_MAX_STEPS. The integration tests usePLG_MAX_STEPS=1000000000to push the step limit out of the way so the depth guard is what fires — so the precedent for env-overridable limits exists, and the depth guard is exactly the same class of safety bound. WithoutPLG_METACALL_DEPTH(or a CLI flag), a user can't tune for a custom stack — and the test itself usesulimit -s 512, which is precisely the case where you'd want to lower the depth limit too. Add aread_env_or_defaultmirror of the step-limit handling.Small observations
3. The fallback IR path isn't
musttail.emit_metacallemits%r2 = call i32 @plg_rt_metacall(...)followed byret i32 %r2. LLVM will typically TCO this at -O2, butmusttailis what gives the guarantee. The fast path uses musttail explicitly; the fallback doesn't. The depth guard catches the case where this matters (exit 3 at depth 1000, not exit 139 deeper), so it's not a bug — but worth one line inemit_metacall's doc noting "the slow path iscall + ret, notmusttail; the depth guard is what makes it safe." Otherwise a future reader looking at the IR may wonder if they should "fix" it.4.
resolve_goal_ptrrecurses oncall/1peeling. Each peeled wrapper is one Rust recursive call. Bounded by the lexical nesting of the goal term, so realistic programs are safe. A pathological synthetic goal — a heap term that's a 100k-deepcall(call(call(...)))chain — could blow Rust stack inside the resolver before any depth guard fires. Not a real-world concern (no program writes that), but awhile let Some(...) = peel_one_call(goal) { goal = ...; }loop would close the class for free. Stylistic; skip unless someone weaponizes it.5.
qbarrieris set twice in the fallback case — once inplg_rt_metacall_resolve, once inplg_rt_metacall. Same value, no observable difference; just slight redundancy. Could elide the resolver's set when returning 0 (the fallback then sets it), but that loses the symmetry with the fast path. Keep as-is.6. The fast path doesn't bump
metacall_depth— correct, sincemusttailleaves no walker frame to bound. Worth a one-line comment nearplg_rt_metacall_resolvesaying "no depth increment here; the trampoline never enterscall_goal." Otherwise a future reader matching the symmetry ofplg_rt_metacallmay "fix" it.What's good
call/1?" — a clean criterion.resolve_simpleas the lookup chokepoint mirrors theset_formal/ErrorSiteGuarddiscipline from the spans work: one function does the marshalling, both call paths route through it. The doc comment explicitly names the "cannot drift" property — future drift attempts are now caught at code-review time, not at "why are call/N args garbage in production" time.emit_metacallis the only place@plg_rt_metacalllands in the IR. Confirmed by grep in the PR description. This is exactly the cross-check pattern earlier reviews have asked for; doing it explicitly is the right discipline.call_goalas the unified depth chokepoint. Bounding here means findall, catch recovery, and direct queries all get the same ceiling without per-path work.deep_findall_goal_fails_gracefullycovers findall because the chokepoint is shared, not because findall was specifically threaded.ulimit -s 512case (which would lower the overflow point but not below 1000 if the depth guard scales).call/1(fast path), graceful guard (slow path), deep findall (chokepoint generality), uncatchable (safety semantics), golden trampoline shape (IR contract). One test per claim, with clear stdout assertions includingresource_error(metacall_depth)so a regression to a different error class would surface.call,findall, andcatch, which are exactly the constructs most likely to drift.Suggested order
#1 (panic-safe
Dropguard formetacall_depth) and #2 (PLG_METACALL_DEPTHenv override) are worth landing before merge — both are small, both close out concrete gaps. #1 is near-term load-bearing given the embedding designs on the branch; #2 matches the existingPLG_MAX_STEPSprecedent and the tests already need to push the step limit out of the way. #3+ are observations.