[compiler-wrapper] Handle double build for clang-tidy

For clang-tidy, the equivalent of -Werror is --warnings-as-errors (or
-warnings-as-errors).  The error message will also have
"warnings-as-errors".  So,
1. Trigger double-build if "warnings-as-errors" is in *stdout*.
Clang-tidy will also fail if clang issues a Werror diagnostic.  In this
case, we'd get "clang-diagnostic" in stdout.  Do a re-build in both
these cases.
2. Remove flags containing "-warnings-as-errors" in the rerun.  Unlike
-Werror/Wno-error, clang-tidy doesn't have a -no-warnings-as-errors to
override a prior -warnings-as-errors.  Existing double-build processing
will disable -Werror flags to Clang.

Originally reviewed at
https://android-review.googlesource.com/c/platform/external/toolchain-utils/+/1322200.

BUG=None
TEST=go test

Change-Id: I18de245972da81e0ac3600f9098b71cec82e9e96
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/2239370
Commit-Queue: Pirama Arumuga Nainar <pirama@google.com>
Tested-by: Pirama Arumuga Nainar <pirama@google.com>
Reviewed-by: George Burgess <gbiv@chromium.org>

3 files changed, 99 insertions, 14 deletions

diff --git a/compiler_wrapper/disable_werror_flag.go b/compiler_wrapper/disable_werror_flag.go
index f68786d4..75f71a0d 100644
--- a/compiler_wrapper/disable_werror_flag.go
+++ b/compiler_wrapper/disable_werror_flag.go
@@ -14,6 +14,8 @@ import (
 	"syscall"
 )
 
+const numWErrorEstimate = 30
+
 func getNewWarningsDir(env env, cfg *config) (dirName string, ok bool) {
 	if cfg.isAndroidWrapper {
 		if cfg.useLlvmNext {
@@ -32,13 +34,17 @@ func getNewWarningsDir(env env, cfg *config) (dirName string, ok bool) {
 }
 
 func disableWerrorFlags(originalArgs []string) []string {
-	noErrors := []string{"-Wno-error"}
+	extraArgs := []string{"-Wno-error"}
+	newArgs := make([]string, 0, len(originalArgs)+numWErrorEstimate)
 	for _, flag := range originalArgs {
 		if strings.HasPrefix(flag, "-Werror=") {
-			noErrors = append(noErrors, strings.Replace(flag, "-Werror", "-Wno-error", 1))
+			extraArgs = append(extraArgs, strings.Replace(flag, "-Werror", "-Wno-error", 1))
+		}
+		if !strings.Contains(flag, "-warnings-as-errors") {
+			newArgs = append(newArgs, flag)
 		}
 	}
-	return noErrors
+	return append(newArgs, extraArgs...)
 }
 
 func isLikelyAConfTest(cfg *config, cmd *command) bool {
@@ -75,9 +81,16 @@ func doubleBuildWithWNoError(env env, cfg *config, newWarningsDir string, origin
 	if err != nil {
 		return 0, err
 	}
+
 	// The only way we can do anything useful is if it looks like the failure
 	// was -Werror-related.
-	if originalExitCode == 0 || !bytes.Contains(originalStderrBuffer.Bytes(), []byte("-Werror")) || isLikelyAConfTest(cfg, originalCmd) {
+	originalStdoutBufferBytes := originalStdoutBuffer.Bytes()
+	shouldRetry := originalExitCode != 0 &&
+		!isLikelyAConfTest(cfg, originalCmd) &&
+		(bytes.Contains(originalStderrBuffer.Bytes(), []byte("-Werror")) ||
+			bytes.Contains(originalStdoutBufferBytes, []byte("warnings-as-errors")) ||
+			bytes.Contains(originalStdoutBufferBytes, []byte("clang-diagnostic-")))
+	if !shouldRetry {
 		originalStdoutBuffer.WriteTo(env.stdout())
 		originalStderrBuffer.WriteTo(env.stderr())
 		return originalExitCode, nil
@@ -87,7 +100,7 @@ func doubleBuildWithWNoError(env env, cfg *config, newWarningsDir string, origin
 	retryStderrBuffer := &bytes.Buffer{}
 	retryCommand := &command{
 		Path:       originalCmd.Path,
-		Args:       append(originalCmd.Args, disableWerrorFlags(originalCmd.Args)...),
+		Args:       disableWerrorFlags(originalCmd.Args),
 		EnvUpdates: originalCmd.EnvUpdates,
 	}
 	retryExitCode, err := wrapSubprocessErrorWithSourceLoc(retryCommand,
diff --git a/compiler_wrapper/disable_werror_flag_test.go b/compiler_wrapper/disable_werror_flag_test.go
index 53b0fb52..28e48371 100644
--- a/compiler_wrapper/disable_werror_flag_test.go
+++ b/compiler_wrapper/disable_werror_flag_test.go
@@ -449,3 +449,72 @@ func TestChromeOSGetNewWarningsDirOff(t *testing.T) {
 		}
 	})
 }
+
+func TestClangTidyNoDoubleBuild(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		ctx.cfg.isAndroidWrapper = true
+		ctx.cfg.useLlvmNext = true
+		ctx.must(callCompiler(ctx, ctx.cfg, ctx.newCommand(clangTidyAndroid, "--", mainCc)))
+		if ctx.cmdCount != 1 {
+			t.Errorf("expected 1 call. Got: %d", ctx.cmdCount)
+		}
+	})
+}
+
+func withAndroidClangTidyTestContext(t *testing.T, work func(ctx *testContext)) {
+	withTestContext(t, func(ctx *testContext) {
+		ctx.cfg.isAndroidWrapper = true
+		ctx.cfg.useLlvmNext = true
+		ctx.env = []string{"OUT_DIR=/tmp"}
+
+		ctx.cmdMock = func(cmd *command, stdin io.Reader, stdout io.Writer, stderr io.Writer) error {
+			hasArg := func(s string) bool {
+				for _, e := range cmd.Args {
+					if strings.Contains(e, s) {
+						return true
+					}
+				}
+				return false
+			}
+			switch ctx.cmdCount {
+			case 1:
+				if hasArg("-Werror") {
+					fmt.Fprint(stdout, "clang-diagnostic-")
+					return newExitCodeError(1)
+				}
+				if hasArg("-warnings-as-errors") {
+					fmt.Fprint(stdout, "warnings-as-errors")
+					return newExitCodeError(1)
+				}
+				return nil
+			case 2:
+				if hasArg("warnings-as-errors") {
+					return fmt.Errorf("Unexpected arg warnings-as-errors found.  All args: %s", cmd.Args)
+				}
+				return nil
+			default:
+				t.Fatalf("unexpected command: %#v", cmd)
+				return nil
+			}
+		}
+		work(ctx)
+	})
+}
+
+func TestClangTidyDoubleBuildClangTidyError(t *testing.T) {
+	withAndroidClangTidyTestContext(t, func(ctx *testContext) {
+		ctx.must(callCompiler(ctx, ctx.cfg, ctx.newCommand(clangTidyAndroid, "-warnings-as-errors=*", "--", mainCc)))
+		if ctx.cmdCount != 2 {
+			t.Errorf("expected 2 calls. Got: %d", ctx.cmdCount)
+		}
+	})
+}
+
+func TestClangTidyDoubleBuildClangError(t *testing.T) {
+	withAndroidClangTidyTestContext(t, func(ctx *testContext) {
+		ctx.must(callCompiler(ctx, ctx.cfg, ctx.newCommand(clangTidyAndroid, "-Werrors=*", "--", mainCc)))
+		if ctx.cmdCount != 2 {
+			t.Errorf("expected 2 calls. Got: %d", ctx.cmdCount)
+		}
+	})
+}
diff --git a/compiler_wrapper/testutil_test.go b/compiler_wrapper/testutil_test.go
index 57a68df2..5d8ef920 100644
--- a/compiler_wrapper/testutil_test.go
+++ b/compiler_wrapper/testutil_test.go
@@ -17,15 +17,18 @@ import (
 	"testing"
 )
 
-const mainCc = "main.cc"
-const clangAndroid = "./clang"
-const clangX86_64 = "./x86_64-cros-linux-gnu-clang"
-const gccX86_64 = "./x86_64-cros-linux-gnu-gcc"
-const gccX86_64Eabi = "./x86_64-cros-eabi-gcc"
-const gccArmV7 = "./armv7m-cros-linux-gnu-gcc"
-const gccArmV7Eabi = "./armv7m-cros-eabi-gcc"
-const gccArmV8 = "./armv8m-cros-linux-gnu-gcc"
-const gccArmV8Eabi = "./armv8m-cros-eabi-gcc"
+const (
+	mainCc           = "main.cc"
+	clangAndroid     = "./clang"
+	clangTidyAndroid = "./clang-tidy"
+	clangX86_64      = "./x86_64-cros-linux-gnu-clang"
+	gccX86_64        = "./x86_64-cros-linux-gnu-gcc"
+	gccX86_64Eabi    = "./x86_64-cros-eabi-gcc"
+	gccArmV7         = "./armv7m-cros-linux-gnu-gcc"
+	gccArmV7Eabi     = "./armv7m-cros-eabi-gcc"
+	gccArmV8         = "./armv8m-cros-linux-gnu-gcc"
+	gccArmV8Eabi     = "./armv8m-cros-eabi-gcc"
+)
 
 type testContext struct {
 	t            *testing.T