refactor: move sigsetjmp platform detection from compiler to runtime #1481
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 centralizes the handling of platform-specific 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the platform-specific sigsetjmp/siglongjmp logic by moving it from the compiler to the runtime, using build tags for platform detection. The changes are well-structured and correctly implement the intended refactoring.
As you've correctly identified in the description, this approach introduces a critical issue with the setjmp/longjmp semantics due to the extra stack frames created by the runtime wrappers. This breaks the non-local jump mechanism.
The correct solution, as you've hinted, would likely involve compiler support. The llgo compiler would need to treat runtime.Sigsetjmp and runtime.Siglongjmp as intrinsics and inline them at the call site, thus eliminating the problematic intermediate stack frames. This would achieve the desired separation of concerns without breaking functionality.
This PR is a valuable exploration of the problem space. I've added one minor suggestion to mark an unused parameter for clarity.
| func Sigsetjmp(env *SigjmpBuf, savemask c.Int) c.Int { | ||
| return Setjmp((*JmpBuf)(env)) | ||
| } |
There was a problem hiding this comment.
The savemask parameter is unused in this function because setjmp (which is used here for baremetal/wasm) does not support it. To make this explicit and avoid potential linter warnings, it's good practice to rename it to _.
| func Sigsetjmp(env *SigjmpBuf, savemask c.Int) c.Int { | |
| return Setjmp((*JmpBuf)(env)) | |
| } | |
| func Sigsetjmp(env *SigjmpBuf, _ c.Int) c.Int { | |
| return Setjmp((*JmpBuf)(env)) | |
| } |
Move platform-specific sigsetjmp/siglongjmp logic from ssa/eh.go to
runtime using build tags. This follows the same pattern as AllocU.
Architecture:
- ssa/eh.go calls rtFunc("Sigsetjmp")/rtFunc("Siglongjmp")
- runtime/setjmp.go wraps clite/setjmp functions
- clite/setjmp uses build tags for platform selection:
- sigsetjmp.go: default (C.sigsetjmp/C.siglongjmp)
- sigsetjmp_linux.go: Linux (C.__sigsetjmp/C.siglongjmp)
- sigsetjmp_baremetal.go: baremetal/wasm (setjmp/longjmp)
Changes:
- Remove platform checks from ssa/eh.go Sigsetjmp/Siglongjmp
- Add runtime wrapper functions for Sigsetjmp/Siglongjmp
- Add platform-specific implementations in clite/setjmp
- Change clite/setjmp LLGoPackage from "decl" to "link"
NOTE: This approach has issues with setjmp/longjmp semantics - the
wrapper functions create intermediate stack frames which break the
non-local jump behavior. This PR is for discussion purposes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
luoliwoshang
force-pushed
the
refactor/sigsetjmp-runtime
branch
from
b6c234d to
0b947ec
Compare
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| // - Linux: uses __sigsetjmp/siglongjmp | ||
| // - Baremetal/wasm: uses setjmp/longjmp | ||
| func Sigsetjmp(env unsafe.Pointer, savemask int32) int32 { | ||
| return int32(setjmp.Sigsetjmp((*setjmp.SigjmpBuf)(env), savemask)) |
There was a problem hiding this comment.
CRITICAL: Wrapper function breaks setjmp semantics
This Go wrapper creates an intermediate stack frame that breaks the fundamental contract of setjmp/longjmp:
caller → runtime.Sigsetjmp() → setjmp.Sigsetjmp() → C.sigsetjmp
↑ wrong stack frame saved
When C.sigsetjmp executes, it saves the stack context of the wrapper function, not the original caller. When longjmp restores this context, it returns to a potentially destroyed stack frame.
Impact: Defer/panic/recover will fail in production.
Solution: The compiler must call setjmp.Sigsetjmp directly without Go wrappers, OR use LLVM intrinsics/inline assembly to avoid creating stack frames.
| // On baremetal and wasm, use setjmp instead of sigsetjmp | ||
| // since these platforms don't support signal handling. | ||
| func Sigsetjmp(env *SigjmpBuf, savemask c.Int) c.Int { | ||
| return Setjmp((*JmpBuf)(env)) |
There was a problem hiding this comment.
Security: Type confusion risk
Direct cast from *SigjmpBuf to *JmpBuf without size validation. If these types have different sizes on some platforms, this causes stack corruption.
Checking the constants shows Darwin amd64 has SigjmpBufSize=196 vs JmpBufSize=192 (4 byte mismatch).
Fix: Add compile-time assertion:
const _ = uint(SigjmpBufSize - JmpBufSize) // compile error if sizes differ
|
|
||
| // On baremetal and wasm, use setjmp instead of sigsetjmp | ||
| // since these platforms don't support signal handling. | ||
| func Sigsetjmp(env *SigjmpBuf, savemask c.Int) c.Int { |
There was a problem hiding this comment.
Documentation: The savemask parameter is silently ignored on baremetal/wasm platforms. This should be explicitly documented to prevent confusion:
// Note: savemask parameter is ignored on baremetal/wasm as these
// platforms don't support signal mask preservation.
| @@ -0,0 +1,31 @@ | |||
| //go:build !linux && !baremetal && !wasm | |||
There was a problem hiding this comment.
Build tag overlap: The build tag !baremetal && !wasm here conflicts with sigsetjmp_baremetal.go which uses baremetal || wasm.
This means wasm matches the baremetal file but is excluded from this file, which is correct. However, for clarity, consider using positive conditions:
//go:build !(linux || baremetal || wasm)
Code Review SummaryThis PR successfully moves platform detection from compiler to runtime, improving code organization. However, there's a critical architectural issue that must be addressed before merging. Critical Issues
Recommended Actions
The refactoring goal is sound, but the execution needs adjustment to preserve setjmp/longjmp semantics. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1481 +/- ##
==========================================
- Coverage 90.58% 90.50% -0.08%
==========================================
Files 43 43
Lines 11429 11421 -8
==========================================
- Hits 10353 10337 -16
- Misses 914 925 +11
+ Partials 162 159 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
Move platform-specific sigsetjmp/siglongjmp logic from
ssa/eh.goto runtime using build tags, following the same pattern asAllocU.Architecture
Changes
ssa/eh.goSigsetjmp/SiglongjmpThis approach has issues with setjmp/longjmp semantics.
The wrapper functions create intermediate stack frames which break the non-local jump behavior:
sigsetjmpsaves the stack frame ofsetjmp.Sigsetjmp(), not the original defer code location. Whenlongjmpjumps back, it goes to the wrong stack position.This PR is for discussion purposes to explore whether there's a way to make this work (e.g., inline expansion in llgo).
Test Results
🤖 Generated with Claude Code