Rules_go: cgo does not propagate mode. sys/unix is broken with race = "on"

[ash2k]

I'm trying version 0.13.0 of rules_go and gazelle. Thanks for the # gazelle:proto disable_global, it is great, now I don't need to wait for the protoc to be compiled.

But I'm getting the following errors:

warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: warning for library: bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/libgo_default_library.cgo_c_lib.lo the table of contents is empty (no object file members in the library define global symbols)
ERROR: /Users/ash2k/go/src/github.com/atlassian/smith/vendor/golang.org/x/sys/unix/BUILD.bazel:3:1: GoCompile vendor/golang.org/x/sys/unix/darwin_amd64_race_stripped/go_default_library~/github.com/atlassian/smith/vendor/golang.org/x/sys/unix.a~partial.a failed (Exit 1)
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:148:5: undefined: raceenabled
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:150:4: undefined: raceWriteRange
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:153:4: undefined: raceAcquire
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:160:5: undefined: raceenabled
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:161:3: undefined: raceReleaseMerge
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:164:5: undefined: raceenabled
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:165:3: undefined: raceReadRange
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:363:5: undefined: raceenabled
/private/var/tmp/_bazel_ash2k/18feaf204f522c9679e75c03985bfc8f/sandbox/darwin-sandbox/582/execroot/com_github_atlassian_smith/bazel-out/darwin-fastbuild/bin/vendor/golang.org/x/sys/unix/darwin_amd64_stripped/go_default_library.cgo_codegen~/syscall_unix.cgo1.go:364:3: undefined: raceReleaseMerge
GoCompile: error running subcommand: exit status 2

I've tried nuking everything with rm -rf /private/var/tmp/_bazel_<user> (I thought maybe this is the recent OS update...) but I'm getting the same error regardless. This is on latest macOS, bazel 0.15.0. Same code and same dependencies work fine with 0.12.1.

To reproduce:

• clone https://github.com/atlassian/smith into your gopath
• checkout rules_0.13.0 branch
• run dep ensure (you need to have dep installed)
• run make test

[jayconrod]

Hmm, very strange. From the error message, it seems like we have a library built in race mode, but it's incorporating cgo-generated code from the non-race library. There were some changes in 0.13.0 around enabling the race tag, which golang.org/x/sys/unix uses.

[jayconrod]

More analysis:

We use an aspect to propagate mode flags like race, pure, goos, and goarch set in go_binary and go_test. The aspect propagates across deps and embed edges in the rule graph. It generates additional files and actions in the file-action graph for each mode. Basically, we end up with an isomorphic subgraph for each mode.

The cgo setup exists largely in the configured target graph in addition to the file-action graph. This means that the aspect doesn't propagate to the CGoCodeGen action. We always perform cgo code generation in the default mode, regardless of the mode flags set in the go_binary or go_test being built. The default mode is global and is determined by command line arguments like --platforms, and --features=race.

Recently, I turned on the race and msan tags when those modes are on. Previously, we always filtered out sources that required race or msan to be on.

The consequence of all this is that when //pkg/controller/bundlec_test:go_default_test (which has race = "on") is built, it performs cgo code generation for //vendor/golang.org/x/sys/unix:go_default_library with the race tag off. Then we compile that generated code with the race tag on. That library has sources for either mode, but they both get filtered out by these actions, and we end up with a bunch of missing symbols because of that.

Note that this bug is not limited to race: platform-specific cgo code will also get generated in the wrong mode.

---

The Right Way to fix this is to pull all of the cgo stuff into actions generated by go_library. That would get the aspect working correctly. That may be unmaintainable though. We currently rely on cc_library and objc_library rules to compile cgo code. Without a more uniform way to generate C/C++/ObjC compile actions, we'd end up with something very brittle.

A quick hack would be generating cgo rules for each mode, then deciding which instance to depend on in the aspect. That may have some load time and analysis cost though. I'll try to think of a better way.

[ixdy]

Is there any update on this? I think I'm running into the same issue trying to update to rules_go 0.15.1 / gazelle 0.14.0 in kubernetes (https://github.com/kubernetes/kubernetes/pull/67911).

W0827 23:04:04.164] GoCompile: error running subcommand: exit status 2
I0827 23:04:04.265] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:115:5: undefined: raceenabled
I0827 23:04:04.265] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:117:4: undefined: raceWriteRange
I0827 23:04:04.265] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:120:4: undefined: raceAcquire
I0827 23:04:04.266] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:127:5: undefined: raceenabled
I0827 23:04:04.266] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:128:3: undefined: raceReleaseMerge
I0827 23:04:04.266] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:131:5: undefined: raceenabled
I0827 23:04:04.267] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:132:3: undefined: raceReadRange
I0827 23:04:04.267] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:271:5: undefined: raceenabled
I0827 23:04:04.267] /bazel-scratch/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/sandbox/linux-sandbox/1163/execroot/__main__/bazel-out/k8-fastbuild/bin/vendor/golang.org/x/sys/unix/linux_amd64_race_stripped/go_default_library.cgo_codegen%/syscall_unix.cgo1.go:272:3: undefined: raceReleaseMerge

This only seems to fail when we try to bazel test certain targets using --features=race.

[ixdy]

We also set # gazelle:proto disable_global, though it doesn't seem like that is an issue here.

[ixdy]

At least for kubernetes, the problem targets are go_tests which explicitly set race to a different mode than the feature flag is set.

[jayconrod]

@ixdy Sorry for the delay. I'm at GopherCon this week, so I'll be slow to respond.

1595 is a solution for this, but I haven't merged it yet. It declares a lot more rules (a set of targets for each cgo library for every mode), and that slows down analysis significantly (~15% across targets in the x repos).

I think the true solution for this problem would be to rewrite cgo to run the C compiler and linker directly. That may be possible with the recent toolchain enhancements, but it wasn't possible before. Still, I expect this is about a week of work, and I haven't had bandwidth. So #1595 may be a good temporary (hopefully not permanent) solution.

Could you tell me more about the tests that set race = "on"? In general, I'd caution against setting any of the mode flags on go_binary / go_test since select doesn't recognize them, and they can cause incorrect dependencies to be selected. However, if you're building something that uses the test as an input (e.g., a container that runs somewhere else), they may be your only option until we have better support for transitions.

[ash2k]

We always set race="on" on all tests. Also on some binaries when that is what we want. Haven't seen any issues so far. Is it a bad idea?
go_binary example https://github.com/atlassian/smith/blob/master/cmd/smith/BUILD.bazel
go_test example https://github.com/atlassian/smith/blob/master/pkg/controller/bundlec_test/BUILD.bazel

[ixdy]

@jayconrod for kubernetes, the issue is when we bazel test with --features=race but have race = "off" specified on a specific go_test.

[ixdy]

As for #1595, I've confirmed it fixes the issue for kubernetes. It seems like the uncached analysis time for bazel test -- ... -//vendor/... increases from about 21s to 25s, but cached analysis increases by less than a second, with similar increases in initialization / loading phases, roughly totaling a second or two overall.

[jayconrod]

@ash2k I'd advise controlling the configuration with command-line flags (e.g., --features=race) when possible and avoiding mode attributes on go_binary and go_test if you can. The mode attributes are needed in some situations, but they break in others (e.g., with select).

The Bazel devs are working on exposing transitions in Starlark, which should solve this problem. That means you'll be able to depend on a target in a different configuration.

[jayconrod]

@ixdy Thanks for confirming the performance numbers. It sounds like something we can live with, at least for a little while.

What is the reason for race = "off" on those tests? I'm guessing they don't work in race mode. Is that a problem to be concerned about? Is it feasible to use // +build !race (which is actually the feature that broke x/sys/unix) to disable parts of the test, or would that substantially reduce coverage?

[ixdy]

There are two test files which already have // +build !race, and have since before we wrote bazel rules. I guess we could arguably just remove the race = "off" annotation on those, since that would be equivalent coverage as what we had before.

Specifically:

• pkg/master/master_openapi_test.go: the race detector made this test slow and often timeout (https://github.com/kubernetes/kubernetes/pull/39649)
• staging/src/k8s.io/client-go/tools/cache/mutation_detector_test.go: I have no idea why it was disabled here

Thanks for merging #1595. When do you expect there will be a new rules_go release?

[jayconrod]

@ixdy I can do a point release with #1595 in a the next day or two.

Again sorry for the slow response times. I'm back from GopherCon now.

[ash2k]

@jayconrod

@ash2k I'd advise controlling the configuration with command-line flags (e.g., --features=race) when possible and avoiding mode attributes on go_binary and go_test if you can. The mode attributes are needed in some situations, but they break in others (e.g., with select).

Could you clarify what breaks and whether it is "visible" or a silent breakage and how exactly select breaks? I don't think we've had any issues caused by those attributes so far 🤔

[jayconrod]

The select issue comes up when you have something like this:

go_binary(
    name = "cmd",
    goos = "linux",
    goarch = "amd64",
    embed = [":go_default_library"],
)

go_library(
    name = "go_default_library",
    srcs = [...],
    deps = select({
        "@io_bazel_rules_go//go/platform:linux": [":linux_only"],
        "@io_bazel_rules_go//go/platform:windows": [":windows_only"],
        "//conditions:default": [],
    }),
)

The problem is that select is not aware (and cannot be made aware) of the mode attributes (goos, goarch, pure, static, msan, race). The mode attributes are implemented using an aspect. So Bazel basically constructs the configured target graph normally (ignoring the mode attributes), then creates a copy of the target graph with the aspect, and the copy is what gets built. select is used to build the original configured target graph before the aspect gets applied.

So in the situation above, if you build the go_binary on Windows, it will compile for Linux, but a Windows-only dependency will be built (that will probably fail the build), and the Linux-only dependency will be missing. I think this is always going to break loudly. One silent situation is when Bazel builds a package for the wrong platform without errors, but the sources that import that package should be filtered out by build constraints, so it wouldn't get linked into the final build.

If you don't have any platform-specific dependencies in your dependency graph (i.e., you don't need select to work correctly), you're probably fine. If you have unix dependencies and you're building from a unix platform (e.g., host is darwin, target is linux), that's probably okay, but be careful.