[wip] reduce embed binrary size #1315
Conversation
PR Review Analysis ✅Reviewing PR #1315 with focus on code quality, potential bugs, improvements, and architecture. Todo List:
✅ Review CompleteI've completed a comprehensive review of PR #1315 and provided detailed feedback with 6 actionable code suggestions focusing on: Key Issues Identified:
Architecture Assessment:The changes enhance LLVM IR processing capabilities which aligns well with the project goals. The implementation adds valuable functionality for module-level IR manipulation when Recommendations Provided:
The core functionality is valuable but needs refinement for production readiness. All suggestions have been provided as actionable code improvements that can be directly applied. 👉 Want me to re-run this review? Just type |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @luoliwoshang, 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 introduces a significant enhancement to the build pipeline by integrating direct support for LLVM Intermediate Representation (IR) files. It allows the system to consume .ll files, link them into a unified LLVM module, compile them into standard object files, and then proceed with the existing linking process. This change provides greater flexibility for incorporating custom or generated LLVM IR into the final executable, potentially opening doors for advanced optimizations or specialized code generation workflows.
Highlights
- LLVM IR File Processing: The build system now supports processing LLVM IR (.ll) files directly. These files are parsed, linked into a main LLVM module, and then compiled into object (.o) files using
clang. - Unified LLVM Module Generation: All processed LLVM IR files are linked into a single 'main' LLVM module, and the combined IR is then written to a temporary file named
temp.all.ll. - Conditional Execution: This new functionality is conditionally enabled based on the
ctx.buildConf.GenLLconfiguration flag, suggesting it's an opt-in feature.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![niupilot[bot]](https://avatars.githubusercontent.com/in/1894735?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR enhances the LLVM IR linking functionality in the build system by adding module-level IR processing when GenLL mode is enabled. The changes introduce LLVM IR parsing, linking, and output generation capabilities.
Issues Found
🔴 Critical Issues
- Memory Leak Risk: LLVM contexts and modules are created but never properly disposed
- File Path Security: Hardcoded file path
./temp.all.llcould cause conflicts in concurrent builds - Error Handling: Multiple
panic()calls instead of proper error propagation
🟡 Medium Priority Issues
- Resource Management: Missing cleanup for temporary files and LLVM objects
- Performance: Unnecessary IR string generation and file write that isn't used
- Code Organization: Large code block added to existing function instead of extracting to separate function
🟢 Minor Issues
- Documentation: No comments explaining the new LLVM IR processing logic
- Consistency: Mixed error handling styles (panic vs return error)
Architectural Considerations
The changes enhance the existing build pipeline with LLVM-level IR manipulation, which aligns well with the project's goals. However, the implementation could be more robust and maintainable.
Recommendations
- Extract to separate function: Move the LLVM IR processing logic to a dedicated function
- Improve resource management: Add proper cleanup and disposal of LLVM objects
- Use unique temp files: Replace hardcoded paths with proper temp file generation
- Add comprehensive error handling: Replace panics with proper error returns
- Add documentation: Include comments explaining the IR linking process
The core functionality is valuable but needs refinement for production readiness.
internal/build/build.go
Outdated
| buildArgs = append(buildArgs, "-gdwarf-4") | ||
| } | ||
|
|
||
| llctx := ll.NewContext() |
There was a problem hiding this comment.
LLVM contexts should be properly disposed to prevent memory leaks. The defer statement ensures cleanup even if errors occur.
| llctx := ll.NewContext() | |
| defer llctx.Dispose() // Ensure LLVM context is properly cleaned up |
internal/build/build.go
Outdated
| } | ||
|
|
||
| llctx := ll.NewContext() | ||
| mainMod := llctx.NewModule("main") |
There was a problem hiding this comment.
LLVM modules should be disposed to free associated memory and prevent resource leaks.
| mainMod := llctx.NewModule("main") | |
| defer mainMod.Dispose() // Ensure LLVM module is properly cleaned up |
internal/build/build.go
Outdated
|
|
||
| irString := mainMod.String() | ||
|
|
||
| file, err := os.Create("./temp.all.ll") |
There was a problem hiding this comment.
Use proper temp file creation instead of hardcoded path to avoid conflicts in concurrent builds and ensure cleanup.
| file, err := os.Create("./temp.all.ll") | |
| tempFile, err := os.CreateTemp("", "llgo-ir-*.ll") | |
| if err != nil { | |
| return fmt.Errorf("failed to create temp IR file: %v", err) | |
| } | |
| defer os.Remove(tempFile.Name()) | |
| defer tempFile.Close() |
| if err != nil { | ||
| panic("failed to read ll file: " + err.Error()) | ||
| } |
There was a problem hiding this comment.
Replace panic with proper error handling. Return error instead of panic to allow graceful error handling and recovery.
| if err != nil { | |
| panic("failed to read ll file: " + err.Error()) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("failed to read ll file %s: %w", objFile, err) | |
| } |
| if err != nil { | ||
| panic("failed to parse ll file: " + err.Error()) | ||
| } |
There was a problem hiding this comment.
Replace panic with proper error handling. Use error wrapping to preserve the original error context.
| if err != nil { | |
| panic("failed to parse ll file: " + err.Error()) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("failed to parse ll file %s: %w", objFile, err) | |
| } |
| if err != nil { | ||
| panic("failed to link ll module: " + err.Error()) | ||
| } |
There was a problem hiding this comment.
Replace panic with proper error handling. Module linking failures should be handled gracefully.
| if err != nil { | |
| panic("failed to link ll module: " + err.Error()) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("failed to link ll module %s: %w", objFile, err) | |
| } |
There was a problem hiding this comment.
Code Review
This pull request adds functionality to process .ll files during the linking phase by parsing, linking, and compiling them. My review focuses on improving error handling to avoid panics, fixing several potential memory leaks related to LLVM objects, removing what appears to be leftover debugging code, and ensuring diagnostic messages are handled correctly.
internal/build/build.go
Outdated
| llctx := ll.NewContext() | ||
| mainMod := llctx.NewModule("main") |
There was a problem hiding this comment.
The ll.Context and ll.Module are not being disposed, which will cause a memory leak. You should defer their Dispose methods right after they are created. Note that the module should be disposed before the context.
For example:
llctx := ll.NewContext()
defer llctx.Dispose()
mainMod := llctx.NewModule("main")
defer mainMod.Dispose()| buf, err := ll.NewMemoryBufferFromFile(objFile) | ||
| if err != nil { | ||
| panic("failed to read ll file: " + err.Error()) | ||
| } | ||
| pkgMod, err := llctx.ParseIR(buf) | ||
| if err != nil { | ||
| panic("failed to parse ll file: " + err.Error()) | ||
| } | ||
| err = ll.LinkModules(mainMod, pkgMod) | ||
| if err != nil { | ||
| panic("failed to link ll module: " + err.Error()) | ||
| } |
There was a problem hiding this comment.
There are a couple of issues in this block:
- Using
panicfor error handling is inconsistent with the function's signature, which returns anerror. Panicking can abruptly terminate the application. It's better to return errors to allow the caller to handle them gracefully. - The
MemoryBuffercreated byll.NewMemoryBufferFromFileis not being disposed, which can lead to a memory leak. You shoulddeferitsDispose()method call right after checking for an error.
| buf, err := ll.NewMemoryBufferFromFile(objFile) | |
| if err != nil { | |
| panic("failed to read ll file: " + err.Error()) | |
| } | |
| pkgMod, err := llctx.ParseIR(buf) | |
| if err != nil { | |
| panic("failed to parse ll file: " + err.Error()) | |
| } | |
| err = ll.LinkModules(mainMod, pkgMod) | |
| if err != nil { | |
| panic("failed to link ll module: " + err.Error()) | |
| } | |
| buf, err := ll.NewMemoryBufferFromFile(objFile) | |
| if err != nil { | |
| return fmt.Errorf("failed to read ll file %s: %w", objFile, err) | |
| } | |
| defer buf.Dispose() | |
| pkgMod, err := llctx.ParseIR(buf) | |
| if err != nil { | |
| return fmt.Errorf("failed to parse ll file %s: %w", objFile, err) | |
| } | |
| if err = ll.LinkModules(mainMod, pkgMod); err != nil { | |
| return fmt.Errorf("failed to link ll module %s: %w", objFile, err) | |
| } |
| if err != nil { | ||
| panic("failed to link ll module: " + err.Error()) | ||
| } | ||
| fmt.Printf("Successfully linked: %s\n", objFile) |
There was a problem hiding this comment.
This fmt.Printf statement writes to standard output. Diagnostic messages like this should be written to standard error (os.Stderr) to separate them from the program's actual output. Also, it should be conditional on the verbose flag, like other similar messages in this file.
| fmt.Printf("Successfully linked: %s\n", objFile) | |
| if verbose { fmt.Fprintf(os.Stderr, "Successfully linked: %s\n", objFile) } |
internal/build/build.go
Outdated
| irString := mainMod.String() | ||
|
|
||
| file, err := os.Create("./temp.all.ll") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create output file: %v", err) | ||
| } | ||
| defer file.Close() | ||
|
|
||
| _, err = file.WriteString(irString) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to write IR: %v", err) | ||
| } |
There was a problem hiding this comment.
This block of code writes the linked LLVM IR to a hardcoded file ./temp.all.ll. This appears to be debugging code that was unintentionally left in.
- Using a hardcoded filename in the current directory can cause conflicts, especially in parallel builds, and assumes write permissions. If a temporary file is needed, consider using
os.CreateTemp. - If this functionality is not required, this entire block should be removed to clean up the code.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1315 +/- ##
=======================================
Coverage 90.10% 90.10%
=======================================
Files 43 43
Lines 12562 12562
=======================================
Hits 11319 11319
Misses 1087 1087
Partials 156 156 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
luoliwoshang
changed the title
No description provided.