cmd: support space and quotes in CC and CXX

The CC and CXX environment variables now support spaces and quotes
(both double and single). This fixes two issues: first, if CC is a
single path that contains spaces (like 'c:\Program
Files\gcc\bin\gcc.exe'), that should now work if the space is quoted
or escaped (#41400). Second, if CC or CXX has multiple arguments (like
'gcc -O2'), they are now split correctly, and the arguments are passed
before other arguments when invoking the C compiler. Previously,
strings.Fields was used to split arguments, and the arguments were
placed later in the command line. (#43078).

Fixes #41400
Fixes #43078

NOTE: This change also includes a fix (CL 341929) for a test that was
broken by the original CL. Commit message for the fix is below.

[dev.cmdgo] cmd/link: fix TestBuildForTvOS

This test was broken in CL 334732 on darwin.

The test invokes 'go build' with a CC containing the arguments
-framework CoreFoundation. Previously, the go command split CC on
whitespace, and inserted the arguments after the command line when
running CC directly. Those arguments weren't passed to cgo though,
so cgo ran CC without -framework CoreFoundation (or any of the other
flags).

In CL 334732, we pass CC through to cgo, and cgo splits arguments
using str.SplitQuotedFields. So -framework CoreFoundation actually
gets passed to the C compiler. It appears that -framework flags are
only meant to be used in linking operations, so when cgo invokes clang
with -E (run preprocessor only), clang emits an error that -framework
is unused.

This change fixes the test by moving -framework CoreFoundation out of
CC and into CGO_LDFLAGS.

Change-Id: I2d5d89ddb19c94adef65982a8137b01f037d5c11
Reviewed-on: https://go-review.googlesource.com/c/go/+/334732
Trust: Jay Conrod <[email protected]>
Trust: Michael Matloob <[email protected]>
Run-TryBot: Jay Conrod <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/341936
Reviewed-by: Bryan C. Mills <[email protected]>

Author: Jay Conrod

src/cmd/cgo/gcc.go

@@ -23,10 +23,13 @@ import (
 	"internal/xcoff"
 	"math"
 	"os"
+	"os/exec"
 	"strconv"
 	"strings"
 	"unicode"
 	"unicode/utf8"
+
+	"cmd/internal/str"
 )
 
 var debugDefine = flag.Bool("debug-define", false, "print relevant #defines")
@@ -382,7 +385,7 @@ func (p *Package) guessKinds(f *File) []*Name {
 		stderr = p.gccErrors(b.Bytes())
 	}
 	if stderr == "" {
-		fatalf("%s produced no output\non input:\n%s", p.gccBaseCmd()[0], b.Bytes())
+		fatalf("%s produced no output\non input:\n%s", gccBaseCmd[0], b.Bytes())
 	}
 
 	completed := false
@@ -457,7 +460,7 @@ func (p *Package) guessKinds(f *File) []*Name {
 	}
 
 	if !completed {
-		fatalf("%s did not produce error at completed:1\non input:\n%s\nfull error output:\n%s", p.gccBaseCmd()[0], b.Bytes(), stderr)
+		fatalf("%s did not produce error at completed:1\non input:\n%s\nfull error output:\n%s", gccBaseCmd[0], b.Bytes(), stderr)
 	}
 
 	for i, n := range names {
@@ -488,7 +491,7 @@ func (p *Package) guessKinds(f *File) []*Name {
 		// to users debugging preamble mistakes. See issue 8442.
 		preambleErrors := p.gccErrors([]byte(f.Preamble))
 		if len(preambleErrors) > 0 {
-			error_(token.NoPos, "\n%s errors for preamble:\n%s", p.gccBaseCmd()[0], preambleErrors)
+			error_(token.NoPos, "\n%s errors for preamble:\n%s", gccBaseCmd[0], preambleErrors)
 		}
 
 		fatalf("unresolved names")
@@ -1545,20 +1548,37 @@ func gofmtPos(n ast.Expr, pos token.Pos) string {
 	return fmt.Sprintf("/*line :%d:%d*/%s", p.Line, p.Column, s)
 }
 
-// gccBaseCmd returns the start of the compiler command line.
+// checkGCCBaseCmd returns the start of the compiler command line.
 // It uses $CC if set, or else $GCC, or else the compiler recorded
 // during the initial build as defaultCC.
 // defaultCC is defined in zdefaultcc.go, written by cmd/dist.
-func (p *Package) gccBaseCmd() []string {
+//
+// The compiler command line is split into arguments on whitespace. Quotes
+// are understood, so arguments may contain whitespace.
+//
+// checkGCCBaseCmd confirms that the compiler exists in PATH, returning
+// an error if it does not.
+func checkGCCBaseCmd() ([]string, error) {
 	// Use $CC if set, since that's what the build uses.
-	if ret := strings.Fields(os.Getenv("CC")); len(ret) > 0 {
-		return ret
+	value := os.Getenv("CC")
+	if value == "" {
+		// Try $GCC if set, since that's what we used to use.
+		value = os.Getenv("GCC")
+	}
+	if value == "" {
+		value = defaultCC(goos, goarch)
+	}
+	args, err := str.SplitQuotedFields(value)
+	if err != nil {
+		return nil, err
+	}
+	if len(args) == 0 {
+		return nil, errors.New("CC not set and no default found")
 	}
-	// Try $GCC if set, since that's what we used to use.
-	if ret := strings.Fields(os.Getenv("GCC")); len(ret) > 0 {
-		return ret
+	if _, err := exec.LookPath(args[0]); err != nil {
+		return nil, fmt.Errorf("C compiler %q not found: %v", args[0], err)
 	}
-	return strings.Fields(defaultCC(goos, goarch))
+	return args[:len(args):len(args)], nil
 }
 
 // gccMachine returns the gcc -m flag to use, either "-m32", "-m64" or "-marm".
@@ -1604,7 +1624,7 @@ func gccTmp() string {
 // gccCmd returns the gcc command line to use for compiling
 // the input.
 func (p *Package) gccCmd() []string {
-	c := append(p.gccBaseCmd(),
+	c := append(gccBaseCmd,
 		"-w",          // no warnings
 		"-Wno-error",  // warnings are not errors
 		"-o"+gccTmp(), // write object to tmp
@@ -2005,7 +2025,7 @@ func (p *Package) gccDebug(stdin []byte, nnames int) (d *dwarf.Data, ints []int6
 // #defines that gcc encountered while processing the input
 // and its included files.
 func (p *Package) gccDefines(stdin []byte) string {
-	base := append(p.gccBaseCmd(), "-E", "-dM", "-xc")
+	base := append(gccBaseCmd, "-E", "-dM", "-xc")
 	base = append(base, p.gccMachine()...)
 	stdout, _ := runGcc(stdin, append(append(base, p.GccOptions...), "-"))
 	return stdout

src/cmd/cgo/main.go

@@ -21,7 +21,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"reflect"
 	"runtime"
@@ -248,6 +247,7 @@ var importSyscall = flag.Bool("import_syscall", true, "import syscall in generat
 var trimpath = flag.String("trimpath", "", "applies supplied rewrites or trims prefixes to recorded source file paths")
 
 var goarch, goos, gomips, gomips64 string
+var gccBaseCmd []string
 
 func main() {
 	objabi.AddVersionFlag() // -V
@@ -305,10 +305,10 @@ func main() {
 	p := newPackage(args[:i])
 
 	// We need a C compiler to be available. Check this.
-	gccName := p.gccBaseCmd()[0]
-	_, err := exec.LookPath(gccName)
+	var err error
+	gccBaseCmd, err = checkGCCBaseCmd()
 	if err != nil {
-		fatalf("C compiler %q not found: %v", gccName, err)
+		fatalf("%v", err)
 		os.Exit(2)
 	}
 

src/cmd/compile/internal/ssa/stmtlines_test.go

@@ -2,6 +2,7 @@ package ssa_test
 
 import (
 	cmddwarf "cmd/internal/dwarf"
+	"cmd/internal/str"
 	"debug/dwarf"
 	"debug/elf"
 	"debug/macho"
@@ -57,7 +58,11 @@ func TestStmtLines(t *testing.T) {
 		if extld == "" {
 			extld = "gcc"
 		}
-		enabled, err := cmddwarf.IsDWARFEnabledOnAIXLd(extld)
+		extldArgs, err := str.SplitQuotedFields(extld)
+		if err != nil {
+			t.Fatal(err)
+		}
+		enabled, err := cmddwarf.IsDWARFEnabledOnAIXLd(extldArgs)
 		if err != nil {
 			t.Fatal(err)
 		}

src/cmd/dist/buildtool.go

@@ -47,6 +47,7 @@ var bootstrapDirs = []string{
 	"cmd/internal/objabi",
 	"cmd/internal/pkgpath",
 	"cmd/internal/src",
+	"cmd/internal/str",
 	"cmd/internal/sys",
 	"cmd/link",
 	"cmd/link/internal/...",

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

@@ -26,6 +26,7 @@ import (
 	"cmd/go/internal/load"
 	"cmd/go/internal/modload"
 	"cmd/go/internal/work"
+	"cmd/internal/str"
 )
 
 var CmdEnv = &base.Command{
@@ -104,13 +105,13 @@ func MkEnv() []cfg.EnvVar {
 		env = append(env, cfg.EnvVar{Name: key, Value: val})
 	}
 
-	cc := cfg.DefaultCC(cfg.Goos, cfg.Goarch)
-	if env := strings.Fields(cfg.Getenv("CC")); len(env) > 0 {
-		cc = env[0]
+	cc := cfg.Getenv("CC")
+	if cc == "" {
+		cc = cfg.DefaultCC(cfg.Goos, cfg.Goarch)
 	}
-	cxx := cfg.DefaultCXX(cfg.Goos, cfg.Goarch)
-	if env := strings.Fields(cfg.Getenv("CXX")); len(env) > 0 {
-		cxx = env[0]
+	cxx := cfg.Getenv("CXX")
+	if cxx == "" {
+		cxx = cfg.DefaultCXX(cfg.Goos, cfg.Goarch)
 	}
 	env = append(env, cfg.EnvVar{Name: "AR", Value: envOr("AR", "ar")})
 	env = append(env, cfg.EnvVar{Name: "CC", Value: cc})
@@ -457,10 +458,23 @@ func checkEnvWrite(key, val string) error {
 		if !filepath.IsAbs(val) && val != "" {
 			return fmt.Errorf("GOPATH entry is relative; must be absolute path: %q", val)
 		}
-	// Make sure CC and CXX are absolute paths
-	case "CC", "CXX", "GOMODCACHE":
-		if !filepath.IsAbs(val) && val != "" && val != filepath.Base(val) {
-			return fmt.Errorf("%s entry is relative; must be absolute path: %q", key, val)
+	case "GOMODCACHE":
+		if !filepath.IsAbs(val) && val != "" {
+			return fmt.Errorf("GOMODCACHE entry is relative; must be absolute path: %q", val)
+		}
+	case "CC", "CXX":
+		if val == "" {
+			break
+		}
+		args, err := str.SplitQuotedFields(val)
+		if err != nil {
+			return fmt.Errorf("invalid %s: %v", key, err)
+		}
+		if len(args) == 0 {
+			return fmt.Errorf("%s entry cannot contain only space", key)
+		}
+		if !filepath.IsAbs(args[0]) && args[0] != filepath.Base(args[0]) {
+			return fmt.Errorf("%s entry is relative; must be absolute path: %q", key, args[0])
 		}
 	}
 

src/cmd/go/internal/work/exec.go

@@ -1487,6 +1487,8 @@ func (b *Builder) getPkgConfigFlags(p *load.Package) (cflags, ldflags []string,
 			return nil, nil, errPrintedOutput
 		}
 		if len(out) > 0 {
+			// NOTE: we don't attempt to parse quotes and unescapes here. pkg-config
+			// is typically used within shell backticks, which treats quotes literally.
 			ldflags = strings.Fields(string(out))
 			if err := checkLinkerFlags("LDFLAGS", "pkg-config --libs", ldflags); err != nil {
 				return nil, nil, err
@@ -2429,12 +2431,6 @@ func (b *Builder) gccld(a *Action, p *load.Package, objdir, outfile string, flag
 	return err
 }
 
-// Grab these before main helpfully overwrites them.
-var (
-	origCC  = cfg.Getenv("CC")
-	origCXX = cfg.Getenv("CXX")
-)
-
 // gccCmd returns a gcc command line prefix
 // defaultCC is defined in zdefaultcc.go, written by cmd/dist.
 func (b *Builder) GccCmd(incdir, workdir string) []string {
@@ -2454,40 +2450,23 @@ func (b *Builder) gfortranCmd(incdir, workdir string) []string {
 
 // ccExe returns the CC compiler setting without all the extra flags we add implicitly.
 func (b *Builder) ccExe() []string {
-	return b.compilerExe(origCC, cfg.DefaultCC(cfg.Goos, cfg.Goarch))
+	return envList("CC", cfg.DefaultCC(cfg.Goos, cfg.Goarch))
 }
 
 // cxxExe returns the CXX compiler setting without all the extra flags we add implicitly.
 func (b *Builder) cxxExe() []string {
-	return b.compilerExe(origCXX, cfg.DefaultCXX(cfg.Goos, cfg.Goarch))
+	return envList("CXX", cfg.DefaultCXX(cfg.Goos, cfg.Goarch))
 }
 
 // fcExe returns the FC compiler setting without all the extra flags we add implicitly.
 func (b *Builder) fcExe() []string {
-	return b.compilerExe(cfg.Getenv("FC"), "gfortran")
-}
-
-// compilerExe returns the compiler to use given an
-// environment variable setting (the value not the name)
-// and a default. The resulting slice is usually just the name
-// of the compiler but can have additional arguments if they
-// were present in the environment value.
-// For example if CC="gcc -DGOPHER" then the result is ["gcc", "-DGOPHER"].
-func (b *Builder) compilerExe(envValue string, def string) []string {
-	compiler := strings.Fields(envValue)
-	if len(compiler) == 0 {
-		compiler = strings.Fields(def)
-	}
-	return compiler
+	return envList("FC", "gfortran")
 }
 
 // compilerCmd returns a command line prefix for the given environment
 // variable and using the default command when the variable is empty.
 func (b *Builder) compilerCmd(compiler []string, incdir, workdir string) []string {
-	// NOTE: env.go's mkEnv knows that the first three
-	// strings returned are "gcc", "-I", incdir (and cuts them off).
-	a := []string{compiler[0], "-I", incdir}
-	a = append(a, compiler[1:]...)
+	a := append(compiler, "-I", incdir)
 
 	// Definitely want -fPIC but on Windows gcc complains
 	// "-fPIC ignored for target (all code is position independent)"
@@ -2658,12 +2637,20 @@ func (b *Builder) gccArchArgs() []string {
 
 // envList returns the value of the given environment variable broken
 // into fields, using the default value when the variable is empty.
+//
+// The environment variable must be quoted correctly for
+// str.SplitQuotedFields. This should be done before building
+// anything, for example, in BuildInit.
 func envList(key, def string) []string {
 	v := cfg.Getenv(key)
 	if v == "" {
 		v = def
 	}
-	return strings.Fields(v)
+	args, err := str.SplitQuotedFields(v)
+	if err != nil {
+		panic(fmt.Sprintf("could not parse environment variable %s with value %q: %v", key, v, err))
+	}
+	return args
 }
 
 // CFlags returns the flags to use when invoking the C, C++ or Fortran compilers, or cgo.

src/cmd/go/internal/work/gc.go

@@ -545,33 +545,18 @@ func packInternal(afile string, ofiles []string) error {
 }
 
 // setextld sets the appropriate linker flags for the specified compiler.
-func setextld(ldflags []string, compiler []string) []string {
+func setextld(ldflags []string, compiler []string) ([]string, error) {
 	for _, f := range ldflags {
 		if f == "-extld" || strings.HasPrefix(f, "-extld=") {
 			// don't override -extld if supplied
-			return ldflags
+			return ldflags, nil
 		}
 	}
-	ldflags = append(ldflags, "-extld="+compiler[0])
-	if len(compiler) > 1 {
-		extldflags := false
-		add := strings.Join(compiler[1:], " ")
-		for i, f := range ldflags {
-			if f == "-extldflags" && i+1 < len(ldflags) {
-				ldflags[i+1] = add + " " + ldflags[i+1]
-				extldflags = true
-				break
-			} else if strings.HasPrefix(f, "-extldflags=") {
-				ldflags[i] = "-extldflags=" + add + " " + ldflags[i][len("-extldflags="):]
-				extldflags = true
-				break
-			}
-		}
-		if !extldflags {
-			ldflags = append(ldflags, "-extldflags="+add)
-		}
+	joined, err := str.JoinAndQuoteFields(compiler)
+	if err != nil {
+		return nil, err
 	}
-	return ldflags
+	return append(ldflags, "-extld="+joined), nil
 }
 
 // pluginPath computes the package path for a plugin main package.
@@ -658,7 +643,10 @@ func (gcToolchain) ld(b *Builder, root *Action, out, importcfg, mainpkg string)
 	}
 	ldflags = append(ldflags, forcedLdflags...)
 	ldflags = append(ldflags, root.Package.Internal.Ldflags...)
-	ldflags = setextld(ldflags, compiler)
+	ldflags, err := setextld(ldflags, compiler)
+	if err != nil {
+		return err
+	}
 
 	// On OS X when using external linking to build a shared library,
 	// the argument passed here to -o ends up recorded in the final
@@ -702,7 +690,10 @@ func (gcToolchain) ldShared(b *Builder, root *Action, toplevelactions []*Action,
 	} else {
 		compiler = envList("CC", cfg.DefaultCC(cfg.Goos, cfg.Goarch))
 	}
-	ldflags = setextld(ldflags, compiler)
+	ldflags, err := setextld(ldflags, compiler)
+	if err != nil {
+		return err
+	}
 	for _, d := range toplevelactions {
 		if !strings.HasSuffix(d.Target, ".a") { // omit unsafe etc and actions for other shared libraries
 			continue