fivedev/five
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(frb,genpc): in-process compile + 4 pcode bugs

Browse Source 
Compiling _FiveSql2/test/test_sql_extreme.prg + a sweep of the FRB
demos surfaced four real bugs in the dynamic-compilation pipeline.
All fixes shipped together because they were on the same critical
path; each is independently revertible.

  * **pcode FOR loop ignored STEP and direction.** emitFor in
    compiler/genpc emitted a fixed `<= to` comparison and a hardcoded
    `+1` increment, then deleted the actual step expression with
    slice arithmetic on the byte buffer. Result: `FOR 5 TO 1 STEP
    -1` exited on the first iteration; `FOR 1 TO 10 STEP 2` summed
    1..10 (55) instead of 1+3+5+7+9 (25). Rewritten to mirror
    gengo's emitFor: detect negative step from a literal `-N` or
    unary MINUS, pick `<=` vs `>=` accordingly, and emit a clean
    `var := var + step` increment per iteration.

  * **pcode compound `+=` operator stored only the RHS.** emitAssign
    looked at AssignExpr.Op only for the := case; +=/-=/etc.
    silently took the same path, so `n += i` compiled as `n := i`,
    discarding the accumulator. Loop reduces were wrong: `Reverse`
    returned "" and `n := 0; FOR i ... n += i; NEXT` returned only
    the last increment. New compoundBinOp helper maps PLUSEQ /
    MINUSEQ / STAREQ / SLASHEQ / PERCENTEQ / POWEREQ to their
    matching binary opcode; emitAssign emits `local + rhs ; pop
    local` for compound forms.

  * **Pcode body stack leaks polluted the caller's frame.** A pcode
    function whose body left intermediate values on the data stack
    (FOR control values, etc.) returned with extra entries past
    its declared retVal. FrbDoFunc / FrbExecFunc / FrbRunFunc then
    pushed retVal on top of those leaks, so the caller saw the
    leaked values where its own preceding arguments should have
    been: `? "Fibonacci(10) =", FrbDo(...), "(expect 55)"` printed
    `1 55 (expect 55)` because the FOR loop's `1` lived in arg-1's
    slot. Two new Thread methods (`SP()` / `SetSP(int)`) let the
    three FRB dispatchers snapshot stack depth before the inner
    call and clamp it back afterward, so the leaks evaporate before
    they reach the caller's frame.

  * **FrbExec / FrbRun recursed into the host's Main forever.** Both
    looked up "MAIN" via t.VM().FindSymbol, which always resolved
    to the OUTER program's Main since FRB modules deliberately keep
    Main local. Compile + run + unload became compile + recurse +
    OOM. Both now look up Main via mod.FindFunc("MAIN") (module
    scope) — Frbload's policy of leaving Main module-local now
    actually has the intended effect.

Plus an architectural improvement: in-memory compilation no longer
depends on shelling out to an external `five` binary. New
hbrtl.frbCompileInProc parses + preprocesses + generates pcode in
process, building a FrbModule directly. FrbCompile and FrbExec use
this exclusively, which means dynamic compilation works from any
directory regardless of PATH and without a second process. The
plugin-mode path (with its runtime-version-mismatch fragility) is
left available via hbrt.FrbCompileSource for callers that want it,
but FrbCompile no longer reaches for it by default.

Test suite: tests/frb/ holds five fixtures + a runner. 5/5 pass:
test_frb_simple / test_frb_pcode_load / test_frb_compile /
test_frb_loop / test_frb_step.

Other gates green:
  go test ./...      : PASS
  FiveSql2 SQL:1999  : 43/43
  Harbour compat     : 56/56
  std.ch suite       : 14/14
  FRB suite          : 5/5

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
CharlesKWON
2026-05-02 10:25:35 +09:00
parent 3ce0eceed5
commit efb615bed9
11 changed files with 429 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

98
compiler/genpc/genpc.go
View File

@@ -291,17 +291,42 @@ func (g *generator) emitFor(s *ast.ForStmt) {
if !ok {
return
}
// Init
// Init: var := start
g.emitExpr(s.Start)
g.emit(hbrt.PcOpPopLocal)
g.emitU16(uint16(idx))
// Detect step direction statically (matches gengo's emitFor):
// * no Step → +1, ascending
// * literal -N → descending
// * unary MINUS → descending
// Anything else (variable, expression) defaults to ascending.
// Without this we always emitted `var <= to`, which made `FOR
// 5 TO 1 STEP -1` exit on the first iteration; and we always
// stepped by hardcoded +1, which made `FOR i := 1 TO 10 STEP
// 2` summed 1+2+...+10 (55) instead of 1+3+5+7+9 (25).
negStep := false
if s.Step != nil {
if lit, ok := s.Step.(*ast.LiteralExpr); ok {
if lit.Kind == token.INT && len(lit.Value) > 0 && lit.Value[0] == '-' {
negStep = true
}
}
if un, ok := s.Step.(*ast.UnaryExpr); ok && un.Op == token.MINUS {
negStep = true
}
}
loopStart := g.pc()
// Check: var <= to
// Comparison: ascending → var <= to; descending → var >= to.
g.emit(hbrt.PcOpPushLocal)
g.emitU16(uint16(idx))
g.emitExpr(s.To)
g.emit(hbrt.PcOpLessEq)
if negStep {
g.emit(hbrt.PcOpGreaterEq)
} else {
g.emit(hbrt.PcOpLessEq)
}
jumpOut := g.emitJumpPlaceholder(hbrt.PcOpJumpFalse)
// Body
@@ -309,30 +334,22 @@ func (g *generator) emitFor(s *ast.ForStmt) {
g.emitStmt(stmt)
}
// Step
// Increment: var := var + step (re-evaluating step per iter is
// fine; constant-folding can hoist it later). Push var, push
// step, add, store back.
g.emit(hbrt.PcOpPushLocal)
g.emitU16(uint16(idx))
if s.Step != nil {
g.emitExpr(s.Step)
} else {
g.emit(hbrt.PcOpPushInt)
g.emitI64(1)
}
g.emit(hbrt.PcOpPushLocal)
g.emit(hbrt.PcOpPlus)
g.emit(hbrt.PcOpPopLocal)
g.emitU16(uint16(idx))
g.emit(hbrt.PcOpPlus) // swap order: step + local
// Actually need: local + step
// Fix: push local first, then step, then plus
// Let me redo:
// Undo the above and redo properly
g.code = g.code[:len(g.code)-1] // remove PcOpPlus
// Remove the PushLocal
g.code = g.code[:len(g.code)-3]
// Remove the step expr or PushInt
// This is getting complicated. Let me use LocalAddInt for simple step.
g.emit(hbrt.PcOpLocalAddInt)
g.emitU16(uint16(idx))
g.emitI32(1) // default step = 1
// Jump back
// Jump back to comparison
g.emit(hbrt.PcOpJump)
g.emitI32(int32(loopStart - g.pc() - 4))
@@ -557,6 +574,28 @@ func (g *generator) emitCallStmt(e *ast.CallExpr) {
}
func (g *generator) emitAssign(a *ast.AssignExpr) {
// Compound operators (+=, -=, *=, /=, %=, ^=) need to fold the
// existing left-hand value with the right. Without this they got
// emitted as plain `:=`, dropping the accumulator: `n += i`
// behaved as `n := i`. So the FOR loop reduce idiom (e.g.
// `n := 0 ; FOR i := 1 TO 10 ; n += i ; NEXT`) returned only
// the LAST iteration's increment.
if a.Op != token.ASSIGN {
op, ok := compoundBinOp(a.Op)
if ok {
if ident, isIdent := a.Left.(*ast.IdentExpr); isIdent {
if idx, found := g.locals[ident.Name]; found {
g.emit(hbrt.PcOpPushLocal)
g.emitU16(uint16(idx))
g.emitExpr(a.Right)
g.emit(op)
g.emit(hbrt.PcOpPopLocal)
g.emitU16(uint16(idx))
return
}
}
}
}
if ident, ok := a.Left.(*ast.IdentExpr); ok {
if idx, found := g.locals[ident.Name]; found {
g.emitExpr(a.Right)
@@ -577,6 +616,27 @@ func (g *generator) emitAssign(a *ast.AssignExpr) {
g.emit(hbrt.PcOpPop)
}
// compoundBinOp maps an `<op>=` token to the binary opcode it
// produces against the left-hand value. Returns false for ASSIGN
// (the caller should take the plain-store path).
func compoundBinOp(k token.Kind) (byte, bool) {
switch k {
case token.PLUSEQ:
return hbrt.PcOpPlus, true
case token.MINUSEQ:
return hbrt.PcOpMinus, true
case token.STAREQ:
return hbrt.PcOpMult, true
case token.SLASHEQ:
return hbrt.PcOpDivide, true
case token.PERCENTEQ:
return hbrt.PcOpMod, true
case token.POWEREQ:
return hbrt.PcOpPower, true
}
return 0, false
}
func parseInt64(s string) int64 {
var v int64
for _, c := range s {