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.

Fixes #54423.

Change-Id: I8fc2127a7d877bb39a1174e398736bb51d03d4d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/425205
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>

diff --git a/src/cmd/go/internal/envcmd/env.go b/src/cmd/go/internal/envcmd/env.go
index 012ea4abafa36ebd07942c6342e841f3b835e343..bbd3318d26b8905b253ba6b22622d5c07b062125 100644 (file)
--- a/src/cmd/go/internal/envcmd/env.go
+++ b/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.

diff --git a/src/cmd/go/internal/list/list.go b/src/cmd/go/internal/list/list.go
index 2e3614d317c76412721e851a24118c41b5a05f57..0b928d27e6c95e71eabce41390cdf2d1e932d9ec 100644 (file)
--- a/src/cmd/go/internal/list/list.go
+++ b/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

diff --git a/src/cmd/go/internal/run/run.go b/src/cmd/go/internal/run/run.go
index ebe161181901bb954a7a932e672601ece3d6a89a..6b253a2c9e52e5caff3bfbb197371ff97cf8d695 100644 (file)
--- a/src/cmd/go/internal/run/run.go
+++ b/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

diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go
index aa5e41e0043801312b1e8bdb42622482772c77fc..028db84cc462bc7a72683c741c7cde0407a1e4cd 100644 (file)
--- a/src/cmd/go/internal/test/test.go
+++ b/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")

diff --git a/src/cmd/go/internal/vet/vet.go b/src/cmd/go/internal/vet/vet.go
index a0b11fdd3dc5f19990215ad71aebdb7533d383be..085c7d23b246998adf1264985aed0ac0a5b8b4dd 100644 (file)
--- a/src/cmd/go/internal/vet/vet.go
+++ b/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 {

diff --git a/src/cmd/go/internal/work/action.go b/src/cmd/go/internal/work/action.go
index c0862c5efe503af8cf6384aebd1468b9d89e024f..4bbd23ab8e43212b10a1303cfec594a16afff59e 100644 (file)
--- a/src/cmd/go/internal/work/action.go
+++ b/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 {

diff --git a/src/cmd/go/internal/work/build.go b/src/cmd/go/internal/work/build.go
index 352e46b48f9955414e8ccf2571534339e8508a98..e34cacca039a47a72c1f5c9e116163b4b7b968f2 100644 (file)
--- a/src/cmd/go/internal/work/build.go
+++ b/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{AutoVCS: 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

diff --git a/src/cmd/go/script_test.go b/src/cmd/go/script_test.go
index d49772433151b33d3acc597df5b8f4a744207a2f..ca76ab5ab8e387e3b380792415331f71bb3d3df1 100644 (file)
--- a/src/cmd/go/script_test.go
+++ b/src/cmd/go/script_test.go
@@ -576,10 +576,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.