build: switch package pipeline to bitcode(BC) linking#1673
build: switch package pipeline to bitcode(BC) linking#1673luoliwoshang wants to merge 10 commits intogoplus:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the build process to leverage bitcode-first compilation and in-process LLVM API linking. The core change involves producing and caching bitcode files as the primary artifact for packages, which are then linked together at a program level before final native compilation. This approach aims to improve optimization opportunities through Link-Time Optimization (LTO) and streamline the linking phase by handling bitcode modules directly within the build system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable change by switching the package build pipeline to a bitcode-first LTO flow. This will enable more aggressive cross-module optimizations. The changes are extensive, touching the build, caching, and compilation logic across several files. My review identifies a resource leak due to unclosed temporary files and suggests a couple of refactorings to improve code maintainability and performance.
| mergedBitcode, err := mergeBitcodeFiles(moduleName, bitcodeFiles) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
The temporary file mergedBitcode created by mergeBitcodeFiles (when there's more than one input file) is not being cleaned up, leading to a resource leak. You should defer its removal after it's created.
}
if len(bitcodeFiles) > 1 {
defer os.Remove(mergedBitcode)
}| func tempNamePrefix(moduleName string) string { | ||
| name := strings.ReplaceAll(moduleName, "/", "_") | ||
| name = strings.ReplaceAll(name, "\\", "_") | ||
| name = strings.ReplaceAll(name, ":", "_") | ||
| name = strings.ReplaceAll(name, ".", "_") | ||
| if name == "" { | ||
| return "llgo" | ||
| } | ||
| return name | ||
| } |
There was a problem hiding this comment.
This function can be made more efficient and readable by using strings.NewReplacer to perform all replacements in a single pass.
func tempNamePrefix(moduleName string) string {
r := strings.NewReplacer("/", "_", "\\", "_", ":", "_", ".", "_")
name := r.Replace(moduleName)
if name == "" {
return "llgo"
}
return name
}![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
internal/build/build.go
Outdated
| } else { | ||
| nativeLinkInputs = append(nativeLinkInputs, linkBitcodeInputs...) | ||
| } |
There was a problem hiding this comment.
When bitcodeLTO is false, raw .bc files are appended directly to nativeLinkInputs and passed to the native linker. The native linker cannot consume .bc without LTO support enabled on the clang driver side. Since bitcodeLTO is hardcoded to true (line 366), this is dead code today, but it's a latent correctness bug.
Consider either removing the else branch (and the field) entirely, or compiling each .bc to .o in the fallback path.
| out, err := os.CreateTemp("", tempNamePrefix(moduleName)+"-*.bc") | ||
| if err != nil { | ||
| return "", fmt.Errorf("create merged bitcode file: %w", err) | ||
| } | ||
| defer out.Close() | ||
|
|
||
| if err := gllvm.WriteBitcodeToFile(mod, out); err != nil { | ||
| return "", fmt.Errorf("write merged bitcode file: %w", err) | ||
| } | ||
| archiveFile.Close() | ||
| archivePath := archiveFile.Name() | ||
| return out.Name(), nil |
There was a problem hiding this comment.
If WriteBitcodeToFile fails, the temp file created here is leaked — the caller receives "" and has no way to clean it up. Same pattern applies to compileLinkedBitcodeToObject (line 691) where if Compile fails, the temp .o file is left on disk.
| out, err := os.CreateTemp("", tempNamePrefix(moduleName)+"-*.bc") | |
| if err != nil { | |
| return "", fmt.Errorf("create merged bitcode file: %w", err) | |
| } | |
| defer out.Close() | |
| if err := gllvm.WriteBitcodeToFile(mod, out); err != nil { | |
| return "", fmt.Errorf("write merged bitcode file: %w", err) | |
| } | |
| archiveFile.Close() | |
| archivePath := archiveFile.Name() | |
| return out.Name(), nil | |
| out, err := os.CreateTemp("", tempNamePrefix(moduleName)+"-*.bc") | |
| if err != nil { | |
| return "", fmt.Errorf("create merged bitcode file: %w", err) | |
| } | |
| defer out.Close() | |
| if err := gllvm.WriteBitcodeToFile(mod, out); err != nil { | |
| os.Remove(out.Name()) | |
| return "", fmt.Errorf("write merged bitcode file: %w", err) | |
| } |
| for _, bitcodeFile := range bitcodeFiles[1:] { | ||
| srcMod, err := ctx.ParseBitcodeFile(bitcodeFile) | ||
| if err != nil { | ||
| return "", fmt.Errorf("parse bitcode %s: %w", bitcodeFile, err) | ||
| } | ||
| if err := gllvm.LinkModules(mod, srcMod); err != nil { | ||
| return "", fmt.Errorf("link bitcode module %s: %w", bitcodeFile, err) | ||
| } |
There was a problem hiding this comment.
If LinkModules fails, the source module srcMod may not have been consumed by LLVM and will be leaked. Consider disposing it on the error path:
| for _, bitcodeFile := range bitcodeFiles[1:] { | |
| srcMod, err := ctx.ParseBitcodeFile(bitcodeFile) | |
| if err != nil { | |
| return "", fmt.Errorf("parse bitcode %s: %w", bitcodeFile, err) | |
| } | |
| if err := gllvm.LinkModules(mod, srcMod); err != nil { | |
| return "", fmt.Errorf("link bitcode module %s: %w", bitcodeFile, err) | |
| } | |
| for _, bitcodeFile := range bitcodeFiles[1:] { | |
| srcMod, err := ctx.ParseBitcodeFile(bitcodeFile) | |
| if err != nil { | |
| return "", fmt.Errorf("parse bitcode %s: %w", bitcodeFile, err) | |
| } | |
| if err := gllvm.LinkModules(mod, srcMod); err != nil { | |
| srcMod.Dispose() | |
| return "", fmt.Errorf("link bitcode module %s: %w", bitcodeFile, err) | |
| } | |
| } |
| args := []string{"-o", objFile.Name(), "-c", mergedBitcode, "-Wno-override-module"} | ||
| if ctx.shouldPrintCommands(verbose) { | ||
| fmt.Fprintf(os.Stderr, "# compiling linked bitcode for %s\n", moduleName) | ||
| fmt.Fprintln(os.Stderr, "clang", args) |
There was a problem hiding this comment.
fmt.Fprintln(os.Stderr, "clang", args) prints the slice with Go formatting (brackets), e.g. clang [-o file.o -c ...]. Other verbose prints in this file use strings.Join(args, " ") for a copy-pasteable command. Same issue exists in exportObject at line 1417.
Review SummaryThe architecture — merging per-package bitcode in-process via LLVM APIs then compiling one native object — is a clean approach. The separation of bitcode vs native inputs and the cache migration are handled well. A few items to address:
See inline comments for details and suggestions. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1673 +/- ##
==========================================
+ Coverage 91.35% 92.97% +1.61%
==========================================
Files 47 47
Lines 12681 13185 +504
==========================================
+ Hits 11585 12259 +674
+ Misses 906 740 -166
+ Partials 190 186 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
luoliwoshang
changed the title
|
1 participant
Summary
.bc(+ optional native link inputs for non-bitcode objects).bcfor LTO (assembly stays native.o)Cache Behavior (Before vs After)
Before
.a+.manifest.aexists for fingerprint.adirectlyAfter
.bc(required) +.manifest, optional native.afor non-bitcode inputs.bcexists for fingerprint (optional.areused when present).bcfiles are linked in-process to one program module, compiled to one.o, then linked with optional native inputsValidation
go test ./internal/build -count=1/tmp/llgo-new build -x -target esp32c3 -obin -a .in_demo/embed/esp32c3/println(verified in-process bitcode link path, nollvm-linkcommand)/tmp/llgo-new build -size -target esp32c3 -obin -a .in_demo/embed/esp32c3/println