cmd/compile: race detector in go1.11rc1 detects race with close(chan) and len(chan)

[ncw]

What version of Go are you using (go version)?

go version go1.11rc1 linux/amd64

Does this issue reproduce with the latest release?

It reproduces with go1.11rc1. It appears to be a regression as go1.10.1 does not have the same problem.

There doesn't appear to be any further commits to release-branch.go1.11 to try.

I tried go-tip go version devel +714c141c4f Sat Aug 18 03:02:40 2018 +0000 linux/amd64 also with the same results.

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ncw/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ncw/go"
GOPROXY=""
GORACE=""
GOROOT="/opt/go/go1.11"
GOTMPDIR=""
GOTOOLDIR="/opt/go/go1.11/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build646888704=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Save this program to race.go: https://play.golang.org/p/wBrXZ2n9BtW

run it with go run -race race.go

What did you expect to see?

length 1
closed

What did you see instead?

==================
WARNING: DATA RACE
Read at 0x00c00005e070 by goroutine 6:
  main.main.func1()
      /home/ncw/Go/race.go:20 +0x86

Previous write at 0x00c00005e070 by goroutine 7:
  runtime.closechan()
      /opt/go/go1.11/src/runtime/chan.go:327 +0x0
  main.main.func2()
      /home/ncw/Go/race.go:27 +0x79

Goroutine 6 (running) created at:
  main.main()
      /home/ncw/Go/race.go:17 +0x118

closed
Goroutine 7 (running) created at:
  main.main()
      /home/ncw/Go/race.go:24 +0x14e
==================
length 1
Found 1 data race(s)
exit status 66

Discussion

The attached program races close(chan) against len(chan). In go1.10.1 this does not cause the race detector to fire. In go1.11rc1 it does.

I believe this to be a regression since it says in

https://golang.org/ref/spec#Channel_types

(emphasis mine)

A single channel may be used in send statements, receive operations, and calls to the built-in functions cap and len by any number of goroutines without further synchronization.

Which implies to me that len() should be usable without further synchronization. Though it doesn't explicitly say about close() so I guess there is some wriggle room.

To have any of the operations on channels able to cause a race is surprising to me.

A bit of git bisect indicates that this is the commit where the problem started:

17df5ed is the first bad commit by @mdempsky

commit 17df5ed910cab9c68bc781b06d83b8db3fd0f75c
Author: Matthew Dempsky <mdempsky@google.com>
Date:   Tue Mar 27 13:50:08 2018 -0700

    cmd/compile: insert instrumentation during SSA building
    
    Insert appropriate race/msan calls before each memory operation during
    SSA construction.
    
    This is conceptually simple, but subtle because we need to be careful
    that inserted instrumentation calls don't clobber arguments that are
    currently being prepared for a user function call.
    
    reorder1 already handles introducing temporary variables for arguments
    in some cases. This CL changes it to use them for all arguments when
    instrumenting.
    
    Also, we can't SSA struct types with more than one field while
    instrumenting. Otherwise, concurrent uses of disjoint fields within an
    SSA-able struct can introduce false races.
    
    This is both somewhat better and somewhat worse than the old racewalk
    instrumentation pass. We're now able to easily recognize cases like
    constructing non-escaping closures on the stack or accessing closure
    variables don't need instrumentation calls. On the other hand,
    spilling escaping parameters to the heap now results in an
    instrumentation call.
    
    Overall, this CL results in a small net reduction in the number of
    instrumentation calls, but a small net increase in binary size for
    instrumented executables. cmd/go ends up with 5.6% fewer calls, but a
    2.4% larger binary.
    
    Fixes #19054.
    
    Change-Id: I70d1dd32ad6340e6fdb691e6d5a01452f58e97f3
    Reviewed-on: https://go-review.googlesource.com/102817
    Reviewed-by: Cherry Zhang <cherryyz@google.com>

:040000 040000 7001cbc4b98c5797c9f02e8035488b04e4016a2a 1a615a1f54919ed79e5eeac22c2e03cb180b1ccb M	src

[ianlancetaylor]

Thanks for the bug report. Marking for 1.12 but it's OK with me if a clean fix goes into 1.11 or 1.11.1.

I would guess that the problem has to do with the fact that len(chan) is inlined. That perhaps shouldn't happen when using the race detector.

[randall77]

We unconditionally inline len and cap of channels (and maps) when building ssa. We've done that for a while. There's no uninlined fallback (except through reflect).

I'm not convinced this is a bug. The statement in the spec does not include close, so it doesn't apply.

As a practical matter, it is true that len and close don't actually race. We could suppress the race detector report in this case.

A simple fix would be to not use the address of len or cap as the "synchronization address" for the channel. All the channel ops (except for len and cap) report a read or write to the starting address of the channel structure (the hchan). That also happens to be the address of the channel length. If we used the address of some other field in the hchan as the synchronization address, it would solve this problem.

[gopherbot]

Change https://golang.org/cl/135698 mentions this issue: runtime: ignore races between close and len/cap

[ncw]

@randall77 wrote:

I'm not convinced this is a bug. The statement in the spec does not include close, so it doesn't apply.

I think that any channel operations causing the race detector to moan is surprising. IMHO the spec should be tightened to include close.

As a practical matter, it is true that len and close don't actually race. We could suppress the race detector report in this case.

That seems like the right direction to go in to me. If it isn't a race then the race detector shouldn't be reporting it IMHO!

[odeke-em]

@randall77 should we backport this to Go1.11.1?

[randall77]

Not sure. I'll open an issue so we can discuss.

Gopherbot, please open a backport to 1.11.