cmd/cover: fix problem with race mode and inlining

This patch fixes a problem in which we can get a data race on a
coverage counter function registration sequence. The scenario is that
package P contains a function F that is built with coverage, then F is
inlined into some other package that isn't being instrumented. Within
F's exported function body counter updates were being done with
atomics, but the initial registration sequence was not, which had the
potential to trigger a race. Fix: if race mode is enabled and we're
using atomics for counter increments, also use atomics in the
registration sequence.

Fixes #56370.

Change-Id: If274b61714b90275ff95fc6529239e9264b0ab0c
Reviewed-on: https://go-review.googlesource.com/c/go/+/444617
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Than McIntosh <thanm@google.com>

diff --git a/src/cmd/cover/cover.go b/src/cmd/cover/cover.go
index 5be64d25c06be56d8c4018420a1ad87c6fda4693..60cfcb5bc21b67c5ae37bd916cdcb2236412336d 100644 (file)
--- a/src/cmd/cover/cover.go
+++ b/src/cmd/cover/cover.go
@@ -20,6 +20,7 @@ import (
        "os"
        "path/filepath"
        "sort"
+       "strconv"
        "strings"
 
        "cmd/internal/edit"
@@ -483,13 +484,29 @@ func (f *File) postFunc(fn ast.Node, funcname string, flit bool, body *ast.Block
        }
        funcId := f.mdb.AddFunc(fd)
 
-       // Generate the registration hook for the function, and insert it
-       // into the prolog.
+       hookWrite := func(cv string, which int, val string) string {
+               return fmt.Sprintf("%s[%d] = %s", cv, which, val)
+       }
+       if *mode == "atomic" {
+               hookWrite = func(cv string, which int, val string) string {
+                       return fmt.Sprintf("%s.StoreUint32(&%s[%d], %s)", atomicPackageName,
+                               cv, which, val)
+               }
+       }
+
+       // Generate the registration hook sequence for the function. This
+       // sequence looks like
+       //
+       //   counterVar[0] = <num_units>
+       //   counterVar[1] = pkgId
+       //   counterVar[2] = fnId
+       //
        cv := f.fn.counterVar
-       regHook := fmt.Sprintf("%s[0] = %d ; %s[1] = %s ; %s[2] = %d",
-               cv, len(f.fn.units), cv, mkPackageIdExpression(), cv, funcId)
+       regHook := hookWrite(cv, 0, strconv.Itoa(len(f.fn.units))) + " ; " +
+               hookWrite(cv, 1, mkPackageIdExpression()) + " ; " +
+               hookWrite(cv, 2, strconv.Itoa(int(funcId)))
 
-       // Insert a function registration sequence into the function.
+       // Insert the registration sequence into the function.
        boff := f.offset(body.Pos())
        ipos := f.fset.File(body.Pos()).Pos(boff + 1)
        f.edit.Insert(f.offset(ipos), regHook+" ; ")

diff --git a/src/cmd/go/testdata/script/cover_test_race_issue56370.txt b/src/cmd/go/testdata/script/cover_test_race_issue56370.txt
new file mode 100644 (file)
index 0000000..2e55f10
--- /dev/null
+++ b/src/cmd/go/testdata/script/cover_test_race_issue56370.txt
@@ -0,0 +1,54 @@
+[short] skip
+[!race] skip
+
+go test -race -cover issue.56370/filter
+
+-- go.mod --
+module issue.56370
+
+go 1.20
+
+-- filter/filter.go --
+
+package filter
+
+func New() func(error) bool {
+       return func(error) bool {
+               return false
+       }
+}
+
+-- filter/filter_test.go --
+
+package filter_test
+
+import (
+       "testing"
+
+       "issue.56370/filter"
+)
+
+func Test1(t *testing.T) {
+       t.Parallel()
+
+       _ = filter.New()
+}
+
+func Test2(t *testing.T) {
+       t.Parallel()
+
+       _ = filter.New()
+}
+
+func Test3(t *testing.T) {
+       t.Parallel()
+
+       _ = filter.New()
+}
+
+func Test4(t *testing.T) {
+       t.Parallel()
+
+       _ = filter.New()
+}
+