Go: x/crypto/blake2b: amd64 SIMD assembly implementions not selected

[aead]

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

[ianlancetaylor]

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]

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]

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).

[agnivade]

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]

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

[bcmills]

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]

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

[bcmills]

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]

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]

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

[tmthrgd]

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]

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

[aead]

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

[FiloSottile]

Fixed by golang/crypto@ae8bce0.