Allow to remove env variables.

Previously, we only supported setting env variables
to empty, but not to remove it. This lead to the case
that we never removed the CCACHE_DISABLE env variable,
which kept the ccache disabled, and caused a performance
regression compared to the old wrapper as the new wrapper
didn't use the ccache in this case.

This cl also adds tests for the real exec and run commands
to prevent regressions in these cases.

BUG=chromium:773875
TEST=new unit tests
TEST=performance analysis for
TEST=emerge-veyron_jerry --nodeps chromeos-kernel-4_19

Change-Id: I5ca88ba8d7b05c3e12e292465fcd4ff9925b0344
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/1802159
Tested-by: Tobias Bosch <tbosch@google.com>
Reviewed-by: George Burgess <gbiv@chromium.org>

16 files changed, 410 insertions, 67 deletions

diff --git a/compiler_wrapper/bisect_flag.go b/compiler_wrapper/bisect_flag.go
index 6c671738..2a805352 100644
--- a/compiler_wrapper/bisect_flag.go
+++ b/compiler_wrapper/bisect_flag.go
@@ -7,11 +7,12 @@ package main
 const bisectPythonCommand = "import bisect_driver; sys.exit(bisect_driver.bisect_driver(sys.argv[1], sys.argv[2], sys.argv[3:]))"
 
 func getBisectStage(env env) string {
-	return env.getenv("BISECT_STAGE")
+	value, _ := env.getenv("BISECT_STAGE")
+	return value
 }
 
 func calcBisectCommand(env env, bisectStage string, compilerCmd *command) *command {
-	bisectDir := env.getenv("BISECT_DIR")
+	bisectDir, _ := env.getenv("BISECT_DIR")
 	if bisectDir == "" {
 		bisectDir = "/tmp/sysroot_bisect"
 	}
diff --git a/compiler_wrapper/ccache_flag.go b/compiler_wrapper/ccache_flag.go
index 640a99fc..312d0f02 100644
--- a/compiler_wrapper/ccache_flag.go
+++ b/compiler_wrapper/ccache_flag.go
@@ -38,7 +38,7 @@ func processCCacheFlag(sysroot string, builder *commandBuilder) {
 		// All of those will get cache hits (ignoring the first one
 		// which will seed the cache) due to this setting.
 		builder.updateEnv("CCACHE_BASEDIR=" + sysroot)
-		if builder.env.getenv("CCACHE_DISABLE") != "" {
+		if _, present := builder.env.getenv("CCACHE_DISABLE"); present {
 			// Portage likes to set this for us when it has FEATURES=-ccache.
 			// The other vars we need to setup manually because of tools like
 			// scons that scrubs the env before we get executed.
@@ -46,7 +46,7 @@ func processCCacheFlag(sysroot string, builder *commandBuilder) {
 		}
 		// If RESTRICT=sandbox is enabled, then sandbox won't be setup,
 		// and the env vars won't be available for appending.
-		if sandboxRewrite := builder.env.getenv("SANDBOX_WRITE"); sandboxRewrite != "" {
+		if sandboxRewrite, present := builder.env.getenv("SANDBOX_WRITE"); present {
 			builder.updateEnv("SANDBOX_WRITE=" + sandboxRewrite + ":" + ccacheDir)
 		}
 
diff --git a/compiler_wrapper/ccache_flag_test.go b/compiler_wrapper/ccache_flag_test.go
index 4d43caf7..61abef06 100644
--- a/compiler_wrapper/ccache_flag_test.go
+++ b/compiler_wrapper/ccache_flag_test.go
@@ -75,7 +75,7 @@ func TestSetCacheUmask(t *testing.T) {
 	})
 }
 
-func TestUpdateSandboxRewrite(t *testing.T) {
+func TestUpdateSandboxRewriteWithValue(t *testing.T) {
 	withCCacheEnabledTestContext(t, func(ctx *testContext) {
 		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
 			ctx.newCommand(gccX86_64, mainCc)))
@@ -93,7 +93,7 @@ func TestUpdateSandboxRewrite(t *testing.T) {
 	})
 }
 
-func TestClearCacheDisable(t *testing.T) {
+func TestUpdateSandboxRewriteWithoutValue(t *testing.T) {
 	withCCacheEnabledTestContext(t, func(ctx *testContext) {
 		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
 			ctx.newCommand(gccX86_64, mainCc)))
@@ -101,6 +101,24 @@ func TestClearCacheDisable(t *testing.T) {
 			t.Error(err)
 		}
 
+		ctx.env = []string{"SANDBOX_WRITE="}
+		cmd = ctx.must(callCompiler(ctx, ctx.cfg,
+			ctx.newCommand(gccX86_64, mainCc)))
+		if err := verifyEnvUpdate(cmd,
+			"SANDBOX_WRITE=:/var/cache/distfiles/ccache"); err != nil {
+			t.Error(err)
+		}
+	})
+}
+
+func TestClearCCacheDisableWithValue(t *testing.T) {
+	withCCacheEnabledTestContext(t, func(ctx *testContext) {
+		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
+			ctx.newCommand(gccX86_64, mainCc)))
+		if err := verifyNoEnvUpdate(cmd, "CCACHE_DISABLE"); err != nil {
+			t.Error(err)
+		}
+
 		ctx.env = []string{"CCACHE_DISABLE=true"}
 		cmd = ctx.must(callCompiler(ctx, ctx.cfg,
 			ctx.newCommand(gccX86_64, mainCc)))
@@ -110,6 +128,23 @@ func TestClearCacheDisable(t *testing.T) {
 	})
 }
 
+func TestClearCCacheDisableWithoutValue(t *testing.T) {
+	withCCacheEnabledTestContext(t, func(ctx *testContext) {
+		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
+			ctx.newCommand(gccX86_64, mainCc)))
+		if err := verifyNoEnvUpdate(cmd, "CCACHE_DISABLE"); err != nil {
+			t.Error(err)
+		}
+
+		ctx.env = []string{"CCACHE_DISABLE="}
+		cmd = ctx.must(callCompiler(ctx, ctx.cfg,
+			ctx.newCommand(gccX86_64, mainCc)))
+		if err := verifyEnvUpdate(cmd, "CCACHE_DISABLE="); err != nil {
+			t.Error(err)
+		}
+	})
+}
+
 func TestAddCCacheCpp2FlagForClang(t *testing.T) {
 	withCCacheEnabledTestContext(t, func(ctx *testContext) {
 		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
diff --git a/compiler_wrapper/clang_flags.go b/compiler_wrapper/clang_flags.go
index 30a0b576..8b76e965 100644
--- a/compiler_wrapper/clang_flags.go
+++ b/compiler_wrapper/clang_flags.go
@@ -13,7 +13,7 @@ import (
 
 func processClangFlags(builder *commandBuilder) error {
 	env := builder.env
-	clangDir := env.getenv("CLANG")
+	clangDir, _ := env.getenv("CLANG")
 
 	if clangDir == "" {
 		if builder.cfg.isHostWrapper {
diff --git a/compiler_wrapper/clang_tidy_flag.go b/compiler_wrapper/clang_tidy_flag.go
index 616ff47c..40a5bdbe 100644
--- a/compiler_wrapper/clang_tidy_flag.go
+++ b/compiler_wrapper/clang_tidy_flag.go
@@ -11,7 +11,8 @@ import (
 )
 
 func processClangTidyFlags(builder *commandBuilder) (cSrcFile string, useClangTidy bool) {
-	if builder.env.getenv("WITH_TIDY") == "" {
+	withTidy, _ := builder.env.getenv("WITH_TIDY")
+	if withTidy == "" {
 		return "", false
 	}
 	srcFileSuffixes := []string{
diff --git a/compiler_wrapper/command.go b/compiler_wrapper/command.go
index 0ef2ee15..925302fa 100644
--- a/compiler_wrapper/command.go
+++ b/compiler_wrapper/command.go
@@ -6,6 +6,7 @@ package main
 
 import (
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -13,8 +14,12 @@ import (
 )
 
 type command struct {
-	Path       string   `json:"path"`
-	Args       []string `json:"args"`
+	Path string   `json:"path"`
+	Args []string `json:"args"`
+	// Updates and additions have the form:
+	// `NAME=VALUE`
+	// Removals have the form:
+	// `NAME=`.
 	EnvUpdates []string `json:"env_updates,omitempty"`
 }
 
@@ -25,25 +30,42 @@ func newProcessCommand() *command {
 	}
 }
 
-func newExecCmd(env env, cmd *command) *exec.Cmd {
-	execCmd := exec.Command(cmd.Path, cmd.Args...)
-	execCmd.Env = append(env.environ(), cmd.EnvUpdates...)
-	ensurePathEnv(execCmd)
-	execCmd.Dir = env.getwd()
-	return execCmd
-}
-
-func ensurePathEnv(cmd *exec.Cmd) {
-	for _, env := range cmd.Env {
-		if strings.HasPrefix(env, "PATH=") {
-			return
+func mergeEnvValues(values []string, updates []string) []string {
+	envMap := map[string]string{}
+	for _, entry := range values {
+		equalPos := strings.IndexRune(entry, '=')
+		envMap[entry[:equalPos]] = entry[equalPos+1:]
+	}
+	for _, update := range updates {
+		equalPos := strings.IndexRune(update, '=')
+		key := update[:equalPos]
+		value := update[equalPos+1:]
+		if value == "" {
+			delete(envMap, key)
+		} else {
+			envMap[key] = value
 		}
 	}
-	cmd.Env = append(cmd.Env, "PATH=")
+	env := []string{}
+	for key, value := range envMap {
+		env = append(env, key+"="+value)
+	}
+	return env
+}
+
+func runCmd(env env, cmd *command, stdin io.Reader, stdout io.Writer, stderr io.Writer) error {
+	execCmd := exec.Command(cmd.Path, cmd.Args...)
+	execCmd.Env = mergeEnvValues(env.environ(), cmd.EnvUpdates)
+	execCmd.Dir = env.getwd()
+	execCmd.Stdin = stdin
+	execCmd.Stdout = stdout
+	execCmd.Stderr = stderr
+	return execCmd.Run()
 }
 
 func resolveAgainstPathEnv(env env, cmd string) (string, error) {
-	for _, path := range strings.Split(env.getenv("PATH"), ":") {
+	path, _ := env.getenv("PATH")
+	for _, path := range strings.Split(path, ":") {
 		resolvedPath := filepath.Join(path, cmd)
 		if _, err := os.Lstat(resolvedPath); err == nil {
 			return resolvedPath, nil
diff --git a/compiler_wrapper/command_test.go b/compiler_wrapper/command_test.go
new file mode 100644
index 00000000..18d05a9c
--- /dev/null
+++ b/compiler_wrapper/command_test.go
@@ -0,0 +1,39 @@
+// Copyright 2019 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package main
+
+import (
+	"reflect"
+	"sort"
+	"testing"
+)
+
+func TestMergeEnvValues(t *testing.T) {
+	testData := []struct {
+		values  []string
+		updates []string
+		result  []string
+	}{
+		{[]string{}, []string{}, []string{}},
+		{[]string{"A=1"}, []string{}, []string{"A=1"}},
+		{[]string{"A=1=2=3"}, []string{}, []string{"A=1=2=3"}},
+		{[]string{}, []string{"A=1"}, []string{"A=1"}},
+		{[]string{}, []string{"A=1=2=3"}, []string{"A=1=2=3"}},
+		{[]string{"A=1"}, []string{"A=2"}, []string{"A=2"}},
+		{[]string{"A="}, []string{}, []string{"A="}},
+		{[]string{"A="}, []string{"A=2"}, []string{"A=2"}},
+		{[]string{"A=1"}, []string{"A="}, []string{}},
+		{[]string{}, []string{"A=1", "A="}, []string{}},
+		{[]string{}, []string{"A=1", "A=", "A=2"}, []string{"A=2"}},
+		{[]string{"A=1", "B=2"}, []string{"C=3", "D=4"}, []string{"A=1", "B=2", "C=3", "D=4"}},
+	}
+	for _, tt := range testData {
+		result := mergeEnvValues(tt.values, tt.updates)
+		sort.Strings(result)
+		if !reflect.DeepEqual(tt.result, result) {
+			t.Errorf("unexpected result: %s", result)
+		}
+	}
+}
diff --git a/compiler_wrapper/disable_werror_flag.go b/compiler_wrapper/disable_werror_flag.go
index b639eb37..864397dd 100644
--- a/compiler_wrapper/disable_werror_flag.go
+++ b/compiler_wrapper/disable_werror_flag.go
@@ -14,7 +14,8 @@ import (
 )
 
 func shouldForceDisableWError(env env) bool {
-	return env.getenv("FORCE_DISABLE_WERROR") != ""
+	value, _ := env.getenv("FORCE_DISABLE_WERROR")
+	return value != ""
 }
 
 func doubleBuildWithWNoError(env env, cfg *config, originalCmd *command) (exitCode int, err error) {
diff --git a/compiler_wrapper/env.go b/compiler_wrapper/env.go
index 588441dd..c8f31357 100644
--- a/compiler_wrapper/env.go
+++ b/compiler_wrapper/env.go
@@ -9,12 +9,11 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"os/exec"
 	"strings"
 )
 
 type env interface {
-	getenv(key string) string
+	getenv(key string) (string, bool)
 	environ() []string
 	getwd() string
 	stdin() io.Reader
@@ -47,8 +46,8 @@ func newProcessEnv() (env, error) {
 
 var _ env = (*processEnv)(nil)
 
-func (env *processEnv) getenv(key string) string {
-	return os.Getenv(key)
+func (env *processEnv) getenv(key string) (string, bool) {
+	return os.LookupEnv(key)
 }
 
 func (env *processEnv) environ() []string {
@@ -72,26 +71,11 @@ func (env *processEnv) stderr() io.Writer {
 }
 
 func (env *processEnv) exec(cmd *command) error {
-	execCmd := exec.Command(cmd.Path, cmd.Args...)
-	// Note: We are not using execve and pass the new environment here
-	// as that sometimes doesn't work well with the gentoo sandbox to
-	// pick update changes to SANDBOX_WRITE env variable (needed for ccache).
-	// Instead, we are updating our own environment and call execv.
-	// This update of global state is ok as we won't execute anything else
-	// after the exec.
-	for _, update := range cmd.EnvUpdates {
-		parts := strings.Split(update, "=")
-		os.Setenv(parts[0], parts[1])
-	}
-	return libcExecv(execCmd.Path, execCmd.Args)
+	return libcExec(cmd)
 }
 
 func (env *processEnv) run(cmd *command, stdin io.Reader, stdout io.Writer, stderr io.Writer) error {
-	execCmd := newExecCmd(env, cmd)
-	execCmd.Stdin = stdin
-	execCmd.Stdout = stdout
-	execCmd.Stderr = stderr
-	return execCmd.Run()
+	return runCmd(env, cmd, stdin, stdout, stderr)
 }
 
 type commandRecordingEnv struct {
diff --git a/compiler_wrapper/env_test.go b/compiler_wrapper/env_test.go
new file mode 100644
index 00000000..05d7d136
--- /dev/null
+++ b/compiler_wrapper/env_test.go
@@ -0,0 +1,215 @@
+// Copyright 2019 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package main
+
+import (
+	"bytes"
+	"flag"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// Attention: The tests in this file execute the test binary again with the `-run` flag.
+// This is needed as they want to test an `exec`, which terminates the test process.
+var internalexececho = flag.Bool("internalexececho", false, "internal flag used for tests that exec")
+
+func TestProcessEnvExecPathAndArgs(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		if *internalexececho {
+			execEcho(ctx, &command{
+				Path: "some_binary",
+				Args: []string{"arg1", "arg2"},
+			})
+			return
+		}
+		logLines := forkAndReadEcho(ctx)
+		if !strings.HasSuffix(logLines[0], "/some_binary arg1 arg2") {
+			t.Errorf("incorrect path or args: %s", logLines[0])
+		}
+	})
+}
+
+func TestProcessEnvExecAddEnv(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		if *internalexececho {
+			execEcho(ctx, &command{
+				Path:       "some_binary",
+				EnvUpdates: []string{"ABC=xyz"},
+			})
+			return
+		}
+
+		logLines := forkAndReadEcho(ctx)
+		for _, ll := range logLines {
+			if ll == "ABC=xyz" {
+				return
+			}
+		}
+		t.Errorf("could not find new env variable: %s", logLines)
+	})
+}
+
+func TestProcessEnvExecUpdateEnv(t *testing.T) {
+	if os.Getenv("PATH") == "" {
+		t.Fatal("no PATH environment variable found!")
+	}
+	withTestContext(t, func(ctx *testContext) {
+		if *internalexececho {
+			execEcho(ctx, &command{
+				Path:       "some_binary",
+				EnvUpdates: []string{"PATH=xyz"},
+			})
+			return
+		}
+		logLines := forkAndReadEcho(ctx)
+		for _, ll := range logLines {
+			if ll == "PATH=xyz" {
+				return
+			}
+		}
+		t.Errorf("could not find updated env variable: %s", logLines)
+	})
+}
+
+func TestProcessEnvExecDeleteEnv(t *testing.T) {
+	if os.Getenv("PATH") == "" {
+		t.Fatal("no PATH environment variable found!")
+	}
+	withTestContext(t, func(ctx *testContext) {
+		if *internalexececho {
+			execEcho(ctx, &command{
+				Path:       "some_binary",
+				EnvUpdates: []string{"PATH="},
+			})
+			return
+		}
+		logLines := forkAndReadEcho(ctx)
+		for _, ll := range logLines {
+			if strings.HasPrefix(ll, "PATH=") {
+				t.Errorf("path env was not removed: %s", ll)
+			}
+		}
+	})
+}
+
+func TestProcessEnvRunCmdPathAndArgs(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		cmd := &command{
+			Path: "some_binary",
+			Args: []string{"arg1", "arg2"},
+		}
+		logLines := runAndEcho(ctx, cmd)
+		if !strings.HasSuffix(logLines[0], "/some_binary arg1 arg2") {
+			t.Errorf("incorrect path or args: %s", logLines[0])
+		}
+	})
+}
+
+func TestProcessEnvRunCmdAddEnv(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		cmd := &command{
+			Path:       "some_binary",
+			EnvUpdates: []string{"ABC=xyz"},
+		}
+		logLines := runAndEcho(ctx, cmd)
+		for _, ll := range logLines {
+			if ll == "ABC=xyz" {
+				return
+			}
+		}
+		t.Errorf("could not find new env variable: %s", logLines)
+	})
+}
+
+func TestProcessEnvRunCmdUpdateEnv(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		if os.Getenv("PATH") == "" {
+			t.Fatal("no PATH environment variable found!")
+		}
+		cmd := &command{
+			Path:       "some_binary",
+			EnvUpdates: []string{"PATH=xyz"},
+		}
+		logLines := runAndEcho(ctx, cmd)
+		for _, ll := range logLines {
+			if ll == "PATH=xyz" {
+				return
+			}
+		}
+		t.Errorf("could not find updated env variable: %s", logLines)
+	})
+}
+
+func TestProcessEnvRunCmdDeleteEnv(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		if os.Getenv("PATH") == "" {
+			t.Fatal("no PATH environment variable found!")
+		}
+		cmd := &command{
+			Path:       "some_binary",
+			EnvUpdates: []string{"PATH="},
+		}
+		logLines := runAndEcho(ctx, cmd)
+		for _, ll := range logLines {
+			if strings.HasPrefix(ll, "PATH=") {
+				t.Errorf("path env was not removed: %s", ll)
+			}
+		}
+	})
+}
+
+func execEcho(ctx *testContext, cmd *command) {
+	env := &processEnv{}
+	err := env.exec(createEcho(ctx, cmd))
+	if err != nil {
+		os.Stderr.WriteString(err.Error())
+	}
+	os.Exit(1)
+}
+
+func forkAndReadEcho(ctx *testContext) []string {
+	testBin, err := os.Executable()
+	if err != nil {
+		ctx.t.Fatalf("unable to read the executable: %s", err)
+	}
+
+	subCmd := exec.Command(testBin, "-internalexececho", "-test.run="+ctx.t.Name())
+	output, err := subCmd.CombinedOutput()
+	if err != nil {
+		ctx.t.Fatalf("error calling test binary again for exec: %s", err)
+	}
+	return strings.Split(string(output), "\n")
+}
+
+func runAndEcho(ctx *testContext, cmd *command) []string {
+	env, err := newProcessEnv()
+	if err != nil {
+		ctx.t.Fatalf("creation of process env failed: %s", err)
+	}
+	buffer := bytes.Buffer{}
+	if err := env.run(createEcho(ctx, cmd), nil, &buffer, &buffer); err != nil {
+		ctx.t.Fatalf("run failed: %s", err)
+	}
+	return strings.Split(buffer.String(), "\n")
+}
+
+func createEcho(ctx *testContext, cmd *command) *command {
+	content := `
+/usr/bin/echo "$0" "$@"
+/usr/bin/env
+`
+	fullPath := filepath.Join(ctx.tempDir, cmd.Path)
+	ctx.writeFile(fullPath, content)
+	// Note: Using a self executable wrapper does not work due to a race condition
+	// on unix systems. See https://github.com/golang/go/issues/22315
+	return &command{
+		Path:       "bash",
+		Args:       append([]string{fullPath}, cmd.Args...),
+		EnvUpdates: cmd.EnvUpdates,
+	}
+}
diff --git a/compiler_wrapper/gomacc_flag.go b/compiler_wrapper/gomacc_flag.go
index 53dce2c5..bdcd92ee 100644
--- a/compiler_wrapper/gomacc_flag.go
+++ b/compiler_wrapper/gomacc_flag.go
@@ -9,7 +9,7 @@ import (
 )
 
 func processGomaCccFlags(builder *commandBuilder) (gomaUsed bool) {
-	if gomaPath := builder.env.getenv("GOMACC_PATH"); gomaPath != "" {
+	if gomaPath, _ := builder.env.getenv("GOMACC_PATH"); gomaPath != "" {
 		if _, err := os.Lstat(gomaPath); err == nil {
 			builder.wrapPath(gomaPath)
 			return true
diff --git a/compiler_wrapper/libc_execv.go b/compiler_wrapper/libc_exec.go
index 19c40ef0..2ee9b7f5 100644
--- a/compiler_wrapper/libc_execv.go
+++ b/compiler_wrapper/libc_exec.go
@@ -4,17 +4,45 @@
 
 package main
 
+// #include <errno.h>
 // #include <stdlib.h>
+// #include <string.h>
 // #include <unistd.h>
-// #include <errno.h>
-// int libc_execv(const char *pathname, char *const argv[]) {
-//   if (execv(pathname, argv) != 0) {
-//	   return errno;
-//   }
-//   return 0;
-// }
+//
+// int libc_exec(const char *pathname, char *const argv[], char *const env_updates[]) {
+//	// Note: We are not using execve and pass the new environment here
+//	// as that sometimes doesn't work well with the gentoo sandbox to
+//	// pick up changes to SANDBOX_WRITE env variable (needed for ccache).
+//	// Instead, we are updating our own environment and call execv.
+//	// This update of global state is ok as we won't execute anything else
+//	// after the exec.
+//	// Note: We don't update the environment already in go as these somehow
+//	// don't seem to update the real environment...
+//	int i;
+//	for (i = 0; env_updates[i] != NULL; ++i) {
+//		const char* update = env_updates[i];
+//		const char* pos = strchr(update, '=');
+//		if (pos == NULL) {
+//			continue;
+//		}
+//		char key[pos - update + 1];
+//		key[pos - update] = 0;
+//		strncpy(key, update, pos - update);
+//		if (pos[1] == 0) {
+//			// update has no value
+//			unsetenv(key);
+//		} else {
+//			setenv(key, &pos[1], /*overwrite=*/1);
+//		}
+//	}
+//	if (execv(pathname, argv) != 0) {
+//		return errno;
+//	}
+//	return 0;
+//}
 import "C"
 import (
+	"os/exec"
 	"unsafe"
 )
 
@@ -23,7 +51,7 @@ import (
 // LD_PRELOAD to work properly (e.g. gentoo sandbox).
 // Note that this changes the go binary to be a dynamically linked one.
 // See crbug.com/1000863 for details.
-func libcExecv(argv0 string, argv []string) error {
+func libcExec(cmd *command) error {
 	freeList := []unsafe.Pointer{}
 	defer func() {
 		for _, ptr := range freeList {
@@ -54,7 +82,8 @@ func libcExecv(argv0 string, argv []string) error {
 		return (**C.char)(cArray)
 	}
 
-	if errno := C.libc_execv(goStrToC(argv0), goSliceToC(argv)); errno != 0 {
+	execCmd := exec.Command(cmd.Path, cmd.Args...)
+	if errno := C.libc_exec(goStrToC(execCmd.Path), goSliceToC(execCmd.Args), goSliceToC(cmd.EnvUpdates)); errno != 0 {
 		return newErrorwithSourceLocf("exec error: %d", errno)
 	}
 
diff --git a/compiler_wrapper/rusage_flag.go b/compiler_wrapper/rusage_flag.go
index c3337364..2a3768c1 100644
--- a/compiler_wrapper/rusage_flag.go
+++ b/compiler_wrapper/rusage_flag.go
@@ -14,7 +14,8 @@ import (
 )
 
 func getRusageLogFilename(env env) string {
-	return env.getenv("GETRUSAGE")
+	value, _ := env.getenv("GETRUSAGE")
+	return value
 }
 
 func logRusage(env env, logFileName string, compilerCmd *command) (exitCode int, err error) {
diff --git a/compiler_wrapper/sysroot_flag.go b/compiler_wrapper/sysroot_flag.go
index 5d85289d..67625b3b 100644
--- a/compiler_wrapper/sysroot_flag.go
+++ b/compiler_wrapper/sysroot_flag.go
@@ -17,10 +17,11 @@ func processSysrootFlag(builder *commandBuilder) string {
 			break
 		}
 	}
-	sysroot := builder.env.getenv("SYSROOT")
-	if sysroot != "" {
+	sysroot, syrootPresent := builder.env.getenv("SYSROOT")
+	if syrootPresent {
 		builder.updateEnv("SYSROOT=")
-	} else {
+	}
+	if sysroot == "" {
 		// Use the bundled sysroot by default.
 		sysroot = filepath.Join(builder.rootPath, "usr", builder.target.target)
 	}
diff --git a/compiler_wrapper/sysroot_flag_test.go b/compiler_wrapper/sysroot_flag_test.go
index 1c757276..308d5e96 100644
--- a/compiler_wrapper/sysroot_flag_test.go
+++ b/compiler_wrapper/sysroot_flag_test.go
@@ -32,12 +32,29 @@ func TestSetSysrootFlagFromEnv(t *testing.T) {
 		ctx.env = []string{"SYSROOT=/envpath"}
 		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
 			ctx.newCommand(gccX86_64, mainCc)))
+		if err := verifyEnvUpdate(cmd, "SYSROOT="); err != nil {
+			t.Error(err)
+		}
 		if err := verifyArgOrder(cmd, "--sysroot=/envpath", mainCc); err != nil {
 			t.Error(err)
 		}
 	})
 }
 
+func TestClearEmptySysrootFlagInEnv(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		ctx.env = []string{"SYSROOT="}
+		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
+			ctx.newCommand(gccX86_64, mainCc)))
+		if err := verifyEnvUpdate(cmd, "SYSROOT="); err != nil {
+			t.Error(err)
+		}
+		if err := verifyArgOrder(cmd, "--sysroot=.*/x86_64-cros-linux-gnu", mainCc); err != nil {
+			t.Error(err)
+		}
+	})
+}
+
 func TestSetSysrootRelativeToWrapperPath(t *testing.T) {
 	withTestContext(t, func(ctx *testContext) {
 		ctx.cfg.rootRelPath = "somepath"
diff --git a/compiler_wrapper/testutil_test.go b/compiler_wrapper/testutil_test.go
index 3e568c77..3eb2c5fa 100644
--- a/compiler_wrapper/testutil_test.go
+++ b/compiler_wrapper/testutil_test.go
@@ -66,14 +66,14 @@ func withTestContext(t *testing.T, work func(ctx *testContext)) {
 
 var _ env = (*testContext)(nil)
 
-func (ctx *testContext) getenv(key string) string {
+func (ctx *testContext) getenv(key string) (string, bool) {
 	for i := len(ctx.env) - 1; i >= 0; i-- {
 		entry := ctx.env[i]
 		if strings.HasPrefix(entry, key+"=") {
-			return entry[len(key)+1:]
+			return entry[len(key)+1:], true
 		}
 	}
-	return ""
+	return "", false
 }
 
 func (ctx *testContext) environ() []string {
@@ -108,10 +108,7 @@ func (ctx *testContext) run(cmd *command, stdin io.Reader, stdout io.Writer, std
 	// Keep calling the old wrapper when we are comparing the output of the
 	// old wrapper to the new wrapper.
 	if isCompareToOldWrapperCmd(cmd) {
-		execCmd := newExecCmd(ctx, cmd)
-		execCmd.Stdout = stdout
-		execCmd.Stderr = stderr
-		return execCmd.Run()
+		return runCmd(ctx, cmd, nil, stdout, stderr)
 	}
 	ctx.cmdCount++
 	ctx.lastCmd = cmd