Retain given color diagnostics options when forcing colored output

ccache currently filters out both -fdiagnostics-color and
-fcolor-diagnostics options when adding -fdiagnostics-color (GCC) or
-fcolor-diagnostics (Clang) to force colored output. The idea in #224
was that only -fdiagnostics-color options should be filtered out when
the compiler is GCC, but -fcolor-diagnostics is also removed, something
which was missed in the code review. This has the unintended side effect
that “ccache gcc -fcolor-diagnostics -c example.c” works since ccache in
effect removes -fcolor-diagnostics in the actual call to the compiler.

The bug can fixed by removing only the compiler-specific options when
forcing colored output, but a more robust method would be to retain all
color diagnostics options as is but exclude them from the input hash.
This commit makes that change and also simplifies the logic for color
diagnostics option handling.

Fixes #711.

diff --git a/src/argprocessing.cpp b/src/argprocessing.cpp
index fc61655bc0c460dd47322d76bc7e09639a8ef74c..8580f2e52950094007adb39c32f210a280d5b778 100644 (file)
--- a/src/argprocessing.cpp
+++ b/src/argprocessing.cpp
@@ -80,6 +80,11 @@ struct ArgumentProcessingState
   // compiler_only_args contains arguments that should only be passed to the
   // compiler, not the preprocessor.
   Args compiler_only_args;
+
+  // compiler_only_args_no_hash contains arguments that should only be passed to
+  // the compiler, not the preprocessor, and that also should not be part of the
+  // hash identifying the result.
+  Args compiler_only_args_no_hash;
 };
 
 bool
@@ -203,16 +208,6 @@ process_profiling_option(Context& ctx, const std::string& arg)
   return true;
 }
 
-// The compiler is invoked with the original arguments in the depend mode.
-// Collect extra arguments that should be added.
-void
-add_depend_mode_extra_original_args(Context& ctx, const std::string& arg)
-{
-  if (ctx.config.depend_mode()) {
-    ctx.args_info.depend_extra_args.push_back(arg);
-  }
-}
-
 optional<Statistic>
 process_arg(Context& ctx,
             Args& args,
@@ -690,15 +685,18 @@ process_arg(Context& ctx,
   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;
   }
 
@@ -1097,22 +1095,16 @@ process_args(Context& ctx)
     state.color_diagnostics != ColorDiagnostics::automatic
       ? state.color_diagnostics == ColorDiagnostics::never
       : !color_output_possible();
+
   // Since output is redirected, compilers will not color their output by
   // default, so force it explicitly.
+  nonstd::optional<std::string> diagnostics_color_arg;
   if (ctx.guessed_compiler == GuessedCompiler::clang) {
     if (args_info.actual_language != "assembler") {
-      if (!config.run_second_cpp()) {
-        state.cpp_args.push_back("-fcolor-diagnostics");
-      }
-      state.compiler_only_args.push_back("-fcolor-diagnostics");
-      add_depend_mode_extra_original_args(ctx, "-fcolor-diagnostics");
+      diagnostics_color_arg = "-fcolor-diagnostics";
     }
   } else if (ctx.guessed_compiler == GuessedCompiler::gcc) {
-    if (!config.run_second_cpp()) {
-      state.cpp_args.push_back("-fdiagnostics-color");
-    }
-    state.compiler_only_args.push_back("-fdiagnostics-color");
-    add_depend_mode_extra_original_args(ctx, "-fdiagnostics-color");
+    diagnostics_color_arg = "-fdiagnostics-color";
   } else {
     // Other compilers shouldn't output color, so no need to strip it.
     args_info.strip_diagnostics_colors = false;
@@ -1151,6 +1143,7 @@ process_args(Context& ctx)
   }
 
   Args compiler_args = state.common_args;
+  compiler_args.push_back(state.compiler_only_args_no_hash);
   compiler_args.push_back(state.compiler_only_args);
 
   if (config.run_second_cpp()) {
@@ -1211,5 +1204,18 @@ process_args(Context& ctx)
     extra_args_to_hash.push_back(state.dep_args);
   }
 
+  if (diagnostics_color_arg) {
+    compiler_args.push_back(*diagnostics_color_arg);
+    if (!config.run_second_cpp()) {
+      // If we're compiling preprocessed code we're keeping any warnings from
+      // the preprocessor, so we need to make sure that they are in color.
+      preprocessor_args.push_back(*diagnostics_color_arg);
+    }
+    if (ctx.config.depend_mode()) {
+      // The compiler is invoked with the original arguments in the depend mode.
+      ctx.args_info.depend_extra_args.push_back(*diagnostics_color_arg);
+    }
+  }
+
   return {preprocessor_args, extra_args_to_hash, compiler_args};
 }

diff --git a/src/ccache.cpp b/src/ccache.cpp
index e56be0ae3c958708efbf1a37a30680cd941b6826..6e9feedb45b3fcce5fb7926cce9bb0f92c6552be 100644 (file)
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -739,7 +739,7 @@ do_execute(Context& ctx,
 
   if (ctx.diagnostics_color_failed
       && ctx.guessed_compiler == GuessedCompiler::gcc) {
-    args.erase_with_prefix("-fdiagnostics-color");
+    args.erase_last("-fdiagnostics-color");
   }
   int status = execute(args.to_argv().data(),
                        std::move(tmp_stdout.fd),

diff --git a/test/suites/base.bash b/test/suites/base.bash
index 34ab75d2a85366395c23f8b02bceaef96670bf46..102eb29d84cb050866024cd4280977c25aaf73c0 100644 (file)
--- a/test/suites/base.bash
+++ b/test/suites/base.bash
@@ -1012,16 +1012,12 @@ EOF
     cat <<'EOF' >test_no_obj.c
 int test_no_obj;
 EOF
-    cat <<'EOF' >prefix-remove.sh
+    cat <<'EOF' >no-object-prefix
 #!/bin/sh
-"$@"
-[ x$2 = x-fcolor-diagnostics ] && shift
-[ x$2 = x-fdiagnostics-color ] && shift
-[ x$2 = x-std=gnu99 ] && shift
-[ x$3 = x-o ] && rm $4
+# Emulate no object file from the compiler.
 EOF
-    chmod +x prefix-remove.sh
-    CCACHE_PREFIX=`pwd`/prefix-remove.sh $CCACHE_COMPILE -c test_no_obj.c
+    chmod +x no-object-prefix
+    CCACHE_PREFIX=$(pwd)/no-object-prefix $CCACHE_COMPILE -c test_no_obj.c
     expect_stat 'compiler produced no output' 1
 
     # -------------------------------------------------------------------------
@@ -1030,16 +1026,13 @@ EOF
     cat <<'EOF' >test_empty_obj.c
 int test_empty_obj;
 EOF
-    cat <<'EOF' >prefix-empty.sh
+    cat <<'EOF' >empty-object-prefix
 #!/bin/sh
-"$@"
-[ x$2 = x-fcolor-diagnostics ] && shift
-[ x$2 = x-fdiagnostics-color ] && shift
-[ x$2 = x-std=gnu99 ] && shift
-[ x$3 = x-o ] && cp /dev/null $4
+# Emulate empty object file from the compiler.
+touch test_empty_obj.o
 EOF
-    chmod +x prefix-empty.sh
-    CCACHE_PREFIX=`pwd`/prefix-empty.sh $CCACHE_COMPILE -c test_empty_obj.c
+    chmod +x empty-object-prefix
+    CCACHE_PREFIX=`pwd`/empty-object-prefix $CCACHE_COMPILE -c test_empty_obj.c
     expect_stat 'compiler produced empty output' 1
 
     # -------------------------------------------------------------------------

diff --git a/test/suites/color_diagnostics.bash b/test/suites/color_diagnostics.bash
index 31ccc078d8dbe619f29b5aa00bc05608a809bf91..3aa5eadc1a1fbf618e7c2597945e5f93e83c94d0 100644 (file)
--- a/test/suites/color_diagnostics.bash
+++ b/test/suites/color_diagnostics.bash
@@ -110,6 +110,15 @@ color_diagnostics_test() {
     expect_stat 'cache hit (preprocessed)' 1
 
     # -------------------------------------------------------------------------
+    if $COMPILER_TYPE_GCC; then
+        TEST "-fcolor-diagnostics not accepted for GCC"
+
+        generate_code 1 test.c
+        if $CCACHE_COMPILE -fcolor-diagnostics -c test.c >&/dev/null; then
+            test_failed "-fcolor-diagnostics unexpectedly accepted by GCC"
+        fi
+    fi
+
     while read -r case; do
         TEST "Cache object shared across ${case} (run_second_cpp=$run_second_cpp)"