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/internal/list: may fail if -export -covermode=atomic is used #68212

Open
RomainMuller opened this issue Jun 27, 2024 · 7 comments
Open
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. gabywins

Comments

@RomainMuller
Copy link

Go version

go version go1.22.4 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE='auto'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/romain.marcadier/Library/Caches/go-build'
GOENV='/Users/romain.marcadier/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/romain.marcadier/go/pkg/mod'
GONOPROXY='github.com/DataDog'
GONOSUMDB='github.com/DataDog,go.ddbuild.io'
GOOS='darwin'
GOPATH='/Users/romain.marcadier/go'
GOPRIVATE='github.com/DataDog'
GOPROXY='binaries.ddbuild.io,proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.4/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.4/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.4'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/romain.marcadier/Development/Datadog/orchestrion/go.mod'
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/qh/q2mpbd0j6xsb7vc8dqzdm81h0000gn/T/go-build3755867500=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Trying to use go list to obtain the exports for a given package and its dependencies; possibly built with coverage instrumentation... Use a command similar to the following (io being a standard library package that runs into the issue, but this happens with virtually any other package that does not "already" depend on sync/atomic):

$ go list -cover -covermode=atomic -coverpkg=io -deps -export -json io

What did you see happen?

Removed -deps and -json arguments as they are largely irrelevant to the issue; but basically:

$ go list -cover -covermode=atomic -coverpkg=io -export io
# io
/opt/homebrew/Cellar/go/1.22.4/libexec/src/io/io.go:13:35: could not import sync/atomic (open : no such file or directory)
io
$ echo $?
1

What did you expect to see?

I'd have expected compilation to succeed and exports to be returned (the go build equivalent command is successful):

$ go list -cover -covermode=atomic -coverpkg=io -deps -export -json io
io
$ echo $?
0
@RomainMuller
Copy link
Author

I'm observing this issue with all go releases tested so far (1.21, 1.22, 1.23-rc), and expect it is present on older releases, too.

Investigations so far (superficially) suggest that:

  • go build would update the -importcfg file passed to the compile command to include sync/atomic when building with -covermode=atomic
  • go list on the other hand fails to do this, resulting in it failing to produce exports for packages that don't depend on sync/atomic before they're instrumented.

This is the same as #65264, which has been closed/resolved on Feb 1st, so I was expecting the fix to be released already?

@RomainMuller
Copy link
Author

Oh my bad, it's fixed in 1.23.0-rc.1 apparently (my tests are failing for another reason).

I'm okay with this being closed; however I'd like to understand if there is any plan to back-port this fix to 1.21 and/or 1.22...

RomainMuller added a commit to DataDog/orchestrion that referenced this issue Jun 27, 2024
@rsc
Copy link
Contributor

rsc commented Jun 27, 2024

The fix in ac08c05 looks simple enough to backport to me.

/cc @matloob @samthanawalla

@matloob
Copy link
Contributor

matloob commented Jun 27, 2024

Okay, created backport issues #68221 and #68222

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

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

@joedian joedian added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Jun 28, 2024
@rsc rsc added the gabywins label Jun 28, 2024
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
FixPending Issues that have a fix which has not yet been reviewed or submitted. gabywins
Projects
None yet
Development

No branches or pull requests

6 participants