Ensure that nogo is enabled.

A recent change stopped using the correct file (the export data, not the archive) and checklocks started failing. Unfortunately, this was suppressed, since the filter command was not failing with findings. This change fixes that problem and adds a test to ensure that this cannot happen again. If nogo starts failing to identify problems, the sanity_test in nogo/sanity will also start to fail. This change also requires updating the WORKSPACE to the latest rules_go and Go version, in order to pick up the fixed go_tools. The latest rules_go in turn required an updated bazel, which in turn required a minor change in the coverdata implementation. Fixing the fact propagation brought forward a number of problems with caching for bazel workers. Its unclear whether this was a core worker issue or whether some caching was broken, but the situation was basically undebugable. Instead, the way facts are stored and loaded is optimized to be able to remove the use of workers altogether and ideally make nogo debuggable. PiperOrigin-RevId: 426327186
gristlabs · Feb 4, 2022 · a87bb4a · a87bb4a
1 parent dce4528
commit a87bb4a
Show file tree

Hide file tree

Showing 34 changed files with 741 additions and 985 deletions.
diff --git a/.buildkite/pipeline.yaml b/.buildkite/pipeline.yaml
@@ -41,6 +41,9 @@ steps:
   - <<: *common
     label: ":fire: Smoke tests"
     command: make smoke-tests
+  - <<: *common
+    label: ":fire: Smoke race tests"
+    command: make smoke-race-tests
   - wait
 
   # Check that the Go branch builds.

diff --git a/Makefile b/Makefile
@@ -185,11 +185,14 @@ debian: ## Builds the debian packages.
 	@$(call build,-c opt //debian:debian)
 .PHONY: debian
 
-smoke-tests: ## Runs a simple smoke test after build runsc.
+smoke-tests: ## Runs a simple smoke test after building runsc.
 	@$(call run,//runsc,--alsologtostderr --network none --debug --TESTONLY-unsafe-nonroot=true --rootless do true)
-	@$(call run,$(RACE_FLAGS) //runsc:runsc-race,--alsologtostderr --network none --debug --TESTONLY-unsafe-nonroot=true --rootless do true)
 .PHONY: smoke-tests
 
+smoke-race-tests: ## Runs a smoke test after build building runsc in race configuration.
+	@$(call run,$(RACE_FLAGS) //runsc:runsc-race,--alsologtostderr --network none --debug --TESTONLY-unsafe-nonroot=true --rootless do true)
+.PHONY: smoke-race-tests
+
 nogo-tests:
 	@$(call test,--build_tag_filters=nogo --test_tag_filters=nogo //:all pkg/... tools/...)
 .PHONY: nogo-tests

diff --git a/WORKSPACE b/WORKSPACE
@@ -39,36 +39,52 @@ http_archive(
         # Newer versions of the rules_go rules will automatically strip test
         # binaries of symbols, which we don't want.
         "//tools:rules_go_symbols.patch",
+        # Allow for patching of the go_sdk.
+        "//tools:rules_go_sdk.patch",
     ],
-    sha256 = "8e968b5fcea1d2d64071872b12737bbb5514524ee5f0a4f54f5920266c261acb",
+    sha256 = "d6b2513456fe2229811da7eb67a444be7785f5323c6708b38d851d2b51e54d83",
     urls = [
-        "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.28.0/rules_go-v0.28.0.zip",
-        "https://github.com/bazelbuild/rules_go/releases/download/v0.28.0/rules_go-v0.28.0.zip",
+        "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.30.0/rules_go-v0.30.0.zip",
+        "https://github.com/bazelbuild/rules_go/releases/download/v0.30.0/rules_go-v0.30.0.zip",
     ],
 )
 
 http_archive(
     name = "bazel_gazelle",
-    sha256 = "62ca106be173579c0a167deb23358fdfe71ffa1e4cfdddf5582af26520f1c66f",
+    sha256 = "de69a09dc70417580aabf20a28619bb3ef60d038470c7cf8442fafcf627c21cb",
     urls = [
-        "https://mirror.bazel.build/github.com/bazelbuild/bazel-gazelle/releases/download/v0.23.0/bazel-gazelle-v0.23.0.tar.gz",
-        "https://github.com/bazelbuild/bazel-gazelle/releases/download/v0.23.0/bazel-gazelle-v0.23.0.tar.gz",
+        "https://mirror.bazel.build/github.com/bazelbuild/bazel-gazelle/releases/download/v0.24.0/bazel-gazelle-v0.24.0.tar.gz",
+        "https://github.com/bazelbuild/bazel-gazelle/releases/download/v0.24.0/bazel-gazelle-v0.24.0.tar.gz",
     ],
 )
 
-load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
+load("@io_bazel_rules_go//go:deps.bzl", "go_download_sdk", "go_rules_dependencies")
+load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository")
 
 go_rules_dependencies()
 
-go_register_toolchains(go_version = "1.16.8")
-
-load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository")
+go_download_sdk(
+    name = "go_sdk",
+    # This implements a fix in the types package which dramatically speeds up
+    # analysis. Without this fix, the nogo rules will often fail to run in
+    # time on our continuous integration.
+    patch = "//tools:go_types_memoize.patch",
+    patch_strip = 2,
+    version = "1.17.6",
+)
 
 gazelle_dependencies()
 
 # Some repository below has a transitive dependency on these repositories.
 # These declarations must precede any later declarations that transitively
-# depend on older versions, since only the first declaration is considered.
+# depend on older versions, since only the first declaration is considered.  go_repository(
+go_repository(
+    name = "org_golang_x_tools",
+    importpath = "golang.org/x/tools",
+    sum = "h1:j9KsMiaP1c3B0OTQGth0/k+miLGTgLsAFUCrF2vLcF8=",
+    version = "v0.1.9",
+)
+
 go_repository(
     name = "org_golang_x_sys",
     importpath = "golang.org/x/sys",
@@ -83,6 +99,20 @@ go_repository(
     version = "v0.0.0-20210503060351-7fd8e65b6420",
 )
 
+go_repository(
+    name = "co_honnef_go_tools",
+    importpath = "honnef.co/go/tools",
+    sum = "h1:MNh1AVMyVX23VUHE2O27jm6lNj3vjO5DexS4A1xvnzk=",
+    version = "v0.2.2",
+)
+
+go_repository(
+    name = "org_golang_x_oauth2",
+    importpath = "golang.org/x/oauth2",
+    sum = "h1:RerP+noqYHUQ8CMRcPlC2nvTa4dcBIjegkuWdcUDuqg=",
+    version = "v0.0.0-20211104180415-d3ed0bb246c8",
+)
+
 # Load C++ rules.
 http_archive(
     name = "rules_cc",
@@ -585,11 +615,11 @@ rbe_autoconfig(name = "rbe_default")
 
 http_archive(
     name = "rules_pkg",
+    sha256 = "62eeb544ff1ef41d786e329e1536c1d541bb9bcad27ae984d57f18f314018e66",
     urls = [
         "https://mirror.bazel.build/github.com/bazelbuild/rules_pkg/releases/download/0.6.0/rules_pkg-0.6.0.tar.gz",
         "https://github.com/bazelbuild/rules_pkg/releases/download/0.6.0/rules_pkg-0.6.0.tar.gz",
     ],
-    sha256 = "62eeb544ff1ef41d786e329e1536c1d541bb9bcad27ae984d57f18f314018e66",
 )
 
 load("@rules_pkg//:deps.bzl", "rules_pkg_dependencies")
@@ -820,13 +850,6 @@ go_repository(
     version = "v0.0.0-20191024005414-555d28b269f0",
 )
 
-go_repository(
-    name = "org_golang_x_tools",
-    importpath = "golang.org/x/tools",
-    sum = "h1:ouewzE6p+/VEB31YYnTbEJdi8pFqKp4P4n85vwo3DHA=",
-    version = "v0.1.5",
-)
-
 go_repository(
     name = "org_golang_x_xerrors",
     importpath = "golang.org/x/xerrors",
@@ -848,13 +871,6 @@ go_repository(
     version = "v1.5.2",
 )
 
-go_repository(
-    name = "org_golang_x_oauth2",
-    importpath = "golang.org/x/oauth2",
-    sum = "h1:B333XXssMuKQeBwiNODx4TupZy7bf4sxFZnN2ZOcvUE=",
-    version = "v0.0.0-20211005180243-6b3c2da341f1",
-)
-
 go_repository(
     name = "com_github_docker_docker",
     importpath = "github.com/docker/docker",
@@ -974,13 +990,6 @@ go_repository(
     version = "v0.23.0",
 )
 
-go_repository(
-    name = "co_honnef_go_tools",
-    importpath = "honnef.co/go/tools",
-    sum = "h1:/EPr//+UMMXwMTkXvCCoaJDq8cpjMO80Ou+L4PDo2mY=",
-    version = "v0.2.1",
-)
-
 go_repository(
     name = "com_github_burntsushi_toml",
     importpath = "github.com/BurntSushi/toml",

diff --git a/go.mod b/go.mod
@@ -4,7 +4,7 @@ go 1.17
 
 require (
 	github.com/BurntSushi/toml v0.3.1
-	github.com/bazelbuild/rules_go v0.27.0
+	github.com/bazelbuild/rules_go v0.30.0
 	github.com/cenkalti/backoff v1.1.1-0.20190506075156-2146c9339422
 	github.com/containerd/cgroups v1.0.1
 	github.com/containerd/console v1.0.1

diff --git a/go.sum b/go.sum
@@ -57,6 +57,8 @@ github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kd
 github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
 github.com/bazelbuild/rules_go v0.27.0 h1:KViqR7qKXwz+LrNdIauCDU21kneCk+4DnYjpvlJwH50=
 github.com/bazelbuild/rules_go v0.27.0/go.mod h1:MC23Dc/wkXEyk3Wpq6lCqz0ZAYOZDw2DR5y3N1q2i7M=
+github.com/bazelbuild/rules_go v0.30.0 h1:kX4jVcstqrsRqKPJSn2mq2o+TI21edRzEJSrEOMQtr0=
+github.com/bazelbuild/rules_go v0.30.0/go.mod h1:MC23Dc/wkXEyk3Wpq6lCqz0ZAYOZDw2DR5y3N1q2i7M=
 github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q=
 github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+CedLV8=
 github.com/cenkalti/backoff v1.1.1-0.20190506075156-2146c9339422 h1:8eZxmY1yvxGHzdzTEhI09npjMVGzNAdrqzruTX6jcK4=

diff --git a/images/default/Dockerfile b/images/default/Dockerfile
@@ -24,6 +24,6 @@ RUN curl https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cloud
     ln -s /google-cloud-sdk/bin/gcloud /usr/bin/gcloud
 
 # Download the official bazel binary. The APT repository isn't used because there is not packages for arm64.
-RUN sh -c 'curl -o /usr/local/bin/bazel https://releases.bazel.build/4.0.0/release/bazel-4.0.0-linux-$(uname -m | sed s/aarch64/arm64/) && chmod ugo+x /usr/local/bin/bazel'
+RUN sh -c 'curl -o /usr/local/bin/bazel https://releases.bazel.build/4.2.1/release/bazel-4.2.1-linux-$(uname -m | sed s/aarch64/arm64/) && chmod ugo+x /usr/local/bin/bazel'
 WORKDIR /workspace
 ENTRYPOINT ["/usr/local/bin/bazel"]
diff --git a/nogo.yaml b/nogo.yaml
@@ -51,6 +51,20 @@ global:
       - "may require checklocks annotation for"
       # Generated proto code creates declarations like 'var start int = iNdEx'
       - "should omit type .* from declaration; it will be inferred from the right-hand side"
+  external:
+    suppress:
+      # buildssa can't handle certain packages (cmd/...).
+      - "panic recovered: interface conversion: types.Type is nil"
+      - "panic recovered: runtime error: invalid memory address or nil"
+      - "panic recovered: no type for \\*ast.CallExpr"
+      - "panic recovered: interface conversion: types.Type is \\*types.Basic"
+      - "panic recovered: no type for \\*ast.BinaryExpr"
+      - "panic recovered: no type for \\*ast.SelectorExpr"
+      - "panic recovered: no types.Object for ast.Ident SetTypeErrors"
+      - "panic recovered: unexpected CompositeLit type: invalid type"
+    exclude:
+      - ".*/vet/testdata/.*"
+      - ".*/runtime/testdata/.*"
   internal:
     suppress:
       # We use ALL_CAPS for system definitions,
@@ -100,6 +114,9 @@ analyzers:
       exclude: [".*"]
   errorsas:
     external: # Enabled.
+      exclude:
+        # Specific broken case.
+        - ".*/cmd/go/internal/modload/list.go"
   httpresponse:
     external: # Enabled.
   loopclosure:
@@ -111,6 +128,7 @@ analyzers:
       exclude:
         - pkg/sentry/platform/kvm/kvm_test.go # Intentional.
         - tools/bigquery/bigquery.go          # False positive.
+        - "-"                                 # No filename.
   printf:
     external: # Enabled.
       suppress:
@@ -157,6 +175,10 @@ analyzers:
     external: # Enabled.
   checkescape:
     external: # Enabled.
+      suppress:
+        # External libraries may not have binaries (e.g. stdlib testdata, etc.),
+        # so these cases can be safely ignored.
+        - "no such file or directory"
   checklinkname:
     external: # Enabled.
       suppress:
@@ -168,6 +190,8 @@ analyzers:
         # targets in the standard library, so we still need to run
         # checklinkname on stdlib generally.
         - "linkname to unknown symbol"
+      exclude:
+        - ".*/containerd/sys/subprocess_unsafe_linux.go"
   SA1019: # Use of deprecated identifier.
     # disable for now due to misattribution from golang.org/issue/44195.
     generated:

diff --git a/pkg/coverage/coverage.go b/pkg/coverage/coverage.go
@@ -39,7 +39,7 @@ import (
 )
 
 var (
-	// coverageMu must be held while accessing coverdata.Cover. This prevents
+	// coverageMu must be held while accessing coverdata.*. This prevents
 	// concurrent reads/writes from multiple threads collecting coverage data.
 	coverageMu sync.RWMutex
 
@@ -61,7 +61,7 @@ const blockBitLength = 16
 
 // Available returns whether any coverage data is available.
 func Available() bool {
-	return len(coverdata.Cover.Blocks) > 0
+	return len(coverdata.Blocks) > 0
 }
 
 // EnableReport sets up coverage reporting.
@@ -102,7 +102,7 @@ func ClearCoverageData() {
 	// We do not use atomic operations while reading/writing to the counters,
 	// which would drastically degrade performance. Slight discrepancies due to
 	// racing is okay for the purposes of kcov.
-	for _, counters := range coverdata.Cover.Counters {
+	for _, counters := range coverdata.Counters {
 		for index := 0; index < len(counters); index++ {
 			counters[index] = 0
 		}
@@ -155,7 +155,7 @@ func ConsumeCoverageData(w io.Writer) int {
 	total := 0
 	var pcBuffer [8]byte
 	for fileNum, file := range globalData.files {
-		counters := coverdata.Cover.Counters[file]
+		counters := coverdata.Counters[file]
 		for index := 0; index < len(counters); index++ {
 			// We do not use atomic operations while reading/writing to the counters,
 			// which would drastically degrade performance. Slight discrepancies due to
@@ -194,13 +194,13 @@ func InitCoverageData() {
 	globalData.once.Do(func() {
 		// First, order all files. Then calculate synthetic PCs for every block
 		// (using the well-defined ordering for files as well).
-		for file := range coverdata.Cover.Blocks {
+		for file := range coverdata.Blocks {
 			globalData.files = append(globalData.files, file)
 		}
 		sort.Strings(globalData.files)
 
 		for fileNum, file := range globalData.files {
-			blocks := coverdata.Cover.Blocks[file]
+			blocks := coverdata.Blocks[file]
 			pcs := make([]uint64, 0, len(blocks))
 			for blockNum := range blocks {
 				pcs = append(pcs, calculateSyntheticPC(fileNum, blockNum))
@@ -226,8 +226,8 @@ func Report() error {
 
 	var err error
 	reportOnce.Do(func() {
-		for file, counters := range coverdata.Cover.Counters {
-			blocks := coverdata.Cover.Blocks[file]
+		for file, counters := range coverdata.Counters {
+			blocks := coverdata.Blocks[file]
 			for i := 0; i < len(counters); i++ {
 				if atomic.LoadUint32(&counters[i]) > 0 {
 					err = writeBlock(reportOutput, file, blocks[i])
@@ -260,7 +260,7 @@ func Symbolize(out io.Writer, pc uint64) error {
 // corresponding synthetic PCs.
 func WriteAllBlocks(out io.Writer) error {
 	for fileNum, file := range globalData.files {
-		for blockNum, block := range coverdata.Cover.Blocks[file] {
+		for blockNum, block := range coverdata.Blocks[file] {
 			if err := writeBlockWithPC(out, calculateSyntheticPC(fileNum, blockNum), file, block); err != nil {
 				return err
 			}
@@ -300,7 +300,7 @@ func fileFromIndex(i int) (string, error) {
 
 // blockFromIndex returns the i-th block in the given file.
 func blockFromIndex(file string, i int) (testing.CoverBlock, error) {
-	blocks, ok := coverdata.Cover.Blocks[file]
+	blocks, ok := coverdata.Blocks[file]
 	if !ok {
 		return testing.CoverBlock{}, fmt.Errorf("instrumented file %s does not exist", file)
 	}

diff --git a/pkg/sentry/fsimpl/fuse/connection_test.go b/pkg/sentry/fsimpl/fuse/connection_test.go
@@ -74,7 +74,9 @@ func TestConnectionAbort(t *testing.T) {
 	testObj := primitive.Uint32(rand.Uint32())
 	for i := 0; i < int(numRequests); i++ {
 		req := conn.NewRequest(creds, uint32(i), uint64(i), 0, &testObj)
+		conn.fd.mu.Lock()
 		fut, err := conn.callFutureLocked(task, req)
+		conn.fd.mu.Unlock()
 		if err != nil {
 			t.Fatalf("callFutureLocked failed: %v", err)
 		}

diff --git a/pkg/sentry/fsimpl/fuse/fusefs.go b/pkg/sentry/fsimpl/fuse/fusefs.go
@@ -303,12 +303,12 @@ func (fs *filesystem) MountOptions() string {
 	return fs.opts.mopts
 }
 
-// Fh data returned by newEntry
+// NewFhData is returned by newEntry.
 type NewFhData struct {
-	// file handler
+	// fh is the file handler.
 	fh uint64
 
-	// Flags of the file.
+	// flags is the flags of the file.
 	flags uint32
 }
 

diff --git a/tools/bazeldefs/BUILD b/tools/bazeldefs/BUILD
@@ -1,4 +1,4 @@
-load("//tools:defs.bzl", "bzl_library", "go_proto_library")
+load("//tools:defs.bzl", "bzl_library")
 
 package(
     default_visibility = ["//:sandbox"],
@@ -53,9 +53,3 @@ genrule(
     ],
     visibility = ["//:sandbox"],
 )
-
-go_proto_library(
-    name = "worker_protocol_go_proto",
-    importpath = "gvisor.dev/bazel/worker_protocol_go_proto",
-    proto = "@bazel_tools//src/main/protobuf:worker_protocol_proto",
-)
diff --git a/tools/bazeldefs/go.bzl b/tools/bazeldefs/go.bzl
@@ -11,8 +11,6 @@ go_embed_data = _go_embed_data
 
 go_path = _go_path
 
-bazel_worker_proto = "//tools/bazeldefs:worker_protocol_go_proto"
-
 def _go_proto_or_grpc_library(go_library_func, name, **kwargs):
     if "importpath" in kwargs:
         # If importpath is explicit, pass straight through.

diff --git a/tools/checkescape/BUILD b/tools/checkescape/BUILD
@@ -8,6 +8,7 @@ go_library(
     nogo = False,
     visibility = ["//tools/nogo:__subpackages__"],
     deps = [
+        "//pkg/log",
         "//tools/nogo/flags",
         "@org_golang_x_tools//go/analysis:go_default_library",
         "@org_golang_x_tools//go/analysis/passes/buildssa:go_default_library",