Re: [PATCH][mingw] Enable colorized diagnostics
On 10/09/2017 01:07 PM, Liu Hao wrote: > On 2017/10/9 19:01, JonY wrote: >> On 10/08/2017 11:39 AM, Liu Hao wrote: >> >> I'm not sure if it should be enabled by default due to the interleaving >> problem, but seeing as the user has to go out to set GCC_COLORS to use >> this feature, I suppose it is OK. >> >> I will commit soon if there are no more comments. >> >> > > Thank you. By the way I noticed a mistake in the comments above > `find_esc_terminator()`. This function returns zero on failure like its > `find_esc_head()` counterpart, while the comments mistakenly referred > -1. Please correct it before committing. > > Committed to trunk r253645 with the appropriate change. signature.asc Description: OpenPGP digital signature
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/10/11 5:04, Manuel López-Ibáñez wrote: Ops! You're obviously right. What was I thinking? I still believe that pretty-printer.c is not the right place for all this color-handling code (diagnostic-color.c or libiberty/ may be better places). No and yes. The colors emerge only when those messages are sent to a terminal. Plain arrays of characters don't have colors. So it is _the_ printer that does colorization. Indeed I hope this host-specific code could go elsewhere. My initial attempt to move it to a separated .c file failed, as I mentioned in the very first mail. Also, your code handles a lot more ANSI codes than those needed for color output. The code in grep seems much simpler. Could your fputs replacement split the string as you suggest above, then if the chunk is an ANSI code, use grep's conversion function to transform the codes, otherwise use fputs to print text. At the moment I implemented it, I knew GCC was making of coloring and erase-line codes, while still had no idea whether more codes should be needed in the future. Hence almost all codes viable have been implemented. I hope one day GREP people will be copying my code from GCC. Don't tell them. XD Cheers, Manuel. -- Best regards, LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On 10 Oct 2017 2:34 am, "Liu Hao" wrote: Since on *nix it is not when `colorize_start()` is called that the terminal color is changed (it is when those ANSI escape codes are delivered to the other peer which will translate them), and the string passed to `fputs()` is free to deliver multiple escape codes, it is not an option unless we output integral diagnostic messages using multiple fputs()` calls. For example, ``` test.c:3:9: warning: 'a' is used uninitialized in this function [-Wuninitialized] ``` The words 'warning' and '-Wuninitialized' should be magenta, so there are four ANSI escape codes (two to set the color and another two to restore the color), and this line of text must be output using five individual calls to the `fputs()` function (one for each segment with the consistent color), which is not the case (this whole line of text is delivered using a single call), so all five segments have to be all in magenta or no color at all. This is not a solution. Ops! You're obviously right. What was I thinking? I still believe that pretty-printer.c is not the right place for all this color-handling code (diagnostic-color.c or libiberty/ may be better places). Also, your code handles a lot more ANSI codes than those needed for color output. The code in grep seems much simpler. Could your fputs replacement split the string as you suggest above, then if the chunk is an ANSI code, use grep's conversion function to transform the codes, otherwise use fputs to print text. Cheers, Manuel.
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/10/10 6:25, Manuel López-Ibáñez wrote:
For what is worth, the color output of GCC comes originally from grep,
and grep does have code for colorizing in Windows:
http://git.savannah.gnu.org/cgit/grep.git/tree/lib
and there are significant differences with this patch. For once,
/* $TERM is not normally defined on DOS/Windows, so don't require
it for highlighting. But some programs, like Emacs, do define
it when running Grep as a subprocess, so make sure they don't
set TERM=dumb. */
char const *t = getenv ("TERM");
return ! (t && strcmp (t, "dumb") == 0);
and they don't need a custom fputs() because their strategy is slightly
different: They only override colorize_start (print_start_colorize) and
colorize_stop (print_end_colorize) and convert ANSI sequences to W32
sequences on the fly. Thus, we wouldn't need to touch pretty-printer.c
Since on *nix it is not when `colorize_start()` is called that the
terminal color is changed (it is when those ANSI escape codes are
delivered to the other peer which will translate them), and the string
passed to `fputs()` is free to deliver multiple escape codes, it is not
an option unless we output integral diagnostic messages using multiple
fputs()` calls.
For example,
```
test.c:3:9: warning: 'a' is used uninitialized in this function
[-Wuninitialized]
```
The words 'warning' and '-Wuninitialized' should be magenta, so there
are four ANSI escape codes (two to set the color and another two to
restore the color), and this line of text must be output using five
individual calls to the `fputs()` function (one for each segment with
the consistent color), which is not the case (this whole line of text is
delivered using a single call), so all five segments have to be all in
magenta or no color at all. This is not a solution.
and all changes will be restricted to diagnostic-color.c (which could be
split into -posix.c and -w32.c like grep does and be moved into
host-specific config/ subdir).
Even if the host-specific part is not done, I honestly think it is a
good idea to match grep's code as much as possible since we may want to
merge bugfixes between the two and eventually this code may end up in
gnulib. Moreover, if somebody else implemented color output for another
OS in grep, it would be very easy to transplant it to GCC (or viceversa)
if the API remains close.
Cheers,
Manuel.
--
Best regards,
LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On 09/10/17 23:25, Manuel López-Ibáñez wrote:
Even if the host-specific part is not done, I honestly think it is a good idea
to match grep's code as much as possible since we may want to merge bugfixes
between the two and eventually this code may end up in gnulib. Moreover, if
somebody else implemented color output for another OS in grep, it would be very
easy to transplant it to GCC (or viceversa) if the API remains close.
Something like the attached should do the trick (I didn't even try to compile
it and completely untested, so it may need some adjustments).
Cheers,
Manuel.
Index: diagnostic-color.c
===
--- diagnostic-color.c (revision 253569)
+++ diagnostic-color.c (working copy)
@@ -19,6 +19,12 @@
#include "config.h"
#include "system.h"
#include "diagnostic-color.h"
+#ifdef __MINGW32__
+# undef DATADIR /* conflicts with objidl.h, which is included by windows.h */
+# include
+static HANDLE hstderr = INVALID_HANDLE_VALUE;
+static SHORT norm_attr;
+#endif
/* Select Graphic Rendition (SGR, "\33[...m") strings. */
/* Also Erase in Line (EL) to Right ("\33[K") by default. */
@@ -104,7 +110,125 @@
#define SGR_SEQ(str) SGR_START str SGR_END
#define SGR_RESET SGR_SEQ("")
+#ifdef __MINGW32__
+/* Convert a color spec, a semi-colon separated list of the form
+ SGR_START"NN;MM;KK;..."SGR_END, where each number is a value of the SGR
+ parameter, into the corresponding Windows console text attribute.
+ This function supports a subset of the SGR rendition aspects that
+ the Windows console can display. */
+static int
+w32_sgr2attr (const char *sgr_seq)
+{
+ const char *s, *p;
+ int code, fg = norm_attr & 15, bg = norm_attr & (15 << 4);
+ int bright = 0, inverse = 0;
+ static const int fg_color[] = {
+0, /* black */
+FOREGROUND_RED, /* red */
+FOREGROUND_GREEN, /* green */
+FOREGROUND_GREEN | FOREGROUND_RED, /* yellow */
+FOREGROUND_BLUE, /* blue */
+FOREGROUND_BLUE | FOREGROUND_RED, /* magenta */
+FOREGROUND_BLUE | FOREGROUND_GREEN, /* cyan */
+FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE /* gray */
+ };
+ static const int bg_color[] = {
+0, /* black */
+BACKGROUND_RED, /* red */
+BACKGROUND_GREEN, /* green */
+BACKGROUND_GREEN | BACKGROUND_RED, /* yellow */
+BACKGROUND_BLUE, /* blue */
+BACKGROUND_BLUE | BACKGROUND_RED, /* magenta */
+BACKGROUND_BLUE | BACKGROUND_GREEN, /* cyan */
+BACKGROUND_RED | BACKGROUND_GREEN | BACKGROUND_BLUE /* gray */
+ };
+
+ sgr_seq = sqr_seq + strlen(SGR_START);
+
+ for (s = p = sgr_seq; strcmp(s, SGR_END) != 0; p++)
+{
+ if (*p == ';' || strcmp(p, SGR_END) == 0)
+{
+ code = strtol (s, NULL, 10);
+ s = p + (strcmp(p, SGR_END) != 0);
+
+ switch (code)
+{
+case 0: /* all attributes off */
+ fg = norm_attr & 15;
+ bg = norm_attr & (15 << 4);
+ bright = 0;
+ inverse = 0;
+ break;
+case 1: /* intensity on */
+ bright = 1;
+ break;
+case 7: /* inverse video */
+ inverse = 1;
+ break;
+case 22: /* intensity off */
+ bright = 0;
+ break;
+case 27: /* inverse off */
+ inverse = 0;
+ break;
+case 30: case 31: case 32: case 33: /* foreground color */
+case 34: case 35: case 36: case 37:
+ fg = fg_color[code - 30];
+ break;
+case 39: /* default foreground */
+ fg = norm_attr & 15;
+ break;
+case 40: case 41: case 42: case 43: /* background color */
+case 44: case 45: case 46: case 47:
+ bg = bg_color[code - 40];
+ break;
+case 49: /* default background */
+ bg = norm_attr & (15 << 4);
+ break;
+default:
+ break;
+}
+}
+}
+ if (inverse)
+{
+ int t = fg;
+ fg = (bg >> 4);
+ bg = (t << 4);
+}
+ if (bright)
+fg |= FOREGROUND_INTENSITY;
+
+ return (bg & (15 << 4)) | (fg & 15);
+}
+
+/* Clear to the end of the current line with the default attribute.
+ This is needed for reasons similar to those that require the "EL to
+ Right after SGR" operation on Posix platforms: if we don't do this,
+ setting the 'mt', 'ms', or 'mc' capabilities to use a non-default
+ background color spills that color to the empty space at the end of
+ the last screen line in a match whose line spans multiple screen
+ lines. */
+static void
+w32_clreol (void)
+{
+ DWORD nchars;
+ COORD start_pos;
+ DWORD written;
+ CONSOLE_SCREEN_BUFFER_INFO csbi;
+
+ GetConsoleScreenBufferInfo (hstderr, &csbi);
+ start_pos = csbi.dwCursorPosition;
+ nchars = csbi.dwSize.X - start_pos.X;
+
+ Fil
Re: [PATCH][mingw] Enable colorized diagnostics
On 08/10/17 12:39, Liu Hao wrote:
On 2017/9/28 4:09, Joseph Myers wrote:
On Thu, 28 Sep 2017, Liu Hao wrote:
Colorized diagnostics used to be disabled for MinGW targets (on which
the macro `_WIN32` is defined), and this patch enables it.
I'd hope this is all to do with MinGW host, and nothing to do with the
target.
Ping? Are there any more opinions about this?
For what is worth, the color output of GCC comes originally from grep, and grep
does have code for colorizing in Windows:
http://git.savannah.gnu.org/cgit/grep.git/tree/lib
and there are significant differences with this patch. For once,
/* $TERM is not normally defined on DOS/Windows, so don't require
it for highlighting. But some programs, like Emacs, do define
it when running Grep as a subprocess, so make sure they don't
set TERM=dumb. */
char const *t = getenv ("TERM");
return ! (t && strcmp (t, "dumb") == 0);
and they don't need a custom fputs() because their strategy is slightly
different: They only override colorize_start (print_start_colorize) and
colorize_stop (print_end_colorize) and convert ANSI sequences to W32 sequences
on the fly. Thus, we wouldn't need to touch pretty-printer.c and all changes
will be restricted to diagnostic-color.c (which could be split into -posix.c
and -w32.c like grep does and be moved into host-specific config/ subdir).
Even if the host-specific part is not done, I honestly think it is a good idea
to match grep's code as much as possible since we may want to merge bugfixes
between the two and eventually this code may end up in gnulib. Moreover, if
somebody else implemented color output for another OS in grep, it would be very
easy to transplant it to GCC (or viceversa) if the API remains close.
Cheers,
Manuel.
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/10/9 22:16, David Malcolm wrote: I have some concerns about adding non-trivial host-specific code to the diagnostics subsystem. I occasionally make changes to the files you're touching, but I don't have access to the host in question, so I can't test that I don't break things on MinGW. Is it OK if this is the MinGW team's problem, and not mine? (i.e. can you please clean up after me if I break something on MinGW?). I am tracing the branch for the latest stable major release (`gcc-7-branch` at the moment) closely. If anything is broken I will let you know. Also, you might want to add some selftests to the code e.g. for find_esc_head and find_esc_terminator. Those functions are static - they will not be visible elsewhere. They are formulated for clarification purposes and nothing else. I looked at the docs for the Windows console API and unfortunately there doesn't seem to be a way to set up a dummy console for unit-testing the parts of the code that call the console API directly. But find_esc_head and find_esc_terminator don't directly call the API, and hence you can write some selftest functions for them. (there's probably a much more involved way to test this using mocks/stubs for the console API, but that's probably overkill). Dave -- Best regards, LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On Mon, 2017-10-09 at 11:01 +, JonY wrote: > On 10/08/2017 11:39 AM, Liu Hao wrote: > > On 2017/9/28 4:09, Joseph Myers wrote: > > > On Thu, 28 Sep 2017, Liu Hao wrote: > > > > > > > Colorized diagnostics used to be disabled for MinGW targets (on > > > > which > > > > the macro `_WIN32` is defined), and this patch enables it. > > > > > > I'd hope this is all to do with MinGW host, and nothing to do > > > with the > > > target. > > > > > > > Ping? Are there any more opinions about this? > > > > I'm not sure if it should be enabled by default due to the > interleaving > problem, but seeing as the user has to go out to set GCC_COLORS to > use > this feature, I suppose it is OK. > > I will commit soon if there are no more comments. I have some concerns about adding non-trivial host-specific code to the diagnostics subsystem. I occasionally make changes to the files you're touching, but I don't have access to the host in question, so I can't test that I don't break things on MinGW. Is it OK if this is the MinGW team's problem, and not mine? (i.e. can you please clean up after me if I break something on MinGW?). Also, you might want to add some selftests to the code e.g. for find_esc_head and find_esc_terminator. I looked at the docs for the Windows console API and unfortunately there doesn't seem to be a way to set up a dummy console for unit-testing the parts of the code that call the console API directly. But find_esc_head and find_esc_terminator don't directly call the API, and hence you can write some selftest functions for them. (there's probably a much more involved way to test this using mocks/stubs for the console API, but that's probably overkill). Dave
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/10/9 19:01, JonY wrote: On 10/08/2017 11:39 AM, Liu Hao wrote: I'm not sure if it should be enabled by default due to the interleaving problem, but seeing as the user has to go out to set GCC_COLORS to use this feature, I suppose it is OK. I will commit soon if there are no more comments. Thank you. By the way I noticed a mistake in the comments above `find_esc_terminator()`. This function returns zero on failure like its `find_esc_head()` counterpart, while the comments mistakenly referred -1. Please correct it before committing. -- Best regards, LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On 10/08/2017 11:39 AM, Liu Hao wrote: > On 2017/9/28 4:09, Joseph Myers wrote: >> On Thu, 28 Sep 2017, Liu Hao wrote: >> >>> Colorized diagnostics used to be disabled for MinGW targets (on which >>> the macro `_WIN32` is defined), and this patch enables it. >> >> I'd hope this is all to do with MinGW host, and nothing to do with the >> target. >> > Ping? Are there any more opinions about this? > I'm not sure if it should be enabled by default due to the interleaving problem, but seeing as the user has to go out to set GCC_COLORS to use this feature, I suppose it is OK. I will commit soon if there are no more comments. signature.asc Description: OpenPGP digital signature
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/10/8 20:24, Hannes Domani wrote: Am Sonntag, 8. Oktober 2017, 14:02:48 MESZ hat Liu Hao Folgendes geschrieben: > On 2017/10/8 19:55, Hannes Domani wrote: > > > So why not just enable it on Win10? > > > It is up to you, GCC maintainers. If dropping support for Windows prior > to Windows 10 TH2 is an option, I may provide another patch, which I > can't test because I primarily work on Windows 7. XD That wouldn't mean dropping support for earlier Windows. If enabling of ENABLE_VIRTUAL_TERMINAL_PROCESSING fails, just disable the colored output. It is exactly what I meant. I really, really want it on Windows 7, nevertheless. -- Best regards, LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/10/8 19:55, Hannes Domani wrote: So why not just enable it on Win10? It is up to you, GCC maintainers. If dropping support for Windows prior to Windows 10 TH2 is an option, I may provide another patch, which I can't test because I primarily work on Windows 7. XD -- Best regards, LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/9/28 4:09, Joseph Myers wrote: On Thu, 28 Sep 2017, Liu Hao wrote: Colorized diagnostics used to be disabled for MinGW targets (on which the macro `_WIN32` is defined), and this patch enables it. I'd hope this is all to do with MinGW host, and nothing to do with the target. Ping? Are there any more opinions about this? -- Best regards, LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/9/28 7:37, JonY wrote: Does it make sense to use a global lock in mingw_ansi_fputs? I was thinking about a named Mutex object. Named Mutexes (as well as Events and Semaphores) can be shared across processes, but there are other considerations: 1. The name of the Mutex should base on the current console window which is shared by all child processes created by `make`, and must be unique. How can it be? Is it possible to create a string basing on the window handle or unique identifier whatsoever? Will the handle or unique id be reused after the window is destroyed? Is it unique after all? 2. This Mutex would only protect diagnostics from interleaving. Diagnostics can interleave with other messages written via stdio functions, including those written to `stdout` which is often output to the console as well. I don't think there are any solutions for this. -- Best regards, LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On 09/27/2017 08:54 PM, Liu Hao wrote: > On 2017/9/28 4:09, Joseph Myers wrote: >> On Thu, 28 Sep 2017, Liu Hao wrote: >> >>> Colorized diagnostics used to be disabled for MinGW targets (on which >>> the macro `_WIN32` is defined), and this patch enables it. >> >> I'd hope this is all to do with MinGW host, and nothing to do with the >> target. >> > Oh you are right. Since I build native compilers, host == target here. > But strictly speaking the patch applies to the host, not the target. > Does it make sense to use a global lock in mingw_ansi_fputs? signature.asc Description: OpenPGP digital signature
Re: [PATCH][mingw] Enable colorized diagnostics
On 2017/9/28 4:09, Joseph Myers wrote: On Thu, 28 Sep 2017, Liu Hao wrote: Colorized diagnostics used to be disabled for MinGW targets (on which the macro `_WIN32` is defined), and this patch enables it. I'd hope this is all to do with MinGW host, and nothing to do with the target. Oh you are right. Since I build native compilers, host == target here. But strictly speaking the patch applies to the host, not the target. -- Best regards, LH_Mouse
Re: [PATCH][mingw] Enable colorized diagnostics
On Thu, 28 Sep 2017, Liu Hao wrote: > Colorized diagnostics used to be disabled for MinGW targets (on which > the macro `_WIN32` is defined), and this patch enables it. I'd hope this is all to do with MinGW host, and nothing to do with the target. -- Joseph S. Myers jos...@codesourcery.com
