fix: Avoid false success for -fcolor-diagnostics probe with GCC

Ccache will produce a false hit in the following scenario, if "cc" is
GCC without being a symlink to "gcc":

  1. ccache cc -c example.c                      # adds a cache entry
  2. ccache cc -c example.c -fcolor-diagnostics  # unexpectedly succeeds

(Most major Linux distributions install cc as a symlink to gcc, and then
the problem doesn't show up since ccache then is able to guess that the
compiler is GCC.)

This bug was introduced by [1] which tried to make yet another tweak of
the color diagnostics guessing/probing by no longer rejecting
-fcolor-diagnostics for an unknown compiler, based on the assumption
that the probing call (speculatively adding -fdiagnostics-color) is only
made when we have guessed a GCC compiler. Unfortunately, this broke the
fix in [2] when the compiler is unknown.

All of this is inherently brittle and I don't see any good way to make
it better with the current compiler guessing approach and adding options
speculatively. The cure is to start doing proper compiler identification
instead (#958). Until that is done, the only reasonable fix is to avoid
doing anything related to color diagnostics flags when the compiler is
unknown. This means that an unknown compiler from now on won't produce
colors when used via ccache even if the compiler actually is GCC or
Clang.

[1]: 97a40af49f56e4be2db5e4318d8131e937bd5598
[2]: 61ce8c44c5b1da0be7a8d10c014f1a85b7967433

Closes #1116.

(cherry picked from commit 96ec6c9d98b88f00e1a69bdd0214c237bc7ed04e)

diff --git a/src/argprocessing.cpp b/src/argprocessing.cpp
index 5bc277060a2be732ab353dc4298bbaa6e7f5886a..2d3a868a3d9f7dbe3da7abff78652a1d065793ce 100644 (file)
--- a/src/argprocessing.cpp
+++ b/src/argprocessing.cpp
@@ -809,43 +809,39 @@ process_arg(const Context& ctx,
     return nullopt;
   }
 
-  if (config.compiler_type() == CompilerType::gcc
-      && (args[i] == "-fcolor-diagnostics"
-          || args[i] == "-fno-color-diagnostics")) {
-    // Special case: If a GCC compiler gets -f(no-)color-diagnostics we'll bail
-    // out and just execute the compiler. The reason is that we don't include
-    // -f(no-)color-diagnostics in the hash so there can be a false cache hit in
-    // the following scenario:
-    //
-    //   1. ccache gcc -c example.c                      # adds a cache entry
-    //   2. ccache gcc -c example.c -fcolor-diagnostics  # unexpectedly succeeds
-    return Statistic::unsupported_compiler_option;
-  }
-
-  // In the "-Xclang -fcolor-diagnostics" form, -Xclang is skipped and the
-  // -fcolor-diagnostics argument which is passed to cc1 is handled below.
-  if (args[i] == "-Xclang" && i + 1 < args.size()
-      && args[i + 1] == "-fcolor-diagnostics") {
-    state.compiler_only_args_no_hash.push_back(args[i]);
-    ++i;
-  }
-
-  if (args[i] == "-fcolor-diagnostics" || args[i] == "-fdiagnostics-color"
-      || args[i] == "-fdiagnostics-color=always") {
-    state.color_diagnostics = ColorDiagnostics::always;
-    state.compiler_only_args_no_hash.push_back(args[i]);
-    return nullopt;
-  }
-  if (args[i] == "-fno-color-diagnostics" || args[i] == "-fno-diagnostics-color"
-      || args[i] == "-fdiagnostics-color=never") {
-    state.color_diagnostics = ColorDiagnostics::never;
-    state.compiler_only_args_no_hash.push_back(args[i]);
-    return nullopt;
-  }
-  if (args[i] == "-fdiagnostics-color=auto") {
-    state.color_diagnostics = ColorDiagnostics::automatic;
-    state.compiler_only_args_no_hash.push_back(args[i]);
-    return nullopt;
+  if (config.compiler_type() == CompilerType::gcc) {
+    if (args[i] == "-fdiagnostics-color"
+        || args[i] == "-fdiagnostics-color=always") {
+      state.color_diagnostics = ColorDiagnostics::always;
+      state.compiler_only_args_no_hash.push_back(args[i]);
+      return nullopt;
+    } else if (args[i] == "-fno-diagnostics-color"
+               || args[i] == "-fdiagnostics-color=never") {
+      state.color_diagnostics = ColorDiagnostics::never;
+      state.compiler_only_args_no_hash.push_back(args[i]);
+      return nullopt;
+    } else if (args[i] == "-fdiagnostics-color=auto") {
+      state.color_diagnostics = ColorDiagnostics::automatic;
+      state.compiler_only_args_no_hash.push_back(args[i]);
+      return nullopt;
+    }
+  } else if (config.is_compiler_group_clang()) {
+    // In the "-Xclang -fcolor-diagnostics" form, -Xclang is skipped and the
+    // -fcolor-diagnostics argument which is passed to cc1 is handled below.
+    if (args[i] == "-Xclang" && i + 1 < args.size()
+        && args[i + 1] == "-fcolor-diagnostics") {
+      state.compiler_only_args_no_hash.push_back(args[i]);
+      ++i;
+    }
+    if (args[i] == "-fcolor-diagnostics") {
+      state.color_diagnostics = ColorDiagnostics::always;
+      state.compiler_only_args_no_hash.push_back(args[i]);
+      return nullopt;
+    } else if (args[i] == "-fno-color-diagnostics") {
+      state.color_diagnostics = ColorDiagnostics::never;
+      state.compiler_only_args_no_hash.push_back(args[i]);
+      return nullopt;
+    }
   }
 
   // GCC

diff --git a/test/suites/basedir.bash b/test/suites/basedir.bash
index e3273f8afeaa45f8b52de9149e5d088a5755a2c6..b02fcb6a81b023a66982a96a4138d5f3df63c15b 100644 (file)
--- a/test/suites/basedir.bash
+++ b/test/suites/basedir.bash
@@ -296,7 +296,7 @@ EOF
     expect_stat cache_miss 1
     expect_equal_content reference.stderr ccache.stderr
 
-    if $COMPILER -fdiagnostics-color=always -c test.c 2>/dev/null; then
+    if $COMPILER_TYPE_GCC && $COMPILER -fdiagnostics-color=always -c test.c 2>/dev/null; then
         $COMPILER -fdiagnostics-color=always -c $pwd/test.c 2>reference.stderr
 
         CCACHE_ABSSTDERR=1 CCACHE_BASEDIR="$pwd" $CCACHE_COMPILE -fdiagnostics-color=always -c $pwd/test.c 2>ccache.stderr

diff --git a/test/suites/color_diagnostics.bash b/test/suites/color_diagnostics.bash
index 004eb15e48cf0000969d9d78b3c0714a30eaa6c1..d8db0484512c952585368e2d0bb72c1d65fe1a38 100644 (file)
--- a/test/suites/color_diagnostics.bash
+++ b/test/suites/color_diagnostics.bash
@@ -122,7 +122,7 @@ color_diagnostics_test() {
         if $CCACHE_COMPILE -fcolor-diagnostics -c test.c >&/dev/null; then
             test_failed "-fcolor-diagnostics unexpectedly accepted by GCC"
         fi
-        expect_stat unsupported_compiler_option 1
+        expect_stat preprocessor_error 1
 
         # ---------------------------------------------------------------------
         TEST "-fcolor-diagnostics not accepted for GCC for cached result"
@@ -132,25 +132,23 @@ color_diagnostics_test() {
         if ! $CCACHE_COMPILE -c test.c >&/dev/null; then
             test_failed "unknown error compiling"
         fi
-
         if $CCACHE_COMPILE -fcolor-diagnostics -c test.c >&/dev/null; then
             test_failed "-fcolor-diagnostics unexpectedly accepted by GCC"
         fi
-        expect_stat unsupported_compiler_option 1
+        expect_stat preprocessor_error 1
 
         # ---------------------------------------------------------------------
         TEST "-fcolor-diagnostics passed to underlying compiler for unknown compiler type"
 
         generate_code 1 test.c
 
+        CCACHE_COMPILERTYPE=other $CCACHE_COMPILE -c test.c
+        expect_stat cache_miss 1
+
         if CCACHE_COMPILERTYPE=other $CCACHE_COMPILE -fcolor-diagnostics -c test.c >&/dev/null; then
             test_failed "-fcolor-diagnostics unexpectedly accepted by GCC"
         fi
-
-        # Verify that -fcolor-diagnostics was passed to the compiler for the
-        # unknown compiler case, i.e. ccache did not exit early with
-        # "unsupported compiler option".
-        expect_stat compile_failed 1
+        expect_stat preprocessor_error 1
     fi
 
     if $COMPILER_TYPE_CLANG; then

diff --git a/unittest/test_argprocessing.cpp b/unittest/test_argprocessing.cpp
index 09c27b946900f1394d8eb2503d74e018f695d25f..f5e8619309b0f3ac5b9f22d03e692dacb70fdf87 100644 (file)
--- a/unittest/test_argprocessing.cpp
+++ b/unittest/test_argprocessing.cpp
@@ -1,4 +1,4 @@
-// Copyright (C) 2010-2021 Joel Rosdahl and other contributors
+// Copyright (C) 2010-2022 Joel Rosdahl and other contributors
 //
 // See doc/AUTHORS.adoc for a complete list of contributors.
 //
@@ -569,6 +569,8 @@ TEST_CASE("-Xclang")
 {
   TestContext test_context;
   Context ctx;
+  ctx.config.set_compiler_type(CompilerType::clang);
+
   const std::string common_args =
     "-Xclang -fno-pch-timestamp"
     " -Xclang unsupported";
@@ -586,17 +588,17 @@ TEST_CASE("-Xclang")
     " -Xclang -include-pth -Xclang pth_path2";
 
   ctx.orig_args =
-    Args::from_string("gcc -c foo.c " + common_args + " " + color_diag + " "
+    Args::from_string("clang -c foo.c " + common_args + " " + color_diag + " "
                       + extra_args + " " + pch_pth_variants);
   Util::write_file("foo.c", "");
 
   const ProcessArgsResult result = process_args(ctx);
   CHECK(result.preprocessor_args.to_string()
-        == "gcc " + common_args + " " + pch_pth_variants);
+        == "clang " + common_args + " " + pch_pth_variants);
   CHECK(result.extra_args_to_hash.to_string() == extra_args);
   CHECK(result.compiler_args.to_string()
-        == "gcc " + common_args + " " + color_diag + " " + extra_args + " "
-             + pch_pth_variants + " -c");
+        == "clang " + common_args + " " + color_diag + " " + extra_args + " "
+             + pch_pth_variants + " -c -fcolor-diagnostics");
 }
 
 TEST_CASE("-x")