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
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector
Milestone

Comments

@bcmills
Copy link
Contributor

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

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector labels May 5, 2020
@bcmills bcmills added this to the Backlog milestone May 5, 2020
@mdempsky
Copy link
Member

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
Member

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

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector
Projects
Status: Triage Backlog
Development

No branches or pull requests

4 participants