go: cmd/compile: go1.8 regression: sync/atomic loop elided
sync/atomic: go1.8 regression: atomic.AddUint64 is unreliable
Sometimes atomic.AddUint64 has no effect.
Update: @cespare has discovered that the loop containing atomic.AddUint64 has been elided.
Update: @rjeczalik has reported that this behavior also occurs with atomic.StoreUint64 and atomic.CompareAndSwapUint64 functions.
package main
import (
"fmt"
"runtime"
"sync/atomic"
"time"
)
var a uint64 = 0
func main() {
runtime.GOMAXPROCS(runtime.NumCPU())
fmt.Println(runtime.NumCPU(), runtime.GOMAXPROCS(0))
go func() {
for {
atomic.AddUint64(&a, uint64(1))
}
}()
for {
val := atomic.LoadUint64(&a)
fmt.Println(val)
time.Sleep(time.Second)
}
}
For go1.4, go1.5, go1.6, and go1.7:
The atomic.AddUint64(&a, uint64(1)) statement works as expected.
$ go version
go version go1.4-bootstrap-20161024 linux/amd64
go version go1.5.4 linux/amd64
go version go1.6.4 linux/amd64
go version go1.7.5 linux/amd64
$ go build atomic.go && ./atomic
4 4
0
96231831
192599210
289043510
385369439
481772231
578143106
674509741
770966820
867408361
963866833
1060299901
<SNIP>
^C
For go1.8 and go tip:
The atomic.AddUint64(&a, uint64(1)) statement appears to have no effect.
go version go1.8 linux/amd64
go version devel +1e69aef Sat Feb 18 19:01:08 2017 +0000 linux/amd64
go version devel +1e69aef Sat Feb 18 19:01:08 2017 +0000 windows/amd64
$ uname -a
Linux peter 4.8.0-36-generic #36~16.04.1-Ubuntu SMP Sun Feb 5 09:39:57 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/peter/gopath"
GORACE=""
GOROOT="/home/peter/go"
GOTOOLDIR="/home/peter/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build842347949=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="0"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
$ go build atomic.go && ./atomic
4 4
0
0
0
0
0
0
0
0
0
<SNIP>
^C
Interestingly, we can make go1.8 and go tip work with a simple code modification that should not have any effect:
package main
import (
"fmt"
"runtime"
"sync/atomic"
"time"
)
var a uint64 = 0
func main() {
runtime.GOMAXPROCS(runtime.NumCPU())
fmt.Println(runtime.NumCPU(), runtime.GOMAXPROCS(0))
go func() {
for {
new := atomic.AddUint64(&a, uint64(1))
if new == 0 {
runtime.Gosched() // or print()
}
}
}()
for {
val := atomic.LoadUint64(&a)
fmt.Println(val)
time.Sleep(time.Second)
}
}
$ go version
go version go1.8 linux/amd64
$ go build atomic_pass.go && ./atomic_pass
4 4
0
126492073
253613883
378888824
506065798
633247293
760383560
887553077
1014723419
<SNIP>
^C
After AddUint64(&a, uint64(1), new should be greater than zero and runtime.Gosched() should not be executed. Substituting print() for runtime.Gosched() also works. If the line runtime.Gosched() // or print() is commented out the program reverts to failure.
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Reactions: 6
- Comments: 31 (23 by maintainers)
Commits related to this issue
- dev-lang/go: 1.8-r1 revision bump This fixes #19182 upstream [1]. [1] https://github.com/golang/go/issues/19182 Package-Manager: Portage-2.3.3, Repoman-2.3.2 — committed to gentoo/gentoo by williamh 7 years ago
- [release-branch.go1.8] cmd/compile: add opcode flag hasSideEffects for do-not-remove Added a flag to generic and various architectures' atomic operations that are judged to have observable side effec... — committed to golang/go by dr2chase 7 years ago
The atomic.AddUint64 loop is compiled to an empty loop. Strictly saying this is confirming behavior as we don’t give any guarantees about scheduler (just like any other language, e.g. C++). This can be explained as “the goroutine is just not scheduled”. However I think we should fix this and not compile this to an empty loop. Akin to the C++ guarantee of “atomic writes should be visible to other threads in a finite amount of time”. It is OK to combine up to inifinte number of non atomic writes, because that’s invisible without data races, and I am not a fan of making programs with data races any more useful. But side effects of atomic writes are observable without data races, so it’s fine to combine any finite number of atomic writes (e.g.
atomic.Store(&x, 1); atomic.Store(&x, 2)can be compiled asatomic.Store(&x, 2)), but not an infinite number of atomic writes. Language implementation become useless when they take formal properties to the extreme.Looking at the assembly, the increment (and in fact the whole for loop) has been (over-)optimized away.
(Edit: I missed the jmp initially – as @dvyukov points out, there is an empty loop.)
I’ll bisect.
The code in the original post is real enough. This program has side-effect – it prints to stdout. Someone can use this output to produce different results.
Is the runtime miscompiled like this?
@cespare thanks for the reply.
Java has volatile (and atomic) variables which provide happens-before semantics that allow them to be used as flags and other types of synchronizers. In Go you would also have provide some type of synchronization along with the atomic variable to ensure memory visibility. But if you are already using another type of synchronization it seems unlikely that you would even need the atomic update. Maybe i’m misssing something, but I just don’t see too many realistic use cases for atomic variables if they don’t have happens-before semantics.
The “atomic writes should be visible to other threads in a finite amount of time” statement seem too “maybe-maybe-not” for a programming language. It seems like this would lead to bugs that are nearly impossible to test for.
I’d love to see happens-before on atomics but mutexes and channels work great for now.
I don’t think the runtime contains any loops like that, so yes (same optimizations) and no (no code matching this amusing optimization).
Is this breaking any real applications? I had to write a compiler test program (for loop rescheduling, hence could not contain any function calls) very carefully to ensure that the loop remained, but that was only a test.
Happens-before is unrelated to this case. Even if we document happens-before relations for atomic variables (note that currently they are still implied), there is nothing that will force the loop to see the updates as there is yet no any happens-before relations. Happens-before is only about visibility of secondary/dependent data, not about primary visibility.
To guarantee that the loop prints increasing values over times we need 2 guarantees:
Yeah…after 584 years (at 1 ns / loop).
It also affects
AddUintptr/LoadUintptr,AddInt32/LoadInt32andAddUint32/LoadUint32.@rjeczalik from @dvyukov’s description above, I don’t think that type of code would be affected (the loop has an exit condition).
A git bisect indicates d6098e4277bab633c2df752ed90e1e826918ca67.
/cc @randall77
This bug applies also to
atomic.StoreUint64andatomic.CompareAndSwapUint64functions, they also reproduce the buggy behaviour shown in the atomic.go repro. Just a heads up, as it may be not obvious just by reading the description.