From 67403a30f61ac6af584796cdfd45c0d86820690c Mon Sep 17 00:00:00 2001
From: Than McIntosh <thanm@google.com>
Date: Fri, 21 Oct 2022 09:18:44 -0400
Subject: [PATCH] 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>
---
 src/cmd/cover/cover.go                        | 27 ++++++++--
 .../script/cover_test_race_issue56370.txt     | 54 +++++++++++++++++++
 2 files changed, 76 insertions(+), 5 deletions(-)
 create mode 100644 src/cmd/go/testdata/script/cover_test_race_issue56370.txt

diff --git a/src/cmd/cover/cover.go b/src/cmd/cover/cover.go
index 5be64d25c0..60cfcb5bc2 100644
--- 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
index 0000000000..2e55f10087
--- /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()
+}
+
-- 
2.50.0