sync/atomic: "synchronizes-before" ordering is broken on Load/Store.

haruyama480 commented 6 months ago

Go version

go version go1.22.2 darwin/arm64

Output of `go env` in your module/workspace:

% go env
GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/xxx/Library/Caches/go-build'
GOENV='/Users/xxx/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/xxx/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/xxx/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/xxx/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.2.darwin-arm64'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/xxx/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.2.darwin-arm64/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.2'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/xxx/ghq/github.com/haruyama480/go-litmus-test/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/7c/vn1g1x4n4nqclvb5dnjkf8q80000gn/T/go-build1289181956=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

go func() {
  atomic.StoreInt32(&x, 1) // W1
  atomic.StoreInt32(&y, 1) // W2
}

r1 := atomic.LoadInt32(&x) // R1
r2 := atomic.LoadInt32(&y) // R2

sample

What did you see happen?

r1 == 0 && r2 == 1 was observed.

https://pkg.go.dev/sync/atomic says,

In the terminology of the Go memory model, if the effect of an atomic operation A is observed by atomic operation B, then A “synchronizes before” B. Additionally, all the atomic operations executed in a program behave as though executed in some sequentially consistent order.

but, it seems to be false.

What did you expect to see?

If W1 synchronized before W2, r1 == 0 && r2 == 1 should not be observed.

haruyama480 commented 6 months ago

sync/atomic uses AArch64's STLR* instructions. https://github.com/golang/go/blob/ac174400f460e9b577079e8606439e0bae62adb0/src/internal/runtime/atomic/atomic_arm64.s#L105-L121

But, arm's hardware memory model allows memory accesses to different addresses to be "Re-order"ed. https://developer.arm.com/documentation/102376/0100/Normal-memory

seankhliao commented 6 months ago

r1 == 0 && r2 == 1

isn't this simply: W1, (the two store ops), W2

haruyama480 commented 6 months ago

W1, (the two store ops), W2

Sorry, I don't understand. Could you give me some details?

haruyama480 commented 6 months ago

(I added "R1" and "R2" for convention)

haruyama480 commented 6 months ago

Sorry, I get it.

You meant R1, W1, W2, R2.

haruyama480 commented 6 months ago

I fixed my code, and confirmed the test works.

-                       r1 := atomic.LoadInt32(&x)
-                       r2 := atomic.LoadInt32(&y)
-                       if r1 == 0 && r2 == 1 {
+                       r1 := atomic.LoadInt32(&y)
+                       r2 := atomic.LoadInt32(&x)
+                       if r1 == 1 && r2 == 0 {

Thank you for comments!

(Though I'm curious there are correct ordering with arm's STLR/LDAR instructions, I will study it. )

haruyama480 commented 6 months ago

In my macOS, TSO mode is enabled. So, If I turn it off or use another arm, fixed test may fail possibly.

% sysctl net.inet.tcp.tso
net.inet.tcp.tso: 1

golang / go