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.

Sign up for GitHub

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

Jump to bottom

x/crypto/blake2b: amd64 SIMD assembly implementions not selected #24828

Closed
aead opened this issue Apr 12, 2018 · 14 comments
Closed

x/crypto/blake2b: amd64 SIMD assembly implementions not selected #24828

aead opened this issue Apr 12, 2018 · 14 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@aead
Copy link
Contributor

aead commented Apr 12, 2018

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

go 1.10.1

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/andreas/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/andreas/go"
GORACE=""
GOROOT="/home/andreas/.go/1.10.1"
GOTMPDIR=""
GOTOOLDIR="/home/andreas/.go/1.10.1/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build324948782=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. go get golang.org/x/crypto
  2. go test -v golang.org/x/crypto/blake2b

What did you expect to see?

=== RUN   TestHashes
--- PASS: TestHashes (0.00s)
	blake2b_test.go:30: AVX2 version
	blake2b_test.go:35: AVX version
	blake2b_test.go:40: SSE4 version
	blake2b_test.go:44: generic version
=== RUN   TestHashes2X
--- PASS: TestHashes2X (0.03s)
	blake2b_test.go:54: AVX2 version
	blake2b_test.go:59: AVX version
	blake2b_test.go:64: SSE4 version
	blake2b_test.go:68: generic version
=== RUN   TestSelfTest
--- PASS: TestSelfTest (0.00s)
PASS
ok  	golang.org/x/crypto/blake2b	0.039s

What did you see instead?

=== RUN   TestHashes
--- PASS: TestHashes (0.00s)
	blake2b_test.go:45: generic version
=== RUN   TestHashes2X
--- PASS: TestHashes2X (0.01s)
	blake2b_test.go:69: generic version
=== RUN   TestMarshal
--- PASS: TestMarshal (0.00s)
=== RUN   TestSelfTest
--- PASS: TestSelfTest (0.00s)
PASS
ok  	golang.org/x/crypto/blake2b	0.016s

CL 106336 switched from runtime.support_avx() (which was removed from the runtime) to a linker-mapping scheme. However it seems like the linker does not set the variables correctly. Furthermore the SSE4 var seems to use the wrong link-name:
s / cpu.X86.HasSSE4 / cpu.X86.HasSSE41 See https://github.com/golang/go/blob/master/src/internal/cpu/cpu.go#L31

Even fixing this seems not to change anything. Further the the AVX and AVX2 code is not selected anyway.
/cc @ianlancetaylor
@gopherbot gopherbot added this to the Unreleased milestone Apr 12, 2018
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 12, 2018
@ianlancetaylor
Copy link
Contributor

CC @tklauser

My apologies, I didn't really think this CL through. We do need to keep x/crypto working well with older releases. I think we're going to need to use build tags here.

@aead
Copy link
Contributor Author

aead commented Apr 12, 2018 •
edited

chacha20poly1305 and the Argon2-AVX2 CL use custom CPU feature detection. Since x/crypto should work with older releases too we have to detect CPU features separately from the standard library (at the movement).

Maybe group CPU feature detection as an internal package to avoid repeating code in chacha20poly1305, blake2{b,s}, argon2 ?!

@tklauser
Copy link
Member

Sorry, I was being too careless with https://golang.org/cl/106336 (and the original https://golang.org/cl/106235, I wasn't considering that anything outside of Go itself would depend on the support_ vars until I saw the x/crypto/blake2b fails on https://build.golang.org). I thought that changing it for the go1.7 version would be enough. But of course it isn't, because https://golang.org/cl/104636 would need to be in the release in order for the cpu.X86.Has* vars to be set early enough.

So I think CPU feature detection as an internal package in x/crypto (similar to internal/cpu) is the best way to go here. This way we don't have to depend on runtime internals (which might change again in the future).

@aead aead mentioned this issue Apr 13, 2018
Closed
@agnivade
Copy link
Contributor

While we wait for x/sys/cpu to get implemented, does it make sense to revert the offending CL which caused this issue ?

This is right now causing degraded performance for all code which is directly building binaries by go geting. I have some code in production which is getting affected by this.

@gopherbot
Copy link

Change https://golang.org/cl/108795 mentions this issue: blake2b: Revert "blake2b: use internal/cpu to determine AVX and SSE 4 support"

gopherbot pushed a commit to golang/crypto that referenced this issue Apr 25, 2018
@tklauser
blake2b: Revert "blake2b: use internal/cpu to determine AVX and SSE 4…
b0697ec 
… support"

This reverts commit d644981 because of golang/go#24828.

Change-Id: I14b4e1265b2e75897fa12548dbdfb77c308afaaf
Reviewed-on: https://go-review.googlesource.com/108795
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
@agnivade agnivade mentioned this issue Apr 26, 2018
Closed
@bcmills
Copy link
Contributor

bcmills commented Apr 26, 2018

The build still seems to be broken after https://golang.org/cl/108795; see https://build.golang.org/?repo=golang.org%2fx%2fcrypto.

Is there something more we need to revert?

@aead
Copy link
Contributor Author

aead commented Apr 26, 2018

@bcmills This is because Go1.11tip removed runtime.support_AVX2 which blake2b relied on.
This will be addressed by #24843

@bcmills
Copy link
Contributor

bcmills commented Apr 26, 2018

This will be addressed by #24843

What kind of timeframe are we looking at for that? We shouldn't leave x/crypto in a broken state.

Should we roll back something from the go repo in the meantime?

@tklauser
Copy link
Member

We could revert https://golang.org/cl/106595 in the meantime and re-submit once #24843 is fixed and x/crypto has switched to x/sys/cpu.

@tklauser
Copy link
Member

...and the same for https://golang.org/cl/106235

@tmthrgd
Copy link
Contributor

tmthrgd commented Apr 27, 2018

Why not just add the following (rough sketch) to the runtime as a temporary shim?

// This is a temporary shim for x/crypto until issue 24843 is resolved.
var (
  supports_avx = cpu.X86.HasAVX
  supports_avx2 = cpu.X86.HasAVX2
)

It can then be yanked out in go1.11 or go1.12 whenever #24843 is solved.

@gopherbot
Copy link

Change https://golang.org/cl/110016 mentions this issue: blake2b: stub out values removed from runtime

@aead
Copy link
Contributor Author

aead commented Apr 30, 2018

I guess this is not needed anymore - since #24843 is fixed and CLs are merged.

@FiloSottile
Copy link
Contributor

Fixed by golang/crypto@ae8bce0.

@golang golang locked and limited conversation to collaborators Apr 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
8 participants
@tmthrgd @tklauser @FiloSottile @agnivade @ianlancetaylor @bcmills @gopherbot @aead