Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/sha256: amd64 block assembly smashes frame pointer register #63508

Open
prattmic opened this issue Oct 11, 2023 · 5 comments
Open

crypto/sha256: amd64 block assembly smashes frame pointer register #63508

prattmic opened this issue Oct 11, 2023 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Linux Performance
Milestone

Comments

@prattmic
Copy link
Member

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:

image

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

image


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
Copy link
Member Author

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
Copy link
Contributor

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

@randall77
Copy link
Contributor

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.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 12, 2023
@dmitshur dmitshur added this to the Backlog milestone Oct 12, 2023
@felixge
Copy link
Contributor

felixge commented Oct 12, 2023

@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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Linux Performance
Projects
None yet
Development

No branches or pull requests

6 participants