From f6957653ca35fc716678eb33d7a8be70e2d808f8 Mon Sep 17 00:00:00 2001 From: Joel Rosdahl Date: Sun, 17 Jul 2022 21:55:06 +0200 Subject: [PATCH] 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) --- src/argprocessing.cpp | 70 ++++++++++++++---------------- test/suites/basedir.bash | 2 +- test/suites/color_diagnostics.bash | 14 +++--- unittest/test_argprocessing.cpp | 12 ++--- 4 files changed, 47 insertions(+), 51 deletions(-) diff --git a/src/argprocessing.cpp b/src/argprocessing.cpp index 5bc277060..2d3a868a3 100644 --- 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 e3273f8af..b02fcb6a8 100644 --- 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 004eb15e4..d8db04845 100644 --- 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 09c27b946..f5e861930 100644 --- 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") -- 2.47.2