crypto/sha256: amd64 block assembly smashes frame pointer register

prattmic commented 10 months ago

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

$ go version
go version devel go1.22-f09750e8ef Wed Oct 11 13:53:10 2023 -0400 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

linux-amd64

go env Output

$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/usr/local/google/home/mpratt/.cache/go-build'
GOENV='/usr/local/google/home/mpratt/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/usr/local/google/home/mpratt/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/usr/local/google/home/mpratt/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/google/home/mpratt/src/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/google/home/mpratt/src/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.22-f09750e8ef Wed Oct 11 13:53:10 2023 -0400'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/usr/local/google/home/mpratt/src/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 -ffile-prefix-map=/tmp/go-build1220716334=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Profile the crypto/sha256 benchmarks, using both the built-in pprof profiling and using Linux perf:

$ go test -c crypto/sha256
$ ./sha256.test -test.run=^$ -test.bench=. -test.cpuprofile=/tmp/cpu.pprof
$ perf record -g ./sha256.test -test.run=^$ -test.bench=.

Then open the profiles with pprof and compare the flame graphs:

$ pprof -http=localhost:8080 /tmp/cpu.pprof
$ pprof -http=localhost:8080 perf.data

What did you expect to see?

Approximately the same profile.

What did you see instead?

The pprof profile looks normal, as expected:

The perf profile has broken call stacks, displaying sha256.block by itself, rather than as a callee of Write.

pprof profiling uses the runtime's internal knowledge of frame sizes to find the caller, so it works fine. The Linux kernel doesn't know about Go's metadata and instead typically uses frame pointers (BP) to find callees.

For some reason, the amd64 implementation of block overwrites BP, which I presume is what is confusing Linux.

I admittedly don't understand everything going on in that function (wow, it is complicated!), but writing to BP doesn't seem to be necessary? The function doesn't seem to actually adjust the stack or create new frames? It looks like BP might just be getting used as a scratch register?

It looks like this has existed since the original https://go.dev/cl/28460043.

cc @FiloSottile @rolandshoemaker @felixge @4a6f656c

prattmic commented 10 months ago

https://cs.opensource.google/go/go/+/master:src/crypto/sha256/sha256block_amd64.s;l=649 may be an attempt to restore BP after scratch use? If so, it is not correct, as the value of BP should be the value of SP before it was decremented on function entry.

ianlancetaylor commented 10 months ago

That line is not attempting to restore BP. It's initializing BP for its use in SHA256ROUND0 and SHA256ROUND1. See MSGSCHEDULE0 and MSGSCHEDULE1.

randall77 commented 10 months ago

There is a vet check for not properly saving BP. See cmd/vendor/golang.org/x/tools/go/analysis/passes/framepointer/framepointer.go. I'm curious if that check isn't firing, why it isn't. I think it is giving up due to the early branch instructions. Maybe it could be improved to catch this kind of error.

felixge commented 10 months ago

@prattmic could this impact execution tracing? Looking at the generated asm, it seems like the BP is saved/restored after the function returns, so the problem would only occur if the runtime tries to fp unwind while the block function is executing. Could this happen due to async preemption?

nsrip-dd commented 10 months ago

Looks like async preemption won't happen during assembly code: https://cs.opensource.google/go/go/+/refs/tags/go1.21.3:src/runtime/preempt.go;l=407-413. This assembly doesn't do anything else that would trigger an execution trace event while BP is clobbered. So I think it's okay as far as execution tracing is concerned.

golang / go