Add param to use alternate stack and restore old signal handler #276

VGalaxies · 2025-07-25T03:41:35Z

This PR improves compatibility and safety when using pprof-rs within a Go program via CGo.

Motivation

When non-Go code that installs signal handlers is linked with a Go program, it must follow specific rules to avoid crashing the application. This is because Go uses small, dynamically-sized stacks for its goroutines. As outlined in the official Go documentation on signals:

If the non-Go code installs any signal handlers, it must use the SA_ONSTACK flag with sigaction. Failing to do so is likely to cause the program to crash if the signal is received.

Also, the previous implementation set SIGPROF handler to SIG_IGN (ignore) upon shutdown. This would incorrectly remove Go's pprof handler, silently breaking Go's profiling capabilities.

This PR fixes these issues by adding the required flag and properly restoring the original signal handler.

Changes

The SA_ONSTACK flag is now included when registering the SIGPROF handler controlled by on_stack param.
The original sigaction is saved when the profiler starts.
This saved sigaction is restored when the profiler stops, ensuring the original handler (e.g., Go's) is correctly reinstated.

Impact

Performance: The performance impact is negligible. The SA_ONSTACK flag has zero cost during normal execution and an insignificant cost during save and restore the original sigaction.
Memory: There is no additional memory overhead from this change. The alternate signal stack is already allocated by the Go runtime; In non-Go environments, the on_stack option is disabled by default, so the SA_ONSTACK flag is not used and no extra memory is allocated.

ti-chi-bot · 2025-07-25T03:41:44Z

Welcome @VGalaxies! It looks like this is your first PR to tikv/pprof-rs 🎉

YangKeao · 2025-07-25T04:05:18Z

src/profiler.rs

+            // SA_ONSTACK will deliver the signal on an alternate stack. This is crucial
+            // to prevent a stack overflow if the signal arrives at a thread with
+            // a small stack, which is common when use pprof-rs in Go runtimes.
+            flags |= signal::SaFlags::SA_ONSTACK;


IIUC, adding SA_ONSTACK will allocate another stack for the signal handler, which will make the result of pprof-rs meaningless because pprof-rs need to unwind the stack.

Maybe the better way to solve this is just to not use pprof-rs with go runtime?

@VGalaxies please check and provide screenshot with this change

Using an alternate stack does not prevent stack unwinding. When a signal handler is invoked with the SA_SIGINFO flag, the kernel provides a ucontext_t pointer as the third argument (see https://man7.org/linux/man-pages/man2/sigaction.2.html). This ucontext_t contains a complete snapshot of the interrupted thread's CPU registers (including the stack pointer RSP and frame pointer RBP), which point to the original application stack, not the alternate signal stack.

In perf_signal_handler function, we pass this ucontext directly to TraceImpl::trace. The unwinding logic then uses this context to correctly walk the original stack from the point of interruption. Therefore, I think running the handler on an alternate stack does not affect the profiling results.

Below are the results of cpu profile using the modified pprof-rs:

The red box section is the tikv stacks, and outside the red box there are some tidb stacks.

Oh TiKV CPU profile now also contains golang's stack. Maybe we can disable golang's CPU profile HTTP API for our build.

In perf_signal_handler function, we pass this ucontext directly to TraceImpl::trace. The unwinding logic then uses this context to correctly walk the original stack from the point of interruption. Therefore, I think running the handler on an alternate stack does not affect the profiling results.

Thanks for the explanation 👍. pprof-rs doesn't always use the ucontext (and the register inside) to unwind. By default, it'll use backtrace_rs as the implementation, which will ignore the ucontext and just use the current stack. Therefore, the sa_stack only works for the frame_pointer implementation.

I'd suggest to only include sa_stack function when feature frame-pointer is enabled, and add some comments to this method.

BTW, does every platforms (mac, linux, bsd) include this flag? If not, we'd better also check the target_os.

#[cfg(all(target_os = "linux", feature = "frame-pointer"))]

By default, it'll use backtrace_rs as the implementation, which will ignore the ucontext and just use the current stack. Therefore, the sa_stack only works for the frame_pointer implementation.

Thanks for the great feedback! You're right, this approach is only effective with the frame-pointer unwinder. The above testing was done within the TiKV release build, where the frame pointer is enabled by default in the Makefile.

BTW, does every platforms (mac, linux, bsd) include this flag? If not, we'd better also check the target_os.

The SA_ONSTACK flag is a standard feature defined by the POSIX.1-2001 standard, so we can expect SA_ONSTACK to be available on all major POSIX-compliant operating systems. It is documented in the man pages for Linux, FreeBSD (which is similar to macOS), OpenBSD, and more...

lance6716 · 2025-07-25T05:55:15Z

src/profiler.rs

+            flags |= signal::SaFlags::SA_ONSTACK;
+        }
+        let sigaction = signal::SigAction::new(handler, flags, signal::SigSet::empty());
+        let old_action = unsafe { signal::sigaction(signal::SIGPROF, &sigaction) }?;


I see in the golang's doc

If the non-Go code installs a signal handler for any of the synchronous signals (SIGBUS, SIGFPE, SIGSEGV), then it should record the existing Go signal handler

and there are also specifications I didn't know before, like "signal mask for threads". Did you also checked that this library and our program can satisfy that?

I've reviewed the entire pprof-rs library and found that it only calls signal::sigaction to handle the SIGPROF signal, which is an asynchronous signal. It does not install any handlers for synchronous signals.

Additionally, pprof-rs has no function calls that modify the thread's signal mask. The only related part is signal::SigSet::empty() in the sigaction call. The sa_mask field temporarily blocks specified signals while the perf_signal_handler is running, and by passing an empty set, it means that no additional signals are requested to be blocked.

Signed-off-by: VGalaxies <[email protected]>

YangKeao

LGTM

YangKeao · 2025-07-29T06:01:18Z

Please fix the clippy:

Then we can merge this PR 🍻 .

Signed-off-by: VGalaxies <[email protected]>

VGalaxies · 2025-07-29T07:01:26Z

Please fix the clippy:
Then we can merge this PR 🍻 .

You're spot on. The unused_mut warning appears because the mut is only required when the frame-pointer feature is enabled, and that feature isn't active in the CI's clippy job.

I wanted to avoid using #[allow(unused_mut)], so I've refactored the code to use variable shadowing instead. This resolves the lint in a way that feels a bit cleaner 😂

ti-chi-bot bot added the dco-signoff: no label Jul 25, 2025

YangKeao self-requested a review July 25, 2025 03:57

YangKeao reviewed Jul 25, 2025

View reviewed changes

lance6716 reviewed Jul 25, 2025

View reviewed changes

VGalaxies added 4 commits July 25, 2025 15:58

try keep old_sigaction

caa661e

Signed-off-by: VGalaxies <[email protected]>

fixup

d1dff8f

Signed-off-by: VGalaxies <[email protected]>

add comments

cae2ae0

Signed-off-by: VGalaxies <[email protected]>

add on_stack param

b78a084

Signed-off-by: VGalaxies <[email protected]>

VGalaxies force-pushed the master branch from 8b8f5a9 to b78a084 Compare July 25, 2025 07:59

ti-chi-bot bot added dco-signoff: yes and removed dco-signoff: no labels Jul 25, 2025

VGalaxies requested a review from YangKeao July 25, 2025 09:16

enable on_stack under frame-pointer feature

b9b1531

Signed-off-by: VGalaxies <[email protected]>

YangKeao approved these changes Jul 29, 2025

View reviewed changes

ti-chi-bot bot added dco-signoff: no and removed dco-signoff: yes labels Jul 29, 2025

fix variable does not need to be mutable by shadowing

7692517

Signed-off-by: VGalaxies <[email protected]>

VGalaxies force-pushed the master branch from 856e150 to 7692517 Compare July 29, 2025 06:57

ti-chi-bot bot added dco-signoff: yes and removed dco-signoff: no labels Jul 29, 2025

VGalaxies requested a review from YangKeao July 29, 2025 07:01

YangKeao merged commit 0c52c5f into tikv:master Jul 29, 2025
24 checks passed

Add param to use alternate stack and restore old signal handler #276

Add param to use alternate stack and restore old signal handler #276

Uh oh!

Conversation

VGalaxies commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Impact

Uh oh!

ti-chi-bot bot commented Jul 25, 2025

Uh oh!

YangKeao Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance6716 Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

VGalaxies Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance6716 Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

YangKeao Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VGalaxies Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance6716 Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VGalaxies Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

YangKeao left a comment

Choose a reason for hiding this comment

Uh oh!

YangKeao commented Jul 29, 2025

Uh oh!

VGalaxies commented Jul 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

VGalaxies commented Jul 25, 2025 •

edited

Loading

YangKeao Jul 25, 2025 •

edited

Loading

VGalaxies Jul 25, 2025 •

edited

Loading

YangKeao Jul 25, 2025 •

edited

Loading

VGalaxies Jul 28, 2025 •

edited

Loading

lance6716 Jul 25, 2025 •

edited

Loading