From 50e8d22943621db33ef30b7333b3be4f923cfad8 Mon Sep 17 00:00:00 2001 From: Joel Rosdahl Date: Mon, 2 Nov 2020 22:09:56 +0100 Subject: [PATCH] Retain given color diagnostics options when forcing colored output MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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. --- src/argprocessing.cpp | 46 +++++++++++++++++------------- src/ccache.cpp | 2 +- test/suites/base.bash | 25 ++++++---------- test/suites/color_diagnostics.bash | 9 ++++++ 4 files changed, 45 insertions(+), 37 deletions(-) diff --git a/src/argprocessing.cpp b/src/argprocessing.cpp index fc61655bc..8580f2e52 100644 --- 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 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 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 e56be0ae3..6e9feedb4 100644 --- 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 34ab75d2a..102eb29d8 100644 --- 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 31ccc078d..3aa5eadc1 100644 --- 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)" -- 2.47.3