[WIP] llgo build#466
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces build-tag separation for the llgo compiler by moving the CallerFrames method to runtime.go (built with !llgo) and adding a new runtime_llgo.go file. It also updates several dependencies in go.mod. However, two critical issues were identified: first, using "main" as a version in go.mod is invalid and will break the Go toolchain; second, excluding runtime.go under the llgo tag leaves debugStack unregistered and breaks API compatibility due to the missing CallerFrames method on Frame. Suggestions have been provided to resolve both issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| import "runtime" | ||
|
|
||
| func runtimeGC(fr *frame) { | ||
| runtime.GC() | ||
| } | ||
|
|
||
| func debugStack(fr *frame) []byte { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Under the llgo build tag, runtime.go is excluded, which means its init() function is not executed. As a result, debugStack is defined here but never registered with RegisterExternal. Additionally, CallerFrames is an exported method on Frame (defined in context.go), and excluding it entirely under llgo breaks API compatibility for downstream consumers.
To fix these issues, we should:
- Add an
init()function to registerdebugStack. - Implement a stub for
CallerFramesonFrameto maintain API compatibility.
| import "runtime" | |
| func runtimeGC(fr *frame) { | |
| runtime.GC() | |
| } | |
| func debugStack(fr *frame) []byte { | |
| return nil | |
| } | |
| import "runtime" | |
| func init() { | |
| RegisterExternal("runtime/debug.Stack", debugStack) | |
| } | |
| func runtimeGC(fr *frame) { | |
| runtime.GC() | |
| } | |
| func debugStack(fr *frame) []byte { | |
| return nil | |
| } | |
| func (fr *Frame) CallerFrames() (frames []runtime.Frame) { | |
| return nil | |
| } |
There was a problem hiding this comment.
FennoAI Review — PR #466 [WIP] llgo build
Non-blocking review (COMMENT). The PR partitions the package into !llgo / llgo build paths and bumps dependencies. The default (!llgo) build compiles cleanly and the CallerFrames() move is correct for that path. Since this is marked [WIP], most items below are expected gaps for the llgo target — the two highest-impact ones are the build-breaking go.mod/go.sum state and the missing init() registrations under llgo.
Blocking-for-merge (not for WIP)
-
go.mod—github.com/goplus/reflectx mainis not a valid module version. The committedgo.modpinsreflectxto the literal branch refmain, andgo.sumcontains noh1:content hash for the intended revision (only stalev1.7.0entries). A defaultgo build ./...(readonly mode) fails with "updates to go.mod needed; to update it: go mod tidy". Rungo mod tidyand commit a proper tagged version or pseudo-version plus the matchinggo.sumhashes. (Confirmed by reproducing the readonly build failure.) -
runtime_llgo.go— theinit()external registrations fromruntime.goare lost underllgo.runtime.gonow carries//go:build !llgo, so under thellgotag its entireinit()is excluded andruntime_llgo.goprovides no replacement. Thatinit()registers interpreter-critical externals — most notablyos.Exit(routes throughinterp.chexitso the interpreter returns an exit code) andruntime.Goexit(panic-based main-goroutine handling), plusruntime.Caller/Callers/FuncForPC/Stack,runtime/debug.Stack,runtime/debug.PrintStack, and thefuncval-based(reflect.Value).Pointeroverride. Underllgoan interpretedos.Exit(N)would call the realos.Exitand hard-exit the host process without propagating the code. Either add aninit()toruntime_llgo.go(at minimumos.Exit/runtime.Goexit) or document explicitly why these overrides are dropped for llgo.
Consistency / documentation
runtime_llgo.gohas no license header. Every other file in the package root carries the GoPlus Authors Apache-2.0 block; the new file has none.debugStackreturnsnilunderllgowith no comment. It feedsPanicError.stack/FatalError.stack, so panic stacks are silently empty under llgo. Add a short comment explaining the intentional stub.runtime.go:1uses only//go:build !llgowhile the rest of the repo (and the newruntime_llgo.go) pairs it with the legacy// +buildline. Minor consistency nit;go 1.24makes the legacy form optional.
API-surface note (body-only)
(*Frame).CallerFrames()(runtime.go:401) is now defined only under!llgo. As an exported method on the exportedFramealias, external callers building with-tags llgowill hit a compile error. If dropping it under llgo is intentional, note it; otherwise add a stub toruntime_llgo.go.go.sumstill retains the orphanreflectx v1.7.0hash entries, indicatinggo mod tidywas not fully run — worth cleaning up alongside thego.modfix above.
Performance: no regressions — the change is additive/build-tag partitioning; the llgo runtimeGC path is strictly cheaper (it skips the frame-walk), which is an acceptable trade-off.
| github.com/goplus/gogen v1.23.3 | ||
| github.com/goplus/mod v0.20.2 | ||
| github.com/goplus/reflectx v1.7.0 | ||
| github.com/goplus/reflectx v1.7.1-0.20260701065815-36c179f3d48f |
There was a problem hiding this comment.
Invalid module version: the committed go.mod pins github.com/goplus/reflectx to the literal branch ref main, which is not a valid version. Combined with a go.sum that lacks the h1: content hash for the intended revision, a default go build ./... fails in readonly mode with "updates to go.mod needed; to update it: go mod tidy". Run go mod tidy and commit a tagged version (or a full pseudo-version) with matching go.sum hashes.
| @@ -1,3 +1,5 @@ | |||
| //go:build !llgo | |||
There was a problem hiding this comment.
Adding //go:build !llgo here excludes the entire file — including its init() — under the llgo tag. That init() registers interpreter-critical externals (os.Exit via interp.chexit, runtime.Goexit, runtime.Caller/Callers/FuncForPC/Stack, runtime/debug.Stack/PrintStack, and the funcval (reflect.Value).Pointer override) that are NOT re-provided by runtime_llgo.go. Under llgo, an interpreted os.Exit(N) would call the real os.Exit and hard-exit the host without returning an exit code. Add a replacement init() in runtime_llgo.go (at least os.Exit/runtime.Goexit) or document why these are intentionally dropped.
Minor: the rest of the repo pairs this with the legacy // +build !llgo line for consistency.
| //go:build llgo | ||
| // +build llgo | ||
|
|
||
| package ixgo |
There was a problem hiding this comment.
Missing license header: every other file in the package root carries the GoPlus Authors Apache-2.0 copyright block; this new file has none. Add it for consistency.
| runtime.GC() | ||
| } | ||
|
|
||
| func debugStack(fr *frame) []byte { |
There was a problem hiding this comment.
debugStack silently returns nil under llgo. Its result is stored in PanicError.stack/FatalError.stack (see builtin.go, opblock.go, interp.go), so panic/fatal stack traces will be empty under llgo. Acceptable as a known limitation, but add a brief comment stating that stack introspection is unsupported under llgo and the empty result is intentional.
No description provided.