runtime: frame pointer unwinding can fail on system goroutines

nsrip-dd commented 1 year ago

What version of Go are you using (`go version`)?

$ go version
go version devel go1.22-30d02e4925

Does this issue reproduce with the latest release?

Not exactly since we don't use frame pointer unwinding in this situation in any release. However I believe the underlying issue is there in the latest release

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/ec2-user/.cache/go-build'
GOENV='/home/ec2-user/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/ec2-user/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/ec2-user/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/ec2-user/repo/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/ec2-user/repo/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.22-30d02e4925 Fri Oct 6 13:02:40 2023 -0400'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/ec2-user/repo/go/src/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1576787342=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Attempted to use frame pointer unwinding to collect call stacks for the memory profiler. The motivation was in part to bring the efficiency gains we got for execution tracing to other places, but also to make frame pointers more widely used and thus more well-tested and reliable for the situations where we really want it to work. I observed this with patchset 1 of this CL and could reproduce it locally with env GODEBUG=memprofilerate=1 go test ./runtime.

What did you expect to see?

Frame pointer unwinding to not fail.

What did you see instead?

Frame pointer unwinding fails (crashing) when collecting memory profile samples:

SIGSEGV: segmentation violation
PC=0x4346d2 m=3 sigcode=2 addr=0x7fe60a5be288

goroutine 0 [idle]:
runtime.fpTracebackPCs(...)
        /home/ec2-user/repo/go/src/runtime/trace.go:1018
runtime.mProf_Malloc(0xc000080000, 0x580)
        /home/ec2-user/repo/go/src/runtime/mprof.go:432 +0x212 fp=0x7fe60afbdc20 sp=0x7fe60afbda98 pc=0x4346d2
runtime.profilealloc(0xc000080000?, 0x580?, 0x578?)
        /home/ec2-user/repo/go/src/runtime/malloc.go:1358 +0x79 fp=0x7fe60afbdc58 sp=0x7fe60afbdc20 pc=0x411bd9
runtime.mallocgc(0x578, 0x8610c0, 0x1)
        /home/ec2-user/repo/go/src/runtime/malloc.go:1206 +0x685 fp=0x7fe60afbdcc0 sp=0x7fe60afbdc58 pc=0x411785
runtime.newobject(0x0?)
        /home/ec2-user/repo/go/src/runtime/malloc.go:1322 +0x25 fp=0x7fe60afbdce8 sp=0x7fe60afbdcc0 pc=0x411aa5
runtime.allocm(0xc000036a00, 0x891888, 0x0?)
        /home/ec2-user/repo/go/src/runtime/proc.go:1934 +0x91 fp=0x7fe60afbdd40 sp=0x7fe60afbdce8 pc=0x443f71
runtime.newm(0x161604c4b3d4da?, 0xc000036a00, 0x0?)
        /home/ec2-user/repo/go/src/runtime/proc.go:2384 +0x35 fp=0x7fe60afbdd70 sp=0x7fe60afbdd40 pc=0x444895
runtime.startm(0xc000036a00?, 0x1, 0x0)
        /home/ec2-user/repo/go/src/runtime/proc.go:2610 +0x158 fp=0x7fe60afbddc0 sp=0x7fe60afbdd70 pc=0x444f78
runtime.wakep()
        /home/ec2-user/repo/go/src/runtime/proc.go:2746 +0xec fp=0x7fe60afbddf0 sp=0x7fe60afbddc0 pc=0x44550c
runtime.resetspinning()
        /home/ec2-user/repo/go/src/runtime/proc.go:3473 +0x3e fp=0x7fe60afbde10 sp=0x7fe60afbddf0 pc=0x4471fe
runtime.schedule()
        /home/ec2-user/repo/go/src/runtime/proc.go:3607 +0x10f fp=0x7fe60afbde48 sp=0x7fe60afbde10 pc=0x44762f
runtime.mstart1()
        /home/ec2-user/repo/go/src/runtime/proc.go:1608 +0xcd fp=0x7fe60afbde70 sp=0x7fe60afbde48 pc=0x4437ed
runtime.mstart0()
        /home/ec2-user/repo/go/src/runtime/proc.go:1558 +0x76 fp=0x7fe60afbdea0 sp=0x7fe60afbde70 pc=0x4436f6
runtime.mstart()
        /home/ec2-user/repo/go/src/runtime/asm_amd64.s:395 +0x5 fp=0x7fe60afbdea8 sp=0x7fe60afbdea0 pc=0x479b65

The unwinding fails after visiting the mstart frame. This is the entry point of the M. This function has a TOPFRAME annotation, which helps unwinders using DWARF know not to proceed, but doesn't help for frame pointer unwinding. Should we could modify the mstart implementations to explicitly set the frame pointer register to 0 on architectures with frame pointers? That resolves this failure, at least. We could also teach the assembler to do that when it sees TOPFRAME annotations.

nsrip-dd commented 1 year ago

We could also teach the assembler to do that when it sees TOPFRAME annotations.

I looked into this a bit. It wouldn't be straightforward because, for example, the sigtramp implementations are marked TOPFRAME, but it's important for those functions to save registers following the platform ABI. So telling the assembler/linker to insert MOV $0, FP or something similar for TOPFRAME functions wouldn't be right.

Other things I can think of:

Just manually fix up mstart implementations to clear the frame pointer on platforms where we want to do unwinding? Assuming it's worth trying to introduce frame-pointer unwinding for the heap profiler, for example, this is the situation where it wouldn't work today. Could do the same for other TOPFRAME functions
Build a table of address ranges for TOPFRAME functions, have frame-pointer unwinders check for them to know when to stop. This might be more robust but would also hurt performance.
Don't try frame-pointer unwinding in this situation

cherrymui commented 1 year ago

There should be only a small number of TOPFRAME functions. Fixing them up manually sounds reasonable. If there are ones that are tricky or not possible to fix (like sigtramp mentioned above), we can think about what to do with them.

Build a table of address ranges for TOPFRAME functions, have frame-pointer unwinders check for them to know when to stop. This might be more robust but would also hurt performance.

I assume we need to keep a table of stack addresses (not PCs)? Maybe doable but it is probably hard to maintain that table, and as you mentioned, also hurt performance. Maybe we can check for whether the new FP falls out of the stack boundary and stop if so? But we also want to handle stack transitions in FP unwinding? Then maybe we'd need to check both g0 and curg's stack bounds.

nsrip-dd commented 1 year ago

Maybe we can check for whether the new FP falls out of the stack boundary and stop if so? But we also want to handle stack transitions in FP unwinding? Then maybe we'd need to check both g0 and curg's stack bounds.

Yeah, that might be more trouble than it's worth. IMO, if there's only Go code in the call chain, frame pointer unwinding should ideally work with no extra guardrails. Since we fully control the compiler and runtime, we can properly fix cases where it doesn't work.

There should be only a small number of TOPFRAME functions. Fixing them up manually sounds reasonable. If there are ones that are tricky or not possible to fix (like sigtramp mentioned above), we can think about what to do with them.

Agreed, seems reasonable to me 👍

gopherbot commented 1 year ago

Change https://go.dev/cl/540476 mentions this issue: runtime: use frame pointer unwinding for the heap profiler

nsrip-dd commented 1 year ago

But we also want to handle stack transitions in FP unwinding?

I have some experience now trying to do this for heap profiling. In the case of mcall I don't think this will be possible all of the time. Specifically, when a goroutine uses mcall to call back into the scheduler (e.g. for blocking), the M will eventually drop the goroutine and wait for more runnable work. Once that happens, the goroutine which initiated the mcall can later be rescheduled on another M. That will invalidate the frame pointer saved by mcall.

I saw this problem trying to do FP unwinding for heap profiling. For example, we might allocate a new M during resetspinning, and FP unwinding from that point to sample that allocation can reach an mcall frame, with a frame pointer that's now invalid. This results in a crash.

I think we should probably have mcall also clear the frame pointer to indicate that unwinding shouldn't cross that stack boundary, since in general it's not safe. Does that make sense?

cherrymui commented 1 year ago

I agree in general it may not always be safe or sensible to unwind through stack transition. I think it may be reasonable to unwind through a user-to-system stack transition (like calling systemstack or asmcgocall) but not the other way around (like the scheduler executing some user goroutine).

I don't know the specifics about resetspinning. Is this mcall special? Could it have a valid frame pointer?

Is there a case we do want to traceback through an mcall? If not, we probably clear the frame pointer in mcall.

nsrip-dd commented 1 year ago

I don't know the specifics about resetspinning. Is this mcall special? Could it have a valid frame pointer?

Sorry, I left out some detail. resetspinning is something called during schedule (here) that can lead to a memory allocation, and recording that allocation for the heap profile using FP unwinding can crash. Mainly I meant it as an example where we might want to record a call stack during an mcall-ed function, where FP unwinding might not be safe any more.

The general pattern is that a goroutine calls mcall(somefunc), which will run somefunc on the g0 stack. somefunc arranges somehow for the goroutine to run later, then calls into the scheduler. gosched_m, for example, is called through mcall when a goroutine voluntarily yields to the scheduler, and it arranges for the goroutine to run by putting it on the global run queue. Once somefunc gives up the goroutine, the goroutine can be rescheduled later and invalidate the frame pointer saved by mcall.

Is there a case we do want to traceback through an mcall? If not, we probably clear the frame pointer in mcall.

No, I don't think so. We have some execution tracing events recorded during mcall-ed functions. In the gosched_m example we record the call stack of the goroutine at the point where it yielded for the traceGoSched event. But, we don't need to go through mcall, since we'll also have the goroutine's frame pointer saved in the gp.sched.bp field and we can start unwinding from there. In any such case, the event is always recorded before making the goroutine runnable again, so we know the unwinding will still work.

golang / go