-
Notifications
You must be signed in to change notification settings - Fork 46
feat: support compiling to object file (.o) for unconditional linking #1537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: support compiling to object file (.o) for unconditional linking #1537
Conversation
Summary of ChangesHello @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 compilation and linking process by allowing the generation and direct linking of individual object files. This change addresses a common problem in embedded development where weak symbols in standard libraries can prevent stronger, custom implementations from being linked. By enabling unconditional linking of object files, the system gains more control over symbol resolution, specifically demonstrated by fixing a QEMU semihosting issue for ESP32-C3 targets. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for compiling source files directly into object files (.o) in addition to static archives (.a). This is a valuable feature for enabling unconditional linking, which is crucial for scenarios like overriding weak symbols, as demonstrated with the semihosting _exit function for ESP32-C3 QEMU support. The changes are well-structured across compile.go, crosscompile.go, and newlibesp.go, and the implementation correctly handles the different linking requirements for object files versus static libraries. My review includes one suggestion to improve code maintainability by reducing duplication.
| lang := "c" | ||
| if filepath.Ext(file) == ".S" { | ||
| lang = "assembler-with-cpp" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic to determine the language from the file extension is also used for the .a archive compilation path later in this function (lines 87-90). To improve maintainability and avoid code duplication, consider extracting this logic into a helper function.
For example:
func getLangFromFile(file string) string {
if filepath.Ext(file) == ".S" {
return "assembler-with-cpp"
}
return "c"
}You could then call this function in both places: lang := getLangFromFile(file).
References
- To improve maintainability, code should not be duplicated. Instead, it should be extracted into a reusable function. This is known as the Don't Repeat Yourself (DRY) principle.
| } | ||
| file := g.Files[0] | ||
| lang := "c" | ||
| if filepath.Ext(file) == ".S" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This language detection logic is duplicated on lines 87-90. Consider extracting it to a helper method to maintain DRY principle:
func getFileLanguage(file string) string {
if filepath.Ext(file) == ".S" {
return "assembler-with-cpp"
}
return "c"
}| } | ||
| if filepath.Ext(group.OutputFileName) == ".o" { | ||
| continue | ||
| // .o files are linked directly (unconditionally) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment explaining the "why" behind this distinction. The rationale from newlibesp.go:52 about overriding weak symbols would help future maintainers understand the importance of unconditional linking:
// Object files (.o) are linked directly (unconditionally) to ensure symbols
// override weak symbols in other libraries (e.g., semihost _exit overrides weak _exit).
// Archive files (.a) use the -l flag for on-demand linking by the linker.
Code Review SummaryThis PR successfully implements Key Strengths✅ Performance Optimization: The fast path for Minor Improvements SuggestedI've left inline comments for three areas where documentation could be improved:
The code is production-ready. These are all low-priority documentation improvements that would help with long-term maintainability. |
Add support for CompileGroup to output .o files instead of .a archives. This enables unconditional linking of object files, which is needed when a strong symbol must override a weak symbol in another library. Changes: - compile.go: Support .o output (single file compilation) - crosscompile.go: Link .o files directly instead of using -l flag - newlibesp.go: Add libsemihost.o for ESP32-C3 QEMU semihosting support The semihosting _exit implementation is compiled as .o to ensure it overrides the weak _exit in syscalls.c, enabling proper exit handling in QEMU emulator. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
ebd6954 to
4416d4e
Compare
Remove conflicting _exit implementations from libnosys and libgloss to ensure the semihosting version (from semihost-sys_exit.c, added as .o in goplus#1537) is used for ESP32-C3 QEMU emulator. Changes: - Comment out libnosys/_exit.c - Comment out libgloss/riscv/sys_exit.c (3 occurrences) This allows proper exit handling in QEMU emulator via semihosting protocol instead of using the `unimp` instruction which causes crashes. Related: goplus#1537 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove conflicting _exit implementations from libnosys and libgloss to
ensure the semihosting version (from semihost-sys_exit.c, added as .o
in goplus#1537) is used for ESP32-C3 QEMU emulator.
Changes:
- Comment out libnosys/_exit.c
- Comment out libgloss/riscv/sys_exit.c (3 occurrences)
This allows proper exit handling in QEMU emulator via semihosting
protocol instead of using the instruction which causes crashes.
Related: goplus#1537
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove conflicting _exit implementations from libnosys and libgloss to
ensure the semihosting version (from semihost-sys_exit.c, added as .o
in goplus#1537) is used for ESP32-C3 QEMU emulator.
Changes:
- Comment out libnosys/_exit.c
- Comment out libgloss/riscv/sys_exit.c (3 occurrences)
This allows proper exit handling in QEMU emulator via semihosting
protocol instead of using the instruction which causes crashes.
Related: goplus#1537
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove conflicting _exit implementations from libnosys and libgloss to
ensure the semihosting version (from semihost-sys_exit.c, added as .o
in goplus#1537) is used for ESP32-C3 QEMU emulator.
Changes:
- Comment out libnosys/_exit.c
- Comment out libgloss/riscv/sys_exit.c (3 occurrences)
This allows proper exit handling in QEMU emulator via semihosting
protocol instead of using the instruction which causes crashes.
Related: goplus#1537
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove conflicting _exit implementations from libnosys and libgloss to
ensure the semihosting version (from semihost-sys_exit.c, added as .o
in goplus#1537) is used for ESP32-C3 QEMU emulator.
Changes:
- Comment out libnosys/_exit.c
- Comment out libgloss/riscv/sys_exit.c (3 occurrences)
This allows proper exit handling in QEMU emulator via semihosting
protocol instead of using the instruction which causes crashes.
Related: goplus#1537
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Summary
CompileGroupto output.ofiles instead of.aarchives.ofiles are linked directly, not on-demand like.a)libsemihost.ofor ESP32-C3 QEMU semihosting supportBackground
Static libraries (
.a) use on-demand linking - object files are only pulled in when they resolve undefined symbols. This causes issues when a strong symbol needs to override a weak symbol in another library.For example,
syscalls.ccontains a weak_exitthat usesasm("unimp"), which crashes in QEMU. The semihosting_exitfromsemihost-sys_exit.cuses proper semihosting protocol, but when compiled into a.a, it won't be linked because the weak_exitis already satisfied.By compiling to
.oand linking directly, the strong_exitis unconditionally included and overrides the weak version.Changes
compile.go: Support.ooutput (single file compilation, noarpackaging)crosscompile.go: Link.ofiles directly as path instead of using-lflagnewlibesp.go: Addlibsemihost.ogroup for ESP32-C3 RISC-V semihostingTest plan
_exituses semihosting implementation (notunimp)🤖 Generated with Claude Code