fix(runtime): restore SIGSEGV signal handler for non-wasm platforms#1383
fix(runtime): restore SIGSEGV signal handler for non-wasm platforms#1383xushiwei merged 1 commit intogoplus:mainfrom
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 reintroduces the SIGSEGV signal handler for non-WebAssembly platforms within the 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 correctly restores the SIGSEGV signal handler for non-wasm platforms by using a platform-specific build file. This change ensures that nil pointer dereferences result in a recoverable Go panic, which aligns with standard Go behavior. The implementation is clean and follows best practices for platform-specific code. I have one suggestion to simplify the new signal handler by removing a redundant switch statement.
| signal.Signal(c.Int(syscall.SIGSEGV), func(v c.Int) { | ||
| switch syscall.Signal(v) { | ||
| case syscall.SIGSEGV: | ||
| panic(errorString("invalid memory address or nil pointer dereference")) | ||
| default: | ||
| var buf [20]byte | ||
| panic(errorString("unexpected signal value: " + string(itoa(buf[:], uint64(v))))) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The signal handler is registered specifically for syscall.SIGSEGV, so it will only be invoked for this signal. The switch statement is therefore redundant, and the default case is unreachable code. You can simplify the handler and remove the unused v parameter by renaming it to _.
signal.Signal(c.Int(syscall.SIGSEGV), func(_ c.Int) {
panic(errorString("invalid memory address or nil pointer dereference"))
})| @@ -0,0 +1,22 @@ | |||
| //go:build !wasm | |||
|
|
|||
| package runtime | |||
There was a problem hiding this comment.
Missing Documentation
This file lacks explanatory comments. Given the platform-specific nature and relationship to PR #1059, please add:
// This file contains platform-specific runtime initialization for non-wasm targets.
// The SIGSEGV signal handler enables Go-style panic recovery for nil pointer dereferences
// instead of immediate process termination.
//
// For wasm platform compatibility, signal handling is excluded via build tags.
// See PR #1059 for wasm platform requirements.| signal.Signal(c.Int(syscall.SIGSEGV), func(v c.Int) { | ||
| switch syscall.Signal(v) { | ||
| case syscall.SIGSEGV: | ||
| panic(errorString("invalid memory address or nil pointer dereference")) |
There was a problem hiding this comment.
This signal handler violates async-signal-safety by calling panic(), which allocates memory via c.Malloc(). Per POSIX standards, signal handlers should only call async-signal-safe functions.
Risk: If SIGSEGV occurs while inside malloc/free, the handler calling malloc again can cause deadlock or memory corruption.
Recommendation: Consider pre-allocating the error value and using only async-signal-safe operations in the handler, similar to how standard Go's runtime handles signals with dedicated signal stacks and atomic operations.
Code Review SummaryOverall: Good approach to platform-specific signal handling with proper build tags. Implementation successfully restores nil pointer panic behavior for non-wasm platforms. Key Issues:
The security concern around async-signal-safety is the most significant technical issue, though it matches the original commented code pattern. |
93532a1 to
11690ba
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1383 +/- ##
=======================================
Coverage 90.98% 90.98%
=======================================
Files 43 43
Lines 11287 11287
=======================================
Hits 10269 10269
Misses 859 859
Partials 159 159 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
45a2521 to
c5a32bc
Compare
6033929 to
6c36935
Compare
…ference to recoverable panic - Add z_rt_default.go with signal handler for SIGSEGV on non-wasm platforms - Convert segmentation faults from nil pointer access to Go panic - Enable recover() to catch nil pointer dereference errors - Use build tag (!wasm) to maintain wasm platform compatibility - Remove commented-out signal handling code from z_rt.go This aligns llgo's behavior with standard Go, where accessing nil pointer fields triggers a recoverable panic instead of immediate program crash. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
6c36935 to
2363d28
Compare
Summary
This PR fixes Issue #1364 by implementing a SIGSEGV signal handler that converts segmentation faults from nil pointer dereferences into recoverable Go panics, matching standard Go runtime behavior.
Changes
Add SIGSEGV signal handler (
runtime/internal/runtime/z_rt_default.go)!wasm && !baremetalto exclude wasm and baremetal platformsAdd regression test (
_demo/go/recover-nil/main.go)Implementation Details
Why use hardcoded constant
SIGSEGV = 0xb?SIGSEGV has the same value (11 / 0xb) across all Unix-like systems (Linux, Darwin, BSD, Solaris, etc.). To avoid unnecessary dependencies, we define the constant directly instead of importing from syscall packages.
Dependency issues when importing syscall:
Using
import "syscall"causes linking failures in c-shared/c-archive build modes:Using
import "github.com/goplus/llgo/runtime/internal/clite/syscall"also fails:Root cause: Both syscall packages import the
errorspackage, which depends oninternal/reflectliteandsync. In c-shared/c-archive build modes, these internal packages are not fully linked by llgo's linker, causing undefined symbol errors.Solution: Use hardcoded constant
SIGSEGV = 0xbto completely avoid the dependency chain.Testing
Tested on all build modes:
llgo run- worksllgo build -buildmode=c-shared- worksllgo build -buildmode=c-archive- worksRelated
go/typesscope.Insert gotsegmentation fault#1357