cmd/go: data race on (*load.PackageInternal).Gcflags field

[bcmills]

The race was observed at head with go test -race cmd/go, but doesn't always reproduce.

        WARNING: DATA RACE
        Read at 0x00c00037e8b0 by goroutine 17:
          cmd/go/internal/work.gcToolchain.gc()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/gc.go:105 +0xc83
          cmd/go/internal/work.(*gcToolchain).gc()
              <autogenerated>:1 +0x107
          cmd/go/internal/work.(*Builder).build()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/exec.go:642 +0x1fd4
          cmd/go/internal/work.(*Builder).Do.func1()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/exec.go:107 +0x465
          cmd/go/internal/work.(*Builder).Do.func2()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/exec.go:164 +0x97

        Previous write at 0x00c00037e8b0 by goroutine 16:
          cmd/go/internal/load.setToolFlags()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/load/pkg.go:1800 +0x184
          cmd/go/internal/load.LoadImportWithFlags()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/load/pkg.go:1726 +0xe9
          cmd/go/internal/work.gcToolchain.symabis()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/gc.go:307 +0x2ea
          cmd/go/internal/work.(*gcToolchain).symabis()
              <autogenerated>:1 +0x8c
          cmd/go/internal/work.(*Builder).build()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/exec.go:606 +0x183f
          cmd/go/internal/work.(*Builder).Do.func1()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/exec.go:107 +0x465
          cmd/go/internal/work.(*Builder).Do.func2()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/exec.go:164 +0x97

        Goroutine 17 (running) created at:
          cmd/go/internal/work.(*Builder).Do()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/exec.go:151 +0x66e
          cmd/go/internal/test.runTest()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/test/test.go:752 +0x15cf
          main.main()
              /usr/local/google/home/bcmills/go/src/cmd/go/main.go:213 +0xa09

        Goroutine 16 (running) created at:
          cmd/go/internal/work.(*Builder).Do()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/work/exec.go:151 +0x66e
          cmd/go/internal/test.runTest()
              /usr/local/google/home/bcmills/go/src/cmd/go/internal/test/test.go:752 +0x15cf
          main.main()
              /usr/local/google/home/bcmills/go/src/cmd/go/main.go:213 +0xa09

[jayconrod]

Note to future self: nothing from Builder.Do should call LoadImportWithFlags. Package loading accesses global mutable caches and is not concurrency safe.

[jayconrod]

@aclements, could you take a look at gcToolchain.symabis? I think there's a bigger issue here, but I'm not sure I understand what's being done well enough to fix it.

The race is triggered by symbabis calling load.LoadImportWithFlags (previously known as LoadPackage), which is not thread safe. It reads and writes global state (in particular, flags in the loaded package), so it shouldn't be called inside an action.

symabis calls load.LoadImportWithFlags for a few specific packages (runtime, runtime/internal/atomic, sync/atomic) for other packages that depend on them. So in addition to the data race, it seems like there's a cyclic dependency that might not be recorded. For example, assembly files in sync/atomic are read when building runtime/internal/atomic, but I don't think that's recorded in the action id, so a change in one of those files wouldn't trigger a rebuild of runtime/internal/atomic.

Is there another way to implement this that avoids reading those files inside an action?

[jayconrod]

@aclements Is there a way to avoid the dependency cycle? A change in one of these packages may not cause the other packages to be rebuilt, since the build system doesn't know about these. The call to LoadImportWithFlags is also not safe.

[aclements]

The goal of this code is to scan for references from assembly in package A to symbols defined in Go in package B and tell the compiler when compiling package B. This is working around "group 2" issues from the internal ABI design. Things that come to mind:

1. We only need the SFiles list from package A. Can we get that without triggering all of the non-thread-safe stuff? Can we include those in the signature of package B? It's not really a cyclic dependency, since compiling B doesn't depend on compiling A; it just depends on some of the same input files. This solution would live in cmd/go.

2. Currently we only construct ABI bridges when compiling the package that defines a symbol, but with the right type information we could potentially construct them in the referencing package. However, these are mostly (all?) references to unexported symbols, so their type information isn't in the export info, so I'm not sure how we would get the right information to package A.

3. We could mark symbols in package B that need to be referenced from assembly in other packages. We avoided any sort of annotations in the internal ABI design, but maybe an annotation is the less-bad hack here. We could restrict that annotation to runtime/.... This solution would live in the compiler.

I'm kind of leaning toward 3 as the least-bad solution.

[jayconrod]

1. I looked at implementing this yesterday. It looked like both the SFiles and the assembler flags were needed (mkSymabis calls asmArgs). Those can vary based on command line arguments. If the assembler flags aren't actually needed, we could probably get the list of SFiles without too much hassle with cfg.GOROOTsrc, the package name, and a ReadDir call.

2. Making the change on the referencing package side sounds good, but it still sounds like a big special case. That's probably going to be true no matter what.

3. I agree this seems like the least bad option. If this can be done before the beta, I think we should move forward with this.

[gopherbot]

Change https://golang.org/cl/179798 mentions this issue: cmd/go: avoid loading runtime packages when generating symabis

[aclements]

Jay's proposed work-around got me to actually work on this again. :)

I chatted with Russ and Cherry and our proposed fix is to make "//go:linkname"d symbols have all of the ABI wrappers, and to mark the symbols that are being called from other packages with self-linknames.

This dovetails with something gccgo already does (and which might be a good idea to do in the gc toolchain) where unexported symbols cannot be referenced from other packages unless they are linknamed. The meaning of "this symbol can be referenced from other packages" goes nicely "this symbol can be referenced from assembly in other packages".

[jayconrod]

That fix sounds much better than my hack! :)

Do you think that will land before the beta? If so, I'll abandon my CL.

[aclements]

I have the CLs mostly prepared already, so yes. I'm just building all of the configurations to make sure I've marked the right symbols, which takes a while.