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: list with -export and -covermode=atomic fails to build #65264

Closed
maxsours opened this issue Jan 24, 2024 · 9 comments
Closed

cmd/go: list with -export and -covermode=atomic fails to build #65264

maxsours opened this issue Jan 24, 2024 · 9 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@maxsours
Copy link

Go version

go version go1.21.6 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='~/Library/Caches/go-build'
GOENV='~/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='~/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='~/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.6'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/y8/kc0q4v_x1454fr__4l32j26w0000gp/T/go-build2219730342=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

$ go list -export -cover -race cmd/nm
# cmd/nm
/usr/local/go/src/cmd/nm/doc.go:41:37: could not import sync/atomic (open : no such file or directory)
cmd/nm

What did you see happen?

$ go list -export -cover -race cmd/nm
# cmd/nm
/usr/local/go/src/cmd/nm/doc.go:41:37: could not import sync/atomic (open : no such file or directory)
cmd/nm

What did you expect to see?

go build -cover -race works as expected. We would expect the go list command to succeed as well.
When running go list ... with -work, we can see sync/atomic is correctly added to the *.cover.go files, but it is not included in importcfg:

# import config
packagefile bufio=~/Library/Caches/go-build/b9/b98806a14c72980fa955342a2ffef07b871d95ac024cd8fd668021668be97816-d
packagefile cmd/internal/objfile=~/Library/Caches/go-build/b9/b9262c3ee809db084e17516b1c55810e3f3150e22831e4f4eafbf1a888855364-d
packagefile flag=~/Library/Caches/go-build/13/139eccd9f3144df8ee6480494307f2b06aa9bf19ba2f74cb4c7f9d14115d7819-d
packagefile fmt=~/Library/Caches/go-build/a3/a37fb0a0c11e799c4992ab511760fee037811f5481a1f7c4cf63ce7e08c3a0e6-d
packagefile log=~/Library/Caches/go-build/f1/f18a69602c944d8e4e1364a2d89a5c45cb101025319fb7c4cbe6a196fe249832-d
packagefile os=~/Library/Caches/go-build/d6/d64010892cba9d91f2fd56ee1177c4e1a48b5012f9ae1f58b8825412d853c590-d
packagefile sort=~/Library/Caches/go-build/0d/0daf4569d16ea11206c9465ae9f23f50a11e2ac546adbf32f8dc054a301e08d5-d
packagefile runtime=~/Library/Caches/go-build/e1/e1997716a043c5092d7ab1465a0a8e05c834335e26d446fedf0f9c4ac0c349d7-d
packagefile runtime/coverage=~/Library/Caches/go-build/e3/e30bc0cbef27b51d08c1ea7c44994cc02766dd15dd6435600a5ede665bb052c0-d
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go compiler/runtime Issues related to the Go compiler and/or runtime. labels Jan 24, 2024
@bcmills
Copy link
Contributor

bcmills commented Jan 24, 2024

cmd/go adds the implicit dependency on sync/atomic here:
https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/load/pkg.go;l=3507-3510;drc=36b14a78b58924a8aea22c8949c3b8a4b7045d8b

The call chain is via:

So the question is: is this a typestate bug (with the error happening before the call to PrepareForCoverageBuild, or is something in that call chain not reaching where we expect it to?

(attn @thanm)

@cixel
Copy link
Contributor

cixel commented Jan 25, 2024

My very hazy understanding is that it's an issue in runList:

EnsureImport updates a package's Internal.Imports and is adding the sync/atomic package.

This needs to happen before AutoAction creates the Action for cmd/nm, because it reads nm's Internal.Imports to populate Deps. These are what are used when printing the importcfg.

Locally, I was able to get the repro command to work by calling PrepareForCoverageBuild before creating the Action:

diff --git a/src/cmd/go/internal/list/list.go b/src/cmd/go/internal/list/list.go
index 92020da946..3fd697f3df 100644
--- a/src/cmd/go/internal/list/list.go
+++ b/src/cmd/go/internal/list/list.go
@@ -723,6 +723,9 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
                b.IsCmdList = true
                b.NeedExport = *listExport
                b.NeedCompiledGoFiles = *listCompiled
+               if cfg.Experiment.CoverageRedesign && cfg.BuildCover {
+                       load.PrepareForCoverageBuild(pkgs)
+               }
                a := &work.Action{}
                // TODO: Use pkgsFilter?
                for _, p := range pkgs {
@@ -730,9 +733,6 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
                                a.Deps = append(a.Deps, b.AutoAction(work.ModeInstall, work.ModeInstall, p))
                        }
                }
-               if cfg.Experiment.CoverageRedesign && cfg.BuildCover {
-                       load.PrepareForCoverageBuild(pkgs)
-               }
                b.Do(ctx, a)
        }

@mknyszek mknyszek added this to the Backlog milestone Jan 31, 2024
@mknyszek
Copy link
Contributor

In triage, I think we're waiting for @thanm to take a look. Don't mean to assign you work though, so feel free to comment and unassign. Thanks! :)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/560236 mentions this issue: cmd/go: fix build config before creating actions for 'go list -cover'

@cixel
Copy link
Contributor

cixel commented Feb 1, 2024

Change https://go.dev/cl/560236 mentions this issue: cmd/go: fix build config before creating actions for 'go list -cover'

I tried my hand at submitting a patch. It's quite small. Happy to take direction if my CL is incomplete or incorrect, or else close it and get out of the way to let a maintainer handle it. 😅

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
When -covermode is set to atomic, instrumented packages need to import
sync/atomic. If this is not already imported by a package being
instrumented, the build needs to ensure that sync/atomic is compiled
whenever 'go list' is run in a way that triggers package builds.

The build config was already being made to ensure the import, but only
after the action graph had been created, so there was no guarantee that
sync/atomic would be built when needed.

Fixes golang#65264.

Change-Id: Ib3f1e102ce2ef554ea08330d9db69a8c98790ac5
Reviewed-on: https://go-review.googlesource.com/c/go/+/560236
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Jun 27, 2024
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 27, 2024
@matloob
Copy link
Contributor

matloob commented Jun 27, 2024

@gopherbot please backport to Go 1.21 and Go 1.22

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #68221 (for 1.21), #68222 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/595495 mentions this issue: cmd/go: fix build config before creating actions for 'go list -cover'

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/595496 mentions this issue: [release-branch.go1.22] cmd/go: fix build config before creating actions for 'go list -cover'

gopherbot pushed a commit that referenced this issue Jul 10, 2024
…ons for 'go list -cover'

When -covermode is set to atomic, instrumented packages need to import
sync/atomic. If this is not already imported by a package being
instrumented, the build needs to ensure that sync/atomic is compiled
whenever 'go list' is run in a way that triggers package builds.

The build config was already being made to ensure the import, but only
after the action graph had been created, so there was no guarantee that
sync/atomic would be built when needed.

For #65264
For #68212
Fixes #68221

Change-Id: Ib3f1e102ce2ef554ea08330d9db69a8c98790ac5
Reviewed-on: https://go-review.googlesource.com/c/go/+/560236
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
(cherry picked from commit ac08c05)
Reviewed-on: https://go-review.googlesource.com/c/go/+/595495
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Sam Thanawalla <samthanawalla@google.com>
gopherbot pushed a commit that referenced this issue Jul 10, 2024
…ons for 'go list -cover'

When -covermode is set to atomic, instrumented packages need to import
sync/atomic. If this is not already imported by a package being
instrumented, the build needs to ensure that sync/atomic is compiled
whenever 'go list' is run in a way that triggers package builds.

The build config was already being made to ensure the import, but only
after the action graph had been created, so there was no guarantee that
sync/atomic would be built when needed.

For #65264.
For #68212
Fixes #68222

Change-Id: Ib3f1e102ce2ef554ea08330d9db69a8c98790ac5
Reviewed-on: https://go-review.googlesource.com/c/go/+/560236
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
(cherry picked from commit ac08c05)
Reviewed-on: https://go-review.googlesource.com/c/go/+/595496
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants