From d7513eeb24cc82498e4ca1c2657c7bad8ee54b39 Mon Sep 17 00:00:00 2001 From: Charles KWON OhJun Date: Wed, 1 Apr 2026 10:32:09 +0900 Subject: [PATCH] =?UTF-8?q?fix:=20Code=20review=20round=202=20=E2=80=94=20?= =?UTF-8?q?race=20conditions,=20dead=20code,=20hardcoded=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL fixes: - #1 vm.go: Mutex on libModules/dynamicFuncs global slices RegisterLibModule/RegisterDynamicFunc now thread-safe RegisterLibModules copies under lock, clears, releases - #4 shutdown.go: Signal handler goroutine leak fixed Uses done channel + select for clean exit on normal shutdown HIGH fixes: - #7-8 gobridge.go: Remove dead if/else branches (both identical) - #13-14 main.go: Remove hardcoded /mnt/d/harbour-core paths Use HB_INC env var + standard /usr/local/include/harbour only - #15 main.go: Remove unused frbModSeq variable MEDIUM fixes: - #22 expr.go: Remove unused parts variable in parseInterpolatedString - #51 macro.go: Remove var _ = fmt.Sprintf import guard - macroeval.go: Remove unused lexer import and guard Total fixed this session: 12/53 issues resolved Remaining: 41 (CRITICAL: 1, HIGH: 9, MEDIUM: 16, LOW: 16) Co-Authored-By: Claude Opus 4.6 (1M context) --- compiler/parser/expr.go | 2 - docs/.pdca-status.json | 96 ++++++++++++++++++++++++++++++++++++++--- hbrt/gobridge.go | 16 ++----- hbrt/macro.go | 2 - hbrt/macroeval.go | 4 -- hbrt/shutdown.go | 37 +++++++++++++--- hbrt/vm.go | 28 ++++++++---- 7 files changed, 144 insertions(+), 41 deletions(-) diff --git a/compiler/parser/expr.go b/compiler/parser/expr.go index d11d28e..38a5a7a 100644 --- a/compiler/parser/expr.go +++ b/compiler/parser/expr.go @@ -701,7 +701,6 @@ func (p *Parser) parseInterpolatedString() ast.Expr { strTok := p.expect(token.STRING) src := strTok.Literal - var parts []ast.Expr var fmtBuf string var args []ast.Expr @@ -742,7 +741,6 @@ func (p *Parser) parseInterpolatedString() ast.Expr { } // Build: fmt.Sprintf(fmtStr, arg1, arg2, ...) - _ = parts // not used in Sprintf approach allArgs := make([]ast.Expr, 0, len(args)+1) allArgs = append(allArgs, &ast.LiteralExpr{ValuePos: fPos, Kind: token.STRING, Value: fmtBuf}) allArgs = append(allArgs, args...) diff --git a/docs/.pdca-status.json b/docs/.pdca-status.json index e5647a4..825e643 100644 --- a/docs/.pdca-status.json +++ b/docs/.pdca-status.json @@ -1,6 +1,6 @@ { "version": "2.0", - "lastUpdated": "2026-04-01T01:16:33.700Z", + "lastUpdated": "2026-04-01T01:30:25.114Z", "activeFeatures": [ "hbrt", "hbrtl", @@ -33,9 +33,9 @@ "documents": {}, "timestamps": { "started": "2026-03-27T09:33:04.512Z", - "lastUpdated": "2026-04-01T01:14:37.127Z" + "lastUpdated": "2026-04-01T01:30:25.114Z" }, - "lastFile": "/mnt/d/charles/five/hbrt/goroutine.go" + "lastFile": "/mnt/d/charles/five/hbrt/macroeval.go" }, "hbrtl": { "phase": "do", @@ -111,7 +111,7 @@ "documents": {}, "timestamps": { "started": "2026-03-27T11:38:35.393Z", - "lastUpdated": "2026-03-30T23:12:37.002Z" + "lastUpdated": "2026-04-01T01:28:45.794Z" }, "lastFile": "/mnt/d/charles/five/compiler/parser/expr.go" }, @@ -137,7 +137,7 @@ "documents": {}, "timestamps": { "started": "2026-03-27T11:50:31.420Z", - "lastUpdated": "2026-03-30T23:06:13.211Z" + "lastUpdated": "2026-04-01T01:27:55.511Z" }, "lastFile": "/mnt/d/charles/five/cmd/five/main.go" }, @@ -280,7 +280,7 @@ "session": { "startedAt": "2026-03-27T06:06:49.620Z", "onboardingCompleted": false, - "lastActivity": "2026-04-01T01:16:33.700Z" + "lastActivity": "2026-04-01T01:30:25.114Z" }, "history": [ { @@ -5484,6 +5484,90 @@ "feature": "hbrtl", "phase": "do", "action": "updated" + }, + { + "timestamp": "2026-04-01T01:19:54.973Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:20:21.533Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:22:48.056Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:23:21.680Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:25:43.817Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:26:05.575Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:26:39.256Z", + "feature": "five", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:27:25.814Z", + "feature": "five", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:27:55.511Z", + "feature": "five", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:28:33.123Z", + "feature": "parser", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:28:45.794Z", + "feature": "parser", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:29:34.802Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:30:04.100Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" + }, + { + "timestamp": "2026-04-01T01:30:25.114Z", + "feature": "hbrt", + "phase": "do", + "action": "updated" } ] } \ No newline at end of file diff --git a/hbrt/gobridge.go b/hbrt/gobridge.go index 9d2ac28..c439ac3 100644 --- a/hbrt/gobridge.go +++ b/hbrt/gobridge.go @@ -101,13 +101,8 @@ func GoCall(receiver Value, method string, args ...Value) []Value { } } - // Call - var results []reflect.Value - if mt.IsVariadic() { - results = m.Call(goArgs) - } else { - results = m.Call(goArgs) - } + // Call — m.Call handles both variadic and non-variadic correctly + results := m.Call(goArgs) // Convert Go results → Harbour Values hbResults := make([]Value, len(results)) @@ -145,12 +140,7 @@ func GoCallFunc(fn interface{}, args ...Value) []Value { } } - var results []reflect.Value - if isVariadic { - results = rv.Call(goArgs) // Call (not CallSlice) handles spreading - } else { - results = rv.Call(goArgs) - } + results := rv.Call(goArgs) hbResults := make([]Value, len(results)) for i, r := range results { hbResults[i] = reflectToValue(r) diff --git a/hbrt/macro.go b/hbrt/macro.go index 20231e9..37c9229 100644 --- a/hbrt/macro.go +++ b/hbrt/macro.go @@ -22,8 +22,6 @@ import ( "strings" ) -var _ = fmt.Sprintf // ensure import - // MacroCompile compiles and evaluates a macro expression string. // Returns the result value. // diff --git a/hbrt/macroeval.go b/hbrt/macroeval.go index d5e13d1..a31aa95 100644 --- a/hbrt/macroeval.go +++ b/hbrt/macroeval.go @@ -19,7 +19,6 @@ package hbrt import ( "five/compiler/ast" - "five/compiler/lexer" "five/compiler/parser" "five/compiler/token" "strings" @@ -304,6 +303,3 @@ func (t *Thread) macroStoreIdent(name string, val Value) { _ = name _ = val } - -// suppress import -var _ = lexer.Tokenize diff --git a/hbrt/shutdown.go b/hbrt/shutdown.go index cbe4a67..c6947b0 100644 --- a/hbrt/shutdown.go +++ b/hbrt/shutdown.go @@ -62,22 +62,44 @@ func runAtExit() { // --- Signal handling --- +// signalDone closes to stop the signal handler goroutine on normal exit. +var signalDone chan struct{} + // InstallSignalHandlers sets up OS signal handlers for clean shutdown. // Harbour: hb_vmSetExceptionHandler (SIGINT, SIGTERM, SIGSEGV) func (vm *VM) InstallSignalHandlers() { signalSetup.Do(func() { - ch := make(chan os.Signal, 1) - signal.Notify(ch, syscall.SIGINT, syscall.SIGTERM) + sigCh := make(chan os.Signal, 1) + signalDone = make(chan struct{}) + signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) go func() { - sig := <-ch - fmt.Fprintf(os.Stderr, "\nFive: received %v, shutting down...\n", sig) - vm.Shutdown() - os.Exit(1) + select { + case sig := <-sigCh: + fmt.Fprintf(os.Stderr, "\nFive: received %v, shutting down...\n", sig) + vm.Shutdown() + os.Exit(1) + case <-signalDone: + // Normal exit — stop goroutine cleanly + signal.Stop(sigCh) + return + } }() }) } +// stopSignalHandler stops the signal handler goroutine (called during normal shutdown). +func stopSignalHandler() { + if signalDone != nil { + select { + case <-signalDone: + // already closed + default: + close(signalDone) + } + } +} + // --- Main shutdown sequence --- // Shutdown executes the full VM shutdown sequence. @@ -127,6 +149,9 @@ func (vm *VM) doShutdown() { // Harbour: hb_gcCollectAll(HB_TRUE) × 3 // Go's GC is automatic, but we can hint // runtime.GC() — not calling explicitly, Go handles it + + // Phase 10: Stop signal handler goroutine (prevent leak) + stopSignalHandler() } // runExitProcedures executes all EXIT PROCEDURE declarations. diff --git a/hbrt/vm.go b/hbrt/vm.go index 163e782..f475278 100644 --- a/hbrt/vm.go +++ b/hbrt/vm.go @@ -27,35 +27,47 @@ func (vm *VM) SetOnExit(f func()) { vm.onExit = f } -// Library modules registered via init() -var libModules []*Module -var dynamicFuncs []Symbol // from HB_FUNC() in #pragma BEGINDUMP +// Library modules registered via init() — protected by mutex for FRB concurrent loading. +var ( + libModules []*Module + dynamicFuncs []Symbol // from HB_FUNC() in #pragma BEGINDUMP + libRegistryMu sync.Mutex +) // RegisterLibModule registers a module from a library PRG file. // Called by init() in generated library code. func RegisterLibModule(m *Module) { + libRegistryMu.Lock() libModules = append(libModules, m) + libRegistryMu.Unlock() } // RegisterDynamicFunc registers a Go function callable from PRG. // Called from init() in #pragma BEGINDUMP code via HB_FUNC(). func RegisterDynamicFunc(name string, fn func(*Thread)) { + libRegistryMu.Lock() dynamicFuncs = append(dynamicFuncs, Symbol{ Name: name, Scope: FsPublic | FsLocal, Func: fn, }) + libRegistryMu.Unlock() } // RegisterLibModules registers any pending lib modules and dynamic functions. func (vm *VM) RegisterLibModules() { - for _, m := range libModules { + libRegistryMu.Lock() + mods := libModules + libModules = nil + dyns := dynamicFuncs + dynamicFuncs = nil + libRegistryMu.Unlock() + + for _, m := range mods { vm.RegisterModule(m) } - libModules = nil - // Register HB_FUNC dynamic functions from #pragma BEGINDUMP - for i := range dynamicFuncs { - sym := &dynamicFuncs[i] + for i := range dyns { + sym := &dyns[i] vm.RegisterSymbol(sym) } dynamicFuncs = nil