-
Notifications
You must be signed in to change notification settings - Fork 46
feat(embed): add baremetal defer support #1464
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 lays the groundwork for adding basic 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.
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 basic defer support for baremetal environments by providing alternative implementations for thread-local storage and defer node memory management, controlled by build tags. For single-threaded baremetal targets, it replaces pthread-based TLS with a simple global variable. For memory management, it adds a no-op FreeDeferNode for baremetal builds with tinygogc, letting the GC handle cleanup.
My review identified a critical issue in the build constraints for z_defer_nogc.go. The current change would lead to a build failure for the nogc && baremetal configuration because no implementation of FreeDeferNode would be compiled. I have provided a suggestion to correct the build tag to cover all nogc scenarios as intended. The rest of the changes appear correct and well-structured for the intended purpose.
| @@ -1,4 +1,4 @@ | |||
| //go:build nogc || baremetal | |||
| //go:build nogc && !baremetal | |||
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.
The build constraint nogc && !baremetal leaves the nogc && baremetal build combination without an implementation for FreeDeferNode, which will likely result in a linker error. The nogc build tag should apply to both baremetal and non-baremetal environments, as both require manual memory management when no garbage collector is present. Changing the constraint to just nogc will resolve this by ensuring this c.Free-based implementation is used for all nogc builds.
| //go:build nogc && !baremetal | |
| //go:build nogc |
| @@ -0,0 +1,28 @@ | |||
| //go:build baremetal && !nogc | |||
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.
Critical: Missing build tag coverage for baremetal && nogc
The current build tag matrix has a gap:
!nogc && !baremetal→z_defer_gc.gonogc && !baremetal→z_defer_nogc.gobaremetal && !nogc→z_defer_baremetal.go✓baremetal && nogc→ NO FILE ❌
When building with both baremetal and nogc tags (valid for embedded systems without GC), FreeDeferNode will be undefined, causing compilation failure.
Recommendation: Change the build tag to //go:build baremetal (remove && !nogc) to cover both baremetal scenarios. The no-op approach is appropriate for all baremetal cases since manual memory management is expected in these environments.
| // globalDeferHead stores the current defer chain head. | ||
| // In baremetal single-threaded environment, a global variable | ||
| // replaces pthread TLS. | ||
| var globalDeferHead *Defer |
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.
High: Race condition risk in ISR context
The global variable globalDeferHead is not protected against concurrent access. While documented as "single-threaded," baremetal systems commonly use Interrupt Service Routines (ISRs) that can preempt execution.
If an ISR calls Go code using defer, this can cause:
- Defer chain corruption (linked list broken)
- Lost defer statements
- Use-after-free bugs
Evidence from codebase:
_demo/embed/export/main.go:10mentions "hardware interrupt handlers"- ISR functions call Go code (c.Printf)
Recommendations:
- Disable interrupts during defer chain operations, or
- Document that
deferMUST NOT be used in ISR context, or - Use separate defer chains for ISR vs normal execution
Reference: CWE-362 (Improper Synchronization)
| // FreeDeferNode is a no-op in baremetal environment. | ||
| // Defer nodes become unreachable after being unlinked from the chain, | ||
| // and tinygogc will reclaim them in the next GC cycle. | ||
| func FreeDeferNode(ptr unsafe.Pointer) { |
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.
Performance: Memory leak risk in defer-heavy loops
The no-op FreeDeferNode causes memory accumulation until GC runs. In constrained baremetal environments (ESP32 has ~520KB RAM):
Impact analysis:
- Each defer iteration allocates ~64-96 bytes
- 1000 iterations = ~96KB held until GC
- GC uses stop-the-world mark-and-sweep with linear heap scanning
- Allocation performance degrades as heap fills
Comparison: Other build configurations (z_defer_gc.go, z_defer_nogc.go) explicitly free defer nodes immediately.
Recommendation: Consider implementing a simple defer node pool/freelist to:
- Eliminate GC pressure from defer nodes
- Provide O(1) allocation/deallocation
- Match performance of other builds
Reference: CWE-400 (Uncontrolled Resource Consumption)
| // globalDeferHead stores the current defer chain head. | ||
| // In baremetal single-threaded environment, a global variable | ||
| // replaces pthread TLS. |
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.
Documentation: Add thread-safety warning
Consider making the concurrency constraints more explicit:
| // globalDeferHead stores the current defer chain head. | |
| // In baremetal single-threaded environment, a global variable | |
| // replaces pthread TLS. | |
| // globalDeferHead stores the current defer chain head. | |
| // In baremetal single-threaded environment, a global variable | |
| // replaces pthread TLS. | |
| // | |
| // WARNING: This implementation is NOT thread-safe and NOT ISR-safe. | |
| // Only use in single-threaded baremetal environments. Do NOT call | |
| // any Go code using defer from interrupt service routines (ISRs). |
This makes the safety constraints clearer for future maintainers.
Code Review SummaryThis PR adds clean baremetal defer support with good separation of concerns. However, there are critical issues that must be addressed: Critical:
High Priority:
Positive aspects:
Please address the critical build tag gap and ISR safety issues before merging. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1464 +/- ##
=======================================
Coverage 90.93% 90.93%
=======================================
Files 44 44
Lines 11528 11528
=======================================
Hits 10483 10483
Misses 883 883
Partials 162 162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
target defer build fail cause by libc's log |
ffbf719 to
26fbf15
Compare
f4cfb7f to
b33ef23
Compare
6acf2c3 to
2eafdb6
Compare
- Add defer_tls_baremetal.go: use global variable instead of pthread TLS for single-threaded baremetal environments - Add z_defer_baremetal.go: no-op FreeDeferNode that lets tinygogc collect unreachable defer nodes - Update defer_tls.go: exclude from baremetal builds - Update z_defer_nogc.go: exclude baremetal (uses different strategy) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Baremetal environments (ESP32, etc.) don't have signal support, so sigsetjmp/siglongjmp are not available. Use setjmp/longjmp instead, which are provided by newlib. - Add Baremetal field to ssa.Target - Set Baremetal based on "baremetal" build tag from target config - In Sigsetjmp/Siglongjmp, fallback to setjmp/longjmp for baremetal 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add support for multiple test scenarios in the embedded target build script: - Accept test directory as required first argument (e.g., empty, defer) - Create separate test directories for different compilation tests - Add empty/main.go for basic compilation test - Add defer/main.go for defer statement compilation test This allows testing different Go features across all embedded targets. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Split the targets build step into two: - Build targets (empty): basic compilation test - Build targets (defer): defer statement compilation test This verifies that defer works correctly on baremetal targets. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Refactor ignore_list to be defined per test directory using case statement. Each target on a separate line for cleaner diffs when modifying ignore lists. Unknown test directories now error out instead of using a default list. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update defer/main.go to test all three defer modes: - DeferAlways: regular defer statements - DeferInCond: defer inside if/else blocks - DeferInLoop: defer inside for loops Use c.Printf instead of println for baremetal compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add targets to defer ignore list: - digispark: inline asm branch target out of range - nintendoswitch: missing bits/wordsize.h header - simavr: incompatible target ISA - Various targets lacking libc symbols (arduino-*, cortex-m*, esp8266, etc.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add platform-specific jmpbuf size files for TinyGo-compatible targets: - jmpbuf_arm.go: generic ARM (Linux ARM, ARM7TDMI) - jmpbuf_avr.go: AVR 8-bit (Arduino) - jmpbuf_cortexm.go: ARM Cortex-M - jmpbuf_riscv.go: generic RISC-V - jmpbuf_riscv32.go: RISC-V 32-bit - jmpbuf_riscv64.go: RISC-V 64-bit - jmpbuf_xtensa.go: Xtensa (ESP32/ESP8266) Update jmpbuf_other.go to exclude baremetal and wasm targets. All sizes are set to 200 bytes as placeholder, to be confirmed from picolibc/newlib machine/setjmp.h for each architecture. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Use maximum size (Windowed ABI) to cover both ESP32 and ESP8266. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Use setjmp.Longjmp to execute deferred functions on panic in baremetal environments instead of immediately exiting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Rename jmpbuf files for baremetal targets to use _baremetal suffix: - jmpbuf_arm.go -> jmpbuf_arm_baremetal.go - jmpbuf_avr.go -> jmpbuf_avr_baremetal.go - jmpbuf_cortexm.go -> jmpbuf_cortexm_baremetal.go - jmpbuf_riscv.go -> jmpbuf_riscv_baremetal.go - jmpbuf_riscv32.go -> jmpbuf_riscv32_baremetal.go - jmpbuf_riscv64.go -> jmpbuf_riscv64_baremetal.go - jmpbuf_xtensa.go -> jmpbuf_xtensa_baremetal.go This fixes Go's go/build package incorrectly parsing filenames like jmpbuf_riscv64.go as requiring GOARCH=riscv64, when the actual GOARCH for baremetal targets is "arm". 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove the non-baremetal arm condition from jmpbuf_arm_baremetal.go. Non-baremetal ARM targets should fallback to jmpbuf_other.go. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update jmp_buf sizes based on picolibc/newlib definitions: - ARM7TDMI (GBA): 160 bytes - AVR: 48 bytes - Cortex-M: 160 bytes - RISC-V 32-bit: 304 bytes - RISC-V 64-bit: 208 bytes - Xtensa: 68 bytes (Windowed ABI, _JBLEN=17) Also update comments with correct source references. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The jmpbuf_riscv_baremetal.go file is no longer needed since jmpbuf_riscv32_baremetal.go and jmpbuf_riscv64_baremetal.go now handle both 32-bit and 64-bit RISC-V architectures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When defer in loop captures variables, panic exit causes issues with preceding defer normal execution. Without variable capture, it works correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
2eafdb6 to
9fa3aed
Compare
Replace Target.Baremetal with Target.Target to directly store the -target flag value (e.g., "esp32", "arm7tdmi", "wasi"). This allows platform-specific setjmp/sigsetjmp selection to be handled via build-tags in runtime package instead of hardcoded logic. Changes: - Remove Target.Baremetal field - Add Target.Target string field for -target parameter - Simplify Sigsetjmp/Siglongjmp logic: use setjmp/longjmp when GOARCH is "wasm" or Target is non-empty - Platform-specific sigsetjmp implementations can now be selected via build-tags (e.g., sigsetjmp_darwin.go) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
Add defer support for baremetal/embedded environments (ESP32, ARM7TDMI, etc.).
Key Changes:
setjmp/longjmpfor baremetal targets instead ofsigsetjmp/siglongjmpreturns_twiceattribute to setjmp/sigsetjmp for correct LLVM optimization-targetflag directly instead ofBaremetalbooleanArchitecture
Target Detection Refactoring
Replaced boolean-based target detection with direct
-targetflag storage:Logic in
Sigsetjmp/Siglongjmp:Target != ""orGOARCH == "wasm"→ usesetjmp/longjmpsigsetjmp/siglongjmp(platform-specific via build-tags)Baremetal Defer Implementation
File Structure:
defer_tls_baremetal.go: Uses global variable for single-threaded baremetalz_defer_baremetal.go: No-opFreeDeferNode(let GC collect), uselongjmpinRethrow//go:build baremetalto separate from pthread-based implementationJMP_BUF Size Definitions
Added platform-specific jmp_buf sizes for all embedded targets:
jmpbuf_xtensa_baremetal.gojmpbuf_riscv32_baremetal.gojmpbuf_riscv64_baremetal.gojmpbuf_arm_baremetal.gojmpbuf_cortexm_baremetal.gojmpbuf_avr_baremetal.gojmpbuf_other.goReturns Twice Attribute
Added LLVM
returns_twiceattribute tosetjmpandsigsetjmpfunction calls:Verification on ESP32
All three defer modes work correctly:
defer 1,defer 2(LIFO order ✅)defer in if(conditional defer ✅)defer in loop 2/1/0(LIFO order per iteration ✅)Related PRs
rtType("SigjmpBuf")instead of cgo)returns_twiceattribute to sigsetjmp for OS platformsTest plan
🤖 Generated with Claude Code