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

Jump to bottom

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

Closed
ncw opened this issue Aug 18, 2018 · 8 comments
Closed

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

ncw opened this issue Aug 18, 2018 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector
Milestone
Go1.12

Comments

@ncw
Copy link
Contributor

ncw commented Aug 18, 2018

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 ianlancetaylor changed the title cmd/go: race detector in go1.11rc1 detects race with close(chan) and len(chan) cmd/compile: race detector in go1.11rc1 detects race with close(chan) and len(chan) Aug 19, 2018
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector labels Aug 19, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Aug 19, 2018
@ianlancetaylor
Copy link
Contributor

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.

ncw added a commit to rclone/rclone that referenced this issue Aug 20, 2018
@ncw
fs/asyncreader: skip some tests to work around race detector bug
760370d 
The race detector currently detects a race with len(chan) against
close(chan).

See: golang/go#27070

Skip the tests which trip this bug under the race detector.
ncw added a commit to rclone/rclone that referenced this issue Aug 20, 2018
@ncw
fs/asyncreader: skip some tests to work around race detector bug
7712a0e 
The race detector currently detects a race with len(chan) against
close(chan).

See: golang/go#27070

Skip the tests which trip this bug under the race detector.
@isaachier isaachier mentioned this issue Sep 9, 2018
Merged
@randall77
Copy link
Contributor

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
Copy link

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

@benesch benesch mentioned this issue Sep 18, 2018
Merged
@ncw
Copy link
Contributor Author

ncw commented Sep 18, 2018

@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
Copy link
Member

@randall77 should we backport this to Go1.11.1?

@randall77
Copy link
Contributor

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

Gopherbot, please open a backport to 1.11.

@randall77 randall77 mentioned this issue Sep 20, 2018
Closed
@randall77
Copy link
Contributor

Hmmm... gopherbot didn't open a CL.
I'll do it manually.

@randall77 randall77 mentioned this issue Sep 20, 2018
Closed
@andybons
Copy link
Member

@randall77 FYI you have to @-mention gopherbot. In your text it didn’t have the @.

@vprithvi vprithvi mentioned this issue Oct 8, 2018
Closed
@kjk kjk mentioned this issue Feb 24, 2019
Closed
@golang golang locked and limited conversation to collaborators Sep 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. RaceDetector
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
6 participants
@andybons @ncw @ianlancetaylor @odeke-em @randall77 @gopherbot