From d50ea217f6f5668d6a89fb965e36ecf8564a45cf Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Thu, 22 Dec 2022 14:10:12 -0500 Subject: [PATCH] cmd/cover: fix problems with "go test -covermode=atomic sync/atomic" This patch fixes an elderly bug with "go test -covermode=atomic sync/atomic". Change the cover tool to avoid adding an import of sync/atomic when processing "sync/atomic" itself in atomic mode; instead make direct calls to AddUint32/StoreUint32. In addition, change the go command to avoid injecting an artificial import of "sync/atomic" for sync/atomic itself. Fixes #57445. Change-Id: I8c8fbd0bcf26c8a8607d4806046f826296508c74 Reviewed-on: https://go-review.googlesource.com/c/go/+/459335 Run-TryBot: Than McIntosh Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot --- src/cmd/cover/cover.go | 52 ++++++++++++++++--- src/cmd/go/internal/test/test.go | 7 ++- .../script/cover_sync_atomic_import.txt | 15 ++++++ 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/src/cmd/cover/cover.go b/src/cmd/cover/cover.go index f4f225ef20..74bb500cb9 100644 --- a/src/cmd/cover/cover.go +++ b/src/cmd/cover/cover.go @@ -382,8 +382,23 @@ func (f *File) Visit(node ast.Node) ast.Visitor { if n.Name.Name == "_" || n.Body == nil { return nil } - // Determine proper function or method name. fname := n.Name.Name + // Skip AddUint32 and StoreUint32 if we're instrumenting + // sync/atomic itself in atomic mode (out of an abundance of + // caution), since as part of the instrumentation process we + // add calls to AddUint32/StoreUint32, and we don't want to + // somehow create an infinite loop. + // + // Note that in the current implementation (Go 1.20) both + // routines are assembly stubs that forward calls to the + // runtime/internal/atomic equivalents, hence the infinite + // loop scenario is purely theoretical (maybe if in some + // future implementation one of these functions might be + // written in Go). See #57445 for more details. + if atomicOnAtomic() && (fname == "AddUint32" || fname == "StoreUint32") { + return nil + } + // Determine proper function or method name. if r := n.Recv; r != nil && len(r.List) == 1 { t := r.List[0].Type star := "" @@ -508,8 +523,8 @@ func (f *File) postFunc(fn ast.Node, funcname string, flit bool, body *ast.Block } if *mode == "atomic" { hookWrite = func(cv string, which int, val string) string { - return fmt.Sprintf("%s.StoreUint32(&%s[%d], %s)", atomicPackageName, - cv, which, val) + return fmt.Sprintf("%sStoreUint32(&%s[%d], %s)", + atomicPackagePrefix(), cv, which, val) } } @@ -612,9 +627,13 @@ func (p *Package) annotateFile(name string, fd io.Writer, last bool) { // We do this even if there is an existing import, because the // existing import may be shadowed at any given place we want // to refer to it, and our name (_cover_atomic_) is less likely to - // be shadowed. - file.edit.Insert(file.offset(file.astFile.Name.End()), - fmt.Sprintf("; import %s %q", atomicPackageName, atomicPackagePath)) + // be shadowed. The one exception is if we're visiting the + // sync/atomic package itself, in which case we can refer to + // functions directly without an import prefix. See also #57445. + if pkgconfig.PkgPath != "sync/atomic" { + file.edit.Insert(file.offset(file.astFile.Name.End()), + fmt.Sprintf("; import %s %q", atomicPackageName, atomicPackagePath)) + } } if pkgconfig.PkgName == "main" { file.edit.Insert(file.offset(file.astFile.Name.End()), @@ -637,7 +656,7 @@ func (p *Package) annotateFile(name string, fd io.Writer, last bool) { // Emit a reference to the atomic package to avoid // import and not used error when there's no code in a file. if *mode == "atomic" { - fmt.Fprintf(fd, "var _ = %s.LoadUint32\n", atomicPackageName) + fmt.Fprintf(fd, "var _ = %sLoadUint32\n", atomicPackagePrefix()) } // Last file? Emit meta-data and converage config. @@ -658,7 +677,7 @@ func incCounterStmt(f *File, counter string) string { // atomicCounterStmt returns the expression: atomic.AddUint32(&__count[23], 1) func atomicCounterStmt(f *File, counter string) string { - return fmt.Sprintf("%s.AddUint32(&%s, 1)", atomicPackageName, counter) + return fmt.Sprintf("%sAddUint32(&%s, 1)", atomicPackagePrefix(), counter) } // newCounter creates a new counter expression of the appropriate form. @@ -1098,3 +1117,20 @@ func (p *Package) emitMetaData(w io.Writer) { log.Fatalf("error writing %s: %v", pkgconfig.OutConfig, err) } } + +// atomicOnAtomic returns true if we're instrumenting +// the sync/atomic package AND using atomic mode. +func atomicOnAtomic() bool { + return *mode == "atomic" && pkgconfig.PkgPath == "sync/atomic" +} + +// atomicPackagePrefix returns the import path prefix used to refer to +// our special import of sync/atomic; this is either set to the +// constant atomicPackageName plus a dot or the empty string if we're +// instrumenting the sync/atomic package itself. +func atomicPackagePrefix() string { + if atomicOnAtomic() { + return "" + } + return atomicPackageName + "." +} diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 0051970cfc..fe6e733538 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -825,8 +825,11 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) { // Prepare build + run + print actions for all packages being tested. for _, p := range pkgs { - // sync/atomic import is inserted by the cover tool. See #18486 - if cfg.BuildCover && cfg.BuildCoverMode == "atomic" { + // sync/atomic import is inserted by the cover tool if we're + // using atomic mode (and not compiling sync/atomic package itself). + // See #18486 and #57445. + if cfg.BuildCover && cfg.BuildCoverMode == "atomic" && + p.ImportPath != "sync/atomic" { load.EnsureImport(p, "sync/atomic") } diff --git a/src/cmd/go/testdata/script/cover_sync_atomic_import.txt b/src/cmd/go/testdata/script/cover_sync_atomic_import.txt index ee29bcbaba..b933cdb4c6 100644 --- a/src/cmd/go/testdata/script/cover_sync_atomic_import.txt +++ b/src/cmd/go/testdata/script/cover_sync_atomic_import.txt @@ -3,6 +3,21 @@ go test -short -cover -covermode=atomic -coverpkg=coverdep/p1 coverdep +# In addition to the above, test to make sure there is no funny +# business if we try "go test -cover" in atomic mode targeting +# sync/atomic itself (see #57445). Just a short test run is needed +# since we're mainly interested in making sure the test builds and can +# execute at least one test. + +go test -short -covermode=atomic -run=TestStoreInt64 sync/atomic +go test -short -covermode=atomic -run=TestAnd8 runtime/internal/atomic + +# Skip remainder if no race detector support. +[!race] skip + +go test -short -cover -race -run=TestStoreInt64 sync/atomic +go test -short -cover -race -run=TestAnd8 runtime/internal/atomic + -- go.mod -- module coverdep