[release-branch.go1.19] cmd/go: avoid registering AtExit handlers in …

…tests

Ever since 'go build' was added (in CL 5483069), it has used an atexit
handler to clean up working directories.

CL 154109 introduced 'cc' command to the script test framework that
called Init on a builder once per invocation. Unfortunately, since
base.AtExit is unsynchronized, the Init added there caused any script
that invokes that command to be unsafe for concurrent use.

This change fixes the race by having the 'cc' command pass in its
working directory instead of allowing the Builder to allocate one.
Following modern Go best practices, it also replaces the in-place Init
method (which is prone to typestate and aliasing bugs) with a
NewBuilder constructor function.

Updates golang#54423.
Fixes golang#54637.

Change-Id: I8fc2127a7d877bb39a1174e398736bb51d03d4d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/425205
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
(cherry picked from commit d5aa088)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425207

src/cmd/go/internal/envcmd/env.go

@@ -174,8 +174,8 @@ func ExtraEnvVars() []cfg.EnvVar {
 // ExtraEnvVarsCostly returns environment variables that should not leak into child processes
 // but are costly to evaluate.
 func ExtraEnvVarsCostly() []cfg.EnvVar {
-	var b work.Builder
-	b.Init()
+	b := work.NewBuilder("")
+
 	cppflags, cflags, cxxflags, fflags, ldflags, err := b.CFlags(&load.Package{})
 	if err != nil {
 		// Should not happen - b.CFlags was given an empty package.

src/cmd/go/internal/list/list.go

@@ -689,8 +689,7 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
 	// Do we need to run a build to gather information?
 	needStale := (listJson && listJsonFields.needAny("Stale", "StaleReason")) || strings.Contains(*listFmt, ".Stale")
 	if needStale || *listExport || *listCompiled {
-		var b work.Builder
-		b.Init()
+		b := work.NewBuilder("")
 		b.IsCmdList = true
 		b.NeedExport = *listExport
 		b.NeedCompiledGoFiles = *listCompiled

src/cmd/go/internal/run/run.go

@@ -91,8 +91,7 @@ func runRun(ctx context.Context, cmd *base.Command, args []string) {
 	}
 
 	work.BuildInit()
-	var b work.Builder
-	b.Init()
+	b := work.NewBuilder("")
 	b.Print = printStderr
 
 	i := 0

src/cmd/go/internal/test/test.go

@@ -744,8 +744,7 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
 		}
 	}
 
-	var b work.Builder
-	b.Init()
+	b := work.NewBuilder("")
 
 	if cfg.BuildI {
 		fmt.Fprint(os.Stderr, "go: -i flag is deprecated\n")
@@ -800,7 +799,16 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
 		if !testC || a.Failed {
 			return
 		}
-		b.Init()
+
+		// TODO(bcmills): I have no idea why the Builder must be reset here, but
+		// without this reset dance, TestGoTestDashIDashOWritesBinary fails with
+		// lots of "vet config not found" errors. This was added in CL 5699088,
+		// which had almost no public discussion, a very short commit description,
+		// and left no comment in the code to explain what is going on here. 🤯
+		//
+		// Maybe this has the effect of removing actions that were registered by the
+		// call to CompileAction above?
+		b = work.NewBuilder("")
 	}
 
 	var builds, runs, prints []*work.Action
@@ -916,7 +924,7 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
 			ensureImport(p, "sync/atomic")
 		}
 
-		buildTest, runTest, printTest, err := builderTest(&b, ctx, pkgOpts, p, allImports[p])
+		buildTest, runTest, printTest, err := builderTest(b, ctx, pkgOpts, p, allImports[p])
 		if err != nil {
 			str := err.Error()
 			str = strings.TrimPrefix(str, "\n")

src/cmd/go/internal/vet/vet.go

@@ -94,8 +94,7 @@ func runVet(ctx context.Context, cmd *base.Command, args []string) {
 		base.Fatalf("no packages to vet")
 	}
 
-	var b work.Builder
-	b.Init()
+	b := work.NewBuilder("")
 
 	root := &work.Action{Mode: "go vet"}
 	for _, p := range pkgs {

src/cmd/go/internal/work/action.go

@@ -240,7 +240,13 @@ const (
 	ModeVetOnly = 1 << 8
 )
 
-func (b *Builder) Init() {
+// NewBuilder returns a new Builder ready for use.
+//
+// If workDir is the empty string, NewBuilder creates a WorkDir if needed
+// and arranges for it to be removed in case of an unclean exit.
+func NewBuilder(workDir string) *Builder {
+	b := new(Builder)
+
 	b.Print = func(a ...any) (int, error) {
 		return fmt.Fprint(os.Stderr, a...)
 	}
@@ -249,7 +255,9 @@ func (b *Builder) Init() {
 	b.toolIDCache = make(map[string]string)
 	b.buildIDCache = make(map[string]string)
 
-	if cfg.BuildN {
+	if workDir != "" {
+		b.WorkDir = workDir
+	} else if cfg.BuildN {
 		b.WorkDir = "$WORK"
 	} else {
 		tmp, err := os.MkdirTemp(cfg.Getenv("GOTMPDIR"), "go-build")
@@ -306,6 +314,8 @@ func (b *Builder) Init() {
 			base.Exit()
 		}
 	}
+
+	return b
 }
 
 func CheckGOOSARCHPair(goos, goarch string) error {

src/cmd/go/internal/work/build.go

@@ -403,8 +403,7 @@ var RuntimeVersion = runtime.Version()
 func runBuild(ctx context.Context, cmd *base.Command, args []string) {
 	modload.InitWorkfile()
 	BuildInit()
-	var b Builder
-	b.Init()
+	b := NewBuilder("")
 
 	pkgs := load.PackagesAndErrors(ctx, load.PackageOpts{LoadVCS: true}, args)
 	load.CheckPackageErrors(pkgs)
@@ -728,8 +727,8 @@ func InstallPackages(ctx context.Context, patterns []string, pkgs []*load.Packag
 	}
 	base.ExitIfErrors()
 
-	var b Builder
-	b.Init()
+	b := NewBuilder("")
+
 	depMode := ModeBuild
 	if cfg.BuildI {
 		depMode = ModeInstall

src/cmd/go/script_test.go

@@ -556,10 +556,8 @@ func (ts *testScript) cmdCc(want simpleStatus, args []string) {
 		ts.fatalf("usage: cc args... [&]")
 	}
 
-	var b work.Builder
-	b.Init()
+	b := work.NewBuilder(ts.workdir)
 	ts.cmdExec(want, append(b.GccCmd(".", ""), args...))
-	robustio.RemoveAll(b.WorkDir)
 }
 
 // cd changes to a different directory.