build: template isolated backend programs#2180
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: build: template isolated backend programs
A careful, well-structured refactor. The preflight/execute/finalize staging, the immutable PackageSummary link-time boundary, and the bounded worker pools are cleanly separated and well-tested. The added synchronization (builtMu, sfilesMu, llvmVersionMu, cacheManagerMu, llssaInitOnce) makes the new concurrent phases race-safe, and collectFingerprint correctly stopped sharing context.fingerprinting by threading a per-call map.
One finding is worth confirming before merge (potential cold-cache cross-compile link failure); the rest are cleanup and can follow. Inline comments carry the concrete diff-line findings.
Correctness — please verify before merge
Skipped runtime packages can trip the new nil-Summary link error. linkMainPkg now requires aPkg.Summary != nil for every entry in linkedOrder, and linkedOrder includes any package with a non-empty ExportFile (build.go:1286). ExportFile comes from go/packages at load time and is non-empty for compiled runtime packages. But buildAllPkgs skips building runtime packages when !needRuntime && !needPyInit && Target != "" (build.go:949-955), so on a cold cache those packages never reach finalizePackageBuild and keep Summary == nil. Preflight does run over the full plan and tryLoadFromCache can populate Summary, so a warm cache masks this — but a clean cross-compile build where the runtime is genuinely not needed appears to hit fmt.Errorf("package %s has no linker summary"). The previous code tolerated this because it only read aPkg.LinkArgs/ArchiveFile for runtime packages and never dereferenced LPkg/Summary. Suggest exempting isRuntimePkg from the up-front nil check and guarding the per-summary reads, or clearing ExportFile on skipped runtime packages. See inline comment on build.go:1294.
Concurrency (not blocking)
- Panic inside a preflight worker goroutine.
appendExternalLinkArgs(build.go:1066andbuild.go:1083)panics when a library cannot be located orCheckLinkArgsfails. It is now invoked from a preflight worker (preflightPackageBuild) for source-lessPkgLinkExternpackages, so a failure crashes the process instead of propagating as theerrorthe pipeline otherwise returns, and the crash is now non-deterministic across workers. Prefer returning an error from this path now that it runs concurrently. - Fingerprint invariant is load-bearing. The parallel-preflight safety relies on
dependencyFingerprintnever concurrently mutating a sharedaPackage: it holds only because the level barrier (wg.Wait()per level) guarantees in-plan deps are fully fingerprinted in an earlier level, and the plan's edges (newPackageBuildPlan) and the fingerprint recursion (collectDependencyInputs) both derive fromeffectiveDependencies. If a future change lets the fingerprint recursion traverse an edge not modeled in the plan, this becomes a real data race on unsynchronizedaPackagefields. Worth a comment documenting the invariant atpreflightPackageBuilds/newPackageBuildPlan.
Performance (not blocking)
- Serial backend caps the realized speedup. Preflight (cache lookup + fingerprint) runs in parallel, but the dominant per-package cost —
executePackageBuild->buildPkg-> LLVMRunPasses+ object emission — stays serial inbuildAllPkgs(build.go:939-955). On cold builds the wall-clock win is bounded to the preflight/SSA-build phases; the headline "parallel build pipeline" delivers little on codegen-bound builds. This is acknowledged in the comments as intentional staging, flagging only so the limitation is explicit. - Per-level barrier + per-level pool re-creation over-serialize preflight.
preflightPackageBuildsdrains andwg.Wait()s each dependency level and re-creates thejobs/resultschannels and goroutines per level (package_build.go:48-73). A single straggler in an intermediate level stalls the whole next level, and deep-but-narrow graphs pay repeated pool setup/teardown. A single persistent pool fed by an in-degree ready-queue would remove both the barrier stall and the churn.
Maintainability (not blocking)
See inline comments on package_build.go:114 (packageBuildResult largely unused), build.go:964 (result built on discarded error paths), funcinfo_table.go:88 (production-dead []Package shims / awkward *Summaries naming), package_build.go:153 (hard error on duplicate spec vs prior silent skip), flagfile.go:25 (argumentListFlagNames now includes scalar -p), and build.go:973 (preflight skip-comment omits the already-built case).
Note: I could not run a cold-cache cross-compile build in this environment, so the correctness finding above is reasoned from the code paths rather than reproduced — please confirm.
| linkedSummaries := make([]*PackageSummary, len(linkedOrder)) | ||
| for i, aPkg := range linkedOrder { | ||
| if aPkg.Summary == nil { | ||
| return fmt.Errorf("package %s has no linker summary", aPkg.PkgPath) |
There was a problem hiding this comment.
This up-front check requires Summary != nil for every package in linkedOrder, which includes runtime packages (non-empty ExportFile). When buildAllPkgs skips runtime packages (!needRuntime && !needPyInit && Target != "", build.go:949-955) on a cold cache, those packages never reach finalizePackageBuild and keep Summary == nil, so this returns "package %s has no linker summary". The old loop only read LinkArgs/ArchiveFile for runtime packages and never touched LPkg. Consider exempting isRuntimePkg(aPkg.PkgPath) from the nil check (and guarding the per-summary reads) or clearing ExportFile on skipped runtime packages. Please verify with a cold-cache cross-compile build that does not need the runtime.
| spec := newPackageBuildSpec(pkg) | ||
| id := spec.pkg.ID | ||
| if _, exists := plan.byID[id]; exists { | ||
| return nil, fmt.Errorf("duplicate package build spec for %s", id) |
There was a problem hiding this comment.
This now hard-fails on a duplicate package ID, whereas the old buildOne used the built map to silently skip already-seen IDs. Correctness now depends on registerSSAPkgs de-duplicating across the initial/dep passes via ctx.pkgByID. That invariant holds today but is load-bearing here — if any future change lets a package appear in both pkgs and depPkgs, the whole build fails instead of degrading. Worth a doc comment stating callers must pass a de-duplicated set.
| // packageBuildResult carries the observable output of a serial package build. | ||
| // Subsequent scheduler PRs can pass this value between worker and finalization | ||
| // stages without exposing the mutable aPackage implementation details. | ||
| type packageBuildResult struct { |
There was a problem hiding this comment.
packageBuildResult is largely speculative: of its five fields only needRuntime/needPyInit are ever read (build.go:944-945). spec, cacheHit, and archiveFile are populated by packageBuildResultFor but never consumed in this PR. Consider trimming to the two used fields (or returning (needRuntime, needPyInit bool)) until the follow-up scheduler PR actually needs the rest.
| // after parallel preflight has completed for the package's dependency level. | ||
| func buildPreflightedPackage(ctx *context, preflight packagePreflight, verbose bool) (packageBuildResult, error) { | ||
| if preflight.skip { | ||
| return packageBuildResultFor(preflight.spec), nil |
There was a problem hiding this comment.
buildPreflightedPackage constructs a full packageBuildResult on both error returns (here and line 967), but callers discard the result whenever err != nil (build.go:940-943, 951). Returning a zero packageBuildResult{} on error paths would be clearer and avoid the wasted work.
| @@ -86,12 +86,16 @@ type funcInfoSymbolIndexRecord struct { | |||
| } | |||
|
|
|||
| func collectFuncInfo(pkgs []Package) []funcInfoRecord { | |||
There was a problem hiding this comment.
collectFuncInfo (and collectPCLineInfo, collectFuncInfoStubRecords, plus linkedModuleGlobals in build.go) are now only reachable from tests — all production call sites use the ...Summaries variants. This leaves two parallel APIs where the shorter name exists only to keep old tests compiling, and collectFuncInfoStubRecordsSummaries reads awkwardly. Consider migrating the tests to build []*PackageSummary directly and deleting the []Package shims, or making the summary-based function the unsuffixed canonical name.
| @@ -23,6 +23,18 @@ import ( | |||
| ) | |||
|
|
|||
| var argumentListFlagNames = [...]string{ | |||
There was a problem hiding this comment.
argumentListFlagNames now includes "p", which is a scalar flag rather than an argument list, so the name is inaccurate for at least one entry (the new wholeLineValueFlagNames comment acknowledges the distinction). Consider renaming to something like normalizedValueFlagNames to reflect that it drives value normalization, not argument-list semantics.
| } | ||
|
|
||
| // preflightPackageBuild performs package classification, cache lookup, and | ||
| // other work that does not create or transform an LLVM module. It returns |
There was a problem hiding this comment.
The comment says skip is returned for "packages with no executable build stage," but the first and most common skip path is the already-built dedup check at build.go:979-981, which returns skip=true for a package that may well have a real build stage (e.g. a shared dep seen in both the initial and dep sets). Suggest amending the comment to also mention already-built packages.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
752707f to
768c0d5
Compare
1 participant
Stacked on #2179 (and therefore includes #2174-#2179).\n\nCentralizes backend Program and C ABI Transformer construction in an immutable template. It preserves target, Go GlobalDCE, pthread, LTO, funcinfo, runtime/python lazy getters, and runtime linkname registration; each template session gets an independent LLVM context. The existing build path remains serial and now uses the same template, preparing worker-local backend sessions for the following PR.