[WIP] internal/build: fix linkname support main.sym#2155
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
7ce9de5 to
3fe2fc7
Compare
There was a problem hiding this comment.
Review summary
Scoped review of the 6 source files in this PR (the other ~174 changed files are regenerated golden test-data). The core change — emitting main-package symbols under the main. prefix and running all init functions in the library constructor — is coherent and the library ctor init ordering correctly mirrors defineEntryFunction (pyInit, rtInit, abiInit, runtimeStub, mainInit) while correctly excluding mainMain/pyFinalize.
Main findings are inline. One cross-cutting note below.
Maintainability — duplicated main-prefix rule. The rule "if the package name is main, the path/prefix is main, else use the import path" now appears in three places: ssa/abi/abi.go PathOf, internal/build/main_module.go genMainModule (the pkgPath block), and internal/build/build.go linkMainPkg. A closely related but different rule already exists in ssa/abitype.go reflectPkgPath (keyed on Path() == "command-line-arguments"). Four near-duplicate implementations of the same concept invite drift. Consider centralizing on the existing abi.PathOf helper and calling it from the build package — this would also remove the raw pkg.Types.Name() dereferences flagged inline.
No security or performance regressions found (build-time codegen over bounded inputs).
Findings without inline locations
internal/build/main_module.go:140: This doc comment ("arranges for the LLGo runtime to be initialized") now understates the behavior: after the variadic refactor the ctor invokes multiple inits (Python init, runtime init, abitypes init, runtime stub, package init), not just the runtime. Consider updating the comment, and noting that the argument order must mirrordefineEntryFunction's init sequence since the two lists are now maintained independently and can silently diverge.
| @@ -1132,6 +1135,9 @@ func linkMainPkg(ctx *context, pkg *packages.Package, pkgs []*aPackage, outputPa | |||
| methodByName := make(map[string]none) | |||
| allPkgs := []*packages.Package{pkg} | |||
| for _, v := range pkgs { | |||
| if v.PkgPath != pkg.PkgPath && v.Types.Name() == "main" { | |||
There was a problem hiding this comment.
v.Types.Name() is dereferenced without a nil check, unlike the guard added a few lines up in filterTestPackages (if pkg.Types != nil && pkg.Types.Name() == "main", line 692). Elsewhere in this file p.Types is defensively nil-checked before use. A package with Types == nil (e.g. an ill-typed or synthetically-loaded dep) reaching this loop would panic. Recommend v.Types != nil && v.Types.Name() == "main" for consistency and crash-safety.
| mainInit := declareNoArgFunc(mainPkg, pkg.PkgPath+".init") | ||
| mainMain := declareNoArgFunc(mainPkg, pkg.PkgPath+".main") | ||
| var pkgPath string | ||
| if pkg.Types.Name() == "main" { |
There was a problem hiding this comment.
pkg.Types.Name() is dereferenced without a nil guard here, whereas filterTestPackages guards the same access with pkg.Types != nil. If pkg.Types can ever be nil at this point, this panics. Add the nil check (or centralize via abi.PathOf, which already special-cases main) so the three new Name()=="main" sites behave consistently.
fix #2156