fix(esp32c3): use newlib startup and merge init_array to rodata for println support#1435
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 focuses on integrating Highlights
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 updates the build process for ESP32 targets to use newlibc's _start function. The changes include removing the custom start.S, adjusting the riscv32 architecture flags, modifying the ESP32-RISCV linker script, and disabling stdio buffering for baremetal targets.
Overall, the changes seem to align with the goal of the pull request. However, I've identified a couple of issues that should be addressed:
- A hardcoded map file name has been added in the build process, which appears to be a leftover from debugging.
- A change to the generic
riscv32architecture flags might be too broad and could affect other targets. It would be better to make this change more specific to the intended ESP32 target.
Details are in the specific comments.
internal/build/build.go
Outdated
| } | ||
|
|
||
| buildArgs := []string{"-o", app} | ||
| buildArgs = append(buildArgs, "-Map=111.map") |
There was a problem hiding this comment.
The map file name is hardcoded to 111.map. This looks like a debugging artifact that should not be in production code. A hardcoded name can cause race conditions if builds are run in parallel, and it is not descriptive. It would be better to derive the map file name from the application name, for example app + ".map".
| buildArgs = append(buildArgs, "-Map=111.map") | |
| buildArgs = append(buildArgs, "-Map="+app+".map") |
| ccflags = append(ccflags, "-mdouble=64") | ||
| case "riscv32": | ||
| ccflags = append(ccflags, "-march=rv32imac", "-fforce-enable-int128") | ||
| ccflags = append(ccflags, "-march=rv32imc", "-fforce-enable-int128") |
There was a problem hiding this comment.
Changing -march=rv32imac to rv32imc for the generic riscv32 architecture case removes support for atomic instructions ('A' extension). While this is correct for some ESP32 chips like the ESP32-C3 (which is RV32IMC), other RISC-V chips, including some ESP32 variants like the ESP32-C6 (RV32IMAC), do support atomics. This general change could break builds for other riscv32 targets that rely on atomic instructions.
It would be safer to make this change specific to the target that requires it (e.g., in a target JSON file) rather than changing the generic riscv32 case.
internal/build/build.go
Outdated
| } | ||
|
|
||
| buildArgs := []string{"-o", app} | ||
| buildArgs = append(buildArgs, "-Map=111.map") |
There was a problem hiding this comment.
Critical Issue: Hardcoded debug artifact
The hardcoded filename "111.map" appears to be a temporary debugging artifact that should not be in production code. This causes several problems:
- Concurrency issues: Parallel builds will overwrite the same file
- Security concern: Linker map files contain sensitive memory layout and symbol information
- Disk waste: Map files accumulate without cleanup
- No configurability: Users cannot disable or customize this
Recommendation: Either remove this line entirely, or make it configurable:
// Generate map file only if explicitly requested
if ctx.buildConf.GenerateMapFile {
mapFile := strings.TrimSuffix(app, filepath.Ext(app)) + ".map"
buildArgs = append(buildArgs, fmt.Sprintf("-Map=%s", mapFile))
}| ccflags = append(ccflags, "-mdouble=64") | ||
| case "riscv32": | ||
| ccflags = append(ccflags, "-march=rv32imac", "-fforce-enable-int128") | ||
| ccflags = append(ccflags, "-march=rv32imc", "-fforce-enable-int128") |
There was a problem hiding this comment.
Missing documentation for architectural flag
The -fforce-enable-int128 flag is added without explanation. Please add a comment explaining:
- Why int128 support needs to be forced on riscv32
- What Go runtime requirements this addresses
- Performance implications (software emulation on 32-bit arch)
Example:
case "riscv32":
// Force enable int128 support on 32-bit RISC-V to match Go's type system requirements
// even though native support is limited. This requires software emulation and may
// have performance overhead. Required for Go runtime compatibility.
ccflags = append(ccflags, "-march=rv32imc", "-fforce-enable-int128")|
|
||
| func init() { | ||
| // Disable buffering for baremetal targets to ensure immediate output | ||
| setvbuf(Stdout, nil, _IONBF, 0) |
There was a problem hiding this comment.
Performance Impact: Unbuffered I/O
Disabling buffering with _IONBF will cause every write to trigger an immediate I/O operation, which can severely degrade performance (10-100x slower for frequent small writes). While this ensures immediate output on baremetal targets, consider:
- Adding nil checks in case Fopen fails:
if Stdout != nil {
setvbuf(Stdout, nil, _IONBF, 0)
}
if Stdin != nil {
setvbuf(Stdin, nil, _IONBF, 0)
}-
Alternative: Use line-buffering (
_IOLBF = 1) as a middle ground - flushes on newlines while still buffering within lines -
Make it configurable via environment variable if the performance trade-off is significant for some use cases
Code Review SummaryReviewed changes for baremetal/newlibc improvements. Key findings: Critical: Remove or fix hardcoded Important: Add documentation for Performance Note: Disabling I/O buffering in baremetal stdio will impact I/O-heavy applications. Consider making configurable or using line-buffering. Otherwise, the linker script updates and target configuration changes look appropriate for ESP32 RISC-V support. |
117c7b9 to
765ab4b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1435 +/- ##
==========================================
+ Coverage 90.59% 90.63% +0.03%
==========================================
Files 43 43
Lines 11400 11400
==========================================
+ Hits 10328 10332 +4
+ Misses 911 907 -4
Partials 161 161 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Separate TinyGo and LLGo RISC-V targets to avoid startup file conflicts: - Create riscv-basic.json as pure config base (no extra-files) - Refactor riscv.json to inherit riscv-basic with target_tinygo tag - Create riscv-llgo.json inheriting riscv-basic with target_llgo tag - Create riscv32-llgo.json inheriting riscv-llgo - Update esp32c3.json to inherit riscv32-llgo This allows ESP32-C3 to use newlib's standard startup flow (crt0) instead of TinyGo's start.S, enabling proper C library initialization including __libc_init_array and constructor functions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add CI test to verify ESP32-C3 uses newlib's standard startup flow instead of TinyGo's start.S: - Create test_esp32c3_startup.sh that builds a minimal program and checks for __libc_init_array call in _start function - Add test step to GitHub Actions workflow This test ensures the target inheritance chain works correctly: esp32c3 → riscv32-llgo → riscv-llgo → riscv-basic (no start.S) The test verifies _start calls __libc_init_array, which is only present in newlib's crt0-riscv32-unknown-none startup file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review SummaryThis PR demonstrates excellent architectural refactoring. The separation of TinyGo and LLGo target configurations through a clean inheritance hierarchy is well-designed and maintainable. Key Strengths
Minor Enhancement OpportunitiesWhile the PR is solid, consider these optional improvements for maintainability:
Recommendation: Approve ✨ |
_demo/embed/test_esp32c3_startup.sh
Outdated
| echo "==> Checking for __libc_init_array call in _start..." | ||
|
|
||
| # Disassemble _start and check for __libc_init_array call | ||
| if llvm-objdump -d "$TEST_ELF" | grep -A30 "<_start>:" | grep "__libc_init_array" > /dev/null; then |
There was a problem hiding this comment.
Consider increasing -A30 to -A50 for additional safety margin. While 30 lines is reasonable for most _start implementations, using a larger value provides extra headroom if the function grows in the future.
|
|
||
| .preinit_array : | ||
| { | ||
| . = ALIGN(4); |
There was a problem hiding this comment.
Documentation note: These sections now reside within .rodata (which maps to iram_seg), ensuring they remain in fast instruction RAM. This placement is both secure (read-only) and performant (single-cycle access). Consider adding a brief comment explaining this design decision for future maintainers.
- Add detailed comment in esp32-riscv.app.elf.ld explaining why .preinit_array, .init_array and .fini_array are merged into .rodata (they get lost during ELF→BIN conversion, following ESP-IDF approach) - Increase grep context from -A30 to -A50 in startup regression test for additional safety margin if _start function grows 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
riscv-llgo -> riscv-nostart verify the bin file have the init_array |
- Rename riscv-llgo.json to riscv-nostart.json (more semantic) - Rename riscv32-llgo.json to riscv32-nostart.json (more semantic) - Update inheritance references in esp32c3.json and riscv32-nostart.json - Remove duplicate gdb config from riscv.json (already defined in riscv-basic.json) The '-nostart' suffix clearly indicates these targets don't include startup files, making the inheritance chain more self-documenting.
Remove target_llgo and target_tinygo build-tags as they are not needed for the current implementation.
Add comprehensive regression test to verify: 1. .init_array section is merged into .rodata section 2. __init_array_start symbol points within .rodata 3. .rodata (including .init_array) is included in BIN file The test uses: - llvm-readelf to verify ELF section layout - llvm-nm to check symbol addresses - esptool.py to validate BIN file segments This ensures constructor function pointers are correctly flashed to ESP32-C3.
0d0b75a to
0f559dc
Compare
part of #1438
part of #1427
Issue 1: Not Using newlib's Startup File
Background
Currently,
targets/riscv.jsoncontains TinyGo's startup filesstart.Sandhandleinterrupt.S, which affect all RISC-V chips (including ESP32-C3) through the inheritance mechanism.ESP32-C3 requires newlib's standard startup process (
crt0-riscv32-unknown-none). This startup file calls__libc_init_arrayto execute C library constructors (such asboard_init()), but it is currently overridden by TinyGo'sstart.S, resulting in incomplete C library initialization.Solution: Isolate Startup Files via Inheritance Chain
Core Approach
extra-filesriscv.jsoninheritsriscv-basicand adds startup files (start.S)riscv-nostart.jsoninheritsriscv-basic(no startup files)riscv32-nostart, will not include TinyGo'sstart.SInheritance Relationship Diagram
graph TD A[riscv-basic.json<br/>Pure config, no extra-files] --> B[riscv.json<br/>+ start.S<br/>+ handleinterrupt.S] A --> C[riscv-nostart.json<br/>no extra-files<br/>no startup files] B --> D[riscv32.json<br/>TinyGo 32-bit config] C --> E[riscv32-nostart.json<br/>LLGo 32-bit config<br/>no startup files] D --> F[Other TinyGo RISC-V chips<br/>auto get start.S] E --> G[esp32c3.json<br/>no start.S]File Modification Checklist
1. Create
targets/riscv-basic.jsonCopy all configurations from
riscv.json, removeextra-filesfield:{ "goos": "linux", "goarch": "arm", "build-tags": ["tinygo.riscv", "baremetal", "linux", "arm"], "gc": "conservative", "linker": "ld.lld", "rtlib": "compiler-rt", "libc": "picolibc", "cflags": [ "-Werror", "-mno-relax", "-fno-exceptions", "-fno-unwind-tables", "-fno-asynchronous-unwind-tables", "-ffunction-sections", "-fdata-sections" ], "ldflags": [ "--gc-sections" ], "gdb": ["riscv64-unknown-elf-gdb"] }2. Modify
targets/riscv.jsonInherit
riscv-basic, keep startup files:{ "inherits": ["riscv-basic"], "extra-files": [ "targets/device/riscv/start.S", "targets/device/riscv/handleinterrupt.S" ] }3. Create
targets/riscv-nostart.jsonInherit
riscv-basic, no startup files:{ "inherits": ["riscv-basic"] }4. Create
targets/riscv32-nostart.jsonCopy content from
riscv32.json, change inheritance toriscv-nostart:{ "inherits": ["riscv-nostart"], "llvm-target": "riscv32-unknown-none", "cpu": "generic-rv32", "target-abi": "ilp32", "build-tags": ["tinygo.riscv32"], "scheduler": "tasks", "default-stack-size": 2048, "cflags": [ "-march=rv32imac" ], "ldflags": [ "-melf32lriscv" ], "gdb": [ "gdb-multiarch", "gdb" ] }5. Modify
targets/esp32c3.jsonChange
inheritsfield:{ "inherits": [ "riscv32-nostart" ], // ... other configurations remain unchanged }Comparison of Effects
TinyGo Path (inherit riscv32)
Build Tags:
tinygo.riscv,tinygo.riscv32, ...LLGo Path (inherit riscv32-nostart)
Build Tags:
tinygo.riscv,tinygo.riscv32,esp32c3,espIssue 2:
.init_arraySection Lost During Firmware ConversionBackground
After using newlib's crt0 startup file, although
__libc_init_arrayis called, the constructor array.init_arraysection is lost during the ELF → BIN conversion process when flashing ESP32-C3 firmware, causing the program to crash on startup.Problem Analysis
Boot Log
Segments loaded during ESP32-C3 startup:
Last IRAM segment end address:
ELF File Analysis
Check ELF section table:
Key Finding:
.init_arraysection address: 0x4038ed50 (exactly at the end of the last LOAD segment!)INIT_ARRAY(14)Check
.init_arraysection content:Little-endian decoding:
c0 00 38 40→0x403800c0(board_init function address)BIN File Comparison
Segment 5 end address:
0x4038e000 + 0xd50 = 0x4038ed50Conclusion: The
.init_arraysection (4 bytes starting at 0x4038ed50) is NOT included in the BIN file!Crash Reason
__init_array_startaddress0x4038ed50(garbage)Crash Flow:
__libc_init_arrayreads value at__init_array_start(0x4038ed50)board_initaddress (0x403800c0)Root Cause
ESP32-C3's firmware conversion tool (
internal/build/esp.go:makeESPFirmareImage) only extracts sections of typeSHT_PROGBITS:The
.init_arraysection type isSHT_INIT_ARRAY(14), notSHT_PROGBITS(1), so it's skipped.Since the
.init_arraysection is only 4 bytes and located exactly at the end of the last LOAD segment, it was omitted during conversion.Solution: Merge
.init_arrayinto.rodataSectionFollowing ESP-IDF's approach, modify linker script
targets/esp32-riscv.app.elf.ld:Before (separate sections):
After (merged into .rodata):
Why It Works
.rodatasection type isSHT_PROGBITS, which will be extracted byesp.go.init_arraycontent is embedded into.rodatasection__init_array_start/endstill point to correct locationsImplementation Commit
Commit:
765ab4b9Modified file:
targets/esp32-riscv.app.elf.ldAfter recompilation, segmentation fault disappeared and
board_init()executes successfully!