4fd14f63efaea0ec42ffb84ecc4aada4f7dde7d4
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
| cde86730b8 |
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>
|
|||
| 29ca02e1bc |
fix(genpc,parser,pcinterp): pcode wider regression sweep (Tier 1 #3)
Six more silent miscompiles in the pcode path, all uncovered by a
new pcode regression sweep that exercises the full PRG surface a
dynamic FrbCompile body could legitimately use.
* **xBase-keyword shadowing of variable names.** parseIdentStmt
and parseExprStmt's fallback switches consumed an entire line
when the leading IDENT matched LABEL / REPORT / ACCEPT / INPUT
/ NOTE / etc. Those words are also extremely common LOCAL /
PRIVATE names — `LOCAL label ; label := "x"` had the
assignment swallowed because the switch didn't peek at the
next token. Both switches now look at peek(1): an assignment
operator, [], (, -, ++, --, or `.` means it's a variable /
call / member access, not the xBase command, and we fall
through to expression parsing. Real silent bug — bit
test_frb_pcode_sweep's `LOCAL label` declaration.
* **`arr[i]` indexing not implemented in genpc.** ast.IndexExpr
fell through to the default PushNil path, so any indexed read
in a pcode-mode body returned NIL. New case emits the array,
the index, and PcOpArrayPush (the get-op; PcOpArrayPop is the
set-op — naming follows Harbour convention). Hashes go
through the same opcode, which already special-cases
IsHash() in ops_collection.go.
* **Hash literals not implemented in genpc + dispatch missing
in pcinterp.** `{ "k" => v, ... }` fell to PushNil. Added
HashLitExpr emit (Push key, Push value pairs, then PcOpHashGen
with count). Also wired up the PcOpHashGen dispatch in
execPcodeBody — it had been declared in pcode.go since the
initial design but the case statement was never added, so
even hand-written modules couldn't use hashes.
* **`x++` / `x--` postfix were silent no-ops.** PostfixExpr fell
to PushNil and the surrounding ExprStmt then popped the NIL.
DO WHILE loops with `n--` couldn't terminate; FOR loops with
`i++` in the body were broken too. New case: PushLocal +
LocalAddInt(±1).
* **BlockExpr (`{|p| body }`) wasn't compiled.** Eval(b, n)
inside a pcode body returned NIL. Added: build the body in a
sub-codebuffer with the block's params occupying its locals,
emit PcOpRetValue at the end, then PushBlock with the
serialized bytes. Format extended with a uint16 nParams field
so the runtime's PcOpPushBlock dispatch can set
PcodeFunc.Params correctly — without it, ExecPcode's
Frame(0, 0) pulled none of Eval's args and the block saw
every parameter as NIL.
* **All g.locals accesses were case-sensitive.** PRG is case-
insensitive, but the pcode generator stored block params via
strings.ToUpper while every other lookup site (function decl,
mid-decl, ForStmt, IdentExpr read, AssignExpr write,
PostfixExpr) used the raw .Name. So `{|x| x*x }` stored "X"
but read "x" and missed. Normalized: all insertions and all
lookups now go through strings.ToUpper.
* **SeqExpr in pcode** — added the matching emit for comma-
separated expression lists in code blocks (`{|| a, b, c }`).
Same shape as the gengo SeqExpr case from Wave 1.
Test fixture: tests/frb/test_frb_pcode_sweep.prg covers 14 shapes
(string ops, arithmetic, comparison chains, array indexing, DO
WHILE with postfix, nested IF, IIf, hash literal + indexing,
block + Eval, character iteration). All 14 pass. Wired into the
FRB runner — suite now stands at 7/7.
Other gates green:
go test ./... : PASS
FiveSql2 SQL:1999 : 43/43
Harbour compat : 56/56
std.ch suite : 15/15
FRB suite : 7/7
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|||
| dca7bb22e5 |
fix(gengo): count nested LOCALs into the function frame
Function-entry Frame() allocation counted only top-level LOCAL
declarations from fn.Body. Mid-function LOCALs hidden inside an
IF / FOR / WHILE / DO CASE / SWITCH / SEQUENCE block weren't
included, so the runtime allocated a frame too small to hold them.
Subsequent reads/writes via PopLocalFast / PushLocalFast / LocalAdd
to those slot indices then either silently scribbled past the frame
(read-back saw NIL) or panicked with "local variable index out of
range" once the index exceeded the underlying slice.
This is the underlying bug behind frb_demo Section 4 — the
`LOCAL ch := Channel(1)` declared inside `IF pAsync != NIL` got
slot N+1 from the codegen but the runtime only allocated N. The
Channel value was scribbled past the frame, ChReceive then read
NIL from a non-existent slot, and the goroutine's ChSend(49) had
nowhere to land.
New helper gen_util.go::countLocalsInStmts walks every nested body
(IF + ElseIfs + ElseBody, ForStmt, ForEachStmt, DoWhileStmt,
SeqStmt's Body + RecoverBody, SwitchStmt's Cases + Otherwise) and
totals every ScopeLocal VarDecl. The function-emit caller adds this
to the top-level count before sizing the Frame.
Test fixture (tests/frb/test_frb_goroutine.prg) reproduces the
demo Section 4 shape — `LOCAL ch := Channel(1)` inside IF, then
`Go("WORKER", ch, 7)`, then ChReceive(ch). Wired into the FRB
runner so it stands at 6/6.
Other gates green:
go test ./... : PASS
FiveSql2 SQL:1999 : 43/43
Harbour compat : 56/56
std.ch suite : 15/15
FRB suite : 6/6
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|||
| efb615bed9 |
fix(frb,genpc): in-process compile + 4 pcode bugs
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>
|
