cmd/go: fix percent covered problems with -coverpkg

This patch fixes some problems with how "go test -cover" was handling
tests involving A) multiple package tests and B) multiple packages
matched by "-coverpkg". In such scenarios the expectation is that the
percent statements covered metric for each package needs to be
relative to all statements in all packages matched by the -coverpkg
arg (this aspect of the reporting here was broken as part of
GOEXPERIMENT=coverageredesign).

The new scheme works as follows.  If -coverpkg is in effect and is
matching multiple packages, and we have multiple test targets, then:

  - each time a package is built for coverage, capture a meta-data
    file fragment corresponding to just the meta-data for that package.

  - create a new "writeCoverMeta" action, and interpose it between the
    build actions for the covered packages and the run actions. The
    "writeCoverMeta" action at runtime will emit a file
    "metafiles.txt" containing a table mapping each covered package
    (by import path) to its corresponding meta-data file fragment.

  - pass in the "metafiles.txt" file to each run action, so that
    when the test finishes running it will have an accurate picture
    of _all_ covered packages, permitting it to calculate the correct
    percentage.

Concrete example: suppose we have a top level directory with three
package subdirs, "a", "b", and "c", and from the top level, a user
runs "go test -coverpkg=./... ./...". This will result in (roughly)
the following action graph:

  build("a")       build("b")         build("c")
      |               |                   |
  link("a.test")   link("b.test")     link("c.test")
      |               |                   |
  run("a.test")    run("b.test")      run("c.test")
      |               |                   |
    print          print              print

With the new scheme, the action graph is augmented with a
writeCoverMeta action and additional dependence edges to form

  build("a")       build("b")         build("c")
      |   \       /   |               /   |
      |    v     v    |              /    |
      | writecovmeta<-|-------------+     |
      |         |||   |                   |
      |         ||\   |                   |
  link("a.test")/\ \  link("b.test")      link("c.test")
      |        /  \ +-|--------------+    |
      |       /    \  |               \   |
      |      v      v |                v  |
  run("a.test")    run("b.test")      run("c.test")
      |               |                   |
    print          print              print

A note on init functions: prior to GOEXPERIMENT=coverageredesign
the "-coverpkg=..." flag was implemented by force-importing
all packages matched by "-coverpkg" into each instrumented package.
This meant that for the example above, when executing "a.test",
the init function for package "c" would fire even if package "a"
did not ordinarily import package "c".  The new implementation
does not do this sort of forced importing, meaning that the coverage
percentages can be slightly different between 1.21 and 1.19 if
there are user-written init funcs.

Fixes #58770.
Updates #24570.

Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest
Change-Id: I7749ed205dce81b96ad7f74ab98bc1e90e377302
Reviewed-on: https://go-review.googlesource.com/c/go/+/495452
Reviewed-by: Bryan Mills <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: thanm

src/cmd/go/internal/test/test.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"internal/coverage"
 	"internal/platform"
 	"io"
 	"io/fs"
@@ -848,6 +849,7 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
 	}()
 
 	var builds, runs, prints []*work.Action
+	var writeCoverMetaAct *work.Action
 
 	if cfg.BuildCoverPkg != nil {
 		match := make([]func(*load.Package) bool, len(cfg.BuildCoverPkg))
@@ -859,6 +861,61 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
 		// patterns.
 		plist := load.TestPackageList(ctx, pkgOpts, pkgs)
 		testCoverPkgs = load.SelectCoverPackages(plist, match, "test")
+		if cfg.Experiment.CoverageRedesign && len(testCoverPkgs) > 0 {
+			// create a new singleton action that will collect up the
+			// meta-data files from all of the packages mentioned in
+			// "-coverpkg" and write them to a summary file. This new
+			// action will depend on all the build actions for the
+			// test packages, and all the run actions for these
+			// packages will depend on it. Motivating example:
+			// supposed we have a top level directory with three
+			// package subdirs, "a", "b", and "c", and
+			// from the top level, a user runs "go test -coverpkg=./... ./...".
+			// This will result in (roughly) the following action graph:
+			//
+			//	build("a")       build("b")         build("c")
+			//	    |               |                   |
+			//	link("a.test")   link("b.test")     link("c.test")
+			//	    |               |                   |
+			//	run("a.test")    run("b.test")      run("c.test")
+			//	    |               |                   |
+			//	  print          print              print
+			//
+			// When -coverpkg=<pattern> is in effect, we want to
+			// express the coverage percentage for each package as a
+			// fraction of *all* the statements that match the
+			// pattern, hence if "c" doesn't import "a", we need to
+			// pass as meta-data file for "a" (emitted during the
+			// package "a" build) to the package "c" run action, so
+			// that it can be incorporated with "c"'s regular
+			// metadata. To do this, we add edges from each compile
+			// action to a "writeCoverMeta" action, then from the
+			// writeCoverMeta action to each run action. Updated
+			// graph:
+			//
+			//	build("a")       build("b")         build("c")
+			//	    |   \       /   |               /   |
+			//	    |    v     v    |              /    |
+			//	    |   writemeta <-|-------------+     |
+			//	    |         |||   |                   |
+			//	    |         ||\   |                   |
+			//	link("a.test")/\ \  link("b.test")      link("c.test")
+			//	    |        /  \ +-|--------------+    |
+			//	    |       /    \  |               \   |
+			//	    |      v      v |                v  |
+			//	run("a.test")    run("b.test")      run("c.test")
+			//	    |               |                   |
+			//	  print          print              print
+			//
+			writeCoverMetaAct = &work.Action{
+				Mode:   "write coverage meta-data file",
+				Actor:  work.ActorFunc(work.WriteCoverMetaFilesFile),
+				Objdir: b.NewObjdir(),
+			}
+			for _, p := range testCoverPkgs {
+				p.Internal.Cover.GenMeta = true
+			}
+		}
 	}
 
 	// Inform the compiler that it should instrument the binary at
@@ -915,8 +972,11 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
 			// design). Do this here (as opposed to in builderTest) so
 			// as to handle the case where we're testing multiple
 			// packages and one of the earlier packages imports a
-			// later package.
+			// later package. Note that if -coverpkg is in effect
+			// p.Internal.Cover.GenMeta will wind up being set for
+			// all matching packages.
 			if len(p.TestGoFiles)+len(p.XTestGoFiles) == 0 &&
+				cfg.BuildCoverPkg == nil &&
 				cfg.Experiment.CoverageRedesign {
 				p.Internal.Cover.GenMeta = true
 			}
@@ -925,7 +985,7 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
 
 	// Prepare build + run + print actions for all packages being tested.
 	for _, p := range pkgs {
-		buildTest, runTest, printTest, err := builderTest(b, ctx, pkgOpts, p, allImports[p])
+		buildTest, runTest, printTest, err := builderTest(b, ctx, pkgOpts, p, allImports[p], writeCoverMetaAct)
 		if err != nil {
 			str := err.Error()
 			str = strings.TrimPrefix(str, "\n")
@@ -987,13 +1047,12 @@ var windowsBadWords = []string{
 	"update",
 }
 
-func builderTest(b *work.Builder, ctx context.Context, pkgOpts load.PackageOpts, p *load.Package, imported bool) (buildAction, runAction, printAction *work.Action, err error) {
+func builderTest(b *work.Builder, ctx context.Context, pkgOpts load.PackageOpts, p *load.Package, imported bool, writeCoverMetaAct *work.Action) (buildAction, runAction, printAction *work.Action, err error) {
 	if len(p.TestGoFiles)+len(p.XTestGoFiles) == 0 {
 		if cfg.BuildCover && cfg.Experiment.CoverageRedesign {
-			if !p.Internal.Cover.GenMeta {
-				panic("internal error: Cover.GenMeta should already be set")
+			if p.Internal.Cover.GenMeta {
+				p.Internal.Cover.Mode = cfg.BuildCoverMode
 			}
-			p.Internal.Cover.Mode = cfg.BuildCoverMode
 		}
 		build := b.CompileAction(work.ModeBuild, work.ModeBuild, p)
 		run := &work.Action{
@@ -1003,6 +1062,23 @@ func builderTest(b *work.Builder, ctx context.Context, pkgOpts load.PackageOpts,
 			Package:    p,
 			IgnoreFail: true, // run (prepare output) even if build failed
 		}
+		if writeCoverMetaAct != nil {
+			// There is no real "run" for this package (since there
+			// are no tests), but if coverage is turned on, we can
+			// collect coverage data for the code in the package by
+			// asking cmd/cover for a static meta-data file as part of
+			// the package build. This static meta-data file is then
+			// consumed by a pseudo-action (writeCoverMetaAct) that
+			// adds it to a summary file, then this summary file is
+			// consumed by the various "run test" actions. Below we
+			// add a dependence edge between the build action and the
+			// "write meta files" pseudo-action, and then another dep
+			// from writeCoverMetaAct to the run action. See the
+			// comment in runTest() at the definition of
+			// writeCoverMetaAct for more details.
+			run.Deps = append(run.Deps, writeCoverMetaAct)
+			writeCoverMetaAct.Deps = append(writeCoverMetaAct.Deps, build)
+		}
 		addTestVet(b, p, run, nil)
 		print := &work.Action{
 			Mode:       "test print",
@@ -1140,22 +1216,42 @@ func builderTest(b *work.Builder, ctx context.Context, pkgOpts load.PackageOpts,
 			runAction = installAction // make sure runAction != nil even if not running test
 		}
 	}
+
 	var vetRunAction *work.Action
 	if testC {
 		printAction = &work.Action{Mode: "test print (nop)", Package: p, Deps: []*work.Action{runAction}} // nop
 		vetRunAction = printAction
 	} else {
 		// run test
-		r := new(runTestActor)
+		rta := &runTestActor{
+			writeCoverMetaAct: writeCoverMetaAct,
+		}
 		runAction = &work.Action{
 			Mode:       "test run",
-			Actor:      r,
+			Actor:      rta,
 			Deps:       []*work.Action{buildAction},
 			Package:    p,
 			IgnoreFail: true, // run (prepare output) even if build failed
-			TryCache:   r.c.tryCache,
-			Objdir:     testDir,
+			TryCache:   rta.c.tryCache,
+		}
+		if writeCoverMetaAct != nil {
+			// If writeCoverMetaAct != nil, this indicates that our
+			// "go test -coverpkg" run actions will need to read the
+			// meta-files summary file written by writeCoverMetaAct,
+			// so add a dependence edge from writeCoverMetaAct to the
+			// run action.
+			runAction.Deps = append(runAction.Deps, writeCoverMetaAct)
+			if !p.IsTestOnly() {
+				// Package p is not test only, meaning that the build
+				// action for p may generate a static meta-data file.
+				// Add a dependence edge from p to writeCoverMetaAct,
+				// which needs to know the name of that meta-data
+				// file.
+				compileAction := b.CompileAction(work.ModeBuild, work.ModeBuild, p)
+				writeCoverMetaAct.Deps = append(writeCoverMetaAct.Deps, compileAction)
+			}
 		}
+		runAction.Objdir = testDir
 		vetRunAction = runAction
 		cleanAction = &work.Action{
 			Mode:       "test clean",
@@ -1217,6 +1313,12 @@ var tooManyFuzzTestsToFuzz = []byte("\ntesting: warning: -fuzz matches more than
 type runTestActor struct {
 	c runCache
 
+	// writeCoverMetaAct points to the pseudo-action for collecting
+	// coverage meta-data files for selected -cover test runs. See the
+	// comment in runTest at the definition of writeCoverMetaAct for
+	// more details.
+	writeCoverMetaAct *work.Action
+
 	// sequencing of json start messages, to preserve test order
 	prev <-chan struct{} // wait to start until prev is closed
 	next chan<- struct{} // close next once the next test can start.
@@ -1391,6 +1493,16 @@ func (r *runTestActor) Act(b *work.Builder, ctx context.Context, a *work.Action)
 			base.Fatalf("failed to create temporary dir: %v", err)
 		}
 		coverdirArg = append(coverdirArg, "-test.gocoverdir="+gcd)
+		if r.writeCoverMetaAct != nil {
+			// Copy the meta-files file over into the test's coverdir
+			// directory so that the coverage runtime support will be
+			// able to find it.
+			src := r.writeCoverMetaAct.Objdir + coverage.MetaFilesFileName
+			dst := filepath.Join(gcd, coverage.MetaFilesFileName)
+			if err := b.CopyFile(dst, src, 0666, false); err != nil {
+				return err
+			}
+		}
 		// Even though we are passing the -test.gocoverdir option to
 		// the test binary, also set GOCOVERDIR as well. This is
 		// intended to help with tests that run "go build" to build

src/cmd/go/internal/work/cover.go

@@ -10,7 +10,10 @@ import (
 	"cmd/go/internal/base"
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/str"
+	"context"
+	"encoding/json"
 	"fmt"
+	"internal/coverage"
 	"internal/coverage/covcmd"
 	"io"
 	"os"
@@ -93,3 +96,57 @@ func WriteCoverageProfile(b *Builder, runAct *Action, mf, outf string, w io.Writ
 	_, werr := w.Write(output)
 	return werr
 }
+
+// WriteCoverMetaFilesFile writes out a summary file ("meta-files
+// file") as part of the action function for the "writeCoverMeta"
+// pseudo action employed during "go test -coverpkg" runs where there
+// are multiple tests and multiple packages covered. It builds up a
+// table mapping package import path to meta-data file fragment and
+// writes it out to a file where it can be read by the various test
+// run actions. Note that this function has to be called A) after the
+// build actions are complete for all packages being tested, and B)
+// before any of the "run test" actions for those packages happen.
+// This requirement is enforced by adding making this action ("a")
+// dependent on all test package build actions, and making all test
+// run actions dependent on this action.
+func WriteCoverMetaFilesFile(b *Builder, ctx context.Context, a *Action) error {
+	// Build the metafilecollection object.
+	var collection coverage.MetaFileCollection
+	for i := range a.Deps {
+		dep := a.Deps[i]
+		if dep.Mode != "build" {
+			panic("unexpected mode " + dep.Mode)
+		}
+		metaFilesFile := dep.Objdir + covcmd.MetaFileForPackage(dep.Package.ImportPath)
+		// Check to make sure the meta-data file fragment exists
+		//  and has content (may be empty if package has no functions).
+		if fi, err := os.Stat(metaFilesFile); err != nil {
+			continue
+		} else if fi.Size() == 0 {
+			continue
+		}
+		collection.ImportPaths = append(collection.ImportPaths, dep.Package.ImportPath)
+		collection.MetaFileFragments = append(collection.MetaFileFragments, metaFilesFile)
+	}
+
+	// Serialize it.
+	data, err := json.Marshal(collection)
+	if err != nil {
+		return fmt.Errorf("marshal MetaFileCollection: %v", err)
+	}
+	data = append(data, '\n') // makes -x output more readable
+
+	// Create the directory for this action's objdir and
+	// then write out the serialized collection
+	// to a file in the directory.
+	if err := b.Mkdir(a.Objdir); err != nil {
+		return err
+	}
+	mfpath := a.Objdir + coverage.MetaFilesFileName
+	if err := b.writeFile(mfpath, data); err != nil {
+		return fmt.Errorf("writing metafiles file: %v", err)
+	}
+
+	// We're done.
+	return nil
+}

src/cmd/go/internal/work/exec.go

@@ -619,7 +619,7 @@ OverlayLoop:
 				from := mkAbs(p.Dir, fs[i])
 				opath, _ := fsys.OverlayPath(from)
 				dst := objdir + filepath.Base(fs[i])
-				if err := b.copyFile(dst, opath, 0666, false); err != nil {
+				if err := b.CopyFile(dst, opath, 0666, false); err != nil {
 					return err
 				}
 				a.nonGoOverlay[from] = dst
@@ -894,17 +894,17 @@ OverlayLoop:
 		switch {
 		case strings.HasSuffix(name, _goos_goarch):
 			targ := file[:len(name)-len(_goos_goarch)] + "_GOOS_GOARCH." + ext
-			if err := b.copyFile(objdir+targ, filepath.Join(p.Dir, file), 0666, true); err != nil {
+			if err := b.CopyFile(objdir+targ, filepath.Join(p.Dir, file), 0666, true); err != nil {
 				return err
 			}
 		case strings.HasSuffix(name, _goarch):
 			targ := file[:len(name)-len(_goarch)] + "_GOARCH." + ext
-			if err := b.copyFile(objdir+targ, filepath.Join(p.Dir, file), 0666, true); err != nil {
+			if err := b.CopyFile(objdir+targ, filepath.Join(p.Dir, file), 0666, true); err != nil {
 				return err
 			}
 		case strings.HasSuffix(name, _goos):
 			targ := file[:len(name)-len(_goos)] + "_GOOS." + ext
-			if err := b.copyFile(objdir+targ, filepath.Join(p.Dir, file), 0666, true); err != nil {
+			if err := b.CopyFile(objdir+targ, filepath.Join(p.Dir, file), 0666, true); err != nil {
 				return err
 			}
 		}
@@ -1029,7 +1029,7 @@ func (b *Builder) loadCachedObjdirFile(a *Action, c cache.Cache, name string) er
 	if err != nil {
 		return err
 	}
-	return b.copyFile(a.Objdir+name, cached, 0666, true)
+	return b.CopyFile(a.Objdir+name, cached, 0666, true)
 }
 
 func (b *Builder) cacheCgoHdr(a *Action) {
@@ -1884,23 +1884,23 @@ func (b *Builder) moveOrCopyFile(dst, src string, perm fs.FileMode, force bool)
 
 	// If the source is in the build cache, we need to copy it.
 	if strings.HasPrefix(src, cache.DefaultDir()) {
-		return b.copyFile(dst, src, perm, force)
+		return b.CopyFile(dst, src, perm, force)
 	}
 
 	// On Windows, always copy the file, so that we respect the NTFS
 	// permissions of the parent folder. https://golang.org/issue/22343.
 	// What matters here is not cfg.Goos (the system we are building
 	// for) but runtime.GOOS (the system we are building on).
 	if runtime.GOOS == "windows" {
-		return b.copyFile(dst, src, perm, force)
+		return b.CopyFile(dst, src, perm, force)
 	}
 
 	// If the destination directory has the group sticky bit set,
 	// we have to copy the file to retain the correct permissions.
 	// https://golang.org/issue/18878
 	if fi, err := os.Stat(filepath.Dir(dst)); err == nil {
 		if fi.IsDir() && (fi.Mode()&fs.ModeSetgid) != 0 {
-			return b.copyFile(dst, src, perm, force)
+			return b.CopyFile(dst, src, perm, force)
 		}
 	}
 
@@ -1930,11 +1930,11 @@ func (b *Builder) moveOrCopyFile(dst, src string, perm fs.FileMode, force bool)
 		}
 	}
 
-	return b.copyFile(dst, src, perm, force)
+	return b.CopyFile(dst, src, perm, force)
 }
 
 // copyFile is like 'cp src dst'.
-func (b *Builder) copyFile(dst, src string, perm fs.FileMode, force bool) error {
+func (b *Builder) CopyFile(dst, src string, perm fs.FileMode, force bool) error {
 	if cfg.BuildN || cfg.BuildX {
 		b.Showcmd("", "cp %s %s", src, dst)
 		if cfg.BuildN {

src/cmd/go/internal/work/gccgo.go

@@ -299,7 +299,7 @@ func (tools gccgoToolchain) link(b *Builder, root *Action, out, importcfg string
 	readAndRemoveCgoFlags := func(archive string) (string, error) {
 		newID++
 		newArchive := root.Objdir + fmt.Sprintf("_pkg%d_.a", newID)
-		if err := b.copyFile(newArchive, archive, 0666, false); err != nil {
+		if err := b.CopyFile(newArchive, archive, 0666, false); err != nil {
 			return "", err
 		}
 		if cfg.BuildN || cfg.BuildX {