Re: [ccache] Support for color diagnostics

2013-11-29 Thread Loïc Yhuel

Le 29/11/2013 14:08, Lubos Lunak a écrit :

On Friday 29 of November 2013, Lubos Lunak wrote:

  Hello,

  the attached patch adds ccache support for compiler color diagnostics
(also reported by somebody as #10075).

...

Caveats:

  I forgot one:

- The function color_output_possible() may look simplistic, but as far as I
can tell it works just fine (it's been in Incecream for a number of years).


Hello,

I think you didn't understand GCC documentation correctly.

From the man page : "The default GCC_COLORS is ... Setting GCC_COLORS 
to the empty string disables colors."

GCC enable colors when GCC_COLORS is not set, and your code doesn't.

In fact you don't have to test GCC_COLORS at all : when it's an empty 
string (not unset !), colors are disabled , and adding 
-fdiagnostics-color doesn't change anything.

___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache


[ccache] Ccache's use of preprocessed source

2013-11-29 Thread Lubos Lunak

 Hello,

 in short, I'd like to ask about the feasibility of CCACHE_CPP2=1 becoming the 
default and the possibility of preventing ccache from running cpp at all.

 From the "cache hides compiler warnings about comments" thread:
> By the way, a workaround is to set CCACHE_CPP2 when compiling.

 I think it is incorrect to call CCACHE_CPP2 a workaround. Current compilers 
are no longer the old-school separate preprocessor-compiler-assembler tools, 
nowadays these steps are done by just one tool and they even somewhat depend 
on each other. So preprocessing source and giving that to a compiler has a 
number of drawbacks:

- The -Wdocumentation feature, discussed in the recent thread, not working.
- Clang and recent GCC quote sources in their errors/warnings. If source is 
preprocessed, it'll have comments removed and macros expanded, meaning the 
errors/warnings will show something that "is not there".
- I don't know about GCC, but Clang supresses some warnings if they come from 
a macro expansion. I think the idea is similar to supressing warnings from 
system headers, but given that a macro from a system header can get expanded 
even in the main source, the most obnoxious (or most often occurring?) 
warnings are simply suppressed completely in macro expansions. With ccache 
this does not work and there are spurious warnings.

 (Note that e.g. -save-temps has the same drawbacks, but who uses that for 
normal work.)

 So I think actually CCACHE_CPP2 set is the normal way and _not_ having it set 
can be considered to be a performance hack.

 Some time back 
(http://www.mail-archive.com/ccache@lists.samba.org/msg00817.html) I measured 
CCACHE_CPP2 adding 8% overhead, today I tried it on a different machine 
compiling LLVM/Clang and I could get only 3% (no idea if that's a real 
improvement somewhere over the year or if it's just a noise). I don't have 
any numbers for GCC, but from what I have heard GCC's preprocessor has gotten 
noticeably faster in the last years.

 So, to my questions:

- does somebody have any CCACHE_CPP2 numbers for GCC?
- would it be possible to alter the documentation to actually recommend 
CCACHE_CPP2, or at least not dismiss it as something that's unlikely to be 
needed?
- (based on performance numbers I guess) would it be possible to switch the 
default for CCACHE_CPP2?
- how difficult would it be to add a possibility of preventing ccache's use of 
cpp completely, thus relying only on the direct mode? This could be useful in 
case the overhead of CCACHE_CPP2 is considered too big, and possibly even in 
the general case (direct hits should be usually much more common and why have 
the overhead of running cpp for a minority of hits). I could give this a 
shot, but I don't know if there isn't some design problem making this 
difficult/impossible.

-- 
 Lubos Lunak
___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache


Re: [ccache] Support for color diagnostics

2013-11-29 Thread Lubos Lunak
On Friday 29 of November 2013, Lubos Lunak wrote:
>  Hello,
>
>  the attached patch adds ccache support for compiler color diagnostics
> (also reported by somebody as #10075).
...
> Caveats:

 I forgot one:

- The function color_output_possible() may look simplistic, but as far as I 
can tell it works just fine (it's been in Incecream for a number of years).

-- 
 Lubos Lunak
___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache


[ccache] Support for color diagnostics

2013-11-29 Thread Lubos Lunak

 Hello,

 the attached patch adds ccache support for compiler color diagnostics (also 
reported by somebody as #10075).

 Clang automatically uses colors for output automatically if used in terminal. 
Ccache's redirecting to a file disables this. GCC 4.8 has got a similar 
support, except that it apparently requires also $GCC_COLORS or an explicit 
option.

 The patch detects if the compiler would use colors if used without ccache and 
explicitly forces an option to achieve this. Note that I do not have GCC 4.8 
here, so I tested with Clang's alias and the GCC_COLORS support is done based 
on documentation.

Caveats:

- GCC developers decided to roll their own name for the option when 
introducing it. Clang has an alias for the GCC way, but versions predating 
that obviously can't support it, so it's necessary to detect the compiler. As 
ccache doesn't do that (and I don't find it worth much effort, as it can't be 
100% reliable anyway), the code merely guesses from the binary name. If the 
compiler used will be e.g. the 'cc' symlink, there'll be no colors. No big 
deal.

- Since the stderr is different, obviously compiling with and without colors 
has different results as well. That means that such a compile 
is "duplicated". It's hopefully not such a common case, although it's 
perfectly possible. I don't know if it's worth the effort to try to be smart 
here. A possibly simple improvement could be to search the cache with and 
without the option set and if stderr is empty, reuse the result regardless of 
the option. I'm not quite sure where exactly this should happen in the code.

 I expect it'd make sense to add $CCACHE_NOCOLORS to disable this support?

 I can also create manpage section for this color support, but I first wanted 
to check here with the code.

-- 
 Lubos Lunak
From d4284a161e733a68ad1c1c84d124d81d3c627db8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lubo=C5=A1=20Lu=C5=88=C3=A1k?= 
Date: Fri, 29 Nov 2013 12:14:03 +0100
Subject: [PATCH] support compiler color diagnostics if possible

Clang and GCC (starting with 4.8) support color output if possible,
but since ccache redirects stderr to a file, they detect the output
is not a terminal and do not enable colors. Try to detect if colors
would be used and force colors explicitly.

Caveats:
- Compiles with and without colors are considered different from each
  other (so they are "duplicated").
- GCC decided to roll its own name for the option, so it's necessary
  to guess which compiler is actually used.
---
 ccache.c | 72 
 1 file changed, 72 insertions(+)

diff --git a/ccache.c b/ccache.c
index 90c38e7..158ad5e 100644
--- a/ccache.c
+++ b/ccache.c
@@ -1065,6 +1065,24 @@ hash_compiler(struct mdfour *hash, struct stat *st, const char *path,
 }
 
 /*
+ * Note that these compiler checks are unreliable, so nothing should hard-depend on them.
+ */
+
+static bool compiler_is_clang()
+{
+const char* name = strrchr( orig_args->argv[ 0 ], '/' );
+name = name ? name + 1 : orig_args->argv[ 0 ];
+return strstr( name, "clang" ) != NULL;
+}
+
+static bool compiler_is_gcc()
+{
+const char* name = strrchr(orig_args->argv[ 0 ], '/' );
+name = name ? name + 1 : orig_args->argv[ 0 ];
+return strstr(name, "gcc") != NULL || strstr(name, "g++") != NULL;
+}
+
+/*
  * Update a hash sum with information common for the direct and preprocessor
  * modes.
  */
@@ -1128,6 +1146,15 @@ calculate_common_hash(struct args *args, struct mdfour *hash)
 		}
 		free(p);
 	}
+
+/* Possibly hash GCC_COLORS (for color diagnostics). */
+if (compiler_is_gcc()) {
+const char* gcc_colors = getenv("GCC_COLORS");
+if (gcc_colors != NULL) {
+hash_delimiter(hash,"gcccolors");
+hash_string(hash, gcc_colors);
+}
+}
 }
 
 /*
@@ -1626,6 +1653,13 @@ is_precompiled_header(const char *path)
 	   || str_eq(get_extension(path), ".pth");
 }
 
+static bool color_output_possible()
+{
+const char* term_env = getenv("TERM");
+
+return isatty(STDERR_FILENO) && term_env && strcasecmp(term_env, "DUMB") != 0;
+}
+
 /*
  * Process the compiler options into options suitable for passing to the
  * preprocessor and the real compiler. The preprocessor options don't include
@@ -1654,6 +1688,7 @@ cc_process_args(struct args *args, struct args **preprocessor_args,
 	int argc;
 	char **argv;
 	bool result = true;
+	bool found_color_diagnostics = false;
 
 	expanded_args = args_copy(args);
 	stripped_args = args_init(0, NULL);
@@ -2010,6 +2045,21 @@ cc_process_args(struct args *args, struct args **preprocessor_args,
 			free(arg);
 		}
 
+		if (str_eq(argv[i], "-fcolor-diagnostics")
+		|| str_eq(argv[i], "-fno-color-diagnostics")
+		|| str_eq(argv[i], "-fdiagnostics-color")
+		|| str_eq(argv[i], "-fdiagnostics-color=always")
+		|| str_eq(argv[i], "-fno-diagnostics-color")
+		|| str_eq(argv[i], "-fdiagnostics-c