compiler_wrapper: fix flaky tests

We're running many tests in parallel that have dependencies on the
`umask` global. We shouldn't be running these in parallel with each
other, since they may read values for this that're set by other
goroutines.

Since these are difficult to spot, this CL does two things:
- Requiring that all tests mark themselves as either readers of or
writers to umask. Any test that does this gets run in serial with other
tests that do it.
- Requires code that modifies/reads the umask to go through `env.umask`,
rather than `syscall.Umask`. This allows us to cheaply and accurately
verify that a test's dependency on the process' umask was stated.

BUG=b:186801841
TEST=`go test -count=100` passed

Change-Id: Ifa871cfa48c005646499b21c1bfa1a4799ca641b
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/2956692
Reviewed-by: Ryan Beltran <ryanbeltran@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>

12 files changed, 84 insertions, 10 deletions

diff --git a/compiler_wrapper/ccache_flag_test.go b/compiler_wrapper/ccache_flag_test.go
index 2b18c8ca..50205312 100644
--- a/compiler_wrapper/ccache_flag_test.go
+++ b/compiler_wrapper/ccache_flag_test.go
@@ -164,6 +164,8 @@ func withCCacheEnabledTestContext(t *testing.T, work func(ctx *testContext)) {
 
 func TestRusagePreventsCCache(t *testing.T) {
 	withCCacheEnabledTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestWritesToUmask()
+
 		ctx.env = append(ctx.env, "TOOLCHAIN_RUSAGE_OUTPUT="+filepath.Join(ctx.tempDir, "rusage.log"))
 		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
 			ctx.newCommand(gccX86_64, mainCc)))
diff --git a/compiler_wrapper/compile_with_fallback_test.go b/compiler_wrapper/compile_with_fallback_test.go
index a67f3eb9..f9da441a 100644
--- a/compiler_wrapper/compile_with_fallback_test.go
+++ b/compiler_wrapper/compile_with_fallback_test.go
@@ -221,6 +221,8 @@ func TestCompileWithFallbackExtraArgs(t *testing.T) {
 
 func TestCompileWithFallbackLogCommandAndErrors(t *testing.T) {
 	withCompileWithFallbackTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestReadsFromUmask()
+
 		ctx.env = append(ctx.env, "ANDROID_LLVM_FALLBACK_DISABLED_WARNINGS=-a -b")
 		ctx.cmdMock = func(cmd *command, stdin io.Reader, stdout io.Writer, stderr io.Writer) error {
 			switch ctx.cmdCount {
diff --git a/compiler_wrapper/compiler_wrapper_test.go b/compiler_wrapper/compiler_wrapper_test.go
index fd59cfe1..74fe3f58 100644
--- a/compiler_wrapper/compiler_wrapper_test.go
+++ b/compiler_wrapper/compiler_wrapper_test.go
@@ -126,6 +126,8 @@ func TestGomaDisablesRusage(t *testing.T) {
 
 func TestLogRusageAndForceDisableWError(t *testing.T) {
 	withTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestWritesToUmask()
+
 		logFileName := filepath.Join(ctx.tempDir, "rusage.log")
 		ctx.env = []string{
 			"FORCE_DISABLE_WERROR=1",
@@ -157,6 +159,8 @@ func TestLogRusageAndForceDisableWError(t *testing.T) {
 
 func TestErrorOnLogRusageAndBisect(t *testing.T) {
 	withTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestWritesToUmask()
+
 		ctx.env = []string{
 			"BISECT_STAGE=xyz",
 			"TOOLCHAIN_RUSAGE_OUTPUT=rusage.log",
@@ -170,6 +174,8 @@ func TestErrorOnLogRusageAndBisect(t *testing.T) {
 
 func TestErrorOnBisectAndForceDisableWError(t *testing.T) {
 	withTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestWritesToUmask()
+
 		ctx.env = []string{
 			"BISECT_STAGE=xyz",
 			"FORCE_DISABLE_WERROR=1",
diff --git a/compiler_wrapper/cros_hardened_config_test.go b/compiler_wrapper/cros_hardened_config_test.go
index 6d96b182..fe9f0213 100644
--- a/compiler_wrapper/cros_hardened_config_test.go
+++ b/compiler_wrapper/cros_hardened_config_test.go
@@ -16,8 +16,15 @@ const crosHardenedGoldenDir = "testdata/cros_hardened_golden"
 const crosHardenedNoCCacheGoldenDir = "testdata/cros_hardened_noccache_golden"
 const crosHardenedLlvmNextGoldenDir = "testdata/cros_hardened_llvmnext_golden"
 
-func TestCrosHardenedConfig(t *testing.T) {
+func withGoldenTestContext(t *testing.T, f func(ctx *testContext)) {
 	withTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestWritesToUmask()
+		f(ctx)
+	})
+}
+
+func TestCrosHardenedConfig(t *testing.T) {
+	withGoldenTestContext(t, func(ctx *testContext) {
 		useLlvmNext := false
 		useCCache := true
 		cfg, err := getConfig("cros.hardened", useCCache, useLlvmNext, "123")
@@ -31,7 +38,7 @@ func TestCrosHardenedConfig(t *testing.T) {
 }
 
 func TestCrosHardenedConfigWithoutCCache(t *testing.T) {
-	withTestContext(t, func(ctx *testContext) {
+	withGoldenTestContext(t, func(ctx *testContext) {
 		useLlvmNext := false
 		useCCache := false
 		cfg, err := getConfig("cros.hardened", useCCache, useLlvmNext, "123")
@@ -56,7 +63,7 @@ func TestCrosHardenedConfigWithoutCCache(t *testing.T) {
 }
 
 func TestCrosHardenedConfigWithLlvmNext(t *testing.T) {
-	withTestContext(t, func(ctx *testContext) {
+	withGoldenTestContext(t, func(ctx *testContext) {
 		useLlvmNext := true
 		useCCache := true
 		cfg, err := getConfig("cros.hardened", useCCache, useLlvmNext, "123")
diff --git a/compiler_wrapper/cros_host_config_test.go b/compiler_wrapper/cros_host_config_test.go
index fee78e62..4f3b5cb2 100644
--- a/compiler_wrapper/cros_host_config_test.go
+++ b/compiler_wrapper/cros_host_config_test.go
@@ -13,7 +13,7 @@ const crosClangHostGoldenDir = "testdata/cros_clang_host_golden"
 const crosGccHostGoldenDir = "testdata/cros_gcc_host_golden"
 
 func TestCrosClangHostConfig(t *testing.T) {
-	withTestContext(t, func(ctx *testContext) {
+	withGoldenTestContext(t, func(ctx *testContext) {
 		useLlvmNext := false
 		useCCache := false
 		cfg, err := getConfig("cros.host", useCCache, useLlvmNext, "123")
@@ -43,7 +43,7 @@ func TestCrosClangHostConfig(t *testing.T) {
 }
 
 func TestCrosGccHostConfig(t *testing.T) {
-	withTestContext(t, func(ctx *testContext) {
+	withGoldenTestContext(t, func(ctx *testContext) {
 		useLlvmNext := false
 		useCCache := false
 		cfg, err := getConfig("cros.host", useCCache, useLlvmNext, "123")
diff --git a/compiler_wrapper/cros_nonhardened_config_test.go b/compiler_wrapper/cros_nonhardened_config_test.go
index 4883c5f9..3d413fb8 100644
--- a/compiler_wrapper/cros_nonhardened_config_test.go
+++ b/compiler_wrapper/cros_nonhardened_config_test.go
@@ -12,6 +12,8 @@ const crosNonHardenedGoldenDir = "testdata/cros_nonhardened_golden"
 
 func TestCrosNonHardenedConfig(t *testing.T) {
 	withTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestWritesToUmask()
+
 		useLlvmNext := false
 		useCCache := true
 		cfg, err := getConfig("cros.nonhardened", useCCache, useLlvmNext, "123")
diff --git a/compiler_wrapper/disable_werror_flag.go b/compiler_wrapper/disable_werror_flag.go
index 5c21b1ad..cb770b7b 100644
--- a/compiler_wrapper/disable_werror_flag.go
+++ b/compiler_wrapper/disable_werror_flag.go
@@ -14,7 +14,6 @@ import (
 	"path"
 	"strconv"
 	"strings"
-	"syscall"
 )
 
 const numWErrorEstimate = 30
@@ -176,8 +175,8 @@ func doubleBuildWithWNoError(env env, cfg *config, originalCmd *command) (exitCo
 
 	// Buildbots use a nonzero umask, which isn't quite what we want: these directories should
 	// be world-readable and world-writable.
-	oldMask := syscall.Umask(0)
-	defer syscall.Umask(oldMask)
+	oldMask := env.umask(0)
+	defer env.umask(oldMask)
 
 	// Allow root and regular users to write to this without issue.
 	if err := os.MkdirAll(cfg.newWarningsDir, 0777); err != nil {
diff --git a/compiler_wrapper/disable_werror_flag_test.go b/compiler_wrapper/disable_werror_flag_test.go
index d0054262..592c35ba 100644
--- a/compiler_wrapper/disable_werror_flag_test.go
+++ b/compiler_wrapper/disable_werror_flag_test.go
@@ -318,6 +318,8 @@ func TestLogWarningsWhenDoubleBuildFails(t *testing.T) {
 
 func withForceDisableWErrorTestContext(t *testing.T, work func(ctx *testContext)) {
 	withTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestWritesToUmask()
+
 		ctx.env = []string{"FORCE_DISABLE_WERROR=1"}
 		work(ctx)
 	})
diff --git a/compiler_wrapper/env.go b/compiler_wrapper/env.go
index 2c48ad30..3fc547c6 100644
--- a/compiler_wrapper/env.go
+++ b/compiler_wrapper/env.go
@@ -10,9 +10,11 @@ import (
 	"io"
 	"os"
 	"strings"
+	"syscall"
 )
 
 type env interface {
+	umask(int) int
 	getenv(key string) (string, bool)
 	environ() []string
 	getwd() string
@@ -52,6 +54,10 @@ func newProcessEnv() (env, error) {
 
 var _ env = (*processEnv)(nil)
 
+func (env *processEnv) umask(newmask int) (oldmask int) {
+	return syscall.Umask(newmask)
+}
+
 func (env *processEnv) getenv(key string) (string, bool) {
 	return os.LookupEnv(key)
 }
diff --git a/compiler_wrapper/rusage_flag.go b/compiler_wrapper/rusage_flag.go
index 4aa40b4d..63469602 100644
--- a/compiler_wrapper/rusage_flag.go
+++ b/compiler_wrapper/rusage_flag.go
@@ -101,8 +101,8 @@ func maybeCaptureRusage(env env, compilerCmd *command, action func(willLogRusage
 
 		// We need to temporarily set umask to 0 to ensure 777 permissions are actually 777
 		// This effects builderbots in particular
-		oldMask := syscall.Umask(0)
-		defer syscall.Umask(oldMask)
+		oldMask := env.umask(0)
+		defer env.umask(oldMask)
 
 		// We want to know what package is being compiled. The working directory gives us a good clue.
 		cwd, err := os.Getwd()
diff --git a/compiler_wrapper/rusage_flag_test.go b/compiler_wrapper/rusage_flag_test.go
index 940f4a79..439cfd15 100644
--- a/compiler_wrapper/rusage_flag_test.go
+++ b/compiler_wrapper/rusage_flag_test.go
@@ -159,6 +159,8 @@ func TestLogRusageAppendsToFile(t *testing.T) {
 
 func withLogRusageTestContext(t *testing.T, work func(ctx *testContext)) {
 	withTestContext(t, func(ctx *testContext) {
+		ctx.NoteTestWritesToUmask()
+
 		ctx.env = []string{"TOOLCHAIN_RUSAGE_OUTPUT=" + filepath.Join(ctx.tempDir, "rusage.log")}
 		work(ctx)
 	})
diff --git a/compiler_wrapper/testutil_test.go b/compiler_wrapper/testutil_test.go
index 21b7169d..67081207 100644
--- a/compiler_wrapper/testutil_test.go
+++ b/compiler_wrapper/testutil_test.go
@@ -14,6 +14,8 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
+	"sync"
+	"syscall"
 	"testing"
 )
 
@@ -43,8 +45,15 @@ type testContext struct {
 	stdinBuffer  bytes.Buffer
 	stdoutBuffer bytes.Buffer
 	stderrBuffer bytes.Buffer
+
+	umaskRestoreAction func()
 }
 
+// We have some tests which modify our umask, and other tests which depend upon the value of our
+// umask remaining consistent. This lock serializes those. Please use `NoteTestWritesToUmask()` and
+// `NoteTestDependsOnUmask()` on `testContext` rather than using this directly.
+var umaskModificationLock sync.RWMutex
+
 func withTestContext(t *testing.T, work func(ctx *testContext)) {
 	t.Parallel()
 	tempDir, err := ioutil.TempDir("", "compiler_wrapper")
@@ -62,11 +71,48 @@ func withTestContext(t *testing.T, work func(ctx *testContext)) {
 	}
 	ctx.updateConfig(&config{})
 
+	defer ctx.maybeReleaseUmaskDependency()
 	work(&ctx)
 }
 
 var _ env = (*testContext)(nil)
 
+func (ctx *testContext) umask(mask int) (oldmask int) {
+	if ctx.umaskRestoreAction == nil {
+		panic("Umask operations requested in test without declaring a umask dependency")
+	}
+	return syscall.Umask(mask)
+}
+
+func (ctx *testContext) initUmaskDependency(lockFn func(), unlockFn func()) {
+	if ctx.umaskRestoreAction != nil {
+		// Use a panic so we get a backtrace.
+		panic("Multiple notes of a test depending on the value of `umask` given -- tests " +
+			"are only allowed up to one.")
+	}
+
+	lockFn()
+	ctx.umaskRestoreAction = unlockFn
+}
+
+func (ctx *testContext) maybeReleaseUmaskDependency() {
+	if ctx.umaskRestoreAction != nil {
+		ctx.umaskRestoreAction()
+	}
+}
+
+// Note that the test depends on a stable value for the process' umask.
+func (ctx *testContext) NoteTestReadsFromUmask() {
+	ctx.initUmaskDependency(umaskModificationLock.RLock, umaskModificationLock.RUnlock)
+}
+
+// Note that the test modifies the process' umask. This implies a dependency on the process' umask,
+// so it's an error to call both NoteTestWritesToUmask and NoteTestReadsFromUmask from the same
+// test.
+func (ctx *testContext) NoteTestWritesToUmask() {
+	ctx.initUmaskDependency(umaskModificationLock.Lock, umaskModificationLock.Unlock)
+}
+
 func (ctx *testContext) getenv(key string) (string, bool) {
 	for i := len(ctx.env) - 1; i >= 0; i-- {
 		entry := ctx.env[i]