cmd/go: serialize access to scriptDir with output

Currently, Builder.fmtcmd can read scriptDir without taking the output
lock. This introduces a potential data race between the read in fmtcmd
and the write in Showcmd. There's also a logical race here: because
fmtcmd doesn't know when its output is going to be printed, Showcmd
may print a "cd" command between when fmtcmd is called and when its
output is printed. As a result, it doesn't make sense to just lock
around the access in fmtcmd.

Instead, move the entire scriptDir substitution to Showcmd. This will
generally result in the same output. In the cases where Builder.run is
called with a non-empty desc, it means we may print a full path in the
comment line above output rather than substituting the script
directory. I think this is okay.

This lets us undo the workaround in CL 536355.

Change-Id: I617fe136eaafcc9bbb7e701b427d956aeab8a2b2
Reviewed-on: https://go-review.googlesource.com/c/go/+/536376
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Austin Clements <austin@google.com>

diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go
index 3c5b9842f2870fd445205d5f6cf89041a6616e29..9378222a567e214f16e83cbd074946752d1cf7cb 100644 (file)
--- a/src/cmd/go/internal/work/exec.go
+++ b/src/cmd/go/internal/work/exec.go
@@ -2157,24 +2157,8 @@ func mayberemovefile(s string) {
 // fmtcmd formats a command in the manner of fmt.Sprintf but also:
 //
 //     fmtcmd replaces the value of b.WorkDir with $WORK.
-//
-//     fmtcmd replaces the name of the current directory with dot (.)
-//     but only when it is at the beginning of a space-separated token.
 func (b *Builder) fmtcmd(dir string, format string, args ...any) string {
        cmd := fmt.Sprintf(format, args...)
-       if dir != "" && dir != "/" && b.scriptDir == dir {
-               // In the output stream, scriptDir is our working directory. Replace it
-               // with "." in the command.
-               //
-               // We intentionally don't lock around the access to scriptDir. If this
-               // access is racy, that means our working directory isn't well-defined
-               // at this call, and we want the race detector to catch that.
-               dot := " ."
-               if dir[len(dir)-1] == filepath.Separator {
-                       dot += string(filepath.Separator)
-               }
-               cmd = strings.ReplaceAll(" "+cmd, " "+dir, dot)[1:]
-       }
        if b.WorkDir != "" && !strings.HasPrefix(cmd, "cat ") {
                cmd = strings.ReplaceAll(cmd, b.WorkDir, "$WORK")
                escaped := strconv.Quote(b.WorkDir)
@@ -2188,17 +2172,34 @@ func (b *Builder) fmtcmd(dir string, format string, args ...any) string {
 
 // Showcmd prints the given command to standard output
 // for the implementation of -n or -x.
+//
+// Showcmd also replaces the name of the current script directory with dot (.)
+// but only when it is at the beginning of a space-separated token.
+//
+// If dir is not "" or "/" and not the current script directory, Showcmd first
+// prints a "cd" command to switch to dir and updates the script directory.
 func (b *Builder) Showcmd(dir string, format string, args ...any) {
        b.output.Lock()
        defer b.output.Unlock()
 
-       if dir != "" && dir != "/" && dir != b.scriptDir {
-               // Show changing to dir and update the current directory.
-               b.Print(b.fmtcmd("", "cd %s\n", dir))
-               b.scriptDir = dir
+       cmd := b.fmtcmd(dir, format, args...)
+
+       if dir != "" && dir != "/" {
+               if dir != b.scriptDir {
+                       // Show changing to dir and update the current directory.
+                       b.Print(b.fmtcmd("", "cd %s\n", dir))
+                       b.scriptDir = dir
+               }
+               // Replace scriptDir is our working directory. Replace it
+               // with "." in the command.
+               dot := " ."
+               if dir[len(dir)-1] == filepath.Separator {
+                       dot += string(filepath.Separator)
+               }
+               cmd = strings.ReplaceAll(" "+cmd, " "+dir, dot)[1:]
        }
 
-       b.Print(b.fmtcmd(dir, format, args...) + "\n")
+       b.Print(cmd + "\n")
 }
 
 // reportCmd reports the output and exit status of a command. The cmdOut and
@@ -2391,7 +2392,7 @@ var cgoTypeSigRe = lazyregexp.New(`\b_C2?(type|func|var|macro)_\B`)
 // and returns a non-nil error.
 func (b *Builder) run(a *Action, dir string, desc string, env []string, cmdargs ...any) error {
        out, err := b.runOut(a, dir, env, cmdargs...)
-       if len(out) > 0 && desc == "" {
+       if desc == "" {
                desc = b.fmtcmd(dir, "%s", strings.Join(str.StringList(cmdargs...), " "))
        }
        return b.reportCmd(a, desc, dir, out, err)