fix(build): ensure linker always consumes .a archives#1526
fix(build): ensure linker always consumes .a archives#1526xushiwei merged 14 commits intogoplus:mainfrom
Conversation
Summary of ChangesHello @cpunion, 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 refines the build system's handling of compiled artifacts, specifically focusing on the consistent use and caching of 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. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a refactoring to consistently use archives for linking, both for cached and non-cached builds. The new normalizeToArchive function and changes to saveToCache are central to this. The logic to use llvm-ar for cross-compilation is a good enhancement. Overall, the changes are well-structured. However, I've identified a critical bug in the implementation of normalizeToArchive that could lead to missing files during the link stage. My review includes a suggested fix for this issue.
Code Review SummaryThis PR successfully implements archive normalization to ensure consistent linking behavior between cached and non-cached builds. The implementation uses proper atomic operations and error handling. However, there are several noteworthy issues that should be addressed: Security Concerns:
Performance:
Documentation:
Code Quality:
See inline comments for specific recommendations. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1526 +/- ##
==========================================
- Coverage 91.07% 91.04% -0.04%
==========================================
Files 45 45
Lines 11996 11999 +3
==========================================
- Hits 10925 10924 -1
- Misses 895 899 +4
Partials 176 176 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Translate Chinese comment to English for consistency - Remove redundant nil check in archiver() method 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
wasm-ld requires archives created with llvm-ar because system ar cannot create valid wasm archive symbol indexes. This change: - Prefers llvm-ar from PATH when available - Removes wasm skip in normalizeToArchive (no longer needed) - Shows actual archiver command in verbose output 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
2b4adb3 to
ab4888d
Compare
Refactor aPackage to have clearer data flow: - Rename LLFiles to ObjFiles (stores .o files from compiler) - Add ArchiveFile field (stores .a file path for linking) Data flow is now single-direction: buildPkg: generate .o → ObjFiles normalizeToArchive: ObjFiles → ArchiveFile link: use ArchiveFile This makes the code easier to understand and removes the ambiguity where LLFiles could contain .o, .ll, or .a files depending on context. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
When -gen-llfiles is enabled, copy the .ll file for debugging instead of using it directly for linking. This ensures the linker always receives .o files, avoiding issues where .ll files would be incorrectly packed into .a archives. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Replace duplicate copyFile with existing copyFileAtomic - Remove unused io import - Add build-cache.yml CI workflow to test: - Native and WASM builds - With and without -gen-llfiles - With and without -a (force rebuild) - Each configuration runs twice to verify cache behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
When -gen-llfiles is enabled, clFile and compileExtraFiles were outputting .ll files that got packed into archives. This caused wasm-ld to fail because it can't handle .ll files in archives. Fix by following the same pattern as exportObject: - Always compile to .o for linking - When GenLL=true, also emit .ll for debugging separately 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add CACHE HIT/MISS verbose output to build.go - Create test/buildcache with 6 snapshot-based test scenarios: 1. First build (all CACHE MISS) 2. Second build (deps CACHE HIT) 3. Force rebuild with -a (all CACHE MISS) 4. Dependency change invalidates cache 5. Partial cache clear (dep2 only) 6. Partial cache clear (dep2 and dep3) - Tests run for both native and WASM builds (12 tests total) - Add dev/build_iwasm.sh to build iwasm with CI-compatible options - test.sh auto-builds iwasm if not found in llgo cache - All build outputs go to temp directory to keep source clean - Add explicit error checking for all build and run commands - Update CI workflow and local_ci.sh to use test.sh Closes goplus#1526
Prevents Go compiler from trying to build llgo-only test code that uses C interop via github.com/goplus/lib/c
- Renamed objFiles to linkInputs to clarify it contains both .a archives and .o objects - Removed unreachable else branches that would append ObjFiles (all packages now use ArchiveFile) - Changed panic to proper error handling using closure variable in packages.Visit - Added error check to ensure all packages have ArchiveFile before linking - Main module (.o) and extra files are directly added to linkInputs This makes the linking logic clearer: all packages use .a archives, only the generated main module uses .o files directly.
…sent - Removed else branch that would append ObjFiles - Packages without ArchiveFile (e.g., runtime when not needed) are simply skipped - This is correct behavior: if a package has no compiled code, it has nothing to link The simple if-check is sufficient because: 1. Packages with code will have ArchiveFile (from cache or normalizeToArchive) 2. Packages without code (e.g., unused runtime) legitimately have no ArchiveFile 3. No need for error checking - absence of ArchiveFile is valid
No longer needed since we don't validate missing ArchiveFile
- Added ESP32-C3 tests using same run_test_suite (6 test scenarios) - Updated run_test_suite to support native/wasm/esp32c3 modes - Added esptool.py installation for Linux CI - Total tests: 18 (6 native + 6 wasm + 6 esp32c3)
- Add CACHE HIT/MISS verbose output to build.go - Create test/buildcache with 6 snapshot-based test scenarios: 1. First build (all CACHE MISS) 2. Second build (deps CACHE HIT) 3. Force rebuild with -a (all CACHE MISS) 4. Dependency change invalidates cache 5. Partial cache clear (dep2 only) 6. Partial cache clear (dep2 and dep3) - Tests run for both native and WASM builds (12 tests total) - Add dev/build_iwasm.sh to build iwasm with CI-compatible options - test.sh auto-builds iwasm if not found in llgo cache - All build outputs go to temp directory to keep source clean - Add explicit error checking for all build and run commands - Update CI workflow and local_ci.sh to use test.sh Closes goplus#1526
Problem
.ofiles, subsequent builds link.aarchives from cache. This violates build reproducibility.arcannot create valid wasm archive indexes thatwasm-ldcan read.LLFilesfield could contain.o,.ll, or.afiles depending on context, making code hard to understand.-gen-llfilesbroken: When enabled,.llfiles were packed into.aarchives which the linker cannot process.Fixes #1520
Solution
aPackagewith clearer fields:ObjFiles- object files from compiler (.o)ArchiveFile- archive file path for linking (.a)llvm-arfrom PATH (systemarcannot create valid wasm archives)ctx.archiver()which checks toolchain directory first for cross-compilation-gen-llfilesnow copies.llfiles for debugging instead of using them for linkingobjFilestolinkInputsfor clarity (contains both .a archives and .o from main module)Cache Verification Tests
Added comprehensive build cache verification with snapshot-based testing:
Test Infrastructure
test/buildcache/test/buildcache/test.shwith 6 test scenarios × 3 modes = 18 testsTest Modes
llgo build -o buildcache.out .GOOS=wasip1 GOARCH=wasm llgo build -tags=nogc .llgo build -target=esp32c3 .Test Scenarios (each mode)
-a) - All packages show CACHE MISSFeatures
dev/build_iwasm.sh)build-cache.yml) and local CI (dev/local_ci.sh)Verbose Output
Added
CACHE HIT/CACHE MISSmessages tollgo build -voutput:$ llgo build -v . CACHE MISS: github.com/goplus/llgo/test/buildcache CACHE HIT: github.com/goplus/llgo/test/buildcache/dep1Testing