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.

Sign up for GitHub

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

x/tools/go/loader: data race in Config.Load #20718

Closed
natefinch opened this issue Jun 18, 2017 · 8 comments
Closed

x/tools/go/loader: data race in Config.Load #20718

natefinch opened this issue Jun 18, 2017 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.9

Comments

@natefinch
Copy link
Contributor

natefinch commented Jun 18, 2017 •
edited

Please answer these questions before submitting your issue. Thanks!

What did you do?

create these two tests in a _test.go file:

import "golang.org/x/tools/go/loader"

func TestLoad(t *testing.T) {
	t.Parallel()
	imports := map[string]bool{
		"io": false,
	}
	conf := &loader.Config{
		ImportPkgs: imports,
	}
	_, err := conf.Load()
	if err != nil {
		t.Fatal(err)
	}
}
func TestLoad2(t *testing.T) {
	t.Parallel()
	imports := map[string]bool{
		"io": false,
	}
	conf := &loader.Config{
		ImportPkgs: imports,
	}
	_, err := conf.Load()
	if err != nil {
		t.Fatal(err)
	}
}

then run go test -race

What did you expect to see?

Tests pass.

What did you see instead?

==================
WARNING: DATA RACE
Write at 0x00c4200b46b8 by goroutine 6:
  go/types.(*Checker).checkFiles()
      /usr/local/Cellar/go/1.8.3/libexec/src/go/types/check.go:244 +0x1f5
  go/types.(*Checker).Files()
      /usr/local/Cellar/go/1.8.3/libexec/src/go/types/check.go:218 +0x56
  golang.org/x/tools/go/loader.(*importer).addFiles()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:1018 +0x1ad
  golang.org/x/tools/go/loader.(*importer).load()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:980 +0x1ea
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:961 +0x42

Previous write at 0x00c4200b46b8 by goroutine 228:
  go/types.(*Checker).checkFiles()
      /usr/local/Cellar/go/1.8.3/libexec/src/go/types/check.go:244 +0x1f5
  go/types.(*Checker).Files()
      /usr/local/Cellar/go/1.8.3/libexec/src/go/types/check.go:218 +0x56
  golang.org/x/tools/go/loader.(*importer).addFiles()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:1018 +0x1ad
  golang.org/x/tools/go/loader.(*importer).load()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:980 +0x1ea
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:961 +0x42

Goroutine 6 (running) created at:
  golang.org/x/tools/go/loader.(*importer).startLoad()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:963 +0x2d4
  golang.org/x/tools/go/loader.(*importer).importAll()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:875 +0x372
  golang.org/x/tools/go/loader.(*importer).addFiles()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:1005 +0x10e
  golang.org/x/tools/go/loader.(*importer).load()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:980 +0x1ea
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:961 +0x42

Goroutine 228 (finished) created at:
  golang.org/x/tools/go/loader.(*importer).startLoad()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:963 +0x2d4
  golang.org/x/tools/go/loader.(*importer).importAll()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:875 +0x372
  golang.org/x/tools/go/loader.(*importer).addFiles()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:1005 +0x10e
  golang.org/x/tools/go/loader.(*importer).load()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:980 +0x1ea
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /Users/finchnat/src/golang.org/x/tools/go/loader/loader.go:961 +0x42
==================
--- FAIL: TestLoad2 (1.25s)
        testing.go:610: race detected during execution of test
--- FAIL: TestLoad (1.44s)
        testing.go:610: race detected during execution of test
FAIL
exit status 1

System details

go version go1.8.3 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/finchnat"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.8.3/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.8.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ch/4d1vgsv17jq0b3yfmnkl9thdk0jm7d/T/go-build171541402=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
GOROOT/bin/go version: go version go1.8.3 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.8.3 X:framepointer
uname -v: Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.12.5
BuildVersion:	16F73
lldb --version: lldb-370.0.42
  Swift-3.1
@mvdan
Copy link
Member

mvdan commented Jun 18, 2017

Just to be sure, what version of go/loader is this?

@natefinch
Copy link
Contributor Author

natefinch commented Jun 18, 2017 via email

@bradfitz bradfitz changed the title Race in golang.org/x/tools/go/loader x/tools/go/loader: data race in Config.Load Jun 18, 2017
@gopherbot gopherbot added this to the Unreleased milestone Jun 18, 2017
@bradfitz bradfitz modified the milestones: Go1.9, Unreleased Jun 18, 2017
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 18, 2017
@bradfitz
Copy link
Contributor

@griesemer and @alandonovan, this looks like legit usage.

Flagging Go 1.9 at least until we understand what's at fault. It might be in go/build or go/types in std.

@alandonovan
Copy link
Contributor

Thanks for the report, Nate. I can reproduce the race, and though I haven't yet identified the root cause, the problem clearly lies in go/loader, not in go/types or any other std package.

@bradfitz bradfitz changed the title x/tools/go/loader: data race in Config.Load go/loader: data race in Config.Load Jun 19, 2017
@alandonovan
Copy link
Contributor

The root cause (thanks Heschi!) is that go/loader is calling (types.Checker).Files on the unsafe package, which is a global variable. Despite the empty list of files, Files is not a no-op because it sets the package's 'complete' flag, which is already set, so it's a benign data race. I'll prepare a fix.

@bradfitz
Copy link
Contributor

"benign data race"

@alandonovan
Copy link
Contributor

Yes, a benign data race: an assignment v = k happens before two concurrent operations: a read of v and another assignment of v to the same value k. The memory model guarantees that the read observes k.

@gopherbot
Copy link

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

@bradfitz bradfitz changed the title go/loader: data race in Config.Load x/tools/go/loader: data race in Config.Load Jun 19, 2017
@golang golang locked and limited conversation to collaborators Jun 19, 2018
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.
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
6 participants
@bradfitz @natefinch @mvdan @alandonovan @griesemer @gopherbot