[dev.fuzz] cmd/go/internal: instrument imports of the test

Previously, the packages that were imported by the
test were not instrumented for coverage. This meant
that a fuzz target in a stand-alone test file would
not be able to perform coverage-guided fuzzing.

The fix uses all of the imports, including those
from the test files, when determining which packages
to instrument. However, certain package should
be ignored when walking the import graph. Otherwise,
packages like internal/fuzz, and its imports, may be
instrumented, which could lead to false positives for
"interesting" corpus values.

There was an additional bug which needed to be fixed
in order for this to work. The bug was in the fact that
the GcFlags which held `-d=libfuzzer` were being
overwritten in some cases. The fix updates the way these
flags are set in order to prevent this behavior.

Change-Id: I21d336c29a33db1181bbae0fd23678d127fe52a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/321960
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

diff --git a/src/cmd/go/internal/load/flag.go b/src/cmd/go/internal/load/flag.go
index 440cb86134489a7e023ce516629fe9bccc00fe0c..274c0f23e27b1dd11e79202f0ff73705b1536440 100644 (file)
--- a/src/cmd/go/internal/load/flag.go
+++ b/src/cmd/go/internal/load/flag.go
@@ -22,8 +22,9 @@ var (
 // that allows specifying different effective flags for different packages.
 // See 'go help build' for more details about per-package flags.
 type PerPackageFlag struct {
-       present bool
-       values  []ppfValue
+       present      bool
+       values       []ppfValue
+       seenPackages map[*Package]bool // the packages for which the flags have already been set
 }
 
 // A ppfValue is a single <pattern>=<flags> per-package flag value.

diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go
index 193a27a713b80abf55cbadcc81d06a0f3c0c94a0..9d7d32b5d1d41f35692bc9f1579701b698c9c1ef 100644 (file)
--- a/src/cmd/go/internal/load/pkg.go
+++ b/src/cmd/go/internal/load/pkg.go
@@ -210,7 +210,6 @@ type PackageInternal struct {
        BuildInfo         string               // add this info to package main
        TestmainGo        *[]byte              // content for _testmain.go
        Embed             map[string][]string  // //go:embed comment mapping
-       FlagsSet          bool                 // whether the flags have been set
        OrigImportPath    string               // original import path before adding '_test' suffix
 
        Asmflags   []string // -asmflags for this package
@@ -2628,18 +2627,20 @@ func (e *mainPackageError) ImportPath() string {
 
 func setToolFlags(pkgs ...*Package) {
        for _, p := range PackageList(pkgs) {
-               // TODO(jayconrod,katiehockman): See if there's a better way to do this.
-               if p.Internal.FlagsSet {
-                       // The flags have already been set, so don't re-run this and
-                       // potentially clear existing flags.
-                       continue
-               } else {
-                       p.Internal.FlagsSet = true
+               appendFlags(p, &p.Internal.Asmflags, &BuildAsmflags)
+               appendFlags(p, &p.Internal.Gcflags, &BuildGcflags)
+               appendFlags(p, &p.Internal.Ldflags, &BuildLdflags)
+               appendFlags(p, &p.Internal.Gccgoflags, &BuildGccgoflags)
+       }
+}
+
+func appendFlags(p *Package, flags *[]string, packageFlag *PerPackageFlag) {
+       if !packageFlag.seenPackages[p] {
+               if packageFlag.seenPackages == nil {
+                       packageFlag.seenPackages = make(map[*Package]bool)
                }
-               p.Internal.Asmflags = BuildAsmflags.For(p)
-               p.Internal.Gcflags = BuildGcflags.For(p)
-               p.Internal.Ldflags = BuildLdflags.For(p)
-               p.Internal.Gccgoflags = BuildGccgoflags.For(p)
+               packageFlag.seenPackages[p] = true
+               *flags = append(*flags, packageFlag.For(p)...)
        }
 }
 

diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go
index d5afae782bc8e719cc81359925d195f8681ffe77..012a75123b51be16994fbf10c5d9ae86375cf6a5 100644 (file)
--- a/src/cmd/go/internal/test/test.go
+++ b/src/cmd/go/internal/test/test.go
@@ -826,11 +826,21 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
                }
        }
 
+       // Inform the compiler that it should instrument the binary at
+       // build-time when fuzzing is enabled.
        fuzzFlags := work.FuzzInstrumentFlags()
        if testFuzz != "" && fuzzFlags != nil {
-               // Inform the compiler that it should instrument the binary at
-               // build-time when fuzzing is enabled.
-               for _, p := range load.PackageList(pkgs) {
+               // Don't instrument packages which may affect coverage guidance but are
+               // unlikely to be useful.
+               var fuzzNoInstrument = map[string]bool{
+                       "testing":       true,
+                       "internal/fuzz": true,
+                       "runtime":       true,
+               }
+               for _, p := range load.TestPackageList(ctx, pkgOpts, pkgs) {
+                       if fuzzNoInstrument[p.ImportPath] {
+                               continue
+                       }
                        p.Internal.Gcflags = append(p.Internal.Gcflags, fuzzFlags...)
                }
        }