build: write back deadcode through ThinLTO - #2398
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
FennoAI Review
This adds a ThinLTO deadcode writeback mode that defers package export until linkMainPkg computes the global plan, then rewrites each package's ABI method tables in-place (dcepass.RewriteTypeMethodTables) rather than emitting entry-module overrides. The design is clean and closely mirrors the existing applyDeadcodeDropOverrides path.
Strengths
- Careful buffer/module lifecycle: canonical buffer disposed via
defer, rewritten buffer ownership transferred toObjBuffersand freed bynormalizeToArchive'sdisposeArchiveBuffers. - Temp-file cleanup in
writeCanonicalThinLTOBitcodeis thorough on all error paths. - The
CacheHitguard plus thetryLoadFromCacheskip form a coherent defensive pairing. - Doc comments are accurate and follow Go conventions; no security or documentation issues found.
The mode is buildenv.Dev-gated (experimental), which lowers the severity of the notes below. Inline findings cover the main points; a few non-line-specific observations:
- No end-to-end test for
materializeThinLTODeadcodePlan.writeCanonicalThinLTOBitcodeandRewriteTypeMethodTablesare tested in isolation, but the orchestration that wires them into the package archive (skip conditions,CacheHitbranch,ObjBuffersappend,normalizeToArchive, and thearchiveInputsrebuild inlinkMainPkg) — the part most likely to regress on refactor — has no direct coverage. - Duplication between the two dcepass paths.
moduleRewriter.rewriteGlobalandoverrideEmitter.emitTypeOverrideshare the per-slot rewrite logic and the identical"[dce] drop method ..."verbose format string. A small shared helper would keep them from drifting. - Performance (dev-mode, non-blocking): materialization runs serially per package on the link path doing full serialize/reparse/rewrite/reserialize, and the mode disables the package cache entirely (full source rebuild every invocation). Both are conscious, correctness-driven tradeoffs and acceptable for an experimental flag; worth revisiting if the mode is ever promoted toward non-dev use.
| return | ||
| } | ||
| defer mod.Dispose() | ||
| dcepass.RewriteTypeMethodTables(mod, liveSlots, verbose) |
There was a problem hiding this comment.
[P1] Rewritten ThinLTO module is emitted without VerifyModule
After RewriteTypeMethodTables, the freshly parsed module is handed directly to WriteThinLTOBitcodeToMemoryBuffer with no verification. The normal compile path verifies each module via gllvm.VerifyModule(mod, gllvm.ReturnStatusAction) (build.go:2191), and this rewrite path bypasses that check. A malformed rewrite (e.g. a mismatched named-struct type) would silently produce invalid bitcode instead of failing fast. Consider adding a VerifyModule call after the rewrite and returning an error on failure, matching the fail-fast pattern used elsewhere.
Relatedly, RewriteTypeMethodTables returns the count of rewritten globals (the unit test asserts it), but the count is discarded here. When verbose, reporting it — or diagnosing a zero-rewrite result for a package expected to have live/dead slots — would surface liveSlots key-format mismatches that currently produce no signal at all.
| defer os.Remove(canonical) | ||
| llvmCtx := gllvm.NewContext() | ||
| defer llvmCtx.Dispose() | ||
| mod, parseErr := llvmCtx.ParseBitcodeFile(canonical) |
There was a problem hiding this comment.
[P3] Avoidable temp-file round-trip; in-memory parse is possible
A memory-buffer parse would avoid a disk round-trip here. writeCanonicalThinLTOBitcode already holds the serialized bytes in a gllvm.MemoryBuffer, but it writes them to a temp .bc file only to have ParseBitcodeFile read them back. The binding already exposes in-memory parsing (ParseIR over a MemoryBuffer, used in deadcode_test.go). Passing the buffer directly would remove one write + one read per package on the link path. Low priority given the dev-mode gating.
| // is emitted in the entry module. | ||
| func materializeThinLTODeadcodePlan(ctx *context, pkgs []Package, liveSlots map[string][]int, verbose bool) error { | ||
| for _, aPkg := range pkgs { | ||
| if aPkg == nil || aPkg.LPkg == nil || aPkg.Package == nil || aPkg.Package.ExportFile == "" { |
There was a problem hiding this comment.
[P3] Empty-ExportFile packages silently un-archived in this mode
This skips any package with an empty ExportFile, and finalizePackageBuild also returns early before normalizeToArchive in this mode. In the normal path such a package (empty ExportFile but non-empty ObjFiles from cgo/asm/alias objects) would still be archived. Today packages reaching here with an empty ExportFile come from SkipToBuild and carry no ObjFiles, so there is likely no live gap — but the invariant is implicit. A short comment (or explicitly archiving their ObjFiles) would prevent a silent drop if that invariant ever changes.
| if err != nil { | ||
| return fmt.Errorf("write canonical ThinLTO bitcode for %s: %w", aPkg.PkgPath, err) | ||
| } | ||
| func() { |
There was a problem hiding this comment.
[P3] Closure mutating enclosing err obscures control flow
The outer err from writeCanonicalThinLTOBitcode is reassigned inside the anonymous func and re-checked after it returns. It works, but relying on a closure to mutate an enclosing err that a different call just consumed is easy to misread. Extracting the closure body into a named helper (e.g. rewriteAndBufferPackage(aPkg, canonical, liveSlots, verbose) error) would make the temp-file lifetime, module disposal, and error propagation self-contained.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
1 participant
Summary
This PR adds a one-shot ThinLTO writeback path for the existing
-deadcodedropoption.The user-facing flag remains unchanged:
-deadcodedropwithout ThinLTO keeps the existing entry-module strong Global override path.-deadcodedrop -lto=thinuses package-owned rewrite before the final ThinLTO link.Implementation
IFn/TFnslots withruntime.unreachableMethod, and preserve the original global linkage/COMDAT.The canonical bitcode is link-scoped and removed after materialization in this first version. Package cache hits are disabled for this mode because the rewrite plan is link-specific.
Why this path
The old deadcode implementation emits strong same-name globals from the entry module. That works as a compatibility path, but it does not let ThinLTO build summaries from the already-pruned package-owned definitions. This implementation moves the rewrite to the owner package, allowing ThinLTO to see the pruned metadata during its normal analysis and optimization.
This PR intentionally does not add MethodByName feedback,
.4.opt.bchandling,--save-temps, archive caching, or multi-round fixed-point analysis.Benchmark experiment
Environment: macOS
darwin/arm64, LLGo built from this PR with-tags dev, Bent serial build (-j=1), one coldtest -c -abuild per configuration. Sizes arebenchsizeELF/Mach-Ototal-bytes; times are Bentbuild-real-ns/op.k8s_workqueueuber_zapgorm_schemaSection-level results:
k8s_workqueue: text 3,422,004 -> 3,869,720; data 334,880 -> 221,672.uber_zap: text 2,544,836 -> 2,848,688; data 333,808 -> 223,216.gorm_schema: text 1,996,248 -> 2,167,036; data 218,224 -> 137,328.The results are intentionally mixed. The owner rewrite reduces data/metadata in all three cases, but ThinLTO can increase text because its package/object optimization and final code generation differ from the legacy non-LTO path. This PR establishes the integration point; further ThinLTO-specific tuning is needed before claiming a universal size win.
Verification
go test ./internal/dcepass ./internal/build -count=1go test -tags dev ./internal/dcepass ./internal/build -run 'Test(RewriteTypeMethodTablesInPlace|ApplyDeadcodeDropOverridesWritesStrongTypeOverride|ThinLTODeadcodeEnabled|WriteCanonicalThinLTOBitcodeRoundTrip)' -count=1go test ./internal/... -run '^$'git diff --checkThe implementation tests pass. A direct macOS demo link was also exercised; unrelated local runtime dependencies (
GC_*,libffi) are unavailable in this environment, so that final executable link could not complete.