-
Notifications
You must be signed in to change notification settings - Fork 46
fix(esp32c3): use semihosting _exit for QEMU emulator support #1539
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?
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 addresses an issue where the ESP32-C3 QEMU emulator was not correctly utilizing the semihosting 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 correctly addresses the conflicting _exit symbol implementations for ESP32-C3 QEMU support by commenting out the stub versions from the build configuration. The changes are logical and well-explained in the description. I've added one comment regarding code duplication in the file list, which could be addressed to improve maintainability.
Code Review SummaryThe PR correctly addresses the immediate issue of conflicting Critical Issues Found:
Recommendation:
See inline comments for specific details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1539 +/- ##
==========================================
- Coverage 91.01% 91.01% -0.01%
==========================================
Files 45 45
Lines 11971 11966 -5
==========================================
- Hits 10896 10891 -5
Misses 899 899
Partials 176 176 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Move _exit implementation from syscalls.c to syscalls_exit.c to allow easy replacement with semihosting version for QEMU emulator support. This separation enables llgo to compile the semihosting _exit as a separate object file that can override the default implementation when building for QEMU targets. Related to: goplus/llgo#1539 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
3f1b5c7 to
ce7bca1
Compare
Move _exit implementation from syscalls.c to syscalls_exit.c to allow easy replacement with semihosting version for QEMU emulator support. This separation enables llgo to compile the semihosting _exit as a separate object file that can override the default implementation when building for QEMU targets. Related to: goplus/llgo#1539 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <[email protected]>
487d0dd to
22bc753
Compare
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_close.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_exit.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_fdtable.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_fstat.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_ftime.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_isatty.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_link.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_lseek.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_open.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_read.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_sbrk.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_stat.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_stat_common.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_unlink.c"), | ||
| filepath.Join(baseDir, "libgloss", "riscv", "semihost-sys_write.c"), |
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.
it's reduant file,actually we not connect symbol from these files.
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.
@xgopilot it's a not a expect diff , make it have not diff of main branch!
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.
Fixed Whitespace Issue ✓
Changes completed:
- Read comment history to understand the issue
- Compare current branch with main branch
- Identify unexpected differences
- Fix the whitespace issue in esp32-riscv.app.elf.ld
- Commit changes locally
What I Fixed
The targets/esp32-riscv.app.elf.ld file had an unintended whitespace change on line 25:
- Before:
.rodata :(4 spaces) - After:
.rodata :(1 space)
This formatting change was unrelated to the PR's purpose (semihosting _exit support). I've reverted it, and the file now matches the main branch exactly.
Permission Required
Hi @luoliwoshang! I've prepared your changes locally but need permission to push to your fork.
View Details & Solution
Target: luoliwoshang/llgo (fork)
Push blocked:
remote: Invalid username or token. Password authentication is not supported for Git operations.
fatal: Authentication failed for 'https://github.com/luoliwoshang/llgo.git/'
Alternative: Manual Push
Since you own the fork, you can pull and push the changes yourself:
# Fetch the changes from the main repo
git fetch origin
# Cherry-pick my commit
git cherry-pick b3ee8066
# Or apply the simple fix directly
# In targets/esp32-riscv.app.elf.ld line 25, change:
# .rodata :
# To:
# .rodata :
# (add 3 more spaces after .rodata)
# Then push
git push origin esp32c3/exitsemiThere 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.
@xgopilot now you can create the fork pr to me ! just do it!
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.
Fork PR Created Successfully ✓
- Check current branch and changes
- Fix whitespace issue in esp32-riscv.app.elf.ld
- Commit and push changes to fork
- Create PR back to your repository
What I Did
I've successfully created a fork PR back to your repository! Here's what was completed:
-
Fixed the whitespace issue in
targets/esp32-riscv.app.elf.ldline 25:- Changed
.rodata :(1 space) →.rodata :(4 spaces) - This matches the formatting in the main branch
- Changed
-
Committed the change with proper attribution
-
Pushed to your fork at
luoliwoshang/llgo -
Created PR sqlite demo #182: fix(esp32c3): restore whitespace in linker script to match main
- Source:
fork-pr-1539-1768440281 - Target:
esp32c3/exitsemi
- Source:
Next Steps
You can now review and merge PR #182 in your fork repository to incorporate this whitespace fix into your esp32c3/exitsemi branch.
ESP32-C3 bootloader starts at flash offset 0x0, unlike ESP32 which starts at 0x1000. The previous code added 4KB padding for all ESP chips, causing QEMU to fail with "invalid header: 0x00000000" when trying to boot ESP32-C3 images. This fix only adds the 4KB padding for ESP32 (Xtensa), while ESP32-C3 (RISC-V) images now start at offset 0x0 as expected by its ROM bootloader. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add emulator command configuration to esp32c3.json target file, enabling ESP32-C3 programs to run in QEMU emulator with: llgo run -target=esp32c3 -emulator . 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add missing newlines at end of esp32c3.json and esp32c3-basic.json to follow standard file formatting conventions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[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]>
Add -semihosting flag to QEMU command to enable proper program exit using semihosting _exit implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update newlib version to patch5 which includes the split of _exit from syscalls.c to syscalls_exit.c. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove libgloss/libnosys/_exit.c from RISC-V compile list to ensure semihosting _exit is used for QEMU emulator support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Update test script to use project directory for temp files (enables go.mod module resolution) - Use real Hello World program in test instead of empty program (ensures .rodata has PROGBITS type for proper BIN generation) - Remove duplicate semihost-sys_exit.c from RISC-V config (was causing conflict with custom _exit implementation) - Fix whitespace in linker script 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Move semihost-sys_exit.c to its own library (libsemihost-{target}.a)
for cleaner separation of semihosting support from core startup code.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
These semihost syscall files are not actually used - only semihost-sys_exit.c is needed for QEMU emulator support (now in separate libsemihost library). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update libc_test.go to reflect the new 4-group structure for RISC-V: - Group 0: libsemihost (semihosting _exit for QEMU) - Group 1: libcrt0 (board startup) - Group 2: libgloss (system stubs) - Group 3: libc (C library) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add comprehensive test framework for running embedded programs in QEMU emulator:
Architecture:
- Tests located in cl/_testemb/ (parallel to existing cl tests)
- Uses build.Do() directly (similar to build_test.go pattern)
- Auto-discovers test cases: any directory with main.go + expect.txt
Features:
- TestEmbRun runs all tests across multiple targets (ESP32C3, ESP32)
- filterEmulatorOutput() extracts program output after "entry 0x..." line
- Logs both filtered and full emulator output for debugging
- Test case structure:
cl/_testemb/
└── cprintf/
├── main.go # Test program
└── expect.txt # Expected output
Usage:
export PATH="/path/to/esp-qemu/bin:$PATH"
go test ./cl -run TestEmbRun -v
Adding new tests: just create new directory with main.go + expect.txt
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add workflow step to install ESP32 RISC-V QEMU emulator for running embedded tests.
Script features (.github/workflows/install-esp-qemu.sh):
- Accepts installation directory as argument (default: .cache/qemu)
- Auto-detects platform (OS + architecture)
- Downloads platform-specific QEMU from espressif/qemu releases (v9.2.2)
- Supports: macOS (x86_64/aarch64) and Linux (x86_64/aarch64)
- Downloads and extracts to specified directory
- Verifies qemu-system-riscv32 binary exists after installation
CI integration (go.yml):
- Installs QEMU to .cache/qemu before running tests
- Adds QEMU bin directory to PATH for subsequent steps
- Verifies installation with version check
- Required for cl/embed_test.go emulator tests
Verified file structure:
.cache/qemu/
└── bin/
└── qemu-system-riscv32
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
ESP32 QEMU requires libSDL2-2.0.so.0 shared library on Linux. Error without SDL2: qemu-system-riscv32: error while loading shared libraries: libSDL2-2.0.so.0: cannot open shared object file: No such file or directory Solution: - Add libsdl2-2.0-0 to Ubuntu common dependencies - macOS already has SDL2 bundled with QEMU or available via brew 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
1. Add SDL2 dependency for macOS - ESP32 QEMU requires libSDL2-2.0.0.dylib on macOS - Error: Library not loaded: /opt/homebrew/opt/sdl2/lib/libSDL2-2.0.0.dylib - Solution: brew install sdl2 2. Remove ESP32 (Xtensa) test from embed_test.go - Only keep ESP32C3 (RISC-V) test - ESP32 Xtensa QEMU requires separate setup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace t.Logf() with fmt.Printf() to always show test output regardless of -v flag. Benefits: - CI logs always show program output even without go test -v - Easier debugging when tests fail - Consistent visibility in all environments Output format: === Test Output for <test_name> === Program output: <filtered output> Full emulator output: <complete emulator log> ======================== 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Restore temporarily deleted workflow files: - build-cache.yml - doc-link-checker.yml - doc.yml - fmt.yml - llgo.yml - release-build.yml - stdlib-coverage.yml - targets.yml 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add //go:build !llgo and // +build !llgo tags to embed_test.go to prevent it from being compiled by llgo itself. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: xgopilot <[email protected]> Co-authored-by: luoliwoshang <[email protected]>
Drop cl/_testemb harness and embed_test.go since the emulator test flow is being replaced. Extend _demo/embed/test_esp32c3_startup.sh to run QEMU and assert the final output line, printing full logs on mismatch. Co-authored-by: Codex <[email protected]>
Introduce setup-embed-deps to install SDL2 runtime requirements for QEMU. Use it in go.yml and llgo.yml before embedded tests.
Move ESP32 RISC-V QEMU install/verify steps into setup-embed-deps. Use setup-embed-deps only in llgo.yml's llgo job and remove duplicate steps from go.yml.
Embed QEMU dependencies now live in setup-embed-deps.
87d8a88 to
c45489f
Compare
Summary
Remove conflicting
_exitimplementations to enable semihosting support for ESP32-C3 QEMU emulator.Background
Previously, multiple
_exitimplementations existed in newlib:libnosys/_exit.c- stub implementationlibgloss/riscv/sys_exit.c- another stubsyscalls.c- usesasm("unimp")which crashes in QEMUsemihost-sys_exit.c- proper semihosting implementation usingebreakThe
syscalls.cversion was being linked, causing QEMU to crash with "Illegal instruction".Solution
After goplus/newlib#10 separates
_exitfromsyscalls.c, we can simply remove the other stub implementations (libnosys/_exit.candlibgloss/riscv/sys_exit.c). This leavessemihost-sys_exit.cas the only_exitimplementation, which will be automatically used from the existing.alibrary.Changes
Remove conflicting
_exitimplementations from compile list:libnosys/_exit.clibgloss/riscv/sys_exit.c(3 occurrences in different compile groups)Result
Now ESP32-C3 programs properly use semihosting
_exitand exit cleanly in QEMU with-semihostingflag.Depends on
_exitfromsyscalls.ctosyscalls_exit.c)Test plan
-semihostingflag🤖 Generated with Claude Code