Skip to content

Commit

Permalink
runtime/coverage: restrict use of all counter-related APIs to atomic …
Browse files Browse the repository at this point in the history 
…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 <[email protected]>
Run-TryBot: Than McIntosh <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
  • Loading branch information
@thanm
thanm committed Mar 1, 2023
1 parent d20e688 commit 61e5ea4
Show file tree
Hide file tree
Showing 2 changed files with 131 additions and 78 deletions.
12 changes: 9 additions & 3 deletions src/runtime/coverage/apis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
197 changes: 122 additions & 75 deletions src/runtime/coverage/emitdata_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
Expand Down Expand Up @@ -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")
})
}

Expand All @@ -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")
})
}

Expand All @@ -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")
})
}

Expand All @@ -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")
})
}

Expand All @@ -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")
})
}

Expand All @@ -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")
Expand Down

0 comments on commit 61e5ea4

Please sign in to comment.