fix(compiler,hbrt,hbrdd,cli): pre-1.0 audit — 13 critical fixes

Senior-engineer / QA audit landed 13 silent-miscompile and data-
integrity fixes spanning the whole compiler+runtime+storage stack.
Each fix is paired with either an integration test in the suite or
a focused regression check; all 6 release gates stay green:
go test ./..., FiveSql2 43/43, Harbour compat 56/56, std.ch 17/17,
FRB 7/7, examples 65/71.

Compiler
--------

* genpc IF/ELSEIF jumpEnd2 patching (compiler/genpc/genpc.go).
  Per-ELSEIF branch terminators were stashed into `_ = jumpEnd2`
  and never patched — the relative offset stayed 0 and the runtime
  walked the next ELSEIF's PcOpJumpFalse opcode as if it were
  jump-offset data. Bytecode-level corruption in pcode mode. Now
  collected into a slice and patched at end-of-IF. Verified via
  Grade(95..50) cases 11a-e added to tests/frb/test_frb_pcode_sweep.

* countLocalsInStmts / scanBodyLocals missing bodies
  (compiler/gengo/gen_util.go, compiler/gengo/gengo.go). Frame-size
  counter skipped WATCH/TIMEOUT/PARALLEL FOR bodies, so a LOCAL
  declared inside one of those constructs got a slot index past
  the runtime's allocated count — silent NIL reads or out-of-range
  stomps.

* emitMethodDeclStandalone nested LOCAL (compiler/gengo/gen_class.go).
  Same bug class but on the *method* side. Pre-fix repro:

      METHOD Stomp(n) CLASS T
         LOCAL a := 1, b := 2
         IF n > 0
            LOCAL c := 30, d := 40, e := 50, f := 60
            Inner( n )
            IF c != 30 .OR. d != 40 .OR. e != 50 .OR. f != 60 ...

  printed `c, d, e, f = 5, NIL, NIL, NIL` because Inner's frame
  collided with Stomp's underallocated slot range. Now counts
  body-nested LOCALs into the frame and pre-allocates indices via
  scanBodyLocals.

* genpc unsupported-AST diagnostic surface (compiler/genpc/genpc.go,
  hbrt/pcode.go, cmd/five/main.go, hbrtl/frb.go). The `default`
  cases in emitStmt / emitExpr silently emitted PushNil / no-op
  for nodes the pcode generator doesn't implement (ClassDecl,
  MethodDecl, xBase commands, concurrency primitives, …). Added
  `PcodeModule.Warnings []string` populated by noteUnsupported,
  surfaced on stderr from the build pipeline. Users now see
  "pcode: AST node not supported in --pcode/FRB-pcode mode: stmt
  *ast.GoBlockStmt" instead of getting a silently broken module.

Runtime
-------

* class.go Send/tryBinaryOp t.self defer-restore (hbrt/class.go).
  Restoration was a plain `t.self = oldSelf` after `fn(t)`. Any
  panic in the method body skipped the line, so the next BEGIN
  SEQUENCE / RECOVER handler ran with the THROWING object's Self
  — `::field` resolved against the wrong receiver. Wrapped both
  restore sites in `defer func() { t.self = oldSelf }()`.
  Verified: pre-fix RECOVER saw "THROWER", post-fix "OUTER".

* hbfunc.go HB_FUNC parameter Frame() (hbrt/hbfunc.go). The
  RegisterDynamicFunc wrapper called `fn(ctx)` without ever
  calling Frame, so `ctx.ParC(1)` / `ctx.Local(n)` read through
  `t.curFrame.localBase + n - 1` against the *caller's* frame.
  Every #pragma BEGINDUMP HB_FUNC taking parameters silently
  returned "" / 0 / "" for them — masked by ParNIDef-style
  defaults. Wrapper now does `t.Frame(t.pendingParams, 0); defer
  t.EndProc()` before dispatch.

* pcode codeblock closure capture (hbrt/pcinterp.go, hbrt/pcode.go,
  hbrt/thread.go, compiler/genpc/genpc.go). PcOpPushBlock recorded
  `nDetached` but never copied enclosing locals; free vars in the
  block body fell through to memvar lookup → NIL. Wired full
  capture pipeline:
  - New opcodes PcOpPushDetached (0x59) / PcOpPopDetached (0x5A).
  - PushBlock now reads per-slot source-local indices and
    snapshots into bb.Detached at construction time.
  - New detachedMap in genpc auto-promotes any free var that
    resolves to an enclosing-frame local into a capture slot.
  - emitAssignAsExpr leaves the assigned value on the eval stack
    so SeqExpr items like `{|v| acc += v, acc }` work.
  - Thread tracks curBlock with paired Set/restore in the block's
    Fn wrapper for nested-block evaluation.
  Mutating capture (acc += v across successive Evals) now works.

* vm.NewThread statics + waFactory propagation (hbrt/vm.go).
  GoLaunch / GoLaunchBlock call NewThread directly. Previously
  the statics map and WA factory were applied only in Run(), so
  goroutine-spawned PRG code panicked on STATIC access ("static
  index out of range") and crashed dereferencing nil WA on any
  DB call. Both now happen inside NewThread under the same lock
  as TID assignment.

Data layer
----------

* dbf concurrent Append lock (hbrdd/dbf/dbf.go,
  hbrdd/dbf/locks_posix.go, hbrdd/dbf/locks_windows.go). Append
  bumped a local recCount with no file-system serialization. Two
  shared-mode processes both wrote at the same RecordOffset; one
  record silently overwrote the other. Added an append-intent
  byte-range lock at offset 0x7FFFFFFE + bounded retry, on-disk
  header refresh inside the locked region, and immediate header
  write so peers refresh past our slot.

* indexer negative numeric key encoding (hbrdd/dbf/indexer.go +
  new hbrdd/dbf/encode_numeric_test.go). `%20.10f` formats `-100`
  as `"     -100.0000000000"` and `99` as `"        99.0000000000"`.
  ASCII ' ' (0x20) < '-' (0x2D), so `99` lex-compared LESS than
  `-100` — every NTX/CDX index over a column that ever held a
  negative number returned wrong rows for SEEK / range scans.
  Replaced with a 1-byte sign prefix + 21-byte zero-padded
  magnitude (negatives use digit-complement) so byte order
  matches numeric order across signs and magnitudes. Format
  change: existing indexes built with the old encoding must be
  REINDEXed. Three unit tests pin the order.

* dbf Append index maintenance hooks (hbrdd/dbf/dbf.go,
  hbrdd/dbf/indexer.go). Append never inserted into open NTX/CDX
  indexes — the audit's canonical scenario `SET INDEX TO …;
  APPEND BLANK; REPLACE …; dbSeek …` silently missed the new
  record. Added optional IndexWriter interface, queue the new
  recNo in pendingIdxInserts, drain after flushRecord by calling
  InsertKey on every open writer-supporting engine. NTX
  participates (its existing rebuild-on-insert is correct);
  CDX online maintenance is deferred to a follow-up — those
  indexes still need REINDEX. Verified: post-fix SEEK("Charlie")
  after APPEND BLANK + REPLACE finds the new record.

* dbf PACK crash-safety (hbrdd/dbf/dbf.go). The old in-place
  rewrite read record N, overwrote slot M<N, then truncated.
  Power loss after partial loop left a file with overwritten
  prefix and no original copies of the records already advanced
  past — silent data loss. Rewrote to:
    1) drop mmap, build `<file>.pack.tmp` with all surviving
       records,
    2) Sync(),
    3) close original handle + os.Rename(tmp, orig) (atomic on
       same FS),
    4) reopen + re-mmap.
  TestComp_Pack passes; readers always see either the pre-PACK
  or post-PACK contents, never a half-state.

* mem RDD torn reads (hbrdd/mem/memrdd.go). The comment claimed
  in-place PutValue was safe because hbrt.Value "fits in a
  single machine word + pointer". hbrt.Value is 24 bytes (3
  words) — a concurrent reader could observe new type tag with
  stale scalar/ptr and type-confuse on the next AsXxx() call.
  Switched mu to sync.RWMutex; GetValue takes RLock,
  Append/PutValue/Delete/Recall take Lock. `go test -race
  ./hbrdd/mem/` clean.

Files touched
-------------

  compiler/gengo/gen_class.go, gen_util.go, gengo.go
  compiler/genpc/genpc.go
  hbrt/class.go, hbfunc.go, pcinterp.go, pcode.go, thread.go, vm.go
  hbrdd/dbf/dbf.go, indexer.go, locks_posix.go, locks_windows.go
  hbrdd/dbf/encode_numeric_test.go  (new)
  hbrdd/mem/memrdd.go
  cmd/five/main.go
  hbrtl/frb.go
  tests/frb/test_frb_pcode_sweep.prg

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-05-13 05:29:56 +09:00
parent c5dd74c044
commit cde86730b8
19 changed files with 832 additions and 49 deletions

View File

@@ -279,9 +279,16 @@ func (t *Thread) Send(methodName string, nArgs int) {
panic(t.runtimeError(fmt.Sprintf("unknown method %s in class %s", methodName, cls.Name)))
}
// Set up Self context
// Set up Self context. Restore via defer so a panic in the
// method body (HbError, BreakValue from `Break(…)`, runtime
// panic) unwinds with `t.self` pointing at the caller's
// receiver — not at this method's. Without defer, a RECOVER
// USING handler that runs after the panic still saw the stale
// `t.self`, so `::field` / `::method()` resolved against the
// wrong object — silent data corruption in the recovery path.
oldSelf := t.self
t.self = objVal
defer func() { t.self = oldSelf }()
// Push args for Frame
for _, arg := range args {
@@ -291,9 +298,6 @@ func (t *Thread) Send(methodName string, nArgs int) {
t.pendingParams = nArgs
fn(t)
// Restore Self
t.self = oldSelf
// Push return value
t.push(t.retVal)
}
@@ -330,10 +334,11 @@ func (t *Thread) tryBinaryOp(op int) bool {
t.pop() // discard a (Self takes over)
oldSelf := t.self
t.self = a
// defer restore — see comment in Send.
defer func() { t.self = oldSelf }()
t.push(b)
t.pendingParams = 1
fn(t)
t.self = oldSelf
t.push(t.retVal)
return true
}

View File

@@ -42,8 +42,22 @@ type HBContext struct {
// HB_FUNC registers a Go function as a Harbour-callable function.
// Equivalent to Harbour's HB_FUNC(name) macro.
//
// The wrapper sets up a runtime Frame so `ctx.ParC(n)` / `ctx.Local(n)`
// resolves through *this* function's locals instead of leaking into
// the caller's frame. Without Frame the body's `t.Local(n)` indexed
// `t.curFrame.localBase + n - 1` against whatever frame the caller
// happened to be on — silently reading the caller's locals, and
// any default-value path (`ParNIDef`) hid the bug by masking NIL.
func HB_FUNC(name string, fn func(ctx *HBContext)) {
RegisterDynamicFunc(strings.ToUpper(name), func(t *Thread) {
// Snapshot pendingParams before Frame consumes it; the body
// expects to declare nParams=actual so each positional arg
// lands in a fresh slot, but EndProc/EndProcFast cleanup
// must run regardless of how the body returns.
nArgs := t.pendingParams
t.Frame(nArgs, 0)
defer t.EndProc() // retains panic propagation for RECOVER
ctx := &HBContext{T: t}
fn(ctx)
})

View File

@@ -298,6 +298,20 @@ func execPcodeBody(t *Thread, fn *PcodeFunc, mod *PcodeModule) {
nDetached := int(binary.LittleEndian.Uint16(code[pc:]))
pc += 2
// Snapshot closure-captured locals from the *current*
// frame into the new block's Detached slice. The body
// reads/writes them via PcOpPushDetached / PcOpPopDetached
// at the indices the compiler reserved. Without this,
// `{|x| x + outer }` saw `outer` as NIL because the
// block fn ran with its own frame and the body's lookup
// for `outer` fell through to the memvar table.
captured := make([]Value, nDetached)
for i := 0; i < nDetached; i++ {
srcIdx := int(binary.LittleEndian.Uint16(code[pc:]))
pc += 2
captured[i] = t.Local(srcIdx)
}
// Create a Go function that interprets the block's pcode.
// Params count must be threaded through so ExecPcode's
// Frame() pulls Eval()'s args off the stack into the
@@ -305,9 +319,42 @@ func execPcodeBody(t *Thread, fn *PcodeFunc, mod *PcodeModule) {
// and `x * x` panicked on the multiplication.
blockFn := &PcodeFunc{Code: blockCode, Params: nParams}
modCopy := mod
t.PushBlock(func(t2 *Thread) {
blockVal := MakeBlock(nil, nDetached) // Fn patched below
bb := (*HbBlock)(blockVal.ptr)
if nDetached > 0 {
copy(bb.Detached, captured)
}
bb.Fn = func(t2 *Thread) {
// Install this block as the currently-executing
// block so PcOpPushDetached / PcOpPopDetached can
// resolve their slots. Restore the previous one on
// exit so nested-block evaluation (`{|| eval(b2) }`)
// pops back to the outer block.
prev := t2.CurBlock()
t2.SetCurBlock(bb)
defer t2.SetCurBlock(prev)
ExecPcode(t2, blockFn, modCopy)
}, nDetached)
}
t.PushValue(blockVal)
case PcOpPushDetached:
slot := int(binary.LittleEndian.Uint16(code[pc:]))
pc += 2
bb := t.CurBlock()
if bb != nil && slot < len(bb.Detached) {
t.PushValue(bb.Detached[slot])
} else {
t.PushNil()
}
case PcOpPopDetached:
slot := int(binary.LittleEndian.Uint16(code[pc:]))
pc += 2
val := t.pop()
bb := t.CurBlock()
if bb != nil && slot < len(bb.Detached) {
bb.Detached[slot] = val
}
// --- Local ops ---
case PcOpLocalAddInt:

View File

@@ -92,8 +92,17 @@ const (
PcOpArrayPush byte = 0x52
PcOpArrayPop byte = 0x53
// Block
PcOpPushBlock byte = 0x58 // + uint32 codeLen + pcode bytes + uint16 nDetached
// Block — operand layout:
// PcOpPushBlock + uint32 codeLen + body bytes
// + uint16 nParams + uint16 nDetached
// + nDetached × uint16 (source-local index per slot)
// Each captured slot snapshots the current frame's Local(idx)
// into the block's Detached[i] at creation time. Body accesses
// captured values via PcOpPushDetached / PcOpPopDetached with the
// 0-based slot index.
PcOpPushBlock byte = 0x58
PcOpPushDetached byte = 0x59 // + uint16 0-based detached slot
PcOpPopDetached byte = 0x5A // + uint16 0-based detached slot
// Local operations
PcOpLocalAddInt byte = 0x60 // + uint16 index + int32 value
@@ -129,4 +138,10 @@ type PcodeModule struct {
Name string
Funcs map[string]*PcodeFunc
Strings []string // string constant pool
// Warnings captures compile-time diagnostics from genpc — most
// commonly "AST node X not supported in pcode mode". Surfaced
// by the build pipeline so users learn their PRG isn't fully
// pcode-compilable instead of seeing silent wrong results from
// no-op fallbacks. Empty slice = clean compile.
Warnings []string
}

View File

@@ -33,6 +33,16 @@ type CallFrame struct {
// CurFrame returns the current call frame (for closure capture).
func (t *Thread) CurFrame() *CallFrame { return t.curFrame }
// CurBlock returns the *HbBlock for the codeblock currently executing
// (or nil outside a block body). Used by pcode dispatch to resolve
// detached-local opcodes against the running block's capture slice.
func (t *Thread) CurBlock() *HbBlock { return t.curBlock }
// SetCurBlock installs the executing block. The pcode block wrapper
// pairs Set(block) with a deferred Set(prev) to nest correctly across
// blocks that call other blocks (`{|| eval(b1) + eval(b2) }`).
func (t *Thread) SetCurBlock(b *HbBlock) { t.curBlock = b }
// LocalsSlice returns the underlying locals array (for closure capture).
func (t *Thread) LocalsSlice() []Value { return t.locals }
@@ -111,6 +121,13 @@ type Thread struct {
// OOP: current Self object (set during method dispatch)
self Value
// Current code block (set while a block body is executing).
// Pcode opcodes PcOpPushDetached / PcOpPopDetached read/write
// Detached[i] through this pointer. The block's wrapper Fn
// sets it before ExecPcode and restores on exit. Stays nil
// outside block bodies; nil-checking opcodes fall back to NIL.
curBlock *HbBlock
// Error handling: last error from BEGIN SEQUENCE
lastError *HbError

View File

@@ -154,13 +154,29 @@ func (t *Thread) GetSym(cache **Symbol, name string) *Symbol {
}
// NewThread creates a new Thread attached to this VM.
//
// Statics + WA are initialized here (not just in Run) so threads
// spawned via GoLaunch / GoLaunchBlock — which call NewThread
// directly — see the same module-static map and have a workarea
// manager available. Without this, PRG code running in a goroutine
// that touched a STATIC panicked with "static index out of range",
// and any DB/RDD call crashed dereferencing nil WA.
func (vm *VM) NewThread() *Thread {
t := NewThread(vm)
vm.mu.Lock()
vm.nextTID++
t.tid = vm.nextTID
vm.threads = append(vm.threads, t)
// Snapshot the statics map under the same lock — late
// goroutines see whatever was registered up to this point.
for k, v := range vm.statics {
t.statics[k] = v
}
wf := vm.waFactory
vm.mu.Unlock()
if t.WA == nil && wf != nil {
t.WA = wf()
}
return t
}