runtime/coverage: restrict use of all counter-related APIs to atomic mode

The existing runtime/coverage API set includes a "ClearCounters()"
function that zeros out the counter values in a running process so as
enable capturing of a coverage profile from a specific execution time
segment. Calling this function is only permitted if the program is
built with "-covermode=atomic", due (in part) to concerns about
processors with relaxed memory models in which normal stores can be
reordered.

In the bug in question, a test that stresses a different set of
counter-related APIs was hitting an invalid counter segment when
running on a machine (ppc64) which does indeed have a relaxed memory
consistency model.

From a post-mortem examination of the counter array for the harness
from the ppc64 test run, it was clear that the thread reading values
from the counter array was seeing the sort of inconsistency that could
result from stores being reordered (specifically the prolog
"packageID" and "number-of-counters" stores).

To preclude the possibility of future similar problems, this patch
extends the "atomic mode only" restriction from ClearCounters to the
other APIs that deal with counters (WriteCounters, WriteCountersDir).

Fixes #56197.

Change-Id: Idb85d67a84d69ead508e0902ab46ab4dc82af466
Reviewed-on: https://go-review.googlesource.com/c/go/+/463695
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

src/runtime/coverage/apis.go

@@ -50,11 +50,14 @@ func WriteMeta(w io.Writer) error {
 // counter data written will be a snapshot taken at the point of the
 // call.
 func WriteCountersDir(dir string) error {
+	if cmode != coverage.CtrModeAtomic {
+		return fmt.Errorf("WriteCountersDir invoked for program built with -covermode=%s (please use -covermode=atomic)", cmode.String())
+	}
 	return emitCounterDataToDirectory(dir)
 }
 
-// WriteCounters writes coverage counter-data content for
+// WriteCounters writes coverage counter-data content for the
-// the currently running program to the writer 'w'. An error will be
+// currently running program to the writer 'w'. An error will be
 // returned if the operation can't be completed successfully (for
 // example, if the currently running program was not built with
 // "-cover", or if a write fails). The counter data written will be a
@@ -63,6 +66,9 @@ func WriteCounters(w io.Writer) error {
 	if w == nil {
 		return fmt.Errorf("error: nil writer in WriteCounters")
 	}
+	if cmode != coverage.CtrModeAtomic {
+		return fmt.Errorf("WriteCounters invoked for program built with -covermode=%s (please use -covermode=atomic)", cmode.String())
+	}
 	// Ask the runtime for the list of coverage counter symbols.
 	cl := getCovCounterList()
 	if len(cl) == 0 {
@@ -92,7 +98,7 @@ func ClearCounters() error {
 		return fmt.Errorf("program not built with -cover")
 	}
 	if cmode != coverage.CtrModeAtomic {
-		return fmt.Errorf("ClearCounters invoked for program build with -covermode=%s (please use -covermode=atomic)", cmode.String())
+		return fmt.Errorf("ClearCounters invoked for program built with -covermode=%s (please use -covermode=atomic)", cmode.String())
 	}
 
 	// Implementation note: this function would be faster and simpler

src/runtime/coverage/emitdata_test.go

@@ -36,43 +36,60 @@ func TestCoverageApis(t *testing.T) {
 		mkdir(t, dir)
 	}
 
-	// Build harness.
+	// Build harness. We need two copies of the harness, one built
-	bdir := mkdir(t, filepath.Join(dir, "build"))
+	// with -covermode=atomic and one built non-atomic.
-	hargs := []string{"-cover", "-coverpkg=all"}
+	bdir1 := mkdir(t, filepath.Join(dir, "build1"))
-	if testing.CoverMode() != "" {
+	hargs1 := []string{"-covermode=atomic", "-coverpkg=all"}
-		hargs = append(hargs, "-covermode="+testing.CoverMode())
+	atomicHarnessPath := buildHarness(t, bdir1, hargs1)
+	nonAtomicMode := testing.CoverMode()
+	if testing.CoverMode() == "atomic" {
+		nonAtomicMode = "set"
 	}
-	harnessPath := buildHarness(t, bdir, hargs)
+	bdir2 := mkdir(t, filepath.Join(dir, "build2"))
+	hargs2 := []string{"-coverpkg=all", "-covermode=" + nonAtomicMode}
+	nonAtomicHarnessPath := buildHarness(t, bdir2, hargs2)
 
-	t.Logf("harness path is %s", harnessPath)
+	t.Logf("atomic harness path is %s", atomicHarnessPath)
+	t.Logf("non-atomic harness path is %s", nonAtomicHarnessPath)
 
 	// Sub-tests for each API we want to inspect, plus
 	// extras for error testing.
 	t.Run("emitToDir", func(t *testing.T) {
 		t.Parallel()
-		testEmitToDir(t, harnessPath, dir)
+		testEmitToDir(t, atomicHarnessPath, dir)
 	})
 	t.Run("emitToWriter", func(t *testing.T) {
 		t.Parallel()
-		testEmitToWriter(t, harnessPath, dir)
+		testEmitToWriter(t, atomicHarnessPath, dir)
 	})
 	t.Run("emitToNonexistentDir", func(t *testing.T) {
 		t.Parallel()
-		testEmitToNonexistentDir(t, harnessPath, dir)
+		testEmitToNonexistentDir(t, atomicHarnessPath, dir)
 	})
 	t.Run("emitToNilWriter", func(t *testing.T) {
 		t.Parallel()
-		testEmitToNilWriter(t, harnessPath, dir)
+		testEmitToNilWriter(t, atomicHarnessPath, dir)
 	})
 	t.Run("emitToFailingWriter", func(t *testing.T) {
 		t.Parallel()
-		testEmitToFailingWriter(t, harnessPath, dir)
+		testEmitToFailingWriter(t, atomicHarnessPath, dir)
 	})
 	t.Run("emitWithCounterClear", func(t *testing.T) {
 		t.Parallel()
-		testEmitWithCounterClear(t, harnessPath, dir)
+		testEmitWithCounterClear(t, atomicHarnessPath, dir)
+	})
+	t.Run("emitToDirNonAtomic", func(t *testing.T) {
+		t.Parallel()
+		testEmitToDirNonAtomic(t, nonAtomicHarnessPath, nonAtomicMode, dir)
+	})
+	t.Run("emitToWriterNonAtomic", func(t *testing.T) {
+		t.Parallel()
+		testEmitToWriterNonAtomic(t, nonAtomicHarnessPath, nonAtomicMode, dir)
+	})
+	t.Run("emitWithCounterClearNonAtomic", func(t *testing.T) {
+		t.Parallel()
+		testEmitWithCounterClearNonAtomic(t, nonAtomicHarnessPath, nonAtomicMode, dir)
 	})
-
 }
 
 // upmergeCoverData helps improve coverage data for this package
@@ -82,8 +99,8 @@ func TestCoverageApis(t *testing.T) {
 // run from the "harness.exe" runs we've just done. We can accomplish
 // this by doing a merge from the harness gocoverdir's to the test
 // gocoverdir.
-func upmergeCoverData(t *testing.T, gocoverdir string) {
+func upmergeCoverData(t *testing.T, gocoverdir string, mode string) {
-	if testing.CoverMode() == "" {
+	if testing.CoverMode() != mode {
 		return
 	}
 	testGoCoverDir := os.Getenv("GOCOVERDIR")
@@ -243,8 +260,8 @@ func testEmitToDir(t *testing.T, harnessPath string, dir string) {
 		if cdc != wantcf {
 			t.Errorf("EmitToDir: want %d counter-data files, got %d\n", wantcf, cdc)
 		}
-		upmergeCoverData(t, edir)
+		upmergeCoverData(t, edir, "atomic")
-		upmergeCoverData(t, rdir)
+		upmergeCoverData(t, rdir, "atomic")
 	})
 }
 
@@ -262,8 +279,8 @@ func testEmitToWriter(t *testing.T, harnessPath string, dir string) {
 		if msg := testForSpecificFunctions(t, edir, want, avoid); msg != "" {
 			t.Errorf("coverage data from %q output match failed: %s", tp, msg)
 		}
-		upmergeCoverData(t, edir)
+		upmergeCoverData(t, edir, "atomic")
-		upmergeCoverData(t, rdir)
+		upmergeCoverData(t, rdir, "atomic")
 	})
 }
 
@@ -276,8 +293,8 @@ func testEmitToNonexistentDir(t *testing.T, harnessPath string, dir string) {
 			t.Logf("%s", output)
 			t.Fatalf("running 'harness -tp %s': %v", tp, err)
 		}
-		upmergeCoverData(t, edir)
+		upmergeCoverData(t, edir, "atomic")
-		upmergeCoverData(t, rdir)
+		upmergeCoverData(t, rdir, "atomic")
 	})
 }
 
@@ -298,8 +315,8 @@ func testEmitToUnwritableDir(t *testing.T, harnessPath string, dir string) {
 			t.Logf("%s", output)
 			t.Fatalf("running 'harness -tp %s': %v", tp, err)
 		}
-		upmergeCoverData(t, edir)
+		upmergeCoverData(t, edir, "atomic")
-		upmergeCoverData(t, rdir)
+		upmergeCoverData(t, rdir, "atomic")
 	})
 }
 
@@ -312,8 +329,8 @@ func testEmitToNilWriter(t *testing.T, harnessPath string, dir string) {
 			t.Logf("%s", output)
 			t.Fatalf("running 'harness -tp %s': %v", tp, err)
 		}
-		upmergeCoverData(t, edir)
+		upmergeCoverData(t, edir, "atomic")
-		upmergeCoverData(t, rdir)
+		upmergeCoverData(t, rdir, "atomic")
 	})
 }
 
@@ -326,73 +343,103 @@ func testEmitToFailingWriter(t *testing.T, harnessPath string, dir string) {
 			t.Logf("%s", output)
 			t.Fatalf("running 'harness -tp %s': %v", tp, err)
 		}
-		upmergeCoverData(t, edir)
+		upmergeCoverData(t, edir, "atomic")
-		upmergeCoverData(t, rdir)
+		upmergeCoverData(t, rdir, "atomic")
 	})
 }
 
 func testEmitWithCounterClear(t *testing.T, harnessPath string, dir string) {
-	// Ensure that we have two versions of the harness: one built with
-	// -covermode=atomic and one built with -covermode=set (we need
-	// both modes to test all of the functionality).
-	var nonatomicHarnessPath, atomicHarnessPath string
-	if testing.CoverMode() != "atomic" {
-		nonatomicHarnessPath = harnessPath
-		bdir2 := mkdir(t, filepath.Join(dir, "build2"))
-		hargs := []string{"-covermode=atomic", "-coverpkg=all"}
-		atomicHarnessPath = buildHarness(t, bdir2, hargs)
-	} else {
-		atomicHarnessPath = harnessPath
-		mode := "set"
-		if testing.CoverMode() != "" && testing.CoverMode() != "atomic" {
-			mode = testing.CoverMode()
-		}
-		// Build a special nonatomic covermode version of the harness
-		// (we need both modes to test all of the functionality).
-		bdir2 := mkdir(t, filepath.Join(dir, "build2"))
-		hargs := []string{"-covermode=" + mode, "-coverpkg=all"}
-		nonatomicHarnessPath = buildHarness(t, bdir2, hargs)
-	}
-
 	withAndWithoutRunner(func(setGoCoverDir bool, tag string) {
-		// First a run with the nonatomic harness path, which we
-		// expect to fail.
 		tp := "emitWithCounterClear"
-		rdir1, edir1 := mktestdirs(t, tag, tp+"1", dir)
+		rdir, edir := mktestdirs(t, tag, tp, dir)
-		output, err := runHarness(t, nonatomicHarnessPath, tp,
+		output, err := runHarness(t, harnessPath, tp,
-			setGoCoverDir, rdir1, edir1)
+			setGoCoverDir, rdir, edir)
-		if err == nil {
-			t.Logf("%s", output)
-			t.Fatalf("running '%s -tp %s': unexpected success",
-				nonatomicHarnessPath, tp)
-		}
-
-		// Next a run with the atomic harness path, which we
-		// expect to succeed.
-		rdir2, edir2 := mktestdirs(t, tag, tp+"2", dir)
-		output, err = runHarness(t, atomicHarnessPath, tp,
-			setGoCoverDir, rdir2, edir2)
 		if err != nil {
 			t.Logf("%s", output)
 			t.Fatalf("running 'harness -tp %s': %v", tp, err)
 		}
 		want := []string{tp, "postClear"}
 		avoid := []string{"preClear", "main", "final"}
-		if msg := testForSpecificFunctions(t, edir2, want, avoid); msg != "" {
+		if msg := testForSpecificFunctions(t, edir, want, avoid); msg != "" {
 			t.Logf("%s", output)
 			t.Errorf("coverage data from %q output match failed: %s", tp, msg)
 		}
-
+		upmergeCoverData(t, edir, "atomic")
-		if testing.CoverMode() == "atomic" {
+		upmergeCoverData(t, rdir, "atomic")
-			upmergeCoverData(t, edir2)
-			upmergeCoverData(t, rdir2)
-		} else {
-			upmergeCoverData(t, edir1)
-			upmergeCoverData(t, rdir1)
-		}
 	})
 }
 
+func testEmitToDirNonAtomic(t *testing.T, harnessPath string, naMode string, dir string) {
+	tp := "emitToDir"
+	tag := "nonatomdir"
+	rdir, edir := mktestdirs(t, tag, tp, dir)
+	output, err := runHarness(t, harnessPath, tp,
+		true, rdir, edir)
+
+	// We expect an error here.
+	if err == nil {
+		t.Logf("%s", output)
+		t.Fatalf("running 'harness -tp %s': did not get expected error", tp)
+	}
+
+	got := strings.TrimSpace(string(output))
+	want := "WriteCountersDir invoked for program built"
+	if !strings.Contains(got, want) {
+		t.Errorf("running 'harness -tp %s': got:\n%s\nwant: %s",
+			tp, got, want)
+	}
+	upmergeCoverData(t, edir, naMode)
+	upmergeCoverData(t, rdir, naMode)
+}
+
+func testEmitToWriterNonAtomic(t *testing.T, harnessPath string, naMode string, dir string) {
+	tp := "emitToWriter"
+	tag := "nonatomw"
+	rdir, edir := mktestdirs(t, tag, tp, dir)
+	output, err := runHarness(t, harnessPath, tp,
+		true, rdir, edir)
+
+	// We expect an error here.
+	if err == nil {
+		t.Logf("%s", output)
+		t.Fatalf("running 'harness -tp %s': did not get expected error", tp)
+	}
+
+	got := strings.TrimSpace(string(output))
+	want := "WriteCounters invoked for program built"
+	if !strings.Contains(got, want) {
+		t.Errorf("running 'harness -tp %s': got:\n%s\nwant: %s",
+			tp, got, want)
+	}
+
+	upmergeCoverData(t, edir, naMode)
+	upmergeCoverData(t, rdir, naMode)
+}
+
+func testEmitWithCounterClearNonAtomic(t *testing.T, harnessPath string, naMode string, dir string) {
+	tp := "emitWithCounterClear"
+	tag := "cclear"
+	rdir, edir := mktestdirs(t, tag, tp, dir)
+	output, err := runHarness(t, harnessPath, tp,
+		true, rdir, edir)
+
+	// We expect an error here.
+	if err == nil {
+		t.Logf("%s", output)
+		t.Fatalf("running 'harness -tp %s' nonatomic: did not get expected error", tp)
+	}
+
+	got := strings.TrimSpace(string(output))
+	want := "ClearCounters invoked for program built"
+	if !strings.Contains(got, want) {
+		t.Errorf("running 'harness -tp %s': got:\n%s\nwant: %s",
+			tp, got, want)
+	}
+
+	upmergeCoverData(t, edir, naMode)
+	upmergeCoverData(t, rdir, naMode)
+}
+
 func TestApisOnNocoverBinary(t *testing.T) {
 	if testing.Short() {
 		t.Skipf("skipping test: too long for short mode")