[release-branch.go1.18] 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 #54423.
Fixes #54636.

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>
(cherry picked from commit d5aa088d822bc8ef3ceb80c20184f40fcb9b8d2e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425208

diff --git a/src/cmd/go/internal/envcmd/env.go b/src/cmd/go/internal/envcmd/env.go
index c1adf8cef4ca1b386042ff17c0da3d9245c9cfb8..2a283fc4f80ba1f53f24826a95aedd50f3ebd128 100644 (file)
--- a/src/cmd/go/internal/envcmd/env.go
+++ b/src/cmd/go/internal/envcmd/env.go
@@ -167,8 +167,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 62c18866c4c65287939d5e08ac65116e382a8b98..c15749ebf198920f486f6bc23ad5d259098ee769 100644 (file)
--- a/src/cmd/go/internal/list/list.go
+++ b/src/cmd/go/internal/list/list.go
@@ -592,8 +592,7 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
        // Do we need to run a build to gather information?
        needStale := *listJson || 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 312b49ef5dfa33828cf65c9ac373dfad88d86e8f..a0aca316fcf546b670bba229f981a1b5230670d7 100644 (file)
--- a/src/cmd/go/internal/run/run.go
+++ b/src/cmd/go/internal/run/run.go
@@ -87,8 +87,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 50e6d5201b0d6e04cc59a3289d30fa8b4e1cb2b0..3f31a8a80d594da3a46010d551ed521e7ace2e2c 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 d3e0dd8116f481ef4d8e72e5c467a75110cde9bc..9b8698295af111f13cf4e91cfd51289fd75734bc 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 394fe9105003c7b149439b19b0f5188b1bfddfc1..e66dad6898157f20143ebb9ddba54aa6b1841521 100644 (file)
--- a/src/cmd/go/internal/work/build.go
+++ b/src/cmd/go/internal/work/build.go
@@ -402,8 +402,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)
@@ -721,8 +720,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 55a88e0e0b03e23ac34dc7e6e6b5db2d46abe0b3..aac0b4792da0de818da05aa59ba2ceaf69a21602 100644 (file)
--- a/src/cmd/go/script_test.go
+++ b/src/cmd/go/script_test.go
@@ -532,10 +532,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.