From 7629f9523536bb41e0911e7c9dd4be59a476b1fa Mon Sep 17 00:00:00 2001 From: Charles KWON OhJun Date: Wed, 27 May 2026 17:17:48 +0900 Subject: [PATCH] =?UTF-8?q?fix(hbrt):=20variadic=20PValue=20support=20?= =?UTF-8?q?=E2=80=94=20snapshot=20pushed=20args=20per=20frame?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Variadic Harbour functions (`FUNCTION foo(...)`) declare 0 params, so Frame() copied no args into the locals slots PValue/CallerLocal read from. Result: PValue(n) returned whatever happened to live in the caller's LOCAL slot n (i.e. the first LOCAL declared after `(...)`, which loops typically use as a counter). fivenode's AP_RPUTS, AP_ECHO and any other variadic dispatcher that walks PValue() saw garbage — fivenode_go shipped a workaround (AP_RPUTS collapsed to a single arg) that this commit now lets us revert. Mechanism CallFrame now carries `actualArgs []Value`. Frame() snapshots every value the caller pushed (the full pendingParams, not the clipped declared count) into this slice before moving sp. The locals[] declared-param region is unchanged so positional LOCAL access keeps working. CallerLocal reads from actualArgs first. Stack handling tightens slightly: t.sp now ends at `sp - pushedArgs` instead of `sp - localsCopy`, dropping the extra-args slots that variadic callers used to leave on the stack. They're no longer needed — actualArgs is the canonical home — and leaving them on the stack was the root of the original "PValue returns the caller's LOCAL" bug because the next push would overwrite them. Slice reuse: when capacity permits, we slice down rather than reallocating, so the hot path (0/1/few args) keeps its no-alloc characteristics. Verified • Variadic 2/1/0-arg case and fixed-arg comparison all print the expected values (test_variadic_only.prg). • Full suite: go test ./compiler/... ./hbrt/... ./hbrtl/..., Compat 56/56, std.ch 17/17, FRB 7/7, FiveSql2 43/43 all green. Co-Authored-By: Claude Opus 4.7 (1M context) --- hbrt/thread.go | 61 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 14 deletions(-) diff --git a/hbrt/thread.go b/hbrt/thread.go index 372e4e2..795d0ae 100644 --- a/hbrt/thread.go +++ b/hbrt/thread.go @@ -28,6 +28,13 @@ type CallFrame struct { module string // current PRG source file (updated by DebugLine) line int // current PRG source line localNames []string // PRG-source names of params+locals (nil = none registered) + // actualArgs snapshots every value the caller pushed, regardless of + // the callee's declared param count. PValue / CallerLocal read this + // so variadic functions (declared params = 0) can see their args — + // the locals[] slots are reserved for declared LOCALs in that case, + // so the args have nowhere else to live. Reused across calls when + // capacity allows. + actualArgs []Value } // CurFrame returns the current call frame (for closure capture). @@ -292,17 +299,20 @@ func (t *Thread) Frame(params, locals int) { } // Save frame - // Handle case where fewer args were pushed than declared params - actual := t.pendingParams - if actual > params { - actual = params + // pushedArgs is the actual count the caller put on the stack. + // localsCopy is how many of those map onto the callee's declared + // LOCAL slot indexes (clipped at the declared param count). + pushedArgs := t.pendingParams + if pushedArgs > t.sp { + pushedArgs = t.sp } - if actual > t.sp { - actual = t.sp + localsCopy := pushedArgs + if localsCopy > params { + localsCopy = params } frame := &t.calls[t.callSP] - frame.base = t.sp - actual // only actual args on stack + frame.base = t.sp - localsCopy // unchanged: only declared args bind to LOCAL slots frame.localBase = localBase frame.localCount = params + locals frame.paramCount = t.pendingParams // actual args passed by caller (not declared count) @@ -310,18 +320,36 @@ func (t *Thread) Frame(params, locals int) { frame.symbol = t.pendingCallSym t.pendingCallSym = nil - // Copy actual parameters from stack to locals - for i := 0; i < actual; i++ { + // Snapshot every pushed arg BEFORE we move sp — extras would be + // overwritten as soon as this frame's body started pushing for its + // own expression evaluation. Reuse the slice when capacity allows + // to keep the no-arg / few-arg path allocation-free. + if pushedArgs > 0 { + snapStart := t.sp - pushedArgs + if cap(frame.actualArgs) >= pushedArgs { + frame.actualArgs = frame.actualArgs[:pushedArgs] + } else { + frame.actualArgs = make([]Value, pushedArgs) + } + copy(frame.actualArgs, t.stack[snapStart:snapStart+pushedArgs]) + } else if frame.actualArgs != nil { + frame.actualArgs = frame.actualArgs[:0] + } + + // Copy actual parameters from stack to locals (declared-param portion). + for i := 0; i < localsCopy; i++ { t.locals[localBase+i] = t.stack[frame.base+i] } // Initialize missing params and locals to NIL - for i := actual; i < params+locals; i++ { + for i := localsCopy; i < params+locals; i++ { t.locals[localBase+i] = MakeNil() } - // Pop args from stack (they're now in locals) - t.sp = frame.base + // Pop args from stack (they're now in locals + actualArgs). + // Extras beyond declared params (variadic) live only in actualArgs; + // drop the stack slots so the caller can't accidentally re-read. + t.sp = t.sp - pushedArgs t.curFrame = frame t.callSP++ @@ -673,11 +701,16 @@ func (t *Thread) CallerParamCount() int { // CallerLocal returns the n-th parameter of the calling PRG function // (1-based). Returns NIL if no caller frame exists or n is out of range. // Pairs with CallerParamCount for implementing the PValue() RTL. +// +// Reads from the caller frame's actualArgs snapshot — works for both +// plain and variadic callers. The locals[] slots would only carry the +// first declared-param-count values, which fails for FUNCTION foo(...) +// where declared params == 0. func (t *Thread) CallerLocal(n int) Value { if t.callSP >= 2 { caller := &t.calls[t.callSP-2] - if n >= 1 && n <= caller.paramCount { - return caller.GetLocal(n, t.locals) + if n >= 1 && n <= len(caller.actualArgs) { + return caller.actualArgs[n-1] } } return MakeNil()