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

internal/runtime/atomic: TestAnd64 panics with "unaligned 64-bit atomic operation" error #67077

Closed
alexbrainman opened this issue Apr 27, 2024 · 10 comments
Closed

internal/runtime/atomic: TestAnd64 panics with "unaligned 64-bit atomic operation" error #67077

alexbrainman opened this issue Apr 27, 2024 · 10 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@alexbrainman
Copy link
Member

Go version

go version 1c47049 arm

Output of go env in your module/workspace:

Not important.

What did you do?

I looked at https://build.golang.org/ and I noticed that all 32 bit arm systems fail after this CL

https://go-review.googlesource.com/c/go/+/557315

What did you see happen?

I see this test failure:

--- FAIL: TestAnd64 (0.00s)
panic: unaligned 64-bit atomic operation [recovered]
	panic: unaligned 64-bit atomic operation

goroutine 65 gp=0x484368 m=3 mp=0x480008 [running]:
panic({0x190920, 0x1e1ac0})
	/tmp/gobuilder-mips64/go/src/runtime/panic.go:778 +0x1c0 fp=0x4ced10 sp=0x4cecbc pc=0x5dfc0
testing.tRunner.func1.2({0x190920, 0x1e1ac0})
	/tmp/gobuilder-mips64/go/src/testing/testing.go:1631 +0x310 fp=0x4ced84 sp=0x4ced10 pc=0x123b64
testing.tRunner.func1()
	/tmp/gobuilder-mips64/go/src/testing/testing.go:1634 +0x41c fp=0x4cee70 sp=0x4ced84 pc=0x123330
panic({0x190920, 0x1e1ac0})
	/tmp/gobuilder-mips64/go/src/runtime/panic.go:759 +0x168 fp=0x4ceec4 sp=0x4cee70 pc=0x5df68
internal/runtime/atomic.panicUnaligned(...)
	/tmp/gobuilder-mips64/go/src/internal/runtime/atomic/unaligned.go:8
internal/runtime/atomic.lockAndCheck(...)
	/tmp/gobuilder-mips64/go/src/internal/runtime/atomic/atomic_mipsx.go:38
internal/runtime/atomic.Cas64(0x4306ec, 0xffffffffffffffff, 0xfffffffffffffffe)
	/tmp/gobuilder-mips64/go/src/internal/runtime/atomic/atomic_mipsx.go:75 +0xf4 fp=0x4ceed0 sp=0x4ceec4 pc=0xac5c0
internal/runtime/atomic.And64(...)
	/tmp/gobuilder-mips64/go/src/internal/runtime/atomic/atomic_andor_generic.go:33
internal/runtime/atomic_test.TestAnd64(0x50c008)
	/tmp/gobuilder-mips64/go/src/internal/runtime/atomic/atomic_andor_test.go:59 +0x5bc fp=0x4cef80 sp=0x4ceed0 pc=0x1708f4
testing.tRunner(0x50c008, 0x1ba528)
	/tmp/gobuilder-mips64/go/src/testing/testing.go:1689 +0x1c0 fp=0x4cefe0 sp=0x4cef80 pc=0x122e38
testing.(*T).Run.gowrap1()
	/tmp/gobuilder-mips64/go/src/testing/testing.go:1742 +0x6c fp=0x4cefec sp=0x4cefe0 pc=0x1242c0
runtime.goexit({})
	/tmp/gobuilder-mips64/go/src/runtime/asm_mipsx.s:641 +0x4 fp=0x4cefec sp=0x4cefec pc=0xa94e4
created by testing.(*T).Run in goroutine 1
	/tmp/gobuilder-mips64/go/src/testing/testing.go:1742 +0x4b4

goroutine 1 gp=0x400128 m=nil [chan receive]:
runtime.gopark(0x1ba630, 0x50a074, 0xe, 0x7, 0x2)
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:401 +0x128 fp=0x4d2c84 sp=0x4d2c78 pc=0x62fbc
runtime.chanrecv(0x50a040, 0x4d2cfb, 0x1)
	/tmp/gobuilder-mips64/go/src/runtime/chan.go:629 +0x5b0 fp=0x4d2cc0 sp=0x4d2c84 pc=0x16cc4
runtime.chanrecv1(0x50a040, 0x4d2cfb)
	/tmp/gobuilder-mips64/go/src/runtime/chan.go:479 +0x2c fp=0x4d2cd4 sp=0x4d2cc0 pc=0x16704
testing.(*T).Run(0x4cc008, {0x1ae74b, 0x9}, 0x1ba528)
	/tmp/gobuilder-mips64/go/src/testing/testing.go:1750 +0x4d4 fp=0x4d2d4c sp=0x4d2cd4 pc=0x124124
testing.runTests.func1(0x4cc008)
	/tmp/gobuilder-mips64/go/src/testing/testing.go:2161 +0x70 fp=0x4d2d6c sp=0x4d2d4c pc=0x1267fc
testing.tRunner(0x4cc008, 0x4d2e24)
	/tmp/gobuilder-mips64/go/src/testing/testing.go:1689 +0x1c0 fp=0x4d2dcc sp=0x4d2d6c pc=0x122e38
testing.runTests(0x4b0020, {0x2a5ba0, 0xf, 0xf}, {0xc183302485517338, 0xa7a363cd21, 0x2aa460})
	/tmp/gobuilder-mips64/go/src/testing/testing.go:2159 +0x47c fp=0x4d2e34 sp=0x4d2dcc pc=0x1266cc
testing.(*M).Run(0x49a050)
	/tmp/gobuilder-mips64/go/src/testing/testing.go:2027 +0x870 fp=0x4d2f74 sp=0x4d2e34 pc=0x124e20
main.main()
	_testmain.go:129 +0x1a4 fp=0x4d2fb4 sp=0x4d2f74 pc=0x176cb8
runtime.main()
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:270 +0x32c fp=0x4d2fec sp=0x4d2fb4 pc=0x62980
runtime.goexit({})
	/tmp/gobuilder-mips64/go/src/runtime/asm_mipsx.s:641 +0x4 fp=0x4d2fec sp=0x4d2fec pc=0xa94e4

goroutine 2 gp=0x400488 m=nil [force gc (idle)]:
runtime.gopark(0x1ba798, 0x2aa1e8, 0x11, 0xa, 0x1)
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:401 +0x128 fp=0x430fd4 sp=0x430fc8 pc=0x62fbc
runtime.goparkunlock(...)
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:407
runtime.forcegchelper()
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:325 +0x128 fp=0x430fec sp=0x430fd4 pc=0x62d98
runtime.goexit({})
	/tmp/gobuilder-mips64/go/src/runtime/asm_mipsx.s:641 +0x4 fp=0x430fec sp=0x430fec pc=0xa94e4
created by runtime.init.5 in goroutine 1
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:313 +0x44

goroutine 3 gp=0x4007e8 m=nil [GC sweep wait]:
runtime.gopark(0x1ba798, 0x2aa360, 0xc, 0x9, 0x1)
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:401 +0x128 fp=0x4317c4 sp=0x4317b8 pc=0x62fbc
runtime.goparkunlock(...)
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:407
runtime.bgsweep(0x4180c0)
	/tmp/gobuilder-mips64/go/src/runtime/mgcsweep.go:277 +0xe8 fp=0x4317e4 sp=0x4317c4 pc=0x44830
runtime.gcenable.gowrap1()
	/tmp/gobuilder-mips64/go/src/runtime/mgc.go:203 +0x64 fp=0x4317ec sp=0x4317e4 pc=0x31ae0
runtime.goexit({})
	/tmp/gobuilder-mips64/go/src/runtime/asm_mipsx.s:641 +0x4 fp=0x4317ec sp=0x4317ec pc=0xa94e4
created by runtime.gcenable in goroutine 1
	/tmp/gobuilder-mips64/go/src/runtime/mgc.go:203 +0xb4

goroutine 4 gp=0x400908 m=nil [GC scavenge wait]:
runtime.gopark(0x1ba798, 0x2aa580, 0xd, 0xa, 0x2)
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:401 +0x128 fp=0x431fbc sp=0x431fb0 pc=0x62fbc
runtime.goparkunlock(...)
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:407
runtime.(*scavengerState).park(0x2aa580)
	/tmp/gobuilder-mips64/go/src/runtime/mgcscavenge.go:425 +0x98 fp=0x431fd0 sp=0x431fbc pc=0x4127c
runtime.bgscavenge(0x4180c0)
	/tmp/gobuilder-mips64/go/src/runtime/mgcscavenge.go:653 +0x74 fp=0x431fe4 sp=0x431fd0 pc=0x41b20
runtime.gcenable.gowrap2()
	/tmp/gobuilder-mips64/go/src/runtime/mgc.go:204 +0x64 fp=0x431fec sp=0x431fe4 pc=0x31a6c
runtime.goexit({})
	/tmp/gobuilder-mips64/go/src/runtime/asm_mipsx.s:641 +0x4 fp=0x431fec sp=0x431fec pc=0xa94e4
created by runtime.gcenable in goroutine 1
	/tmp/gobuilder-mips64/go/src/runtime/mgc.go:204 +0x114

goroutine 17 gp=0x484248 m=nil [finalizer wait]:
runtime.gopark(0x1ba64c, 0x2b4500, 0x10, 0xa, 0x1)
	/tmp/gobuilder-mips64/go/src/runtime/proc.go:401 +0x128 fp=0x42c794 sp=0x42c788 pc=0x62fbc
runtime.runfinq()
	/tmp/gobuilder-mips64/go/src/runtime/mfinal.go:193 +0x164 fp=0x42c7ec sp=0x42c794 pc=0x30618
runtime.goexit({})
	/tmp/gobuilder-mips64/go/src/runtime/asm_mipsx.s:641 +0x4 fp=0x42c7ec sp=0x42c7ec pc=0xa94e4
created by runtime.createfing in goroutine 1
	/tmp/gobuilder-mips64/go/src/runtime/mfinal.go:163 +0x98
FAIL	internal/runtime/atomic	0.023s

Some builder links

https://build.golang.org/log/fc4f9825ea90abcfad3e7c6e79bb711a9415da3a

https://build.golang.org/log/193b07fe1ac02bf014beddbec8de9dc7914a8132

https://ci.chromium.org/ui/inv/build-8749553805143842657/test-results?sortby=&groupby=

What did you expect to see?

I expected all tests to pass.
@alexbrainman alexbrainman mentioned this issue Apr 27, 2024
Closed
@egonelbre
Copy link
Contributor

egonelbre commented Apr 27, 2024 •
edited
Loading

I'm trying to come up with a good reason why things started failing after my changes, but I don't have a good theory yet.

My best guess is when x is allocated on stack, it's position somehow changed to testing.T struct definition changes:

func TestAnd64(t *testing.T) {
	x := uint64(0xffffffffffffffff) // <-- it seems that this is not aligned correctly anymore
	// ...
	v := atomic.And64(&x, ^(1 << i))

If that's the case then the test should use something that guarantees alignment of x. Alternatively, if the compiler should provide that guarantee, then there's some other mistake.

I need to find a system where I can test this theory.

@egonelbre
Copy link
Contributor

If my theory is correct, then this should fix it:

	// Basic sanity check.
	// NOTE: this cannot use x := uint64(...),
	// because it may not be aligned to 64 bit on 32 bit systems.
	x := new(uint64)
	*x = 0xffffffffffffffff
	for i := uint64(0); i < 64; i++ {
		old := x
		v := atomic.And64(x, ^(1 << i))
		if r := uint64(0xffffffffffffffff) << (i + 1); *x != r || v != old {
			t.Fatalf("clearing bit %#x: want %#x, got new %#x and old %#v", uint64(1<<i), r, *x, v)
		}
	}

And there might be other similar tests. Maybe there is a need for a general vet / staticcheck that ensures you don't use atomic.And64 (and similar) on potentially unaligned stack values?

@egonelbre
Copy link
Contributor

For reference, issue that added those atomic implementations #61395.

@alexbrainman
Copy link
Member Author

My best guess is when x is allocated on stack, it's position somehow changed to testing.T struct definition changes:

Sounds reasonable.

If my theory is correct, then this should fix it:

Do you want to prepare a CL to fix the test? Then we could, perhaps, figure out how to test it on linux/arm try-bots.

And there might be other similar tests. Maybe there is a need for a general vet / staticcheck that ensures you don't use atomic.And64 (and similar) on potentially unaligned stack values?

I agree. It is a mess. Let's not try to fix that now. Let's try and make all arm pass by aligning TestAnd64 to test your theory. Alternatively we could just skip TestAnd64 on arm.

What do you think?

Alex

@alexbrainman
Copy link
Member Author

For my own reference. How to run slow bots

https://go.dev/wiki/SlowBots#using-slowbots

from

https://groups.google.com/g/golang-dev/c/Oq1LDUmIyLg/m/QV9wEb75AAAJ

Alex

@egonelbre
Copy link
Contributor

egonelbre commented Apr 27, 2024 •
edited
Loading

https://pkg.go.dev/sync/atomic#pkg-note-BUG mentions that:

On ARM, 386, and 32-bit MIPS, it is the caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically via the primitive atomic functions (types Int64 and Uint64 are automatically aligned). The first word in an allocated struct, array, or slice; in a global variable; or in a local variable (because the subject of all atomic operations will escape to the heap) can be relied upon to be 64-bit aligned.

@egonelbre
Copy link
Contributor

Oh, I just discovered this thread in the mailing list https://groups.google.com/g/golang-dev/c/ArIkltx1ses

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/581916 mentions this issue: internal/runtime/atomic: fix TestAnd64 and TestOr64

@egonelbre
Copy link
Contributor

Using what was suggested in the mailing list seems to fix the issue:

func TestAnd64(t *testing.T) {
	// Basic sanity check.
	x := uint64(0xffffffffffffffff)
	sink = &x

gotip-linux-arm is now passing, however I wasn't able to start windows-arm and openbsd-arm for some reason.

@alexbrainman
Copy link
Member Author

gotip-linux-arm is now passing,

I can see that.

however I wasn't able to start windows-arm and openbsd-arm for some reason.

I suspect that windows-arm and openbsd-arm are not available to be run from Gerrit.

I +2 your https://go-review.googlesource.com/c/go/+/581916 , now we wait for Googlers to review it and submit.

Thanks for your quick help.

Alex

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 27, 2024
@dmitshur dmitshur added this to the Go1.23 milestone Apr 27, 2024
@dmitshur dmitshur added 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 27, 2024
@egonelbre egonelbre mentioned this issue Apr 30, 2024
Closed
@mattdurham mattdurham mentioned this issue Jun 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
4 participants
@egonelbre @dmitshur @gopherbot @alexbrainman