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 <thanm@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

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