wrapper: add support for Tricium clang-tidy
This adds a special `WITH_TIDY` mode that's specifically for use with
Tricium. Crucially, this has us dump diagnostics as YAML, and stash a
fair amount of information about each clang-tidy invocation in the same
place where we dump YAML.
These bits are intended to be used by the script added in
I54ecc88d38faa4bfd502d632d3fd5c74734dabc0.
BUG=chromium:1035951
TEST=Ran on all platform2 packages `emerge`able on amd64-generic.
Change-Id: I63ef06dc6ddc016ebb6ba0c4a0cea8320fef7415
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/2245785
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
diff --git a/compiler_wrapper/clang_tidy_flag.go b/compiler_wrapper/clang_tidy_flag.go
index d8bf3cb..edbbb41 100644
--- a/compiler_wrapper/clang_tidy_flag.go
+++ b/compiler_wrapper/clang_tidy_flag.go
@@ -5,15 +5,26 @@
package main
import (
+ "encoding/json"
"fmt"
+ "io/ioutil"
+ "os"
"path/filepath"
"strings"
)
-func processClangTidyFlags(builder *commandBuilder) (cSrcFile string, useClangTidy bool) {
+type useTidyMode int
+
+const (
+ tidyModeNone useTidyMode = iota
+ tidyModeAll
+ tidyModeTricium
+)
+
+func processClangTidyFlags(builder *commandBuilder) (cSrcFile string, mode useTidyMode) {
withTidy, _ := builder.env.getenv("WITH_TIDY")
if withTidy == "" {
- return "", false
+ return "", tidyModeNone
}
srcFileSuffixes := []string{
".c",
@@ -24,48 +35,138 @@
".c++",
}
cSrcFile = ""
+ srcSuffix := ""
lastArg := ""
for _, arg := range builder.args {
- if hasAtLeastOneSuffix(arg.value, srcFileSuffixes) && lastArg != "-o" {
- cSrcFile = arg.value
+ if lastArg != "-o" {
+ for _, suffix := range srcFileSuffixes {
+ if strings.HasSuffix(arg.value, suffix) {
+ srcSuffix = suffix
+ cSrcFile = arg.value
+ break
+ }
+ }
}
lastArg = arg.value
}
- useClangTidy = cSrcFile != ""
- return cSrcFile, useClangTidy
+
+ if cSrcFile == "" {
+ return "", tidyModeNone
+ }
+
+ if withTidy == "tricium" {
+ // Files generated from protobufs can result in _many_ clang-tidy complaints, and aren't
+ // worth linting in general. Don't.
+ if strings.HasSuffix(cSrcFile, ".pb"+srcSuffix) {
+ mode = tidyModeNone
+ } else {
+ mode = tidyModeTricium
+ }
+ } else {
+ mode = tidyModeAll
+ }
+ return cSrcFile, mode
}
-func runClangTidy(env env, clangCmd *command, cSrcFile string) error {
- defaultTidyChecks := strings.Join([]string{
- "*",
- "-bugprone-narrowing-conversions",
- "-cppcoreguidelines-*",
- "-fuchsia-*",
- "-google-readability*",
- "-google-runtime-references",
- "-hicpp-*",
- "-llvm-*",
- "-misc-non-private-member-variables-in-classes",
- "-misc-unused-parameters",
- "-modernize-*",
- "-readability-*",
- }, ",")
-
+func calcClangTidyInvocation(env env, clangCmd *command, cSrcFile string, tidyFlags ...string) (*command, error) {
resourceDir, err := getClangResourceDir(env, clangCmd.Path)
if err != nil {
+ return nil, err
+ }
+
+ clangTidyPath := filepath.Join(filepath.Dir(clangCmd.Path), "clang-tidy")
+ args := append([]string{}, tidyFlags...)
+ args = append(args, cSrcFile, "--", "-resource-dir="+resourceDir)
+ args = append(args, clangCmd.Args...)
+ return &command{
+ Path: clangTidyPath,
+ Args: args,
+ EnvUpdates: clangCmd.EnvUpdates,
+ }, nil
+}
+
+func runClangTidyForTricium(env env, clangCmd *command, cSrcFile, fixesDir string) error {
+ if err := os.MkdirAll(fixesDir, 0777); err != nil {
+ return fmt.Errorf("creating fixes directory at %q: %v", fixesDir, err)
+ }
+
+ f, err := ioutil.TempFile(fixesDir, "lints-")
+ if err != nil {
+ return fmt.Errorf("making tempfile for tidy: %v", err)
+ }
+ f.Close()
+
+ // `f` is an 'anchor'; it ensures we won't create a similarly-named file in the future.
+ // Hence, we can't delete it.
+ fixesFilePath := f.Name() + ".yaml"
+ fixesMetadataPath := f.Name() + ".json"
+
+ // FIXME(gbiv): Remove `-checks=*` when testing is complete; we should defer to .clang-tidy
+ // files, which are both more expressive and more approachable than `-checks=*`.
+ clangTidyCmd, err := calcClangTidyInvocation(env, clangCmd, cSrcFile, "-checks=*", "--export-fixes="+fixesFilePath)
+ if err != nil {
+ return fmt.Errorf("calculating tidy invocation: %v", err)
+ }
+
+ stdstreams := &strings.Builder{}
+ // Note: We pass nil as stdin as we checked before that the compiler
+ // was invoked with a source file argument.
+ exitCode, err := wrapSubprocessErrorWithSourceLoc(clangTidyCmd,
+ env.run(clangTidyCmd, nil, stdstreams, stdstreams))
+ if err != nil {
return err
}
- clangTidyPath := filepath.Join(filepath.Dir(clangCmd.Path), "clang-tidy")
- clangTidyCmd := &command{
- Path: clangTidyPath,
- Args: append([]string{
- "-checks=" + defaultTidyChecks,
- cSrcFile,
- "--",
- "-resource-dir=" + resourceDir,
- }, clangCmd.Args...),
- EnvUpdates: clangCmd.EnvUpdates,
+ type metadata struct {
+ Args []string `json:"args"`
+ Executable string `json:"executable"`
+ ExitCode int `json:"exit_code"`
+ LintTarget string `json:"lint_target"`
+ Stdstreams string `json:"stdstreams"`
+ Wd string `json:"wd"`
+ }
+
+ f, err = os.Create(fixesMetadataPath)
+ if err != nil {
+ return fmt.Errorf("creating fixes metadata: %v", err)
+ }
+
+ meta := &metadata{
+ Args: clangTidyCmd.Args,
+ Executable: clangTidyCmd.Path,
+ ExitCode: exitCode,
+ LintTarget: cSrcFile,
+ Stdstreams: stdstreams.String(),
+ Wd: env.getwd(),
+ }
+ if err := json.NewEncoder(f).Encode(meta); err != nil {
+ return fmt.Errorf("writing fixes metadata: %v", err)
+ }
+
+ if err := f.Close(); err != nil {
+ return fmt.Errorf("finalizing fixes metadata: %v", err)
+ }
+ return nil
+}
+
+func runClangTidy(env env, clangCmd *command, cSrcFile string) error {
+ clangTidyCmd, err := calcClangTidyInvocation(env, clangCmd, cSrcFile, "-checks="+
+ strings.Join([]string{
+ "*",
+ "-bugprone-narrowing-conversions",
+ "-cppcoreguidelines-*",
+ "-fuchsia-*",
+ "-google-readability*",
+ "-google-runtime-references",
+ "-hicpp-*",
+ "-llvm-*",
+ "-misc-non-private-member-variables-in-classes",
+ "-misc-unused-parameters",
+ "-modernize-*",
+ "-readability-*",
+ }, ","))
+ if err != nil {
+ return fmt.Errorf("calculating clang-tidy invocation: %v", err)
}
// Note: We pass nil as stdin as we checked before that the compiler
diff --git a/compiler_wrapper/clang_tidy_flag_test.go b/compiler_wrapper/clang_tidy_flag_test.go
index baf5219..d45d1bd 100644
--- a/compiler_wrapper/clang_tidy_flag_test.go
+++ b/compiler_wrapper/clang_tidy_flag_test.go
@@ -273,6 +273,66 @@
})
}
+func TestTriciumClangTidyIsProperlyDetectedFromEnv(t *testing.T) {
+ withClangTidyTestContext(t, func(ctx *testContext) {
+ ctx.env = []string{"WITH_TIDY=tricium"}
+ ctx.cmdMock = func(cmd *command, stdin io.Reader, stdout io.Writer, stderr io.Writer) error {
+ switch ctx.cmdCount {
+ case 1:
+ if err := verifyPath(cmd, "usr/bin/clang"); err != nil {
+ t.Error(err)
+ }
+ return nil
+ case 2:
+ if err := verifyPath(cmd, "usr/bin/clang-tidy"); err != nil {
+ return err
+ }
+
+ hasFixesFile := false
+ for _, arg := range cmd.Args {
+ if path := strings.TrimPrefix(arg, "--export-fixes="); path != arg {
+ hasFixesFile = true
+ if !strings.HasPrefix(path, ctx.cfg.triciumNitsDir+"/") {
+ t.Errorf("fixes file was %q; expected it to be in %q", path, ctx.cfg.triciumNitsDir)
+ }
+ break
+ }
+ }
+
+ if !hasFixesFile {
+ t.Error("no fixes file was provided to a tricium invocation")
+ }
+
+ return nil
+ default:
+ return nil
+ }
+ }
+ cmd := ctx.must(callCompiler(ctx, ctx.cfg,
+ ctx.newCommand(clangX86_64, mainCc)))
+ if ctx.cmdCount != 3 {
+ t.Errorf("expected 3 calls. Got: %d", ctx.cmdCount)
+ }
+ if err := verifyPath(cmd, "usr/bin/clang"); err != nil {
+ t.Error(err)
+ }
+ })
+}
+
+func TestTriciumClangTidySkipsProtobufFiles(t *testing.T) {
+ withClangTidyTestContext(t, func(ctx *testContext) {
+ ctx.env = []string{"WITH_TIDY=tricium"}
+ cmd := ctx.must(callCompiler(ctx, ctx.cfg,
+ ctx.newCommand(clangX86_64, mainCc+".pb.cc")))
+ if ctx.cmdCount != 1 {
+ t.Errorf("expected tricium clang-tidy to not execute on a protobuf file")
+ }
+ if err := verifyPath(cmd, "usr/bin/clang"); err != nil {
+ t.Error(err)
+ }
+ })
+}
+
func withClangTidyTestContext(t *testing.T, work func(ctx *testContext)) {
withTestContext(t, func(ctx *testContext) {
ctx.env = []string{"WITH_TIDY=1"}
diff --git a/compiler_wrapper/compiler_wrapper.go b/compiler_wrapper/compiler_wrapper.go
index 4c3db14..14de210 100644
--- a/compiler_wrapper/compiler_wrapper.go
+++ b/compiler_wrapper/compiler_wrapper.go
@@ -90,16 +90,29 @@
return 0, newErrorwithSourceLocf("unsupported compiler: %s", mainBuilder.target.compiler)
}
} else if mainBuilder.target.compilerType == clangType {
- cSrcFile, useClangTidy := processClangTidyFlags(mainBuilder)
+ cSrcFile, tidyMode := processClangTidyFlags(mainBuilder)
err := prepareClangCommand(mainBuilder)
if err != nil {
return 0, err
}
allowCCache := true
- if useClangTidy {
+ if tidyMode != tidyModeNone {
allowCCache = false
clangCmdWithoutGomaAndCCache := mainBuilder.build()
- if err := runClangTidy(env, clangCmdWithoutGomaAndCCache, cSrcFile); err != nil {
+ var err error
+ switch tidyMode {
+ case tidyModeTricium:
+ if cfg.triciumNitsDir == "" {
+ return 0, newErrorwithSourceLocf("tricium linting was requested, but no nits directory is configured")
+ }
+ err = runClangTidyForTricium(env, clangCmdWithoutGomaAndCCache, cSrcFile, cfg.triciumNitsDir)
+ case tidyModeAll:
+ err = runClangTidy(env, clangCmdWithoutGomaAndCCache, cSrcFile)
+ default:
+ panic(fmt.Sprintf("Unknown tidy mode: %v", tidyMode))
+ }
+
+ if err != nil {
return 0, err
}
}
diff --git a/compiler_wrapper/config.go b/compiler_wrapper/config.go
index a122b5a..3c3668d 100644
--- a/compiler_wrapper/config.go
+++ b/compiler_wrapper/config.go
@@ -29,6 +29,8 @@
rootRelPath string
// Directory to store errors that were prevented with -Wno-error.
newWarningsDir string
+ // Directory to store nits in when using `WITH_TIDY=tricium`.
+ triciumNitsDir string
// Version. Only used for printing via -print-cmd.
version string
}
@@ -137,6 +139,7 @@
"-Wno-implicit-int-float-conversion",
},
newWarningsDir: "/tmp/fatal_clang_warnings",
+ triciumNitsDir: "/tmp/linting_output/clang-tidy",
}
// Flags to be added to non-hardened toolchain.
@@ -166,6 +169,7 @@
"-Wno-implicit-int-float-conversion",
},
newWarningsDir: "/tmp/fatal_clang_warnings",
+ triciumNitsDir: "/tmp/linting_output/clang-tidy",
}
// Flags to be added to host toolchain.
@@ -200,6 +204,7 @@
"-Wno-implicit-int-float-conversion",
},
newWarningsDir: "/tmp/fatal_clang_warnings",
+ triciumNitsDir: "/tmp/linting_output/clang-tidy",
}
var androidConfig = &config{
@@ -211,4 +216,5 @@
clangFlags: []string{},
clangPostFlags: []string{},
newWarningsDir: "",
+ triciumNitsDir: "",
}
diff --git a/compiler_wrapper/testutil_test.go b/compiler_wrapper/testutil_test.go
index 5d8ef92..d23a843 100644
--- a/compiler_wrapper/testutil_test.go
+++ b/compiler_wrapper/testutil_test.go
@@ -141,6 +141,7 @@
func (ctx *testContext) updateConfig(cfg *config) {
*ctx.cfg = *cfg
ctx.cfg.newWarningsDir = filepath.Join(ctx.tempDir, "fatal_clang_warnings")
+ ctx.cfg.triciumNitsDir = filepath.Join(ctx.tempDir, "tricium_nits")
}
func (ctx *testContext) newCommand(path string, args ...string) *command {
