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

cmd/compile: data race in concurrent compiler #20144

Closed
bradfitz opened this issue Apr 27, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@bradfitz
Copy link
Member

commented Apr 27, 2017

Thanks to the new racecompile builder (#19962), we have our first data race failure bug report for the new cmd/compile -c concurrent compilation:

https://build.golang.org/log/16ef1dc48a907d5a57439aa9475f727a04f86a88

# runtime
==================
WARNING: DATA RACE
Write at 0x00c420671ab8 by goroutine 17:
  cmd/compile/internal/gc.(*ssafn).AllocFrame()
      /tmp/workdir/go/src/cmd/compile/internal/gc/bitset.go:23 +0xf9a
  cmd/compile/internal/ssa.stackframe()
      /tmp/workdir/go/src/cmd/compile/internal/ssa/stackframe.go:9 +0x58
  cmd/compile/internal/ssa.Compile()
      /tmp/workdir/go/src/cmd/compile/internal/ssa/compile.go:70 +0x458
  cmd/compile/internal/gc.buildssa()
      /tmp/workdir/go/src/cmd/compile/internal/gc/ssa.go:212 +0x11c4
  cmd/compile/internal/gc.compileSSA()
      /tmp/workdir/go/src/cmd/compile/internal/gc/pgen.go:236 +0x52
  cmd/compile/internal/gc.compileFunctions.func1()
      /tmp/workdir/go/src/cmd/compile/internal/gc/pgen.go:262 +0x5a

Previous read at 0x00c420671ab8 by goroutine 35:
  [failed to restore the stack]

Goroutine 17 (running) created at:
  cmd/compile/internal/gc.compileFunctions()
      /tmp/workdir/go/src/cmd/compile/internal/gc/pgen.go:260 +0x108
  cmd/compile/internal/gc.Main()
      /tmp/workdir/go/src/cmd/compile/internal/gc/main.go:580 +0x3d9f
  main.main()
      /tmp/workdir/go/src/cmd/compile/main.go:49 +0xf5

Goroutine 35 (running) created at:
  cmd/compile/internal/gc.compileFunctions()
      /tmp/workdir/go/src/cmd/compile/internal/gc/pgen.go:260 +0x108
  cmd/compile/internal/gc.Main()
      /tmp/workdir/go/src/cmd/compile/internal/gc/main.go:580 +0x3d9f
  main.main()
      /tmp/workdir/go/src/cmd/compile/main.go:49 +0xf5
==================
Found 1 data race(s)

@bradfitz bradfitz added the NeedsFix label Apr 27, 2017

@bradfitz bradfitz added this to the Go1.9 milestone Apr 27, 2017

@dcheney-atlassian

This comment has been minimized.

Copy link

commented Apr 27, 2017

Previous read at 0x00c420671ab8 by goroutine 35:
[failed to restore the stack]

I'd say we have two bugs.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 27, 2017

CL https://golang.org/cl/41973 mentions this issue.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

It disturbs me that I haven't been able to reproduce this locally. My past experience is that such issues reproduce very readily.

It seems very likely that I've done this to myself, by packing Node.Used into a single field with other Node data, notably Class. But before attempting to fix it, I'd like to reproduce it...

gopherbot pushed a commit that referenced this issue Apr 27, 2017

cmd/compile: randomize compilation order when race-enabled
There's been one failure on the race builder so far,
before we started sorting functions by length.

The race detector can only detect actual races,
and ordering functions by length might reduce the odds
of catching some kinds of races. Give it more to chew on.

Updates #20144

Change-Id: I0206ac182cb98b70a729dea9703ecb0fef54d2d0
Reviewed-on: https://go-review.googlesource.com/41973
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Apr 27, 2017

CL https://golang.org/cl/42015 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 27, 2017

cmd/compile: move Used from gc.Node to gc.Name
Node.Used was written to from the backend
concurrently with reads of Node.Class
for the same ONAME Nodes.
I do not know why it was not failing consistently
under the race detector, but it is a race.

This is likely also a problem with Node.HasVal and Node.HasOpt.
They will be handled in a separate CL.

Fix Used by moving it to gc.Name and making it a separate bool.
There was one non-Name use of Used, marking OLABELs as used.
That is no longer needed, now that goto and label checking
happens early in the front end.

Leave the getters and setters in place,
to ease changing the representation in the future
(or changing to an interface!).

Updates #20144

Change-Id: I9bec7c6d33dcb129a4cfa9d338462ea33087f9f7
Reviewed-on: https://go-review.googlesource.com/42015
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

I manually instrumented Node.flags with a poison pill, so as to be sure I've caught all such races. There were legit races with Node.Used and Node.HasOpt. CL 42015 fixed Node.Used. I'm working on Node.HasOpt.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 28, 2017

CL https://golang.org/cl/42135 mentions this issue.

@gopherbot gopherbot closed this in 794d29a Apr 28, 2017

josharian added a commit to josharian/go that referenced this issue May 2, 2017

cmd/compile: randomize compilation order when race-enabled
There's been one failure on the race builder so far,
before we started sorting functions by length.

The race detector can only detect actual races,
and ordering functions by length might reduce the odds
of catching some kinds of races. Give it more to chew on.

Updates golang#20144

Change-Id: I0206ac182cb98b70a729dea9703ecb0fef54d2d0

@golang golang locked and limited conversation to collaborators Apr 28, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.