From 61e5ea492905ba71c5c53ad27e72d042e60bb66a Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Thu, 26 Jan 2023 11:46:45 -0500 Subject: [PATCH] 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 Run-TryBot: Than McIntosh TryBot-Result: Gopher Robot --- src/runtime/coverage/apis.go | 12 +- src/runtime/coverage/emitdata_test.go | 197 ++++++++++++++++---------- 2 files changed, 131 insertions(+), 78 deletions(-) diff --git a/src/runtime/coverage/apis.go b/src/runtime/coverage/apis.go index 7d851f9362c1da..4366ef47ab41b1 100644 --- a/src/runtime/coverage/apis.go +++ b/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 -// the currently running program to the writer 'w'. An error will be +// WriteCounters writes coverage counter-data content for the +// 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 diff --git a/src/runtime/coverage/emitdata_test.go b/src/runtime/coverage/emitdata_test.go index 3839e4437f8d7a..f6c47e998d55bb 100644 --- a/src/runtime/coverage/emitdata_test.go +++ b/src/runtime/coverage/emitdata_test.go @@ -36,43 +36,60 @@ func TestCoverageApis(t *testing.T) { mkdir(t, dir) } - // Build harness. - bdir := mkdir(t, filepath.Join(dir, "build")) - hargs := []string{"-cover", "-coverpkg=all"} - if testing.CoverMode() != "" { - hargs = append(hargs, "-covermode="+testing.CoverMode()) + // Build harness. We need two copies of the harness, one built + // with -covermode=atomic and one built non-atomic. + bdir1 := mkdir(t, filepath.Join(dir, "build1")) + hargs1 := []string{"-covermode=atomic", "-coverpkg=all"} + 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) { - if testing.CoverMode() == "" { +func upmergeCoverData(t *testing.T, gocoverdir string, mode string) { + 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, rdir) + upmergeCoverData(t, edir, "atomic") + 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, rdir) + upmergeCoverData(t, edir, "atomic") + 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, rdir) + upmergeCoverData(t, edir, "atomic") + 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, rdir) + upmergeCoverData(t, edir, "atomic") + 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, rdir) + upmergeCoverData(t, edir, "atomic") + 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, rdir) + upmergeCoverData(t, edir, "atomic") + 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) - output, err := runHarness(t, nonatomicHarnessPath, tp, - setGoCoverDir, rdir1, edir1) - 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) + rdir, edir := mktestdirs(t, tag, tp, dir) + output, err := runHarness(t, harnessPath, tp, + setGoCoverDir, rdir, edir) 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) } - - if testing.CoverMode() == "atomic" { - upmergeCoverData(t, edir2) - upmergeCoverData(t, rdir2) - } else { - upmergeCoverData(t, edir1) - upmergeCoverData(t, rdir1) - } + upmergeCoverData(t, edir, "atomic") + upmergeCoverData(t, rdir, "atomic") }) } +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")