Change nogo failures to test failures, instead of build failures.

PiperOrigin-RevId: 329408633

7 files changed, 129 insertions, 164 deletions

diff --git a/tools/nogo/BUILD b/tools/nogo/BUILD
index fb35c5ffd..9f1fcd9c7 100644
--- a/tools/nogo/BUILD
+++ b/tools/nogo/BUILD
@@ -27,7 +27,6 @@ go_library(
     deps = [
         "//tools/checkescape",
         "//tools/checkunsafe",
-        "//tools/nogo/dump",
         "@org_golang_x_tools//go/analysis:go_tool_library",
         "@org_golang_x_tools//go/analysis/internal/facts:go_tool_library",
         "@org_golang_x_tools//go/analysis/passes/asmdecl:go_tool_library",
diff --git a/tools/nogo/config.go b/tools/nogo/config.go
index 451cd4a4c..cfe7b4aa4 100644
--- a/tools/nogo/config.go
+++ b/tools/nogo/config.go
@@ -122,3 +122,8 @@ var analyzerConfig = map[*analysis.Analyzer]matcher{
 	checkescape.Analyzer: internalMatches(),
 	checkunsafe.Analyzer: internalMatches(),
 }
+
+var escapesConfig = map[*analysis.Analyzer]matcher{
+	// Informational only: include all packages.
+	checkescape.EscapeAnalyzer: alwaysMatches(),
+}
diff --git a/tools/nogo/defs.bzl b/tools/nogo/defs.bzl
index 963084d53..480438047 100644
--- a/tools/nogo/defs.bzl
+++ b/tools/nogo/defs.bzl
@@ -50,6 +50,7 @@ NogoStdlibInfo = provider(
     "information for nogo analysis (standard library facts)",
     fields = {
         "facts": "serialized standard library facts",
+        "findings": "package findings (if relevant)",
     },
 )
 
@@ -59,18 +60,18 @@ def _nogo_stdlib_impl(ctx):
 
     # Build the standard library facts.
     facts = ctx.actions.declare_file(ctx.label.name + ".facts")
+    findings = ctx.actions.declare_file(ctx.label.name + ".findings")
     config = struct(
         Srcs = [f.path for f in go_ctx.stdlib_srcs],
         GOOS = go_ctx.goos,
         GOARCH = go_ctx.goarch,
         Tags = go_ctx.tags,
-        FactOutput = facts.path,
     )
     config_file = ctx.actions.declare_file(ctx.label.name + ".cfg")
     ctx.actions.write(config_file, config.to_json())
     ctx.actions.run(
         inputs = [config_file] + go_ctx.stdlib_srcs,
-        outputs = [facts],
+        outputs = [facts, findings],
         tools = depset(go_ctx.runfiles.to_list() + ctx.files._dump_tool),
         executable = ctx.files._nogo[0],
         mnemonic = "GoStandardLibraryAnalysis",
@@ -78,12 +79,15 @@ def _nogo_stdlib_impl(ctx):
         arguments = go_ctx.nogo_args + [
             "-dump_tool=%s" % ctx.files._dump_tool[0].path,
             "-stdlib=%s" % config_file.path,
+            "-findings=%s" % findings.path,
+            "-facts=%s" % facts.path,
         ],
     )
 
     # Return the stdlib facts as output.
     return [NogoStdlibInfo(
         facts = facts,
+        findings = findings,
     )]
 
 nogo_stdlib = go_rule(
@@ -108,6 +112,7 @@ NogoInfo = provider(
     "information for nogo analysis",
     fields = {
         "facts": "serialized package facts",
+        "findings": "package findings (if relevant)",
         "importpath": "package import path",
         "binaries": "package binary files",
         "srcs": "original source files (for go_test support)",
@@ -203,6 +208,8 @@ def _nogo_aspect_impl(target, ctx):
 
     # The nogo tool operates on a configuration serialized in JSON format.
     facts = ctx.actions.declare_file(target.label.name + ".facts")
+    findings = ctx.actions.declare_file(target.label.name + ".findings")
+    escapes = ctx.actions.declare_file(target.label.name + ".escapes")
     config = struct(
         ImportPath = importpath,
         GoFiles = [src.path for src in srcs if src.path.endswith(".go")],
@@ -213,14 +220,13 @@ def _nogo_aspect_impl(target, ctx):
         FactMap = fact_map,
         ImportMap = import_map,
         StdlibFacts = stdlib_facts.path,
-        FactOutput = facts.path,
     )
     config_file = ctx.actions.declare_file(target.label.name + ".cfg")
     ctx.actions.write(config_file, config.to_json())
     inputs.append(config_file)
     ctx.actions.run(
         inputs = inputs,
-        outputs = [facts],
+        outputs = [facts, findings, escapes],
         tools = depset(go_ctx.runfiles.to_list() + ctx.files._dump_tool),
         executable = ctx.files._nogo[0],
         mnemonic = "GoStaticAnalysis",
@@ -229,17 +235,30 @@ def _nogo_aspect_impl(target, ctx):
             "-binary=%s" % target_objfile.path,
             "-dump_tool=%s" % ctx.files._dump_tool[0].path,
             "-package=%s" % config_file.path,
+            "-findings=%s" % findings.path,
+            "-facts=%s" % facts.path,
+            "-escapes=%s" % escapes.path,
         ],
     )
 
     # Return the package facts as output.
-    return [NogoInfo(
-        facts = facts,
-        importpath = importpath,
-        binaries = binaries,
-        srcs = srcs,
-        deps = deps,
-    )]
+    return [
+        NogoInfo(
+            facts = facts,
+            findings = findings,
+            importpath = importpath,
+            binaries = binaries,
+            srcs = srcs,
+            deps = deps,
+        ),
+        OutputGroupInfo(
+            # Expose all findings (should just be a single file). This can be
+            # used for build analysis of the nogo findings.
+            nogo_findings = depset([findings]),
+            # Expose all escape analysis findings (see above).
+            nogo_escapes = depset([escapes]),
+        ),
+    ]
 
 nogo_aspect = go_rule(
     aspect,
@@ -250,15 +269,9 @@ nogo_aspect = go_rule(
         "embed",
     ],
     attrs = {
-        "_nogo": attr.label(
-            default = "//tools/nogo/check:check",
-        ),
-        "_nogo_stdlib": attr.label(
-            default = "//tools/nogo:stdlib",
-        ),
-        "_dump_tool": attr.label(
-            default = "//tools/nogo:dump_tool",
-        ),
+        "_nogo": attr.label(default = "//tools/nogo/check:check"),
+        "_nogo_stdlib": attr.label(default = "//tools/nogo:stdlib"),
+        "_dump_tool": attr.label(default = "//tools/nogo:dump_tool"),
     },
 )
 
@@ -270,13 +283,26 @@ def _nogo_test_impl(ctx):
     # this way so that any test applied is effectively pushed down to all
     # upstream dependencies through the aspect.
     inputs = []
+    findings = []
     runner = ctx.actions.declare_file("%s-executer" % ctx.label.name)
     runner_content = ["#!/bin/bash"]
     for dep in ctx.attr.deps:
+        # Extract the findings.
         info = dep[NogoInfo]
-        inputs.append(info.facts)
+        inputs.append(info.findings)
+        findings.append(info.findings)
+
+        # Include all source files, transitively. This will make this target
+        # "directly affected" for the purpose of build analysis.
+        inputs += info.srcs
+
+        # If there are findings, dump them and fail.
+        runner_content.append("if [[ -s \"%s\" ]]; then cat \"%s\" && exit 1; fi" % (
+            info.findings.short_path,
+            info.findings.short_path,
+        ))
 
-        # Draw a sweet unicode checkmark with the package name (in green).
+        # Otherwise, draw a sweet unicode checkmark with the package name (in green).
         runner_content.append("echo -e \"\\033[0;32m\\xE2\\x9C\\x94\\033[0;31m\\033[0m %s\"" % info.importpath)
     runner_content.append("exit 0\n")
     ctx.actions.write(runner, "\n".join(runner_content), is_executable = True)
diff --git a/tools/nogo/dump/BUILD b/tools/nogo/dump/BUILD
deleted file mode 100644
index dfa29d651..000000000
--- a/tools/nogo/dump/BUILD
+++ /dev/null
@@ -1,10 +0,0 @@
-load("//tools:defs.bzl", "go_library")
-
-package(licenses = ["notice"])
-
-go_library(
-    name = "dump",
-    srcs = ["dump.go"],
-    nogo = False,
-    visibility = ["//tools:__subpackages__"],
-)
diff --git a/tools/nogo/dump/dump.go b/tools/nogo/dump/dump.go
deleted file mode 100644
index f06567e0f..000000000
--- a/tools/nogo/dump/dump.go
+++ /dev/null
@@ -1,78 +0,0 @@
-// Copyright 2019 The gVisor Authors.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-// Package dump contains data dump tools.
-//
-// The interface used by the package corresponds to the tool generated by the
-// nogo_dump_tool rule.
-//
-// This package is separate in order to avoid a dependency cycle.
-package dump
-
-import (
-	"flag"
-	"fmt"
-	"io"
-	"os"
-	"os/exec"
-)
-
-var (
-	// Binary is the binary under analysis.
-	//
-	// See Reader, below.
-	binary = flag.String("binary", "", "binary under analysis")
-
-	// Reader is the input stream.
-	//
-	// This may be set instead of Binary.
-	Reader io.Reader
-
-	// Tool is the tool used to dump a binary.
-	tool = flag.String("dump_tool", "", "tool used to dump a binary")
-)
-
-// Command returns a command that will emit the dumped object on stdout.
-//
-// You must call Wait on the resulting command.
-func Command() (*exec.Cmd, io.Reader, error) {
-	var (
-		args  []string
-		stdin io.Reader
-	)
-	if *binary != "" {
-		args = append(args, *binary)
-		*binary = "" // Clear.
-	} else if Reader != nil {
-		stdin = Reader
-		Reader = nil // Clear.
-	} else {
-		// We have no input stream or binary.
-		return nil, nil, fmt.Errorf("no binary or reader provided!")
-	}
-
-	// Construct our command.
-	cmd := exec.Command(*tool, args...)
-	cmd.Stdin = stdin
-	cmd.Stderr = os.Stderr
-	out, err := cmd.StdoutPipe()
-	if err != nil {
-		return nil, nil, err
-	}
-	if err := cmd.Start(); err != nil {
-		return nil, nil, err
-	}
-
-	return cmd, out, err
-}
diff --git a/tools/nogo/nogo.go b/tools/nogo/nogo.go
index e44f32d4c..40e48540d 100644
--- a/tools/nogo/nogo.go
+++ b/tools/nogo/nogo.go
@@ -40,18 +40,19 @@ import (
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/internal/facts"
 	"golang.org/x/tools/go/gcexportdata"
-	"gvisor.dev/gvisor/tools/nogo/dump"
+
+	// Special case: flags live here and change overall behavior.
+	"gvisor.dev/gvisor/tools/checkescape"
 )
 
 // stdlibConfig is serialized as the configuration.
 //
 // This contains everything required for stdlib analysis.
 type stdlibConfig struct {
-	Srcs       []string
-	GOOS       string
-	GOARCH     string
-	Tags       []string
-	FactOutput string
+	Srcs   []string
+	GOOS   string
+	GOARCH string
+	Tags   []string
 }
 
 // packageConfig is serialized as the configuration.
@@ -66,7 +67,6 @@ type packageConfig struct {
 	GOARCH      string
 	ImportMap   map[string]string
 	FactMap     map[string]string
-	FactOutput  string
 	StdlibFacts string
 }
 
@@ -111,14 +111,6 @@ func (c *packageConfig) factLoader() (loader, error) {
 	}, nil
 }
 
-// factSaver may be used directly as a saver.
-func (c *packageConfig) factSaver(factData []byte) error {
-	if c.FactOutput == "" {
-		return nil // Nothing to save.
-	}
-	return ioutil.WriteFile(c.FactOutput, factData, 0644)
-}
-
 // shouldInclude indicates whether the file should be included.
 //
 // NOTE: This does only basic parsing of tags.
@@ -200,9 +192,9 @@ var ErrSkip = errors.New("skipped")
 //
 // Note that not all parts of the source are expected to build. We skip obvious
 // test files, and cmd files, which should not be dependencies.
-func checkStdlib(config *stdlibConfig) ([]string, error) {
+func checkStdlib(config *stdlibConfig, ac map[*analysis.Analyzer]matcher) ([]string, []byte, error) {
 	if len(config.Srcs) == 0 {
-		return nil, nil
+		return nil, nil, nil
 	}
 
 	// Ensure all paths are normalized.
@@ -283,23 +275,21 @@ func checkStdlib(config *stdlibConfig) ([]string, error) {
 		}
 
 		// Provide the input.
-		oldReader := dump.Reader
-		dump.Reader = rc // For analysis.
+		oldReader := checkescape.Reader
+		checkescape.Reader = rc // For analysis.
 		defer func() {
 			rc.Close()
-			dump.Reader = oldReader // Restore.
+			checkescape.Reader = oldReader // Restore.
 		}()
 
 		// Run the analysis.
-		findings, err := checkPackage(config, func(factData []byte) error {
-			stdlibFacts[pkg] = factData
-			return nil
-		}, checkOne)
+		findings, factData, err := checkPackage(config, ac, checkOne)
 		if err != nil {
 			// If we can't analyze a package from the standard library,
 			// then we skip it. It will simply not have any findings.
 			return nil
 		}
+		stdlibFacts[pkg] = factData
 		allFindings = append(allFindings, findings...)
 		return nil
 	}
@@ -316,14 +306,11 @@ func checkStdlib(config *stdlibConfig) ([]string, error) {
 	// Write out all findings.
 	factData, err := json.Marshal(stdlibFacts)
 	if err != nil {
-		return nil, fmt.Errorf("error saving stdlib facts: %w", err)
-	}
-	if err := ioutil.WriteFile(config.FactOutput, factData, 0644); err != nil {
-		return nil, fmt.Errorf("error saving findings to %q: %v", config.FactOutput, err)
+		return nil, nil, fmt.Errorf("error saving stdlib facts: %w", err)
 	}
 
 	// Return all findings.
-	return allFindings, nil
+	return allFindings, factData, nil
 }
 
 // checkPackage runs all analyzers.
@@ -334,7 +321,7 @@ func checkStdlib(config *stdlibConfig) ([]string, error) {
 //
 // [1] bazelbuid/rules_go/tools/builders/nogo_main.go
 // [2] golang.org/x/tools/go/checker/internal/checker
-func checkPackage(config *packageConfig, factSaver saver, importCallback func(string) error) ([]string, error) {
+func checkPackage(config *packageConfig, ac map[*analysis.Analyzer]matcher, importCallback func(string) error) ([]string, []byte, error) {
 	imp := &importer{
 		packageConfig: config,
 		fset:          token.NewFileSet(),
@@ -347,14 +334,14 @@ func checkPackage(config *packageConfig, factSaver saver, importCallback func(st
 	for _, file := range config.GoFiles {
 		include, err := config.shouldInclude(file)
 		if err != nil {
-			return nil, fmt.Errorf("error evaluating file %q: %v", file, err)
+			return nil, nil, fmt.Errorf("error evaluating file %q: %v", file, err)
 		}
 		if !include {
 			continue
 		}
 		s, err := parser.ParseFile(imp.fset, file, nil, parser.ParseComments)
 		if err != nil {
-			return nil, fmt.Errorf("error parsing file %q: %v", file, err)
+			return nil, nil, fmt.Errorf("error parsing file %q: %v", file, err)
 		}
 		syntax = append(syntax, s)
 	}
@@ -372,17 +359,17 @@ func checkPackage(config *packageConfig, factSaver saver, importCallback func(st
 	}
 	types, err := typeConfig.Check(config.ImportPath, imp.fset, syntax, typesInfo)
 	if err != nil && imp.lastErr != ErrSkip {
-		return nil, fmt.Errorf("error checking types: %w", err)
+		return nil, nil, fmt.Errorf("error checking types: %w", err)
 	}
 
 	// Load all package facts.
 	loader, err := config.factLoader()
 	if err != nil {
-		return nil, fmt.Errorf("error loading facts: %w", err)
+		return nil, nil, fmt.Errorf("error loading facts: %w", err)
 	}
 	facts, err := facts.Decode(types, loader)
 	if err != nil {
-		return nil, fmt.Errorf("error decoding facts: %w", err)
+		return nil, nil, fmt.Errorf("error decoding facts: %w", err)
 	}
 
 	// Register fact types and establish dependencies between analyzers.
@@ -404,7 +391,7 @@ func checkPackage(config *packageConfig, factSaver saver, importCallback func(st
 		}
 
 		// Prepare the matcher.
-		m := analyzerConfig[a]
+		m := ac[a]
 		report := func(d analysis.Diagnostic) {
 			if m.ShouldReport(d, imp.fset) {
 				diagnostics[a] = append(diagnostics[a], d)
@@ -445,22 +432,16 @@ func checkPackage(config *packageConfig, factSaver saver, importCallback func(st
 		return nil // Success.
 	}
 
-	// Visit all analysis recursively.
-	for a, _ := range analyzerConfig {
+	// Visit all analyzers recursively.
+	for a, _ := range ac {
 		if imp.lastErr == ErrSkip {
 			continue // No local analysis.
 		}
 		if err := visit(a); err != nil {
-			return nil, err // Already has context.
+			return nil, nil, err // Already has context.
 		}
 	}
 
-	// Write the output file.
-	factData := facts.Encode()
-	if err := factSaver(factData); err != nil {
-		return nil, fmt.Errorf("error: unable to save facts: %v", err)
-	}
-
 	// Convert all diagnostics to strings.
 	findings := make([]string, 0, len(diagnostics))
 	for a, ds := range diagnostics {
@@ -471,12 +452,16 @@ func checkPackage(config *packageConfig, factSaver saver, importCallback func(st
 	}
 
 	// Return all findings.
-	return findings, nil
+	factData := facts.Encode()
+	return findings, factData, nil
 }
 
 var (
-	packageFile = flag.String("package", "", "package configuration file (in JSON format)")
-	stdlibFile  = flag.String("stdlib", "", "stdlib configuration file (in JSON format)")
+	packageFile    = flag.String("package", "", "package configuration file (in JSON format)")
+	stdlibFile     = flag.String("stdlib", "", "stdlib configuration file (in JSON format)")
+	findingsOutput = flag.String("findings", "", "output file (or stdout, if not specified)")
+	factsOutput    = flag.String("facts", "", "output file for facts (optional)")
+	escapesOutput  = flag.String("escapes", "", "output file for escapes (optional)")
 )
 
 func loadConfig(file string, config interface{}) interface{} {
@@ -503,6 +488,7 @@ func Main() {
 
 	var (
 		findings []string
+		factData []byte
 		err      error
 	)
 
@@ -510,15 +496,50 @@ func Main() {
 	if *packageFile != "" && *stdlibFile != "" {
 		log.Fatalf("unable to perform stdlib and package analysis; provide only one!")
 	} else if *stdlibFile != "" {
+		// Perform basic analysis.
 		c := loadConfig(*stdlibFile, new(stdlibConfig)).(*stdlibConfig)
-		findings, err = checkStdlib(c)
+		findings, factData, err = checkStdlib(c, analyzerConfig)
 	} else if *packageFile != "" {
+		// Perform basic analysis.
 		c := loadConfig(*packageFile, new(packageConfig)).(*packageConfig)
-		findings, err = checkPackage(c, c.factSaver, nil)
+		findings, factData, err = checkPackage(c, analyzerConfig, nil)
+		// Do we need to do escape analysis?
+		if *escapesOutput != "" {
+			escapes, _, err := checkPackage(c, escapesConfig, nil)
+			if err != nil {
+				log.Fatalf("error performing escape analysis: %v", err)
+			}
+			f, err := os.OpenFile(*escapesOutput, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
+			if err != nil {
+				log.Fatalf("unable to open output %q: %v", *escapesOutput, err)
+			}
+			defer f.Close()
+			for _, escape := range escapes {
+				fmt.Fprintf(f, "%s\n", escape)
+			}
+		}
 	} else {
 		log.Fatalf("please provide at least one of package or stdlib!")
 	}
 
+	// Save facts.
+	if *factsOutput != "" {
+		if err := ioutil.WriteFile(*factsOutput, factData, 0644); err != nil {
+			log.Fatalf("error saving findings to %q: %v", *factsOutput, err)
+		}
+	}
+
+	// Open the output file.
+	var w io.Writer = os.Stdout
+	if *findingsOutput != "" {
+		f, err := os.OpenFile(*findingsOutput, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
+		if err != nil {
+			log.Fatalf("unable to open output %q: %v", *findingsOutput, err)
+		}
+		defer f.Close()
+		w = f
+	}
+
 	// Handle findings & errors.
 	if err != nil {
 		log.Fatalf("error checking package: %v", err)
@@ -527,9 +548,8 @@ func Main() {
 		return
 	}
 
-	// Print findings and exit with non-zero code.
+	// Print findings.
 	for _, finding := range findings {
-		fmt.Fprintf(os.Stdout, "%s\n", finding)
+		fmt.Fprintf(w, "%s\n", finding)
 	}
-	os.Exit(1)
 }
diff --git a/tools/nogo/register.go b/tools/nogo/register.go
index 62b499661..34b173937 100644
--- a/tools/nogo/register.go
+++ b/tools/nogo/register.go
@@ -26,6 +26,9 @@ func analyzers() (all []*analysis.Analyzer) {
 	for a, _ := range analyzerConfig {
 		all = append(all, a)
 	}
+	for a, _ := range escapesConfig {
+		all = append(all, a)
+	}
 	return all
 }