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

dev.boringcrypto: TestGoAMD64v1 broken at HEAD #49402

Closed
rolandshoemaker opened this issue Nov 5, 2021 · 5 comments
Closed

dev.boringcrypto: TestGoAMD64v1 broken at HEAD #49402

rolandshoemaker opened this issue Nov 5, 2021 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Unreleased

Comments

@rolandshoemaker
Copy link
Member

The TestGoAMD64v1 test in cmd/compile/internal/amd64 is currently broken in the dev.boringcrypto branch (as of https://golang.org/cl/361714):

--- FAIL: TestGoAMD64v1 (3.02s)
    versions_test.go:133: clobbering instruction   4998cf:	4d 0f 38 f0 6e 58    	movbe  0x58(%r14),%r13
    versions_test.go:133: clobbering instruction   4998da:	4d 0f 38 f0 66 50    	movbe  0x50(%r14),%r12
    versions_test.go:133: clobbering instruction   499979:	4d 0f 38 f0 6e 48    	movbe  0x48(%r14),%r13
    versions_test.go:133: clobbering instruction   49998e:	4d 0f 38 f0 66 40    	movbe  0x40(%r14),%r12
    versions_test.go:133: clobbering instruction   4999e6:	4d 0f 38 f0 6e 38    	movbe  0x38(%r14),%r13
    versions_test.go:133: clobbering instruction   499a01:	4d 0f 38 f0 66 30    	movbe  0x30(%r14),%r12
    versions_test.go:133: clobbering instruction   499a53:	4d 0f 38 f0 6e 28    	movbe  0x28(%r14),%r13
    versions_test.go:133: clobbering instruction   499a68:	4d 0f 38 f0 66 20    	movbe  0x20(%r14),%r12
    versions_test.go:133: clobbering instruction   499abc:	4d 0f 38 f0 6e 18    	movbe  0x18(%r14),%r13
    versions_test.go:133: clobbering instruction   499ac7:	4d 0f 38 f0 66 10    	movbe  0x10(%r14),%r12
    versions_test.go:133: clobbering instruction   499b1c:	4d 0f 38 f0 6e 08    	movbe  0x8(%r14),%r13
    versions_test.go:133: clobbering instruction   499b27:	4d 0f 38 f0 26       	movbe  (%r14),%r12
    versions_test.go:133: clobbering instruction   49b339:	0f 38 f1 44 24 0c    	movbe  %eax,0xc(%rsp)
    versions_test.go:133: clobbering instruction   49b34a:	0f 38 f1 44 24 1c    	movbe  %eax,0x1c(%rsp)
    versions_test.go:133: clobbering instruction   49b360:	0f 38 f1 44 24 2c    	movbe  %eax,0x2c(%rsp)
    versions_test.go:133: clobbering instruction   49b37c:	0f 38 f1 44 24 3c    	movbe  %eax,0x3c(%rsp)
    versions_test.go:133: clobbering instruction   49b38d:	0f 38 f1 44 24 4c    	movbe  %eax,0x4c(%rsp)
    versions_test.go:133: clobbering instruction   49b3a3:	0f 38 f1 44 24 5c    	movbe  %eax,0x5c(%rsp)
    versions_test.go:133: clobbering instruction   4b4d00:	c4 62 98 f2 e0       	andn   %rax,%r12,%r12
    versions_test.go:133: clobbering instruction   4b4d09:	c4 62 90 f2 e8       	andn   %rax,%r13,%r13
    versions_test.go:133: clobbering instruction   4b4d0e:	c4 62 88 f2 f0       	andn   %rax,%r14,%r14
    versions_test.go:133: clobbering instruction   4b4d13:	c4 62 80 f2 f8       	andn   %rax,%r15,%r15
    versions_test.go:133: clobbering instruction   571c7f:	f3 0f b8 d2          	popcnt %edx,%edx
    versions_test.go:133: clobbering instruction   571cbb:	f3 0f b8 d2          	popcnt %edx,%edx
    versions_test.go:133: clobbering instruction   571cf4:	f3 0f b8 d2          	popcnt %edx,%edx
    versions_test.go:133: clobbering instruction   571d2c:	f3 0f b8 d2          	popcnt %edx,%edx
    versions_test.go:133: clobbering instruction   571d5c:	f3 0f b8 d2          	popcnt %edx,%edx
    versions_test.go:133: clobbering instruction   571da0:	f3 0f b8 d2          	popcnt %edx,%edx
    versions_test.go:133: clobbering instruction   593f8b:	f3 4d 0f b8 db       	popcnt %r11,%r11
    versions_test.go:133: clobbering instruction   59b2e5:	f3 48 0f b8 ff       	popcnt %rdi,%rdi
    versions_test.go:133: clobbering instruction   59bb24:	f3 4d 0f b8 c0       	popcnt %r8,%r8
    versions_test.go:133: clobbering instruction   59bb95:	f3 48 0f b8 c8       	popcnt %rax,%rcx
    versions_test.go:133: clobbering instruction   59bbff:	f3 4d 0f b8 c9       	popcnt %r9,%r9
    versions_test.go:133: clobbering instruction   59bc5f:	f3 48 0f b8 cb       	popcnt %rbx,%rcx
    versions_test.go:133: clobbering instruction   7c4452:	f3 41 0f b8 d0       	popcnt %r8d,%edx
    versions_test.go:133: clobbering instruction   853051:	f3 48 0f b8 d0       	popcnt %rax,%rdx
    versions_test.go:133: clobbering instruction   85980e:	f3 48 0f b8 d2       	popcnt %rdx,%rdx
    versions_test.go:133: clobbering instruction   8598c7:	f3 48 0f b8 d2       	popcnt %rdx,%rdx
    versions_test.go:133: clobbering instruction   85b1a8:	f3 4d 0f b8 fb       	popcnt %r11,%r15
    versions_test.go:133: clobbering instruction   85b406:	f3 4d 0f b8 fc       	popcnt %r12,%r15
    versions_test.go:133: clobbering instruction   85b805:	f3 4c 0f b8 d3       	popcnt %rbx,%r10
    versions_test.go:133: clobbering instruction   85c47c:	f3 48 0f b8 d2       	popcnt %rdx,%rdx
    versions_test.go:133: clobbering instruction   8644bb:	f3 4c 0f b8 d3       	popcnt %rbx,%r10
    versions_test.go:133: clobbering instruction   8ef2cb:	f3 4c 0f b8 c7       	popcnt %rdi,%r8
    versions_test.go:133: clobbering instruction   8ef3ea:	f3 44 0f b8 c7       	popcnt %edi,%r8d
    versions_test.go:133: clobbering instruction   8f05ef:	66 0f 3a 0b d0 00    	roundsd $0x0,%xmm0,%xmm2
    versions_test.go:133: clobbering instruction   8f086b:	c4 e2 f9 b9 d1       	vfmadd231sd %xmm1,%xmm0,%xmm2
    versions_test.go:75: couldn't execute test: exit status 2

Presumably this is because linking boringcrypto requires some weird opcodes? Not entirely sure.

cc @randall77, who added this test.
@rolandshoemaker rolandshoemaker added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 5, 2021
@cherrymui
Copy link
Member

I vaguely recall boringcrypto verifies itself before executing? If so, this test modifies the binary which makes it fail to verify.

@dmitshur dmitshur added this to the Unreleased milestone Nov 8, 2021
@randall77
Copy link
Contributor

Yes, that verification looks like the problem. With this patch:

--- a/src/cmd/compile/internal/amd64/versions_test.go
+++ b/src/cmd/compile/internal/amd64/versions_test.go
@@ -72,7 +72,7 @@ func TestGoAMD64v1(t *testing.T) {
        cmd.Env = append(cmd.Env, fmt.Sprintf("GODEBUG=%s", strings.Join(features, ",")))
        out, err := cmd.CombinedOutput()
        if err != nil {
-               t.Fatalf("couldn't execute test: %s", err)
+               t.Fatalf("couldn't execute test: %s\n%s", err, string(out))
        }
        if string(out) != "PASS\n" {
                t.Fatalf("test reported error: %s", string(out))

It prints

    versions_test.go:75: couldn't execute test: exit status 2
        FIPS integrity test failed.
        Expected: 18d35ae031f649825a4269d68d2e62583d060a31d359690f97b9c8bf8120cdf75b405f74be7018094da7eb5261f2f86d0f481cc3b5a9c7c432268d94bf91aad9
        Calculated: 8158676f5144474ae568e99cc8cc3d3e00f78054aa5640f79a7d76b367f2957a2c121a3969869c1155c058da33a5c1b622d025b7298ea5e5d9d30b46d3fd1b01
        SIGABRT: abort
        PC=0x7f9561e7ae71 m=0 sigcode=18446744073709551610
        signal arrived during cgo execution
        
        goroutine 1 [syscall, locked to thread]:
        runtime.cgocall(0x4010b0, 0xc00005d970)
        	/home/khr/sandbox/boringcrypto/src/runtime/cgocall.go:157 +0x5c fp=0xc00005d948 sp=0xc00005d910 pc=0x57397c
        crypto/internal/boring._Cfunc__goboringcrypto_BORINGSSL_bcm_power_on_self_test()
        	_cgo_gotypes.go:391 +0x45 fp=0xc00005d970 sp=0xc00005d948 pc=0x6adc25
        crypto/internal/boring.init.0()
        	/home/khr/sandbox/boringcrypto/src/crypto/internal/boring/boring.go:20 +0x19 fp=0xc00005d990 sp=0xc00005d970 pc=0x6ae379
        runtime.doInit(0xc7e680)
        	/home/khr/sandbox/boringcrypto/src/runtime/proc.go:6395 +0x126 fp=0xc00005dac0 sp=0xc00005d990 pc=0x5b5046
        runtime.doInit(0xc7cde0)
        	/home/khr/sandbox/boringcrypto/src/runtime/proc.go:6372 +0x71 fp=0xc00005dbf0 sp=0xc00005dac0 pc=0x5b4f91
        runtime.doInit(0xc80860)
        	/home/khr/sandbox/boringcrypto/src/runtime/proc.go:6372 +0x71 fp=0xc00005dd20 sp=0xc00005dbf0 pc=0x5b4f91
        runtime.doInit(0xc7eb80)
        	/home/khr/sandbox/boringcrypto/src/runtime/proc.go:6372 +0x71 fp=0xc00005de50 sp=0xc00005dd20 pc=0x5b4f91
        runtime.doInit(0xc7c480)
        	/home/khr/sandbox/boringcrypto/src/runtime/proc.go:6372 +0x71 fp=0xc00005df80 sp=0xc00005de50 pc=0x5b4f91
        runtime.main()
        	/home/khr/sandbox/boringcrypto/src/runtime/proc.go:238 +0x1e6 fp=0xc00005dfe0 sp=0xc00005df80 pc=0x5a7f66
        runtime.goexit()
        	/home/khr/sandbox/boringcrypto/src/runtime/asm_amd64.s:1579 +0x1 fp=0xc00005dfe8 sp=0xc00005dfe0 pc=0x5d7f81
        
        rax    0x0
        rbx    0x7f9561e3b740
        rcx    0x7f9561e7ae71
        rdx    0x0
        rdi    0x2
        rsi    0x7ffef91ef9e0
        rbp    0xc00005d938
        rsp    0x7ffef91ef9e0
        r8     0x0
        r9     0x7ffef91ef9e0
        r10    0x8
        r11    0x246
        r12    0xc00005d850
        r13    0x8
        r14    0x7ffef91efc88
        r15    0x7ffef91efc48
        rip    0x7f9561e7ae71
        rflags 0x246
        cs     0x33
        fs     0x0
        gs     0x0
FAIL
FAIL	cmd/compile/internal/amd64	1.817s
FAIL

Note that it succeeds on Darwin - the failure seems to be Linux-specific.

@cherrymui
Copy link
Member

Note that it succeeds on Darwin - the failure seems to be Linux-specific.

I think boringcrypto is only enabled on Linux. On darwin it doesn't use boringcrypto.

@gopherbot
Copy link

Change https://golang.org/cl/362654 mentions this issue: [dev.boringcrypto] cmd/compile: disable version test on boringcrypto

gopherbot pushed a commit that referenced this issue Nov 9, 2021
@randall77
[dev.boringcrypto] cmd/compile: disable version test on boringcrypto
16215e5 
This test modifies a binary, which the FIPS integrity test doesn't like.

Fixes #49402

Change-Id: I817d1f49e779ce1ea5f9477bf74e729e82b42875
Reviewed-on: https://go-review.googlesource.com/c/go/+/362654
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@dmitshur dmitshur added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 13, 2022
@dmitshur
Copy link
Contributor

Closing as fixed by CL 362654, where it was determined the test is incompatible with the dev.boringcrypto branch, and shouldn't be run.

@golang golang locked and limited conversation to collaborators Apr 13, 2023
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. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@dmitshur @rolandshoemaker @randall77 @gopherbot @cherrymui