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/go: data race on (*load.PackageInternal).Gcflags field #31230

Closed
bcmills opened this issue Apr 3, 2019 · 16 comments
Closed

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

bcmills opened this issue Apr 3, 2019 · 16 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Apr 3, 2019

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

@bcmills bcmills added this to the Go1.13 milestone Apr 3, 2019
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 3, 2019
@jayconrod
Copy link
Contributor

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

@jayconrod
Copy link
Contributor

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

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

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

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

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

@aclements
Copy link
Member

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

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

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.

@gopherbot
Copy link

Change https://golang.org/cl/179861 mentions this issue: cmd/compile: make the second argument to go:linkname optional

@gopherbot
Copy link

Change https://golang.org/cl/179860 mentions this issue: cmd/compile: generate ABI wrappers for //go:linkname'd symbols

@gopherbot
Copy link

Change https://golang.org/cl/179862 mentions this issue: runtime: mark all Go symbols called from assembly in other packages

@gopherbot
Copy link

Change https://golang.org/cl/179863 mentions this issue: cmd/go: remove cross-package assembly reference discovery

gopherbot pushed a commit that referenced this issue Jun 6, 2019
Calling a Go symbol from assembly in another package currently results
in a link failure because the Go symbol is defined as ABIInternal, but
the assembly call is from ABI0. In general this is okay because you
shouldn't do this anyway, but there are special cases where this is
necessary, especially between the runtime and packages closely tied to
the runtime in std.

Currently, we address this for runtime symbols with a hack in cmd/go
that knows to scan related packages when building the symabis file for
the runtime and runtime/internal/atomic. However, in addition to being
a messy solution in the first place, this hack causes races in cmd/go
that are difficult to work around.

We considered creating dummy references from assembly in the runtime
to these symbols, just to make sure they get ABI0 wrappers. However,
there are a fairly large number of these symbols on some platforms,
and it can vary significantly depending on build flags (e.g., race
mode), so even this solution is fairly unpalatable.

This CL addresses this by providing a way to mark symbols in Go code
that should be made available to assembly in other packages. Rather
than introduce a new pragma, we lightly expand the meaning of
"//go:linkname", since that pragma already generally indicates that
you're making the symbol available in a way it wasn't before. This
also dovetails nicely with the behavior of go:linkname in gccgo, which
makes unexported symbols available to other packages.

Follow-up CLs will make use of this and then remove the hack from
cmd/go.

Updates #31230.

Change-Id: I23060c97280626581f025c5c01fb8d24bb4c5159
Reviewed-on: https://go-review.googlesource.com/c/go/+/179860
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit that referenced this issue Jun 6, 2019
The //go:linkname directive can be used to make a symbol accessible to
another package (when it wouldn't normally be). Sometimes you want to
do this without actually changing the symbol's object file symbol
name; for example, in gccgo this makes unexported symbols non-static,
and in gc this provides ABI0 wrappers for Go symbols so they can be
called from assembly in other packages. Currently, this results in
stutter like

   //go:linkname entersyscall runtime.entersyscall

This CL makes the second argument to go:linkname optional for the case
where the intent is simply to expose the symbol rather than to rename
it in the object file.

Updates #31230.

Change-Id: Id06d9c4b2ec3d8e27f9b8a0d65212ab8048d734f
Reviewed-on: https://go-review.googlesource.com/c/go/+/179861
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit that referenced this issue Jun 6, 2019
This marks all Go symbols called from assembly in other packages with
"go:linkname" directives to ensure they get ABI wrappers.

Now that we have this go:linkname convention, this also removes the
abi0Syms definition in the runtime, which was used to give morestackc
an ABI0 wrapper. Instead, we now just mark morestackc with a
go:linkname directive.

This was tested with buildall.bash in the default configuration, with
-race, and with -gcflags=all=-d=ssa/intrinsics/off. Since I couldn't
test cgo on non-Linux configurations, I manually grepped for runtime
symbols in runtime/cgo.

Updates #31230.

Change-Id: I6c8aa56be2ca6802dfa2bf159e49c411b9071bf1
Reviewed-on: https://go-review.googlesource.com/c/go/+/179862
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/181077 mentions this issue: runtime/internal/atomic: export more ABI0 wrappers

@gopherbot
Copy link

Change https://golang.org/cl/181078 mentions this issue: runtime/internal/atomic: remove erroneous ABI wrappers

gopherbot pushed a commit that referenced this issue Jun 6, 2019
Somehow I missed these two functions in CL 179863. This should fix the
linux-arm builders.

Updates #31230.

Change-Id: I3f8bef3fac331b505a55c0850b0fbc799b7c06c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/181077
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Jun 6, 2019
CL 179862 introduced go:linkname directives to create ABI wrappers for
Store and Store64 on s390x, but a concurrent change (CL 180439)
replaced the Go definitions of these functions with assembly
definitions. This resulted in conflicting definitions for the ABI0
symbols, which led to a bootstrap linking failure.

Fix this by removing the now-incorrect go:linkname directives for
Store and Store64. This should fix the linux-s390x builders.

Updates #31230.

Change-Id: I8de8c03c23412fc217d428c0018cc56eb2f9996f
Reviewed-on: https://go-review.googlesource.com/c/go/+/181078
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/181079 mentions this issue: cmd/dist,cmd/compile: remove -allabis mode

gopherbot pushed a commit that referenced this issue Jun 7, 2019
dist passes the -allabis flag to the compiler to avoid having to
recreate the cross-package ABI logic from cmd/go. However, we removed
that logic from cmd/go in CL 179863 and replaced it with a different
mechanism that doesn't depend on the build system. Hence, passing
-allabis in dist is no longer necessary.

This CL removes -allabis from dist and, since that was the only use of
it, removes support for it from the compiler as well.

Updates #31230.

Change-Id: Ib005db95755a7028f49c885785e72c3970aea4f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/181079
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@golang golang locked and limited conversation to collaborators Jun 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants