]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/cover: fix problems with "go test -covermode=atomic sync/atomic"
authorThan McIntosh <thanm@google.com>
Thu, 22 Dec 2022 19:10:12 +0000 (14:10 -0500)
committerThan McIntosh <thanm@google.com>
Thu, 5 Jan 2023 17:02:45 +0000 (17:02 +0000)
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>

src/cmd/cover/cover.go
src/cmd/go/internal/test/test.go
src/cmd/go/testdata/script/cover_sync_atomic_import.txt

index f4f225ef20859cfb02fa774f303d2d1dd54266f8..74bb500cb9d3296b99cfdfbdf1feaafc521e1062 100644 (file)
@@ -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 + "."
+}
index 0051970cfc65a3a2c7879e88a5bedaaedadde086..fe6e733538a3fa8a3b7d91a7f662736e9d2f5478 100644 (file)
@@ -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")
                }
 
index ee29bcbaba5fe3154b7657b71a3c8739e0e30048..b933cdb4c67fc27704e7fcdcc5d246d436892598 100644 (file)
@@ -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