-
Notifications
You must be signed in to change notification settings - Fork 18k
cmd/go: data race on (*load.PackageInternal).Gcflags field #31230
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
Comments
Note to future self: nothing from |
@aclements, could you take a look at The race is triggered by
Is there another way to implement this that avoids reading those files inside an action? |
@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 |
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:
I'm kind of leaning toward 3 as the least-bad solution. |
|
Change https://golang.org/cl/179798 mentions this issue: |
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". |
That fix sounds much better than my hack! :) Do you think that will land before the beta? If so, I'll abandon my CL. |
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. |
Change https://golang.org/cl/179861 mentions this issue: |
Change https://golang.org/cl/179860 mentions this issue: |
Change https://golang.org/cl/179862 mentions this issue: |
Change https://golang.org/cl/179863 mentions this issue: |
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>
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>
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>
Change https://golang.org/cl/181077 mentions this issue: |
Change https://golang.org/cl/181078 mentions this issue: |
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>
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>
Change https://golang.org/cl/181079 mentions this issue: |
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>
The race was observed at head with
go test -race cmd/go
, but doesn't always reproduce.The text was updated successfully, but these errors were encountered: