runtime: enable `debugCheckBP` by default for language version 1.26

[prattmic]

For performance reasons, the Go runtime increasingly uses framepointer unwinding. The execution tracer uses framepointer unwinding since Go 1.21. Block and mutex profiles use framepointer unwinding since 1.23.

Unfortunately, framepointer unwinding is easily corrupted by custom assembly that clobbers the framepointer. For example, #69629 and #74851 were due to assembly in a dependency clobbering the framepointer, leading to crashes when unwinding.

go vet -framepointer can catch some cases of framepointer clobbering, but it can't cover every case with static analysis, and more importantly, it is often not run on dependencies.

Discover of these problems is also often delayed because framepointer unwinding is used primarily in optional diagnostics, so users may not see crashes until they try to collect one of the diagnostics.

The runtime has global constant debugCheckBP (default false). When enabled, stack copying verifies that framepointers are reasonable, and crashes if not. This discovers problems more proactively.

I propose we convert this check from a constant to a GODEBUG option (GODEBUG=checkfp=1), and enable this option by default with language version 1.26.

Enabling by default will help discover bad code more proactively. On the other hand, it will trigger crashes in code that works fine provided none of the framepointer diagnostics are used. Limiting the behavior change to an explicit upgrade to 1.26 in go.mod is intended to strike a balance here.

If we do this, we should also enhance the check to include more context and instructions to help users identify the source of the issue. We could also consider running this check more often than just during stack copy. Every traceback could theoretically do it?

cc @golang/runtime @mknyszek @nsrip-dd @felixge

[prattmic]

I think we could make pinpointing these bugs much easier by having ABI0 wrappers verify that BP is unmodified across the call. We probably don't want that on by default, though. If it is on by default, we might as well finish the job by saving and restoring BP in the caller.

[mknyszek]

I think it might be OK to slow down ABI0 as the reasons to to use it dwindle (SIMD work), and if/when we stamp out an ABI1. Though clearly that may be a long time from now.

[nsrip-dd]

If we do this, we should also enhance the check to include more context and instructions to help users identify the source of the issue. We could also consider running this check more often than just during stack copy. Every traceback could theoretically do it?

In #75478 (comment), @mknyszek made this suggestion:

As an aside, we should probably have a crash in fpTracebackPCs print an additional message to run the vet check to create a better user experience here. Ideally we would be able to recover from such a crash and only then walk the stack the slow way, and that may be worth considering as well.

Perhaps this should be where we include more context? Like, if frame pointer unwinding fails, we can track the last good frame and on recovery we can go through regular unwinding and report more information about the frame where things failed?

[gabyhelp]

Related Issues

• cmd/vet: improve the frame pointer check #69838
• proposal: cmd/compiler,cmd/asm: add `-d=checkframepointer` option #74752
• runtime: frame pointer unwinding can fail on system goroutines #63630

Related Code Changes

• runtime: save frame pointer to the stack in signal handlers for arm64
• runtime: turn frame pointer unwinding for tracing off by default
• runtime: adjust frame pointer on stack copy on ARM64
• runtime: unwind BP in jmpdefer to match SP unwind
• runtime: eliminate uses of BP on amd64
• runtime: use frame pointer unwinding for the heap profiler

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[nsrip-dd]

Err... can we actually recover from frame pointer crashes? I just tried adding a defer func() { if recover() != nil { ... }}() to fpTracebackPCs and forcing a crash, and it looks like we fail with a throw rather than panic. I assume throws aren't recoverable? Can/should we make it a panic?

[mknyszek]

Err... can we actually recover from frame pointer crashes? I just tried adding a defer func() { if recover() != nil { ... }}() to fpTracebackPCs and forcing a crash, and it looks like we fail with a throw rather than panic. I assume throws aren't recoverable? Can/should we make it a panic?

Crash on SIGSEGV is by default not a panic. You can make it a panic with a runtime/debug API: https://pkg.go.dev/runtime/debug#SetPanicOnFault. That might work?

RE: recovering, I didn't mean recover necessarily. 😅 My bad. I meant like, adding a check in the signal handler on SIGSEGV. That does limit what we can check, of course, but I think we can at least take a stack trace in the signal handler and simultaneously walk the frame pointers and pinpoint the bad frame. (The SIGSEGV handler would also be where we'd output a more descriptive message, perhaps upon identifying the function of the PC of the raised SIGSEGV.)

[cherrymui]

If we are mostly worrying about assembly code clobber the frame pointer, maybe we can save and restore the frame pointer register at ABI boundaries, i.e. when a Go function calls into ABI0 assembly? Most user assembly functions are nonpreemptible leaf functions, so we won't unwind from the middle of it. We just need it to have the correct value after the call. This doesn't cover assembly function calling Go function, but that is probably extremely rare.

We can have a debug mode that checks it, instead of restoring it.

[prattmic]

Keith notes in triage that we could recompute the frame pointer for restore, so we technically don't need to save.