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

runtime/race: should 32-bit atomic ops be flagged as racing with 64-bit atomic ops? #38881

Open
bcmills opened this issue May 5, 2020 · 3 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented May 5, 2020

(Broken out from #5045 (comment).)

My instinct is that mixing 32-bit and 64-bit atomic ops on the same memory must not be allowed, because their implementations may differ on some 32-bit hardware. However, the race detector does not currently flag them.

For example, this program exits without error when run with go test -race using go version devel +9b189686 Tue May 5 05:13:26 2020 +0000 linux/amd64:

package main

import (
	"fmt"
	"runtime"
	"sync/atomic"
	"unsafe"
)

func main() {
	var x uint64
	xa := (*[2]uint32)(unsafe.Pointer(&x))
	xl := &xa[0]
	xh := &xa[1]

	done := make(chan struct{})
	go func() {
		atomic.StoreUint64(&x, 0xbadc0ffee)
		close(done)
	}()
	runtime.Gosched()
	x0 := atomic.LoadUint32(xl)
	x1 := atomic.LoadUint32(xh)

	<-done
	fmt.Printf("Stored %x; loaded %x.", *xa, [2]uint32{x0, x1})
}

CC @dvyukov @aclements @cherrymui

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 5, 2020

We decided to suppress integer alignment diagnostics on architectures that don't require strict alignment, so it seems consistent to only diagnose mixed-size atomics on architectures where that's actually not safe?

I think it's safe on amd64 and probably all 64-bit architectures, since they provide native 64-bit atomic instructions.

It looks safe on 386 and arm too, since they use native 64-bit atomic instructions.

On 32-bit mips, I think the particular test case above is safe, because the 64-bit store is decomposed into two aligned 32-bit stores. But if it was changed to atomically load an unaligned 32-bit value, it could see tearing between the two 32-bit stores. (But maybe mips doesn't allow unaligned 32-bit memory accesses anyway?)

On wasm I think it's safe because there's no real concurrency?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 5, 2020

What "safe" do we want to guarantee here? Do we forbid x0 observing new value while x1 observing old value? I think this could happen on 32-bit MIPS and maybe old 32-bit ARM. Do we say this "unsafe"?

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 5, 2020

@cherrymui Good point. I forgot the Loads themselves should be ordered, so you probably wouldn't expect them to return new and then old, respectively.

Preventing that on 32-bit MIPS seems like it might be prohibitive though. We'd have to add mutex protection to all atomics, not just the 64-bit variants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants