[00/16] Add styling to the gdb CLI and TUI

Message ID 20181128001435.12703-1-tom@tromey.com
Headers show
Series
  • Add styling to the gdb CLI and TUI
Related show

Message

Tom Tromey Nov. 28, 2018, 12:14 a.m.
This patch series adds terminal styling (colorizing) to gdb's CLI and
TUI.  It is a refresh of an earlier RFC series, but some parts have
changed, and now there are tests and documentation.  It also includes
some TUI colorizing patches that were sent separately.

This series doesn't support the Windows console.  I don't know
anything about it.  So, it defaults to disabling styling on Windows
hosts.  This could be fixed by doing something like what the TUI does:
filter escape sequences from the output and apply them by some other
means.

More styling could be done.  It's relatively easy to add a new
style-able object.

If you want to try this out, it is the branch "submit/colorize" in my
github repository.

Regression tested on x86-64 Fedora 28.  I think it was too big for the
buildbot.

Tom

Comments

Eli Zaretskii Nov. 28, 2018, 7:02 a.m. | #1
> From: Tom Tromey <tom@tromey.com>

> Date: Tue, 27 Nov 2018 17:14:19 -0700

> 

> This series doesn't support the Windows console.  I don't know

> anything about it.  So, it defaults to disabling styling on Windows

> hosts.  This could be fixed by doing something like what the TUI does:

> filter escape sequences from the output and apply them by some other

> means.


Will the Windows TUI build support styling out of the box?  It uses
ncurses.

And I don't think I understand what you mean by "filter escape
sequences from the output".  Where (on what level) would such filter
be installed, given that output is written directly to the console?

I see that you introduced the emit_style_escape function that switches
styles.  What I don't think I understand is whether it will work to
have a Windows implementation of that that calls a function which
causes the text output after that to use given colors?  It seems it
will, because the code calls emit_style_escape before and after each
string, but I cannot be sure.

Thanks.
Tom Tromey Nov. 29, 2018, 10:43 p.m. | #2
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


Eli> Will the Windows TUI build support styling out of the box?  It uses
Eli> ncurses.

I think it should work, but you'd have to "set style enabled on" first.

Eli> And I don't think I understand what you mean by "filter escape
Eli> sequences from the output".  Where (on what level) would such filter
Eli> be installed, given that output is written directly to the console?

I think either utils.c would have to be modified to change where it
sends output, or stdio_file::puts would have to be modified.  The idea
there would be to call a host-specific function; and then on Windows do
the filtering+styling if the output is going to the terminal.

Eli> I see that you introduced the emit_style_escape function that switches
Eli> styles.  What I don't think I understand is whether it will work to
Eli> have a Windows implementation of that that calls a function which
Eli> causes the text output after that to use given colors?  It seems it
Eli> will, because the code calls emit_style_escape before and after each
Eli> string, but I cannot be sure.

Doing it that way can't work due to buffering.  Also, this approach
would be undesirable anyway, because GNU Source Highlight emits escape
codes -- that's why I abandoned my earlier plan of implementing styling
as objects in the utils.c buffer.  Instead, I think filtering the escape
sequences is really the only way.

Tom
Eli Zaretskii Nov. 30, 2018, 7:03 a.m. | #3
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Thu, 29 Nov 2018 15:43:59 -0700

> 

> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

> 

> Eli> Will the Windows TUI build support styling out of the box?  It uses

> Eli> ncurses.

> 

> I think it should work, but you'd have to "set style enabled on" first.


Maybe a TUI invocation should "set style enabled on" on all platforms?
Or at least on those that use ncurses?

> Eli> And I don't think I understand what you mean by "filter escape

> Eli> sequences from the output".  Where (on what level) would such filter

> Eli> be installed, given that output is written directly to the console?

> 

> I think either utils.c would have to be modified to change where it

> sends output, or stdio_file::puts would have to be modified.  The idea

> there would be to call a host-specific function; and then on Windows do

> the filtering+styling if the output is going to the terminal.


Ouch!  I hoped we could avoid such kludges.  Although it could, of
course, be done, I did that at some point for Gnu Grep.  One problem
with this approach is that it needs to fix the escape sequences for
the relevant attributes, whereas AFAIU your code simply uses the
terminfo that happens to be in effect, is that right?

> Eli> I see that you introduced the emit_style_escape function that switches

> Eli> styles.  What I don't think I understand is whether it will work to

> Eli> have a Windows implementation of that that calls a function which

> Eli> causes the text output after that to use given colors?  It seems it

> Eli> will, because the code calls emit_style_escape before and after each

> Eli> string, but I cannot be sure.

> 

> Doing it that way can't work due to buffering.


Not sure I understand.  Console output is generally line-buffered, and
there's fflush to force writing any buffered output before applying
text attributes.  Am I missing something?

> Also, this approach would be undesirable anyway, because GNU Source

> Highlight emits escape codes -- that's why I abandoned my earlier

> plan of implementing styling as objects in the utils.c buffer.


What is GNU Source Highlight, and what is its relevance to the issue
at hand?

> Instead, I think filtering the escape sequences is really the only

> way.


The problem with that is that it hard-codes the SGR sequences concepts
right into the design, and the escape sequences are unknown in advance
on systems where there's no terminfo.

Thanks.
Tom Tromey Nov. 30, 2018, 4:17 p.m. | #4
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


Tom> I think it should work, but you'd have to "set style enabled on" first.

Eli> Maybe a TUI invocation should "set style enabled on" on all platforms?
Eli> Or at least on those that use ncurses?

That would possibly override the user's setting.  It's probably better
to simply implement the console support for Windows.  Or, change how the
disabling is done on Windows -- right now it is done by changing the
default for this setting, but perhaps it could be done some other way.

Is it possible to ssh in to a Windows machine and then use gdb?  That's
one scenario where you may want to keep the ANSI terminal escape
sequences in the output.  It also may affect how calls to the Windows
API are handled.

Eli> One problem with this approach is that it needs to fix the escape
Eli> sequences for the relevant attributes, whereas AFAIU your code
Eli> simply uses the terminfo that happens to be in effect, is that
Eli> right?

This patch series uses ANSI escape sequences for the styling; and the
TUI decodes these and turns them into curses calls.

Tom> Doing it that way can't work due to buffering.

Eli> Not sure I understand.  Console output is generally line-buffered, and
Eli> there's fflush to force writing any buffered output before applying
Eli> text attributes.  Am I missing something?

utils.c implements its own buffering.  Previous to this series this was
only done when "wrap_column > 0", but patch #1 changes this code to
always buffer.

This is a problem for other approaches because styles are emitted inline
with other text; and then flushed as a unit.

Tom> Also, this approach would be undesirable anyway, because GNU Source
Tom> Highlight emits escape codes -- that's why I abandoned my earlier
Tom> plan of implementing styling as objects in the utils.c buffer.

Eli> What is GNU Source Highlight, and what is its relevance to the issue
Eli> at hand?

GNU Source Highlight is used to style source text.

    https://www.gnu.org/software/src-highlite/

Support for using it is added to gdb in patch #15.

Tom> Instead, I think filtering the escape sequences is really the only
Tom> way.

Eli> The problem with that is that it hard-codes the SGR sequences concepts
Eli> right into the design, and the escape sequences are unknown in advance
Eli> on systems where there's no terminfo.

Only terminals using the ANSI sequences are supported by this series, at
least for the CLI.  The TUI can support more in theory, though I have no
way to test that.

Tom
Joel Brobecker Dec. 23, 2018, 10:48 a.m. | #5
[/me starting to look into this patch series...]

To answer one specific question:

> Is it possible to ssh in to a Windows machine and then use gdb?  That's

> one scenario where you may want to keep the ANSI terminal escape

> sequences in the output.  It also may affect how calls to the Windows

> API are handled.


Yes, it is possible to ssh into a Windows machine. The SSH server
we used is cygwin's one, and it's a bit of a special situation as stdin
is actually not viewed as a TTY, IIRC.  This has various consequences
because we actually want GDB to think it's a TTY. This is why we have
the "set interactive mode" command.

I am no longer sure whether this is related to TTY-ness or not, but
buffering was also an issue, that caused stdout/stderr to be output
in random order. That caused a lot of grief trying to run the testsuite,
so we "fixed" that internally at AdaCore by turning buffering off
entirely.

-- 
Joel
Joel Brobecker Dec. 24, 2018, 9:27 a.m. | #6
> This patch series adds terminal styling (colorizing) to gdb's CLI and

> TUI.  It is a refresh of an earlier RFC series, but some parts have

> changed, and now there are tests and documentation.  It also includes

> some TUI colorizing patches that were sent separately.

> 

> This series doesn't support the Windows console.  I don't know

> anything about it.  So, it defaults to disabling styling on Windows

> hosts.  This could be fixed by doing something like what the TUI does:

> filter escape sequences from the output and apply them by some other

> means.

> 

> More styling could be done.  It's relatively easy to add a new

> style-able object.

> 

> If you want to try this out, it is the branch "submit/colorize" in my

> github repository.

> 

> Regression tested on x86-64 Fedora 28.  I think it was too big for the

> buildbot.


I gave the whole series a look. The TUI changes were a bit alien
to me. I sent comments on the patches where I had questions;
otherwise, those I didn't comment on looked fine to me.

I really look forward to this series making it to master, it would
be a nice feature to advertise on the release that includes it.
The source highlighting is also a very nice touch!

One feature I might volunteer to work on once the series is in
is to provide two sets of defaults for the colors - one to be used
when the terminal has a dark background vs one to be used when
the terminal has a light background. I think the way ou added
colorizing should make it easy to do.

That's all I had to say as a summary. Thanks a lot for those.
I think people are going to enjoy this enchancement. At least
I know I will!

-- 
Joel
Tom Tromey Dec. 28, 2018, 8:57 p.m. | #7
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:


Joel> I gave the whole series a look. The TUI changes were a bit alien
Joel> to me. I sent comments on the patches where I had questions;
Joel> otherwise, those I didn't comment on looked fine to me.

Thanks for the reviews!

Joel> One feature I might volunteer to work on once the series is in
Joel> is to provide two sets of defaults for the colors - one to be used
Joel> when the terminal has a dark background vs one to be used when
Joel> the terminal has a light background. I think the way ou added
Joel> colorizing should make it easy to do.

This sounds like a good idea, and we can certainly tinker with it as
need be.

I have a few patches to GNU Source Highlight that I am trying to get in.
I have some updates for the C and C++ support, plus Rust support.  The
project seems a bit dead so I've asked maintainers@gnu to give me
permissions to check in some patches and do a new release.

Joel> That's all I had to say as a summary. Thanks a lot for those.
Joel> I think people are going to enjoy this enchancement. At least
Joel> I know I will!

Thanks.  I am going to check in the series now.

Tom
Eli Zaretskii March 1, 2019, 7:47 a.m. | #8
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Thu, 29 Nov 2018 15:43:59 -0700

> 

> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

> 

> Eli> Will the Windows TUI build support styling out of the box?  It uses

> Eli> ncurses.

> 

> I think it should work, but you'd have to "set style enabled on" first.


For the record, it doesn't work in GDB 8.2.90, even after I disabled
the test for $TERM being defined in the environment.  I will try to
find out why at some point.

> I think either utils.c would have to be modified to change where it

> sends output, or stdio_file::puts would have to be modified.  The idea

> there would be to call a host-specific function; and then on Windows do

> the filtering+styling if the output is going to the terminal.


I'm looking into this now.  The simplest change would be to modify
stdio_file::puts that performs the filtering only on Windows, but the
fact that you mentioned calling a host-specific function confuses me a
little.  Can you elaborate on what you had in mind, and specifically
what kind of a host-specific function should be involved here, and
how?

Thanks.
Pedro Alves March 1, 2019, 1:10 p.m. | #9
On 11/30/2018 04:17 PM, Tom Tromey wrote:
>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

> 

> Tom> I think it should work, but you'd have to "set style enabled on" first.

> 

> Eli> Maybe a TUI invocation should "set style enabled on" on all platforms?

> Eli> Or at least on those that use ncurses?

> 

> That would possibly override the user's setting.  It's probably better

> to simply implement the console support for Windows.  Or, change how the

> disabling is done on Windows -- right now it is done by changing the

> default for this setting, but perhaps it could be done some other way.

> 

> Is it possible to ssh in to a Windows machine and then use gdb?  That's

> one scenario where you may want to keep the ANSI terminal escape

> sequences in the output.  It also may affect how calls to the Windows

> API are handled.

> 

> Eli> One problem with this approach is that it needs to fix the escape

> Eli> sequences for the relevant attributes, whereas AFAIU your code

> Eli> simply uses the terminfo that happens to be in effect, is that

> Eli> right?

> 

> This patch series uses ANSI escape sequences for the styling; and the

> TUI decodes these and turns them into curses calls.

> 

> Tom> Doing it that way can't work due to buffering.

> 

> Eli> Not sure I understand.  Console output is generally line-buffered, and

> Eli> there's fflush to force writing any buffered output before applying

> Eli> text attributes.  Am I missing something?

> 

> utils.c implements its own buffering.  Previous to this series this was

> only done when "wrap_column > 0", but patch #1 changes this code to

> always buffer.

> 

> This is a problem for other approaches because styles are emitted inline

> with other text; and then flushed as a unit.

> 

> Tom> Also, this approach would be undesirable anyway, because GNU Source

> Tom> Highlight emits escape codes -- that's why I abandoned my earlier

> Tom> plan of implementing styling as objects in the utils.c buffer.

> 

> Eli> What is GNU Source Highlight, and what is its relevance to the issue

> Eli> at hand?

> 

> GNU Source Highlight is used to style source text.

> 

>     https://www.gnu.org/software/src-highlite/

> 

> Support for using it is added to gdb in patch #15.

> 

> Tom> Instead, I think filtering the escape sequences is really the only

> Tom> way.

> 

> Eli> The problem with that is that it hard-codes the SGR sequences concepts

> Eli> right into the design, and the escape sequences are unknown in advance

> Eli> on systems where there's no terminfo.

> 

> Only terminals using the ANSI sequences are supported by this series, at

> least for the CLI.  The TUI can support more in theory, though I have no

> way to test that.


The Windows console in Windows 10 Anniversary Update supports ANSI escape sequences:

 [1] https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences
 [2] https://blogs.msdn.microsoft.com/commandline/2017/04/11/windows-10-creators-update-whats-new-in-bashwsl-windows-console/

Basically, GDB needs to call SetConsoleMode with
ENABLE_VIRTUAL_TERMINAL_PROCESSING at early startup to enable the feature.
There's an example at the bottom of page [1] above.

I have no idea whether that interacts badly with readline or curses, but
it seems like the ideal solution going forward.

Thanks,
Pedro Alves
Eli Zaretskii March 1, 2019, 1:56 p.m. | #10
> Cc: gdb-patches@sourceware.org

> From: Pedro Alves <palves@redhat.com>

> Date: Fri, 1 Mar 2019 13:10:35 +0000

> 

> The Windows console in Windows 10 Anniversary Update supports ANSI escape sequences:

> 

>  [1] https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences

>  [2] https://blogs.msdn.microsoft.com/commandline/2017/04/11/windows-10-creators-update-whats-new-in-bashwsl-windows-console/


Yes, I know.  I guess GDB will have to use that, at least at some
point, or maybe as an option even today.  Thanks for the pointers,
regardless.

> Basically, GDB needs to call SetConsoleMode with

> ENABLE_VIRTUAL_TERMINAL_PROCESSING at early startup to enable the feature.

> There's an example at the bottom of page [1] above.

> 

> I have no idea whether that interacts badly with readline or curses, but

> it seems like the ideal solution going forward.


We don't want to require Windows 10 for the styling support, do we?
Older versions of Windows still have quite a few years before they are
unsupported even by Microsoft, let alone GDB.

Or are you saying that you will be unwilling to accept patches that
enable styling on all versions of Windows (by converting SGR sequences
to corresponding Windows API calls)?
Pedro Alves March 1, 2019, 2:10 p.m. | #11
On 03/01/2019 01:56 PM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org

>> From: Pedro Alves <palves@redhat.com>


>> Basically, GDB needs to call SetConsoleMode with

>> ENABLE_VIRTUAL_TERMINAL_PROCESSING at early startup to enable the feature.

>> There's an example at the bottom of page [1] above.

>>

>> I have no idea whether that interacts badly with readline or curses, but

>> it seems like the ideal solution going forward.

> 

> We don't want to require Windows 10 for the styling support, do we?

> Older versions of Windows still have quite a few years before they are

> unsupported even by Microsoft, let alone GDB.


I'm saying that there's an easy way out that likely
gets you styling without much work.  Over time, older versions of
Windows will fade away.  I'm comparing to the current state, where
there's no styling at all.

> Or are you saying that you will be unwilling to accept patches that

> enable styling on all versions of Windows (by converting SGR sequences

> to corresponding Windows API calls)?


Of course not.  If someone's willing to implement that, it's super
fine with me.  It's not unlike what Tom did for TUI / curses.

Thanks,
Pedro Alves
Eli Zaretskii March 1, 2019, 2:50 p.m. | #12
> Cc: tom@tromey.com, gdb-patches@sourceware.org

> From: Pedro Alves <palves@redhat.com>

> Date: Fri, 1 Mar 2019 14:10:00 +0000

> 

> I'm saying that there's an easy way out that likely

> gets you styling without much work.  Over time, older versions of

> Windows will fade away.


That's a long wait, IMO.

> I'm comparing to the current state, where there's no styling at all.


That's not yet a "state", since GDB 8.3 was not yet released ;-)

> If someone's willing to implement that, it's super fine with me.


Thank you, then I will continue working on this.
Tom Tromey March 1, 2019, 6:42 p.m. | #13
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


>> I think either utils.c would have to be modified to change where it

>> sends output, or stdio_file::puts would have to be modified.  The idea

>> there would be to call a host-specific function; and then on Windows do

>> the filtering+styling if the output is going to the terminal.


Eli> I'm looking into this now.  The simplest change would be to modify
Eli> stdio_file::puts that performs the filtering only on Windows, but the
Eli> fact that you mentioned calling a host-specific function confuses me a
Eli> little.  Can you elaborate on what you had in mind, and specifically
Eli> what kind of a host-specific function should be involved here, and
Eli> how?

Right now styling works because, in the end, ANSI escape sequences are
emitted via stdio_file::puts, which just calls fputs.

However, my understanding is that the Windows console instead requires
styling to be done by making various API calls.

So, the idea is to replace stdio_file::puts with a function that, for
the Windows console only, parses out the ANSI escapes from the output
string and replaces those with the appropriate API calls.

This made sense to me as a host-dependent function of some kind.  But I
don't really know.

The TUI does essentially this in order to convert ANSI escapes into
curses calls.  See tui-io.c:tui_puts_internal and apply_ansi_escape.
The parsing work is done by the method ui_file_style::parse.

Tom
Eli Zaretskii March 1, 2019, 7:40 p.m. | #14
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Fri, 01 Mar 2019 11:42:39 -0700

> 

> Right now styling works because, in the end, ANSI escape sequences are

> emitted via stdio_file::puts, which just calls fputs.

> 

> However, my understanding is that the Windows console instead requires

> styling to be done by making various API calls.

> 

> So, the idea is to replace stdio_file::puts with a function that, for

> the Windows console only, parses out the ANSI escapes from the output

> string and replaces those with the appropriate API calls.


Right.  My question is about the details, or, more accurately, about
stylistic conventions and policies in such cases.

> This made sense to me as a host-dependent function of some kind.  But I

> don't really know.


I'm not sure I understand how to call that host-dependent function.
You don't mean #ifdef, do you?  My idea was to do something like

  void
  stdio_file::puts (const char *linebuffer)
  {
  #ifdef _WIN32
    w32_fputs (linebuffer, m_file);
  #else
    /* Calling error crashes when we are called from the exception framework.  */
    if (fputs (linebuffer, m_file))
      {
	/* Nothing.  */
      }
  #endif
  }

where w32_fputs will DTRT for Windows.  If this is not what you had in
mind, can you maybe point me to a place where host-dependent functions
are called?

> The TUI does essentially this in order to convert ANSI escapes into

> curses calls.  See tui-io.c:tui_puts_internal and apply_ansi_escape.

> The parsing work is done by the method ui_file_style::parse.


Right, but TUI overrides the puts method, which is not what I need to
do here.

Thanks.
Tom Tromey March 1, 2019, 9:03 p.m. | #15
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


Eli> I'm not sure I understand how to call that host-dependent function.
Eli> You don't mean #ifdef, do you?  My idea was to do something like
[...]

Eli> where w32_fputs will DTRT for Windows.  If this is not what you had in
Eli> mind, can you maybe point me to a place where host-dependent functions
Eli> are called?

Another way would be to just make a new function and implement it
differently in mingw-hdep.c and in posix-hdep.c.

Tom
Eli Zaretskii March 2, 2019, 7:15 a.m. | #16
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Fri, 01 Mar 2019 14:03:58 -0700

> 

> Eli> where w32_fputs will DTRT for Windows.  If this is not what you had in

> Eli> mind, can you maybe point me to a place where host-dependent functions

> Eli> are called?

> 

> Another way would be to just make a new function and implement it

> differently in mingw-hdep.c and in posix-hdep.c.


OK, thanks, I think I see what I need to do.
Eli Zaretskii March 3, 2019, 3:42 p.m. | #17
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Fri, 01 Mar 2019 14:03:58 -0700

> 

> Another way would be to just make a new function and implement it

> differently in mingw-hdep.c and in posix-hdep.c.


Here's a working prototype I'm currently testing.  If this
implementation strategy is acceptable, I will post a proper patch
soon.

Thanks for the guidance.

--- ./gdb/cli/cli-style.c~1	2019-02-27 06:51:50.000000000 +0200
+++ ./gdb/cli/cli-style.c	2019-03-03 17:26:34.122234300 +0200
@@ -24,7 +24,7 @@
 
 /* True if styling is enabled.  */
 
-#if defined(_WIN32) || defined (__CYGWIN__)
+#if defined (__MSDOS__) || defined (__CYGWIN__)
 int cli_styling = 0;
 #else
 int cli_styling = 1;
--- ./gdb/mingw-hdep.c~1	2019-02-27 06:51:50.000000000 +0200
+++ ./gdb/mingw-hdep.c	2019-03-03 17:15:24.608418300 +0200
@@ -177,3 +177,166 @@ gdb_select (int n, fd_set *readfds, fd_s
 
   return num_ready;
 }
+
+/* Zero if not yet initialized, 1 if stdout is a console device, else -1.  */
+static int mingw_console_initialized;
+
+/* Handle to stdout . */
+static HANDLE hstdout = INVALID_HANDLE_VALUE;
+
+/* Text attribute to use for normal text (the "none" pseudo-color).  */
+static SHORT  norm_attr;
+
+/* The most recently applied style.  */
+static ui_file_style last_style;
+
+/* Alternative for the libc 'fputs' which handles embedded SGR
+   sequences in support of styling.  */
+
+int
+gdb_console_fputs (const char *linebuf, FILE *fstream)
+{
+  if (!mingw_console_initialized)
+    {
+      hstdout = (HANDLE)_get_osfhandle (fileno (fstream));
+      DWORD cmode;
+      CONSOLE_SCREEN_BUFFER_INFO csbi;
+
+      if (hstdout != INVALID_HANDLE_VALUE
+	  && GetConsoleMode (hstdout, &cmode) != 0
+	  && GetConsoleScreenBufferInfo (hstdout, &csbi))
+	{
+	  norm_attr = csbi.wAttributes;
+	  mingw_console_initialized = 1;
+	}
+      else if (hstdout != INVALID_HANDLE_VALUE)
+	mingw_console_initialized = -1; /* valid, but not a console device */
+    }
+  /* If our stdout is not a console device, let the default 'fputs'
+     handle the task. */
+  if (mingw_console_initialized <= 0)
+    return 0;
+
+  /* Mapping between 8 ANSI colors and Windows console attributes.  */
+  static 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 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 */
+  };
+
+  ui_file_style style = last_style;
+  unsigned char c;
+  size_t n_read;
+
+  for ( ; (c = *linebuf) != 0; linebuf += n_read)
+    {
+      if (c == '\033')
+	{
+	  fflush (fstream);
+	  bool parsed = style.parse (linebuf, &n_read);
+	  if (n_read <= 0)	/* should never happen */
+	    n_read = 1;
+	  if (!parsed)
+	    {
+	      /* This means we silently swallow SGR sequences we
+		 cannot parse.  */
+	      continue;
+	    }
+	  /* Colors.  */
+	  const ui_file_style::color &fg = style.get_foreground ();
+	  const ui_file_style::color &bg = style.get_background ();
+	  int fgcolor, bgcolor, bright, inverse;
+	  if (fg.is_none ())
+	    fgcolor = norm_attr & 15;
+	  else
+	    fgcolor = fg_color[fg.get_value () & 15];
+	  if (bg.is_none ())
+	    bgcolor = norm_attr & (15 << 4);
+	  else
+	    bgcolor = bg_color[bg.get_value () & 15];
+
+	  /* Intensity.  */
+	  switch (style.get_intensity ())
+	    {
+	    case ui_file_style::NORMAL:
+	    case ui_file_style::DIM:
+	      bright = 0;
+	      break;
+	    case ui_file_style::BOLD:
+	      bright = 1;
+	      break;
+	    default:
+	      gdb_assert_not_reached ("invalid intensity");
+	    }
+
+	  /* Inverse video.  */
+	  if (style.is_reverse ())
+	    inverse = 1;
+	  else
+	    inverse = 0;
+
+	  /* Construct the attribute.  */
+	  if (inverse)
+	    {
+	      int t = fgcolor;
+	      fgcolor = (bgcolor >> 4);
+	      bgcolor = (t << 4);
+	    }
+	  if (bright)
+	    fgcolor |= FOREGROUND_INTENSITY;
+
+	  SHORT attr = (bgcolor & (15 << 4)) | (fgcolor & 15);
+
+	  /* Apply the attribute.  */
+	  SetConsoleTextAttribute (hstdout, attr);
+	}
+      else
+	{
+	  /* When we are about to write newline, we need to clear to
+	     EOL with the normal attribute, to avoid spilling the
+	     colors to the next screen line.  We assume here that no
+	     non-default attribute extends beyond the newline.  */
+	  if (c == '\n')
+	    {
+	      DWORD nchars;
+	      COORD start_pos;
+	      DWORD written;
+	      CONSOLE_SCREEN_BUFFER_INFO csbi;
+
+	      fflush (fstream);
+	      GetConsoleScreenBufferInfo (hstdout, &csbi);
+
+	      if (csbi.wAttributes != norm_attr)
+		{
+		  start_pos = csbi.dwCursorPosition;
+		  nchars = csbi.dwSize.X - start_pos.X;
+
+		  FillConsoleOutputAttribute (hstdout, norm_attr, nchars,
+					      start_pos, &written);
+		  FillConsoleOutputCharacter (hstdout, ' ', nchars,
+					      start_pos, &written);
+		}
+	    }
+	  fputc (c, fstream);
+	  n_read = 1;
+	}
+    }
+
+  last_style = style;
+  return 1;
+}
--- ./gdb/posix-hdep.c~1	2019-02-27 06:51:50.000000000 +0200
+++ ./gdb/posix-hdep.c	2019-03-03 17:34:32.870539500 +0200
@@ -30,3 +30,11 @@ gdb_select (int n, fd_set *readfds, fd_s
 {
   return select (n, readfds, writefds, exceptfds, timeout);
 }
+
+/* Host-dependent console fputs method.  POSIX platforms always return
+   zero, to use the default C 'fputs'.  */
+int
+gdb_console_fputs (const char *buf, FILE *f)
+{
+  return 0;
+}
--- ./gdb/ui-file.c~1	2019-02-27 06:51:50.000000000 +0200
+++ ./gdb/ui-file.c	2019-03-03 17:32:02.526701100 +0200
@@ -236,6 +236,12 @@ stdio_file::write_async_safe (const char
 void
 stdio_file::puts (const char *linebuffer)
 {
+  /* This host-dependent function (with implementations in
+     posix-hdep.c and mingw-hdep.c) is given the opportunity to
+     process the output first in host-dependent way.  If it does, it
+     should return non-zero, to avoid calling fputs below.  */
+  if (gdb_console_fputs (linebuffer, m_file))
+    return;
   /* Calling error crashes when we are called from the exception framework.  */
   if (fputs (linebuffer, m_file))
     {
--- ./gdb/ui-file.h~1	2019-02-27 06:51:50.000000000 +0200
+++ ./gdb/ui-file.h	2019-03-03 17:40:00.605222700 +0200
@@ -101,6 +101,8 @@
 
 extern long ui_file_read (struct ui_file *file, char *buf, long length_buf);
 
+extern int gdb_console_fputs (const char *, FILE *);
+
 /* A std::string-based ui_file.  Can be used as a scratch buffer for
    collecting output.  */
 
--- ./gdb/utils.c~1	2019-02-27 06:51:50.000000000 +0200
+++ ./gdb/utils.c	2019-03-03 17:39:30.222675900 +0200
@@ -1456,8 +1456,18 @@ can_emit_style_escape (struct ui_file *s
       || !ui_file_isatty (stream))
     return false;
   const char *term = getenv ("TERM");
+  /* Windows doesn't by default define $TERM, but can support styles
+     regardless.  */
+#ifndef _WIN32
   if (term == nullptr || !strcmp (term, "dumb"))
     return false;
+#else
+  /* But if they do define $TERM, let us behave the same as on Posix
+     platforms, for the benefit of programs which invoke GDB as their
+     back-end.  */
+  if (term && !strcmp (term, "dumb"))
+    return false;
+#endif
   return true;
 }
Eli Zaretskii March 3, 2019, 3:52 p.m. | #18
A few minor comments and questions related to this feature:

 . What should be the behavior wrt styling when GDB is used as a
   back-end debugging engine?  E.g., Emacs can act as the GUI
   front-end to GDB -- should styling be turned off in that case?  On
   Windows, it will be urned off, because Emacs communicates with GDB
   via pipes, but on Posix hosts Emacs uses pty's, so GDB will think
   its stdout is a tty device, right?

 . I'm surprised there's no CLI support for requesting the
   "inverse-video" style, although the infrastructure does support the
   corresponding escape sequences.  Is that on purpose?

 . The default foreground color used for addresses, blue, is too dark
   when the terminal's background is black.  Maybe we should change to
   magenta, which should look OK on both dark and light backgrounds.

And one more issue, only tangentially related: if I put on my
~/.gdbinit settings that customize style, older versions of GDB
complain when they start up, because they don't know about styles.  Is
there any way of conditioning scripting commands on the GDB version,
or some other way of avoiding such problems?

Thanks.
Matt Rice March 3, 2019, 4:16 p.m. | #19
On Sun, Mar 3, 2019 at 7:53 AM Eli Zaretskii <eliz@gnu.org> wrote:
>

> A few minor comments and questions related to this feature:

>

> And one more issue, only tangentially related: if I put on my

> ~/.gdbinit settings that customize style, older versions of GDB

> complain when they start up, because they don't know about styles.  Is

> there any way of conditioning scripting commands on the GDB version,

> or some other way of avoiding such problems?


I didn't see a easy way to get the gdb version from python,
except parsing the output of "show version", perhaps I missed it.

but I have added stuff conditional on the inferior binary name before..
py
def on_bin_echo(): gdb.execute("set arg -d")
exec_funcs = {"/bin/echo" : on_bin_echo}
map(lambda x: exec_funcs[x.filename]() if
exec_funcs.has_key(x.filename) else None, gdb.objfiles())
end

You should be able to do the same by parsing the output of 'show version',
However, given that, I think it'd be better in this case to just try
and use the gdb.parameter API from python to set the setting,
and handling any exception which should be thrown for old gdb without
the appropriate parameter.

If the gdb in question python or guile, I really have no idea, but i
hope that helps.
Eli Zaretskii March 3, 2019, 5:13 p.m. | #20
> From: Matt Rice <ratmice@gmail.com>

> Date: Sun, 3 Mar 2019 08:16:27 -0800

> Cc: Tom Tromey <tom@tromey.com>, 

> 	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>

> 

> I didn't see a easy way to get the gdb version from python,

> except parsing the output of "show version", perhaps I missed it.

> 

> but I have added stuff conditional on the inferior binary name before..

> py

> def on_bin_echo(): gdb.execute("set arg -d")

> exec_funcs = {"/bin/echo" : on_bin_echo}

> map(lambda x: exec_funcs[x.filename]() if

> exec_funcs.has_key(x.filename) else None, gdb.objfiles())

> end

> 

> You should be able to do the same by parsing the output of 'show version',

> However, given that, I think it'd be better in this case to just try

> and use the gdb.parameter API from python to set the setting,

> and handling any exception which should be thrown for old gdb without

> the appropriate parameter.


Thanks.  I hoped for a way that would avoid using Python or Guile, but
I guess you are saying there is no such way, is that right?

Maybe we should add a convenience variable $_gdb_version ?
Matt Rice March 3, 2019, 6:04 p.m. | #21
On Sun, Mar 3, 2019 at 9:13 AM Eli Zaretskii <eliz@gnu.org> wrote:
>

> > From: Matt Rice <ratmice@gmail.com>

> > Date: Sun, 3 Mar 2019 08:16:27 -0800

> > Cc: Tom Tromey <tom@tromey.com>,

> >       "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>

> >

> > I didn't see a easy way to get the gdb version from python,

> > except parsing the output of "show version", perhaps I missed it.

> >

> > but I have added stuff conditional on the inferior binary name before..

> > py

> > def on_bin_echo(): gdb.execute("set arg -d")

> > exec_funcs = {"/bin/echo" : on_bin_echo}

> > map(lambda x: exec_funcs[x.filename]() if

> > exec_funcs.has_key(x.filename) else None, gdb.objfiles())

> > end

> >

> > You should be able to do the same by parsing the output of 'show version',

> > However, given that, I think it'd be better in this case to just try

> > and use the gdb.parameter API from python to set the setting,

> > and handling any exception which should be thrown for old gdb without

> > the appropriate parameter.

>

> Thanks.  I hoped for a way that would avoid using Python or Guile, but

> I guess you are saying there is no such way, is that right?

>

> Maybe we should add a convenience variable $_gdb_version ?


I'm not going to say there is no way, I just don't know of one which
is robust/portable/palatable.
If you can use the gdb shell command , and get the parent PID/path to
the gdb from there, then write out a gdb script
after that source the newly generated script

So, perhaps I'm missing some obvious/nice way, but i don't really know, sorry.
Tom Tromey March 4, 2019, 3:01 p.m. | #22
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


Eli>  . What should be the behavior wrt styling when GDB is used as a
Eli>    back-end debugging engine?  E.g., Emacs can act as the GUI
Eli>    front-end to GDB -- should styling be turned off in that case?  On
Eli>    Windows, it will be urned off, because Emacs communicates with GDB
Eli>    via pipes, but on Posix hosts Emacs uses pty's, so GDB will think
Eli>    its stdout is a tty device, right?

Yes, it will.  I think for cases like gud-gdb (where the ordinary CLI is
used) that it makes sense for Emacs to use gdb's styling.  For MI, I
think there is no styling, and Emacs can do what it likes.

Eli>  . I'm surprised there's no CLI support for requesting the
Eli>    "inverse-video" style, although the infrastructure does support the
Eli>    corresponding escape sequences.  Is that on purpose?

It just didn't seem super important, but I'm not opposed if you want to
add it.

It would also be possible to add support for 24 bit color, but I didn't
do that either.

Eli>  . The default foreground color used for addresses, blue, is too dark
Eli>    when the terminal's background is black.  Maybe we should change to
Eli>    magenta, which should look OK on both dark and light backgrounds.

For me magenta is even darker.  Setting the intensity to bold might be
more acceptable?

Eli> And one more issue, only tangentially related: if I put on my
Eli> ~/.gdbinit settings that customize style, older versions of GDB
Eli> complain when they start up, because they don't know about styles.  Is
Eli> there any way of conditioning scripting commands on the GDB version,
Eli> or some other way of avoiding such problems?

Nothing built in, this is a longstanding issue when adding new settings
to gdb.

Tom
Tom Tromey March 4, 2019, 3:08 p.m. | #23
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


Eli> Here's a working prototype I'm currently testing.  If this
Eli> implementation strategy is acceptable, I will post a proper patch
Eli> soon.

It looks reasonable to me.

Eli> +	  if (fg.is_none ())
Eli> +	    fgcolor = norm_attr & 15;
Eli> +	  else
Eli> +	    fgcolor = fg_color[fg.get_value () & 15];

get_value is only defined for basic colors; you'll get an assertion here
if the color is not basic.  For non-basic colors, you can get the rgb
values for the color instead via the get_rgb method.

Tom
Eli Zaretskii March 4, 2019, 3:56 p.m. | #24
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Mon, 04 Mar 2019 08:08:47 -0700

> 

> Eli> +	  if (fg.is_none ())

> Eli> +	    fgcolor = norm_attr & 15;

> Eli> +	  else

> Eli> +	    fgcolor = fg_color[fg.get_value () & 15];

> 

> get_value is only defined for basic colors; you'll get an assertion here

> if the color is not basic.  For non-basic colors, you can get the rgb

> values for the color instead via the get_rgb method.


I've seen the assertion in the code, but it wasn't clear to me when/if
I could get non-basic colors on MS-Windows.  Could you describe a
situation where that could happen?

Thanks.
Eli Zaretskii March 4, 2019, 4:04 p.m. | #25
> Date: Fri, 01 Mar 2019 09:47:23 +0200

> From: Eli Zaretskii <eliz@gnu.org>

> CC: gdb-patches@sourceware.org

> 

> > Eli> Will the Windows TUI build support styling out of the box?  It uses

> > Eli> ncurses.

> > 

> > I think it should work, but you'd have to "set style enabled on" first.

> 

> For the record, it doesn't work in GDB 8.2.90, even after I disabled

> the test for $TERM being defined in the environment.  I will try to

> find out why at some point.


OK, I now know why.  The colors do work, but only if the style
specifies both the foreground and the background colors explicitly;
specifying just one of them causes no colors at all (but other
attributes, like intensity, do still work).

I think this means that the MS-Windows console belongs to the devices
which don't support resetting to the default colors.  I even tried to
add the call to assume_default_colors, per the ncurses docs, to no
avail.

Are there any other consoles out there with such an issue?

In any case, I think we should at least tell in the manual that on
some systems the color pairs need to be specified completely, even if
only one of the two colors changes from the default.  Does anyone have
ideas how to do better than just documenting this?  Let the user
define the default fore- and back-ground colors, perhaps?
Tom Tromey March 4, 2019, 4:16 p.m. | #26
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


Eli> I've seen the assertion in the code, but it wasn't clear to me when/if
Eli> I could get non-basic colors on MS-Windows.  Could you describe a
Eli> situation where that could happen?

The user can do it from the command line:

(gdb) printf "\e[38;2;255;100;0mTRUECOLOR\e[0m\n"

Maybe it can be done via GNU Source Highlight as well, though I am not
totally sure.

Tom
Eli Zaretskii March 4, 2019, 5:37 p.m. | #27
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Mon, 04 Mar 2019 08:01:49 -0700

> 

> Eli>  . The default foreground color used for addresses, blue, is too dark

> Eli>    when the terminal's background is black.  Maybe we should change to

> Eli>    magenta, which should look OK on both dark and light backgrounds.

> 

> For me magenta is even darker.  Setting the intensity to bold might be

> more acceptable?


Still too dark.  I guess we will need to leave this to personal
customizations.

> Eli> And one more issue, only tangentially related: if I put on my

> Eli> ~/.gdbinit settings that customize style, older versions of GDB

> Eli> complain when they start up, because they don't know about styles.  Is

> Eli> there any way of conditioning scripting commands on the GDB version,

> Eli> or some other way of avoiding such problems?

> 

> Nothing built in, this is a longstanding issue when adding new settings

> to gdb.


What do you think about the idea to add a convenience variable that
would provide the GDB version?
Tom Tromey March 4, 2019, 5:40 p.m. | #28
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


>> Nothing built in, this is a longstanding issue when adding new settings

>> to gdb.


Eli> What do you think about the idea to add a convenience variable that
Eli> would provide the GDB version?

Seems reasonable to me.

Something I've done in the past is have an "ignore-errors" command in
Python so I can just "ignore-errors set some-new-thing ...".

Tom
Eli Zaretskii March 5, 2019, 3:38 p.m. | #29
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Mon, 04 Mar 2019 09:16:25 -0700

> 

> (gdb) printf "\e[38;2;255;100;0mTRUECOLOR\e[0m\n"


Thanks, I used this example (and its variations) to extend the support
to any kind of color.
Eli Zaretskii March 6, 2019, 4:02 p.m. | #30
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Mon, 04 Mar 2019 10:40:46 -0700

> 

> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

> 

> Eli> What do you think about the idea to add a convenience variable that

> Eli> would provide the GDB version?

> 

> Seems reasonable to me.


How about the patch below?  Is it okay to go in?  (Note that I took
this opportunity to clean up whitespace in top.c, I hope it's OK to do
that as part of unrelated code changes.)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ac61e65..f2915d0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-03-06  Eli Zaretskii  <eliz@gnu.org>
+
+	* NEWS: Announce $_gdb_version.
+
+	* top.c (init_gdb_version_var): New function.
+	(gdb_init): Call init_gdb_version_var.
+
 2019-03-06  Tom Tromey  <tromey@adacore.com>
 
 	* remote-sim.c (gdbsim_target_open): Use result of
diff --git a/gdb/NEWS b/gdb/NEWS
index cc7c35c..260e6cc 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,12 @@
 
 *** Changes since GDB 8.3
 
+* New built-in convenience variable $_gdb_version provides the GDB
+  version.  It is handy for conditionally using features available
+  only in or since specific GDB versions, in scripts that should work
+  error-free with many different versions, such as in system-wide init
+  files.
+
 *** Changes in GDB 8.3
 
 * GDB and GDBserver now support access to additional registers on
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 0380322..313a061 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2019-03-06  Eli Zaretskii  <eliz@gnu.org>
+
+	* gdb.texinfo (Convenience Vars): Document $_gdb_version.
+
 2019-03-05  Simon Marchi  <simon.marchi@efficios.com>
 
 	* python.texi (Values From Inferior): Change synopsys of the
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f2028f8..9d15337 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -11197,7 +11197,7 @@
 @vindex $_tlb@r{, convenience variable}
 The variable @code{$_tlb} is automatically set when debugging
 applications running on MS-Windows in native mode or connected to
-gdbserver that supports the @code{qGetTIBAddr} request. 
+gdbserver that supports the @code{qGetTIBAddr} request.
 @xref{General Query Packets}.
 This variable contains the address of the thread information block.
 
@@ -11211,6 +11211,17 @@
 @item $_gthread
 The global number of the current thread.  @xref{global thread numbers}.
 
+@item $_gdb_version
+@vindex $_gdb_version@r{, convenience variable}
+The version of the running @value{GDBN}.  The value is an integer
+number that encodes the major and minor @value{GDBN} versions as
+@w{@code{@var{major}*100 + @var{minor}}}, so, e.g., @value{GDBN}
+version 9.10 will produce the value @code{910}.  Development snapshots
+and pretest versions have their minor version incremented by one;
+thus, @value{GDBN} pretest 9.11.90 will produce the value 912.  This
+variable allows you to write scripts that work with different versions
+of @value{GDBN} without errors caused by features unavailable in some
+of those versions.
 @end table
 
 @node Convenience Funs
diff --git a/gdb/top.c b/gdb/top.c
index 22e6f7e..97b349a 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -147,22 +147,22 @@ int server_command;
 
 /* Timeout limit for response from target.  */
 
-/* The default value has been changed many times over the years.  It 
-   was originally 5 seconds.  But that was thought to be a long time 
+/* The default value has been changed many times over the years.  It
+   was originally 5 seconds.  But that was thought to be a long time
    to sit and wait, so it was changed to 2 seconds.  That was thought
-   to be plenty unless the connection was going through some terminal 
+   to be plenty unless the connection was going through some terminal
    server or multiplexer or other form of hairy serial connection.
 
-   In mid-1996, remote_timeout was moved from remote.c to top.c and 
+   In mid-1996, remote_timeout was moved from remote.c to top.c and
    it began being used in other remote-* targets.  It appears that the
    default was changed to 20 seconds at that time, perhaps because the
    Renesas E7000 ICE didn't always respond in a timely manner.
 
    But if 5 seconds is a long time to sit and wait for retransmissions,
-   20 seconds is far worse.  This demonstrates the difficulty of using 
+   20 seconds is far worse.  This demonstrates the difficulty of using
    a single variable for all protocol timeouts.
 
-   As remote.c is used much more than remote-e7000.c, it was changed 
+   As remote.c is used much more than remote-e7000.c, it was changed
    back to 2 seconds in 1999.  */
 
 int remote_timeout = 2;
@@ -188,9 +188,9 @@ int (*deprecated_ui_loop_hook) (int);
 
 /* Called from print_frame_info to list the line we stopped in.  */
 
-void (*deprecated_print_frame_info_listing_hook) (struct symtab * s, 
+void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 						  int line,
-						  int stopline, 
+						  int stopline,
 						  int noerror);
 /* Replaces most of query.  */
 
@@ -237,7 +237,7 @@ ptid_t (*deprecated_target_wait_hook) (ptid_t ptid,
 /* Used by UI as a wrapper around command execution.  May do various
    things like enabling/disabling buttons, etc...  */
 
-void (*deprecated_call_command_hook) (struct cmd_list_element * c, 
+void (*deprecated_call_command_hook) (struct cmd_list_element * c,
 				      const char *cmd, int from_tty);
 
 /* Called when the current thread changes.  Argument is thread id.  */
@@ -1339,8 +1339,9 @@ There is NO WARRANTY, to the extent permitted by law.");
 resources online at:\n    <http://www.gnu.org/software/gdb/documentation/>."));
   fprintf_filtered (stream, "\n\n");
   fprintf_filtered (stream, _("For help, type \"help\".\n"));
-  fprintf_filtered (stream, _("Type \"apropos word\" to search for \
-commands related to \"word\"."));
+  fprintf_filtered (stream,
+		    _("Type \"apropos word\" to search for commands \
+related to \"word\"."));
 }
 
 /* Print the details of GDB build-time configuration.  */
@@ -1608,7 +1609,7 @@ quit_force (int *exit_arg, int from_tty)
 
   undo_terminal_modifications_before_exit ();
 
-  /* An optional expression may be used to cause gdb to terminate with the 
+  /* An optional expression may be used to cause gdb to terminate with the
      value of that expression.  */
   if (exit_arg)
     exit_code = *exit_arg;
@@ -1998,11 +1999,21 @@ set_history_filename (const char *args,
      directories the file written will be the same as the one
      that was read.  */
   if (!IS_ABSOLUTE_PATH (history_filename))
-    history_filename = reconcat (history_filename, current_directory, "/", 
+    history_filename = reconcat (history_filename, current_directory, "/",
 				 history_filename, (char *) NULL);
 }
 
 static void
+init_gdb_version_var (void)
+{
+  struct internalvar *version_var = create_internalvar ("_gdb_version");
+  int vmajor = 0, vminor = 0, vrevision = 0;
+  sscanf (version, "%d.%d.%d", &vmajor, &vminor, &vrevision);
+  set_internalvar_integer (version_var,
+			   vmajor * 100 + vminor + (vrevision > 0));
+}
+
+static void
 init_main (void)
 {
   struct cmd_list_element *c;
@@ -2206,4 +2217,7 @@ gdb_init (char *argv0)
      prefix to be installed.  Keep things simple and just do final
      script initialization here.  */
   finish_ext_lang_initialization ();
+
+  /* Create $_gdb_version convenience variable.  */
+  init_gdb_version_var ();
 }
Joel Brobecker March 7, 2019, 6:02 a.m. | #31
> > Eli>  . The default foreground color used for addresses, blue, is too dark

> > Eli>    when the terminal's background is black.  Maybe we should change to

> > Eli>    magenta, which should look OK on both dark and light backgrounds.

> > 

> > For me magenta is even darker.  Setting the intensity to bold might be

> > more acceptable?

> 

> Still too dark.  I guess we will need to leave this to personal

> customizations.


My idea was to mimick what vim does: It actually has two sets of
settings, one for dark terminals, and one for bright ones. Each set
uses similar colors by default, but adapted for the brightness
of the terminal.

For more info, check out vim's help for "set background=[dark|light]".

-- 
Joel
Eli Zaretskii March 7, 2019, 2:52 p.m. | #32
> Date: Thu, 7 Mar 2019 10:02:24 +0400

> From: Joel Brobecker <brobecker@adacore.com>

> Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org

> 

> > > For me magenta is even darker.  Setting the intensity to bold might be

> > > more acceptable?

> > 

> > Still too dark.  I guess we will need to leave this to personal

> > customizations.

> 

> My idea was to mimick what vim does: It actually has two sets of

> settings, one for dark terminals, and one for bright ones. Each set

> uses similar colors by default, but adapted for the brightness

> of the terminal.


Yes, Emacs does something similar.  But experience taught me that
automatically deducing whether the background is dark or light is a
Pandora box, so I won't suggest this for GDB, where this is after all
just a minor issue (unlike in editors).

But if you meant something like "set style background light" which
will change all the default colors, then I'm with you.  We just need
to come up with agreed-upon sets of colors.
Joel Brobecker March 8, 2019, 5:40 a.m. | #33
> Yes, Emacs does something similar.  But experience taught me that

> automatically deducing whether the background is dark or light is a

> Pandora box, so I won't suggest this for GDB, where this is after all

> just a minor issue (unlike in editors).


Agreed.

> But if you meant something like "set style background light" which

> will change all the default colors, then I'm with you.  We just need

> to come up with agreed-upon sets of colors.


Sounds like we're on the same page. I would go with the same colors
as right now, except for the few were it's hard to read; e.g. change
the dark blue for a cyan, for instance.

-- 
Joel
Eli Zaretskii March 8, 2019, 2:55 p.m. | #34
> Date: Tue, 05 Mar 2019 17:38:32 +0200

> From: Eli Zaretskii <eliz@gnu.org>

> CC: gdb-patches@sourceware.org

> 

> > From: Tom Tromey <tom@tromey.com>

> > Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> > Date: Mon, 04 Mar 2019 09:16:25 -0700

> > 

> > (gdb) printf "\e[38;2;255;100;0mTRUECOLOR\e[0m\n"

> 

> Thanks, I used this example (and its variations) to extend the support

> to any kind of color.


Here's the patch I propose to enable styling on native MS-Windows
console.

OK to commit, master and the release branch?

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index de5e74a..bb10c7d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,21 @@
+2019-03-08  Eli Zaretskii  <eliz@gnu.org>
+
+	Support styling on native MS-Windows console
+
+	PR/24315
+	* utils.c (can_emit_style_escape) [_WIN32]: Don't disable styling
+	on MS-Windows if $TERM is not defined.
+
+	* cli/cli-style.c: Set cli_styling to 1 in the MinGW build.
+
+	* posix-hdep.c (gdb_console_fputs):
+	* mingw-hdep.c (rgb_to_16colors, gdb_console_fputs): New
+	functions.
+	* ui-file.h (gdb_console_fputs): Add prototype.
+
+	* ui-file.c (stdio_file::puts): Call gdb_console_fputs, and fall
+	back to fputs only if the former returns zero.
+
 2019-03-07  Tom Tromey  <tom@tromey.com>
 
 	* symmisc.c (print_symbol_bcache_statistics): Update.
diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
index 234cc41..ec385da 100644
--- a/gdb/cli/cli-style.c
+++ b/gdb/cli/cli-style.c
@@ -24,7 +24,7 @@
 
 /* True if styling is enabled.  */
 
-#if defined(_WIN32) || defined (__CYGWIN__)
+#if defined (__MSDOS__) || defined (__CYGWIN__)
 int cli_styling = 0;
 #else
 int cli_styling = 1;
diff --git a/gdb/mingw-hdep.c b/gdb/mingw-hdep.c
index 1a4b93b..8ed4b44 100644
--- a/gdb/mingw-hdep.c
+++ b/gdb/mingw-hdep.c
@@ -177,3 +177,195 @@ gdb_select (int n, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 
   return num_ready;
 }
+
+/* Map COLOR's RGB triplet, with 8 bits per component, into 16 Windows
+   console colors, where each component has just 1 bit, plus a single
+   intensity bit which affects all 3 components.  */
+static int
+rgb_to_16colors (const ui_file_style::color &color)
+{
+  uint8_t rgb[3];
+  color.get_rgb (rgb);
+
+  int retval = 0;
+  for (int i = 0; i < 3; i++)
+    {
+      /* Subdivide 256 possible values of each RGB component into 3
+	 regions: no color, normal color, bright color.  256 / 3 = 85,
+	 but ui-style.c follows xterm and uses 92 for R and G
+	 components of the bright-blue color, so we bias the divisor a
+	 bit to have the bright colors between 9 and 15 identical to
+	 what ui-style.c expects.  */
+      int bits = rgb[i] / 93;
+      retval |= ((bits > 0) << (2 - i)) | ((bits > 1) << 3);
+    }
+
+  return retval;
+}
+
+/* Zero if not yet initialized, 1 if stdout is a console device, else -1.  */
+static int mingw_console_initialized;
+
+/* Handle to stdout . */
+static HANDLE hstdout = INVALID_HANDLE_VALUE;
+
+/* Text attribute to use for normal text (the "none" pseudo-color).  */
+static SHORT  norm_attr;
+
+/* The most recently applied style.  */
+static ui_file_style last_style;
+
+/* Alternative for the libc 'fputs' which handles embedded SGR
+   sequences in support of styling.  */
+
+int
+gdb_console_fputs (const char *linebuf, FILE *fstream)
+{
+  if (!mingw_console_initialized)
+    {
+      hstdout = (HANDLE)_get_osfhandle (fileno (fstream));
+      DWORD cmode;
+      CONSOLE_SCREEN_BUFFER_INFO csbi;
+
+      if (hstdout != INVALID_HANDLE_VALUE
+	  && GetConsoleMode (hstdout, &cmode) != 0
+	  && GetConsoleScreenBufferInfo (hstdout, &csbi))
+	{
+	  norm_attr = csbi.wAttributes;
+	  mingw_console_initialized = 1;
+	}
+      else if (hstdout != INVALID_HANDLE_VALUE)
+	mingw_console_initialized = -1; /* valid, but not a console device */
+    }
+  /* If our stdout is not a console device, let the default 'fputs'
+     handle the task. */
+  if (mingw_console_initialized <= 0)
+    return 0;
+
+  /* Mapping between 8 ANSI colors and Windows console attributes.  */
+  static 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 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 */
+  };
+
+  ui_file_style style = last_style;
+  unsigned char c;
+  size_t n_read;
+
+  for ( ; (c = *linebuf) != 0; linebuf += n_read)
+    {
+      if (c == '\033')
+	{
+	  fflush (fstream);
+	  bool parsed = style.parse (linebuf, &n_read);
+	  if (n_read <= 0)	/* should never happen */
+	    n_read = 1;
+	  if (!parsed)
+	    {
+	      /* This means we silently swallow SGR sequences we
+		 cannot parse.  */
+	      continue;
+	    }
+	  /* Colors.  */
+	  const ui_file_style::color &fg = style.get_foreground ();
+	  const ui_file_style::color &bg = style.get_background ();
+	  int fgcolor, bgcolor, bright, inverse;
+	  if (fg.is_none ())
+	    fgcolor = norm_attr & 15;
+	  else if (fg.is_basic ())
+	    fgcolor = fg_color[fg.get_value () & 15];
+	  else
+	    fgcolor = rgb_to_16colors (fg);
+	  if (bg.is_none ())
+	    bgcolor = norm_attr & (15 << 4);
+	  else if (bg.is_basic ())
+	    bgcolor = bg_color[bg.get_value () & 15];
+	  else
+	    bgcolor = rgb_to_16colors (bg) << 4;
+
+	  /* Intensity.  */
+	  switch (style.get_intensity ())
+	    {
+	    case ui_file_style::NORMAL:
+	    case ui_file_style::DIM:
+	      bright = 0;
+	      break;
+	    case ui_file_style::BOLD:
+	      bright = 1;
+	      break;
+	    default:
+	      gdb_assert_not_reached ("invalid intensity");
+	    }
+
+	  /* Inverse video.  */
+	  if (style.is_reverse ())
+	    inverse = 1;
+	  else
+	    inverse = 0;
+
+	  /* Construct the attribute.  */
+	  if (inverse)
+	    {
+	      int t = fgcolor;
+	      fgcolor = (bgcolor >> 4);
+	      bgcolor = (t << 4);
+	    }
+	  if (bright)
+	    fgcolor |= FOREGROUND_INTENSITY;
+
+	  SHORT attr = (bgcolor & (15 << 4)) | (fgcolor & 15);
+
+	  /* Apply the attribute.  */
+	  SetConsoleTextAttribute (hstdout, attr);
+	}
+      else
+	{
+	  /* When we are about to write newline, we need to clear to
+	     EOL with the normal attribute, to avoid spilling the
+	     colors to the next screen line.  We assume here that no
+	     non-default attribute extends beyond the newline.  */
+	  if (c == '\n')
+	    {
+	      DWORD nchars;
+	      COORD start_pos;
+	      DWORD written;
+	      CONSOLE_SCREEN_BUFFER_INFO csbi;
+
+	      fflush (fstream);
+	      GetConsoleScreenBufferInfo (hstdout, &csbi);
+
+	      if (csbi.wAttributes != norm_attr)
+		{
+		  start_pos = csbi.dwCursorPosition;
+		  nchars = csbi.dwSize.X - start_pos.X;
+
+		  FillConsoleOutputAttribute (hstdout, norm_attr, nchars,
+					      start_pos, &written);
+		  FillConsoleOutputCharacter (hstdout, ' ', nchars,
+					      start_pos, &written);
+		}
+	    }
+	  fputc (c, fstream);
+	  n_read = 1;
+	}
+    }
+
+  last_style = style;
+  return 1;
+}
diff --git a/gdb/posix-hdep.c b/gdb/posix-hdep.c
index 7911bc5..d0f467e 100644
--- a/gdb/posix-hdep.c
+++ b/gdb/posix-hdep.c
@@ -30,3 +30,11 @@ gdb_select (int n, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 {
   return select (n, readfds, writefds, exceptfds, timeout);
 }
+
+/* Host-dependent console fputs method.  POSIX platforms always return
+   zero, to use the default C 'fputs'.  */
+int
+gdb_console_fputs (const char *buf, FILE *f)
+{
+  return 0;
+}
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 690fc62..77f6b31 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -236,6 +236,12 @@ stdio_file::write_async_safe (const char *buf, long length_buf)
 void
 stdio_file::puts (const char *linebuffer)
 {
+  /* This host-dependent function (with implementations in
+     posix-hdep.c and mingw-hdep.c) is given the opportunity to
+     process the output first in host-dependent way.  If it does, it
+     should return non-zero, to avoid calling fputs below.  */
+  if (gdb_console_fputs (linebuffer, m_file))
+    return;
   /* Calling error crashes when we are called from the exception framework.  */
   if (fputs (linebuffer, m_file))
     {
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 26cf2bf..6e6ca1c 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -101,6 +101,8 @@ extern void ui_file_write_async_safe (struct ui_file *file, const char *buf,
 
 extern long ui_file_read (struct ui_file *file, char *buf, long length_buf);
 
+extern int gdb_console_fputs (const char *, FILE *);
+
 /* A std::string-based ui_file.  Can be used as a scratch buffer for
    collecting output.  */
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 258614a..840779a 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1445,8 +1445,18 @@ can_emit_style_escape (struct ui_file *stream)
       || !ui_file_isatty (stream))
     return false;
   const char *term = getenv ("TERM");
+  /* Windows doesn't by default define $TERM, but can support styles
+     regardless.  */
+#ifndef _WIN32
   if (term == nullptr || !strcmp (term, "dumb"))
     return false;
+#else
+  /* But if they do define $TERM, let us behave the same as on Posix
+     platforms, for the benefit of programs which invoke GDB as their
+     back-end.  */
+  if (term && !strcmp (term, "dumb"))
+    return false;
+#endif
   return true;
 }
Simon Marchi via Gdb-patches March 8, 2019, 4:14 p.m. | #35
Am Freitag, 8. März 2019, 15:55:50 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:

> > Date: Tue, 05 Mar 2019 17:38:32 +0200

> > From: Eli Zaretskii <eliz@gnu.org>

> > CC: gdb-patches@sourceware.org

> >

> > > From: Tom Tromey <tom@tromey.com>

> > > Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> > > Date: Mon, 04 Mar 2019 09:16:25 -0700

> > >

> > > (gdb) printf "\e[38;2;255;100;0mTRUECOLOR\e[0m\n"

> >

> > Thanks, I used this example (and its variations) to extend the support

> > to any kind of color.

> 

> Here's the patch I propose to enable styling on native MS-Windows

> console.

> 

> OK to commit, master and the release branch?


I just tried this patch out, and it's working great for me.
I really like this new syntax highlighting.


@Tom, you mentioned some time ago that you had some patches
for source-highlight itself, are they available somewhere?


Regards
Hannes Domani
Tom Tromey March 8, 2019, 9:08 p.m. | #36
>>>>> "Hannes" == Hannes Domani via gdb-patches <gdb-patches@sourceware.org> writes:


Hannes> @Tom, you mentioned some time ago that you had some patches
Hannes> for source-highlight itself, are they available somewhere?

I sent them to the source highlight mailing list.  The main one was to
add Rust highlighting, but I also have some here (can't recall if I sent
them) that update the highlighters for C and C++ to handle newer
versions.

I have to send a follow-up email to the source highlight maintainer, so
I can get permissions to land the patches and send out a new release.
I'll try to get to that soon.

Tom
Tom Tromey March 8, 2019, 9:11 p.m. | #37
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


Eli> Here's the patch I propose to enable styling on native MS-Windows
Eli> console.

Eli> OK to commit, master and the release branch?

Looks reasonable to me.
Thank you for doing this.

Tom
Eli Zaretskii March 9, 2019, 6:49 a.m. | #38
> From: Tom Tromey <tom@tromey.com>

> Cc: tom@tromey.com,  gdb-patches@sourceware.org

> Date: Fri, 08 Mar 2019 14:11:01 -0700

> 

> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

> 

> Eli> Here's the patch I propose to enable styling on native MS-Windows

> Eli> console.

> 

> Eli> OK to commit, master and the release branch?

> 

> Looks reasonable to me.

> Thank you for doing this.


Thanks for the review, I've pushed this now to both branches.
Eli Zaretskii March 20, 2019, 7:35 p.m. | #39
Ping!

> Date: Wed, 06 Mar 2019 18:02:00 +0200

> From: Eli Zaretskii <eliz@gnu.org>

> CC: gdb-patches@sourceware.org

> 

> > From: Tom Tromey <tom@tromey.com>

> > Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> > Date: Mon, 04 Mar 2019 10:40:46 -0700

> > 

> > >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

> > 

> > Eli> What do you think about the idea to add a convenience variable that

> > Eli> would provide the GDB version?

> > 

> > Seems reasonable to me.

> 

> How about the patch below?  Is it okay to go in?  (Note that I took

> this opportunity to clean up whitespace in top.c, I hope it's OK to do

> that as part of unrelated code changes.)

> 

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog

> index ac61e65..f2915d0 100644

> --- a/gdb/ChangeLog

> +++ b/gdb/ChangeLog

> @@ -1,3 +1,10 @@

> +2019-03-06  Eli Zaretskii  <eliz@gnu.org>

> +

> +	* NEWS: Announce $_gdb_version.

> +

> +	* top.c (init_gdb_version_var): New function.

> +	(gdb_init): Call init_gdb_version_var.

> +

>  2019-03-06  Tom Tromey  <tromey@adacore.com>

>  

>  	* remote-sim.c (gdbsim_target_open): Use result of

> diff --git a/gdb/NEWS b/gdb/NEWS

> index cc7c35c..260e6cc 100644

> --- a/gdb/NEWS

> +++ b/gdb/NEWS

> @@ -3,6 +3,12 @@

>  

>  *** Changes since GDB 8.3

>  

> +* New built-in convenience variable $_gdb_version provides the GDB

> +  version.  It is handy for conditionally using features available

> +  only in or since specific GDB versions, in scripts that should work

> +  error-free with many different versions, such as in system-wide init

> +  files.

> +

>  *** Changes in GDB 8.3

>  

>  * GDB and GDBserver now support access to additional registers on

> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog

> index 0380322..313a061 100644

> --- a/gdb/doc/ChangeLog

> +++ b/gdb/doc/ChangeLog

> @@ -1,3 +1,7 @@

> +2019-03-06  Eli Zaretskii  <eliz@gnu.org>

> +

> +	* gdb.texinfo (Convenience Vars): Document $_gdb_version.

> +

>  2019-03-05  Simon Marchi  <simon.marchi@efficios.com>

>  

>  	* python.texi (Values From Inferior): Change synopsys of the

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo

> index f2028f8..9d15337 100644

> --- a/gdb/doc/gdb.texinfo

> +++ b/gdb/doc/gdb.texinfo

> @@ -11197,7 +11197,7 @@

>  @vindex $_tlb@r{, convenience variable}

>  The variable @code{$_tlb} is automatically set when debugging

>  applications running on MS-Windows in native mode or connected to

> -gdbserver that supports the @code{qGetTIBAddr} request. 

> +gdbserver that supports the @code{qGetTIBAddr} request.

>  @xref{General Query Packets}.

>  This variable contains the address of the thread information block.

>  

> @@ -11211,6 +11211,17 @@

>  @item $_gthread

>  The global number of the current thread.  @xref{global thread numbers}.

>  

> +@item $_gdb_version

> +@vindex $_gdb_version@r{, convenience variable}

> +The version of the running @value{GDBN}.  The value is an integer

> +number that encodes the major and minor @value{GDBN} versions as

> +@w{@code{@var{major}*100 + @var{minor}}}, so, e.g., @value{GDBN}

> +version 9.10 will produce the value @code{910}.  Development snapshots

> +and pretest versions have their minor version incremented by one;

> +thus, @value{GDBN} pretest 9.11.90 will produce the value 912.  This

> +variable allows you to write scripts that work with different versions

> +of @value{GDBN} without errors caused by features unavailable in some

> +of those versions.

>  @end table

>  

>  @node Convenience Funs

> diff --git a/gdb/top.c b/gdb/top.c

> index 22e6f7e..97b349a 100644

> --- a/gdb/top.c

> +++ b/gdb/top.c

> @@ -147,22 +147,22 @@ int server_command;

>  

>  /* Timeout limit for response from target.  */

>  

> -/* The default value has been changed many times over the years.  It 

> -   was originally 5 seconds.  But that was thought to be a long time 

> +/* The default value has been changed many times over the years.  It

> +   was originally 5 seconds.  But that was thought to be a long time

>     to sit and wait, so it was changed to 2 seconds.  That was thought

> -   to be plenty unless the connection was going through some terminal 

> +   to be plenty unless the connection was going through some terminal

>     server or multiplexer or other form of hairy serial connection.

>  

> -   In mid-1996, remote_timeout was moved from remote.c to top.c and 

> +   In mid-1996, remote_timeout was moved from remote.c to top.c and

>     it began being used in other remote-* targets.  It appears that the

>     default was changed to 20 seconds at that time, perhaps because the

>     Renesas E7000 ICE didn't always respond in a timely manner.

>  

>     But if 5 seconds is a long time to sit and wait for retransmissions,

> -   20 seconds is far worse.  This demonstrates the difficulty of using 

> +   20 seconds is far worse.  This demonstrates the difficulty of using

>     a single variable for all protocol timeouts.

>  

> -   As remote.c is used much more than remote-e7000.c, it was changed 

> +   As remote.c is used much more than remote-e7000.c, it was changed

>     back to 2 seconds in 1999.  */

>  

>  int remote_timeout = 2;

> @@ -188,9 +188,9 @@ int (*deprecated_ui_loop_hook) (int);

>  

>  /* Called from print_frame_info to list the line we stopped in.  */

>  

> -void (*deprecated_print_frame_info_listing_hook) (struct symtab * s, 

> +void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,

>  						  int line,

> -						  int stopline, 

> +						  int stopline,

>  						  int noerror);

>  /* Replaces most of query.  */

>  

> @@ -237,7 +237,7 @@ ptid_t (*deprecated_target_wait_hook) (ptid_t ptid,

>  /* Used by UI as a wrapper around command execution.  May do various

>     things like enabling/disabling buttons, etc...  */

>  

> -void (*deprecated_call_command_hook) (struct cmd_list_element * c, 

> +void (*deprecated_call_command_hook) (struct cmd_list_element * c,

>  				      const char *cmd, int from_tty);

>  

>  /* Called when the current thread changes.  Argument is thread id.  */

> @@ -1339,8 +1339,9 @@ There is NO WARRANTY, to the extent permitted by law.");

>  resources online at:\n    <http://www.gnu.org/software/gdb/documentation/>."));

>    fprintf_filtered (stream, "\n\n");

>    fprintf_filtered (stream, _("For help, type \"help\".\n"));

> -  fprintf_filtered (stream, _("Type \"apropos word\" to search for \

> -commands related to \"word\"."));

> +  fprintf_filtered (stream,

> +		    _("Type \"apropos word\" to search for commands \

> +related to \"word\"."));

>  }

>  

>  /* Print the details of GDB build-time configuration.  */

> @@ -1608,7 +1609,7 @@ quit_force (int *exit_arg, int from_tty)

>  

>    undo_terminal_modifications_before_exit ();

>  

> -  /* An optional expression may be used to cause gdb to terminate with the 

> +  /* An optional expression may be used to cause gdb to terminate with the

>       value of that expression.  */

>    if (exit_arg)

>      exit_code = *exit_arg;

> @@ -1998,11 +1999,21 @@ set_history_filename (const char *args,

>       directories the file written will be the same as the one

>       that was read.  */

>    if (!IS_ABSOLUTE_PATH (history_filename))

> -    history_filename = reconcat (history_filename, current_directory, "/", 

> +    history_filename = reconcat (history_filename, current_directory, "/",

>  				 history_filename, (char *) NULL);

>  }

>  

>  static void

> +init_gdb_version_var (void)

> +{

> +  struct internalvar *version_var = create_internalvar ("_gdb_version");

> +  int vmajor = 0, vminor = 0, vrevision = 0;

> +  sscanf (version, "%d.%d.%d", &vmajor, &vminor, &vrevision);

> +  set_internalvar_integer (version_var,

> +			   vmajor * 100 + vminor + (vrevision > 0));

> +}

> +

> +static void

>  init_main (void)

>  {

>    struct cmd_list_element *c;

> @@ -2206,4 +2217,7 @@ gdb_init (char *argv0)

>       prefix to be installed.  Keep things simple and just do final

>       script initialization here.  */

>    finish_ext_lang_initialization ();

> +

> +  /* Create $_gdb_version convenience variable.  */

> +  init_gdb_version_var ();

>  }

>
Simon Marchi March 21, 2019, 1:55 a.m. | #40
On 2019-03-06 11:02 a.m., Eli Zaretskii wrote:
>> From: Tom Tromey <tom@tromey.com>

>> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

>> Date: Mon, 04 Mar 2019 10:40:46 -0700

>>

>>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>>

>> Eli> What do you think about the idea to add a convenience variable that

>> Eli> would provide the GDB version?

>>

>> Seems reasonable to me.

> 

> How about the patch below?  Is it okay to go in?  (Note that I took

> this opportunity to clean up whitespace in top.c, I hope it's OK to do

> that as part of unrelated code changes.)


Ah, I noticed this because you changed the subject in your ping :).  This would
be very useful, especially that we can quite easily query this from MI as well
as Python.

> 

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog

> index ac61e65..f2915d0 100644

> --- a/gdb/ChangeLog

> +++ b/gdb/ChangeLog

> @@ -1,3 +1,10 @@

> +2019-03-06  Eli Zaretskii  <eliz@gnu.org>

> +

> +	* NEWS: Announce $_gdb_version.

> +

> +	* top.c (init_gdb_version_var): New function.

> +	(gdb_init): Call init_gdb_version_var.

> +

>  2019-03-06  Tom Tromey  <tromey@adacore.com>

>  

>  	* remote-sim.c (gdbsim_target_open): Use result of

> diff --git a/gdb/NEWS b/gdb/NEWS

> index cc7c35c..260e6cc 100644

> --- a/gdb/NEWS

> +++ b/gdb/NEWS

> @@ -3,6 +3,12 @@

>  

>  *** Changes since GDB 8.3

>  

> +* New built-in convenience variable $_gdb_version provides the GDB

> +  version.  It is handy for conditionally using features available

> +  only in or since specific GDB versions, in scripts that should work

> +  error-free with many different versions, such as in system-wide init

> +  files.

> +

>  *** Changes in GDB 8.3

>  

>  * GDB and GDBserver now support access to additional registers on

> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog

> index 0380322..313a061 100644

> --- a/gdb/doc/ChangeLog

> +++ b/gdb/doc/ChangeLog

> @@ -1,3 +1,7 @@

> +2019-03-06  Eli Zaretskii  <eliz@gnu.org>

> +

> +	* gdb.texinfo (Convenience Vars): Document $_gdb_version.

> +

>  2019-03-05  Simon Marchi  <simon.marchi@efficios.com>

>  

>  	* python.texi (Values From Inferior): Change synopsys of the

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo

> index f2028f8..9d15337 100644

> --- a/gdb/doc/gdb.texinfo

> +++ b/gdb/doc/gdb.texinfo

> @@ -11197,7 +11197,7 @@

>  @vindex $_tlb@r{, convenience variable}

>  The variable @code{$_tlb} is automatically set when debugging

>  applications running on MS-Windows in native mode or connected to

> -gdbserver that supports the @code{qGetTIBAddr} request. 

> +gdbserver that supports the @code{qGetTIBAddr} request.

>  @xref{General Query Packets}.

>  This variable contains the address of the thread information block.


This looks like an unintended change.

> @@ -11211,6 +11211,17 @@

>  @item $_gthread

>  The global number of the current thread.  @xref{global thread numbers}.

>  

> +@item $_gdb_version

> +@vindex $_gdb_version@r{, convenience variable}

> +The version of the running @value{GDBN}.  The value is an integer

> +number that encodes the major and minor @value{GDBN} versions as

> +@w{@code{@var{major}*100 + @var{minor}}}, so, e.g., @value{GDBN}

> +version 9.10 will produce the value @code{910}.  Development snapshots

> +and pretest versions have their minor version incremented by one;

> +thus, @value{GDBN} pretest 9.11.90 will produce the value 912.  This

> +variable allows you to write scripts that work with different versions

> +of @value{GDBN} without errors caused by features unavailable in some

> +of those versions.

>  @end table


I know we plan to move to a version scheme where we don't have a "patch"
number (a third number), but just in case, maybe we could plan for it anyway
just in case it ever changes again in the future (I don't expect it will,
but we never know.

So something like MAJOR * 10000 + MINOR * 100 + PATCH * 100

Also, it means that in your example, 9.11.90 would produce 091190.  I think
it's better if we are able to distinguish 9.11.90 from 9.12.

Also, we should consider doing like Python does, and encode different numbers
in different bytes:

  https://docs.python.org/3/c-api/apiabiversion.html

So we could say ((MAJOR << 16) | (MINOR << 8) | PATCH), for example.  The
advantage with this is that it's easy to to isolate a particular number
using bitshifts and masks.  I know it would be possible as well in decimal
to isolate a particular number, but it's just more convenient in hex.

>  

>  @node Convenience Funs

> diff --git a/gdb/top.c b/gdb/top.c

> index 22e6f7e..97b349a 100644

> --- a/gdb/top.c

> +++ b/gdb/top.c

> @@ -147,22 +147,22 @@ int server_command;

>  

>  /* Timeout limit for response from target.  */

>  

> -/* The default value has been changed many times over the years.  It 

> -   was originally 5 seconds.  But that was thought to be a long time 

> +/* The default value has been changed many times over the years.  It

> +   was originally 5 seconds.  But that was thought to be a long time

>     to sit and wait, so it was changed to 2 seconds.  That was thought

> -   to be plenty unless the connection was going through some terminal 

> +   to be plenty unless the connection was going through some terminal

>     server or multiplexer or other form of hairy serial connection.

>  

> -   In mid-1996, remote_timeout was moved from remote.c to top.c and 

> +   In mid-1996, remote_timeout was moved from remote.c to top.c and

>     it began being used in other remote-* targets.  It appears that the

>     default was changed to 20 seconds at that time, perhaps because the

>     Renesas E7000 ICE didn't always respond in a timely manner.

>  

>     But if 5 seconds is a long time to sit and wait for retransmissions,

> -   20 seconds is far worse.  This demonstrates the difficulty of using 

> +   20 seconds is far worse.  This demonstrates the difficulty of using

>     a single variable for all protocol timeouts.

>  

> -   As remote.c is used much more than remote-e7000.c, it was changed 

> +   As remote.c is used much more than remote-e7000.c, it was changed

>     back to 2 seconds in 1999.  */


Some more unintended changes?  There are more in the patch, I won't point
out all of them.

I have to go for now, I didn't look at the implementation yet.

Simon
Eli Zaretskii March 21, 2019, 2:38 p.m. | #41
> Cc: gdb-patches@sourceware.org

> From: Simon Marchi <simark@simark.ca>

> Date: Wed, 20 Mar 2019 21:55:43 -0400

> 

> Ah, I noticed this because you changed the subject in your ping :).


The original one evidently failed to draw attention ;-)

> This would be very useful, especially that we can quite easily query

> this from MI as well as Python.


Thanks, I agree.

> > --- a/gdb/doc/gdb.texinfo

> > +++ b/gdb/doc/gdb.texinfo

> > @@ -11197,7 +11197,7 @@

> >  @vindex $_tlb@r{, convenience variable}

> >  The variable @code{$_tlb} is automatically set when debugging

> >  applications running on MS-Windows in native mode or connected to

> > -gdbserver that supports the @code{qGetTIBAddr} request. 

> > +gdbserver that supports the @code{qGetTIBAddr} request.

> >  @xref{General Query Packets}.

> >  This variable contains the address of the thread information block.

> 

> This looks like an unintended change.


No, it's not unintended.  I mentioned that in my preamble:

> (Note that I took this opportunity to clean up whitespace in top.c,

> I hope it's OK to do that as part of unrelated code changes.)


> I know we plan to move to a version scheme where we don't have a "patch"

> number (a third number), but just in case, maybe we could plan for it anyway

> just in case it ever changes again in the future (I don't expect it will,

> but we never know.

> 

> So something like MAJOR * 10000 + MINOR * 100 + PATCH * 100

> 

> Also, it means that in your example, 9.11.90 would produce 091190.  I think

> it's better if we are able to distinguish 9.11.90 from 9.12.


The idea was that we don't need to distinguish between them.  This
feature is intended to be used in scripts that need to know when a
certain feature became available, so IMO resolution below XX.YY is not
needed, because any XX.YY.ZZ version is just a snapshot of XX.YY+1.

> Also, we should consider doing like Python does, and encode different numbers

> in different bytes:

> 

>   https://docs.python.org/3/c-api/apiabiversion.html

> 

> So we could say ((MAJOR << 16) | (MINOR << 8) | PATCH), for example.  The

> advantage with this is that it's easy to to isolate a particular number

> using bitshifts and masks.  I know it would be possible as well in decimal

> to isolate a particular number, but it's just more convenient in hex.


I don't have a strong opinion, but I do have a weak one: the decimal
"encoding" makes it much easier for humans to construct version
numbers, they don't need to convert to hex.

> > -   As remote.c is used much more than remote-e7000.c, it was changed 

> > +   As remote.c is used much more than remote-e7000.c, it was changed

> >     back to 2 seconds in 1999.  */

> 

> Some more unintended changes?


It's intended, see above.

Thanks for the review.
Simon Marchi March 21, 2019, 3:02 p.m. | #42
> No, it's not unintended.  I mentioned that in my preamble:


Sorry about that, I see it now.  IMO, you could directly push a patch 
that does these cleanups.

>> (Note that I took this opportunity to clean up whitespace in top.c,

>> I hope it's OK to do that as part of unrelated code changes.)

> 

>> I know we plan to move to a version scheme where we don't have a 

>> "patch"

>> number (a third number), but just in case, maybe we could plan for it 

>> anyway

>> just in case it ever changes again in the future (I don't expect it 

>> will,

>> but we never know.

>> 

>> So something like MAJOR * 10000 + MINOR * 100 + PATCH * 100

>> 

>> Also, it means that in your example, 9.11.90 would produce 091190.  I 

>> think

>> it's better if we are able to distinguish 9.11.90 from 9.12.

> 

> The idea was that we don't need to distinguish between them.  This

> feature is intended to be used in scripts that need to know when a

> certain feature became available, so IMO resolution below XX.YY is not

> needed, because any XX.YY.ZZ version is just a snapshot of XX.YY+1.


Ok, fair enough.  Since the .90 is supposed to behave exactly how the 
next version will (and then never used in production again), I'm fine 
with that.

I'm just a little worried that some day we'll introduce back some 
"patch" number, and then we'll need to change how this number is 
encoded, which will cause some headaches :).

>> Also, we should consider doing like Python does, and encode different 

>> numbers

>> in different bytes:

>> 

>>   https://docs.python.org/3/c-api/apiabiversion.html

>> 

>> So we could say ((MAJOR << 16) | (MINOR << 8) | PATCH), for example.  

>> The

>> advantage with this is that it's easy to to isolate a particular 

>> number

>> using bitshifts and masks.  I know it would be possible as well in 

>> decimal

>> to isolate a particular number, but it's just more convenient in hex.

> 

> I don't have a strong opinion, but I do have a weak one: the decimal

> "encoding" makes it much easier for humans to construct version

> numbers, they don't need to convert to hex.


There's not much difference in hex, you would just write "if 
$_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >= 
901".

But again, my argument for hex is weak too, it's easy enough to extract 
the GDB version components using some "% 100" and "/ 100".  If there's 
no more compelling argument for hex, I'm fine with decimal.

Simon
Eli Zaretskii March 21, 2019, 4:01 p.m. | #43
> Date: Thu, 21 Mar 2019 11:02:00 -0400

> From: Simon Marchi <simark@simark.ca>

> Cc: tom@tromey.com, gdb-patches@sourceware.org

> 

> > I don't have a strong opinion, but I do have a weak one: the decimal

> > "encoding" makes it much easier for humans to construct version

> > numbers, they don't need to convert to hex.

> 

> There's not much difference in hex, you would just write "if 

> $_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >= 

> 901".


That's true for small values of major/minor.  But imagine GDB 12.13
for a moment.

> But again, my argument for hex is weak too, it's easy enough to extract 

> the GDB version components using some "% 100" and "/ 100".  If there's 

> no more compelling argument for hex, I'm fine with decimal.


Maybe someone else have an opinion on this?

Thanks.
Simon Marchi March 21, 2019, 4:06 p.m. | #44
On 2019-03-21 12:01, Eli Zaretskii wrote:
>> Date: Thu, 21 Mar 2019 11:02:00 -0400

>> From: Simon Marchi <simark@simark.ca>

>> Cc: tom@tromey.com, gdb-patches@sourceware.org

>> 

>> > I don't have a strong opinion, but I do have a weak one: the decimal

>> > "encoding" makes it much easier for humans to construct version

>> > numbers, they don't need to convert to hex.

>> 

>> There's not much difference in hex, you would just write "if

>> $_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >=

>> 901".

> 

> That's true for small values of major/minor.  But imagine GDB 12.13

> for a moment.


Hmm, true :).

Simon
Pedro Alves March 21, 2019, 4:12 p.m. | #45
On 03/21/2019 04:06 PM, Simon Marchi wrote:
> On 2019-03-21 12:01, Eli Zaretskii wrote:

>>> Date: Thu, 21 Mar 2019 11:02:00 -0400

>>> From: Simon Marchi <simark@simark.ca>

>>> Cc: tom@tromey.com, gdb-patches@sourceware.org

>>>

>>> > I don't have a strong opinion, but I do have a weak one: the decimal

>>> > "encoding" makes it much easier for humans to construct version

>>> > numbers, they don't need to convert to hex.

>>>

>>> There's not much difference in hex, you would just write "if

>>> $_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >=

>>> 901".


Is there an advantage of a single variable that encodes the version,
compared to separate major/minor variables?  Separate variables
would render the discussion of how to encode this moot.

GCC has these as predefined preprocessor macros:

 __GNUC__
 __GNUC_MINOR__
 __GNUC_PATCHLEVEL__

We could likewise have separate variables for these.

Thanks,
Pedro Alves
John Baldwin March 21, 2019, 4:54 p.m. | #46
On 3/21/19 9:12 AM, Pedro Alves wrote:
> On 03/21/2019 04:06 PM, Simon Marchi wrote:

>> On 2019-03-21 12:01, Eli Zaretskii wrote:

>>>> Date: Thu, 21 Mar 2019 11:02:00 -0400

>>>> From: Simon Marchi <simark@simark.ca>

>>>> Cc: tom@tromey.com, gdb-patches@sourceware.org

>>>>

>>>>> I don't have a strong opinion, but I do have a weak one: the decimal

>>>>> "encoding" makes it much easier for humans to construct version

>>>>> numbers, they don't need to convert to hex.

>>>>

>>>> There's not much difference in hex, you would just write "if

>>>> $_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >=

>>>> 901".

> 

> Is there an advantage of a single variable that encodes the version,

> compared to separate major/minor variables?  Separate variables

> would render the discussion of how to encode this moot.

> 

> GCC has these as predefined preprocessor macros:

> 

>  __GNUC__

>  __GNUC_MINOR__

>  __GNUC_PATCHLEVEL__

> 

> We could likewise have separate variables for these.


I think splitting it out is perhaps the simplest.  FWIW, in FreeBSD we
have a single __FreeBSD_version macro that uses decimal.  We also had
some drama when we originally had a single digit field for the minor
version but had minor versions beyond 9 (4.10, 4.11).  The current
format is:

 * scheme is:  <major><two digit minor>Rxx
 *		'R' is in the range 0 to 4 if this is a release branch or
 *		X.0-CURRENT before releng/X.0 is created, otherwise 'R' is
 *		in the range 5 to 9.
 */
#undef __FreeBSD_version
#define __FreeBSD_version 1300013	/* Master, propagated to newvers */

So release 7.2 would be 702000 and each time a patch is issued to the
release due to an errata or security, the last two digits get bumped
(kind of like a patch number), e.g. 702001, 702002.  The 7.x branch
in between 7.2 and 7.3 is 702500 so it sorts between 7.2 release and
7.3 release.  FreeBSD also bumps the 'patch level' field somewhat
frequently even on 'master' as it's used to denote API (or ABI on
'master') changes.  This has mostly worked out ok, and I do think using
decimal is easier to "read" than hex for this, but split out version
numbers is probably simpler for users to work with.

-- 
John Baldwin
Eli Zaretskii March 21, 2019, 5:02 p.m. | #47
> Cc: tom@tromey.com, gdb-patches@sourceware.org

> From: Pedro Alves <palves@redhat.com>

> Date: Thu, 21 Mar 2019 16:12:26 +0000

> 

> Is there an advantage of a single variable that encodes the version,

> compared to separate major/minor variables?


Just that it makes comparison simpler, and IME less error-prone.  Not
a very important argument, admittedly.
Simon Marchi March 21, 2019, 6:08 p.m. | #48
On 2019-03-21 1:02 p.m., Eli Zaretskii wrote:
>> Cc: tom@tromey.com, gdb-patches@sourceware.org

>> From: Pedro Alves <palves@redhat.com>

>> Date: Thu, 21 Mar 2019 16:12:26 +0000

>>

>> Is there an advantage of a single variable that encodes the version,

>> compared to separate major/minor variables?

> 

> Just that it makes comparison simpler, and IME less error-prone.  Not

> a very important argument, admittedly.

> 


Yeah, if you want to check if you are running 8.5 or more, you can do:

if $_gdb_version >= 805

With separate components, you have to do something like

if $_gdb_version_major >= 9 \
    || ($_gdb_version_major >= 8 && $gdb_version_minor >= 5

If you need to check for a range, it's even more complicated.

But then, if the version is provided as separate components, nothing 
prevents you to re-encode it however you want after that.  You can do 
easily enough

set $my_gdb_version = ($_gdb_version_major * 100) + $_gdb_version_minor

if you want, and use the simple comparison.  And everyone is free to 
choose their own encoding :).  So, to me separate components sounds better.

Simon
Pedro Alves March 21, 2019, 6:19 p.m. | #49
On 03/21/2019 06:08 PM, Simon Marchi wrote:
> On 2019-03-21 1:02 p.m., Eli Zaretskii wrote:

>>> Cc: tom@tromey.com, gdb-patches@sourceware.org

>>> From: Pedro Alves <palves@redhat.com>

>>> Date: Thu, 21 Mar 2019 16:12:26 +0000

>>>

>>> Is there an advantage of a single variable that encodes the version,

>>> compared to separate major/minor variables?

>>

>> Just that it makes comparison simpler, and IME less error-prone.  Not

>> a very important argument, admittedly.

>>

> 

> Yeah, if you want to check if you are running 8.5 or more, you can do:

> 

> if $_gdb_version >= 805

> 

> With separate components, you have to do something like

> 

> if $_gdb_version_major >= 9 \

>    || ($_gdb_version_major >= 8 && $gdb_version_minor >= 5

> 

> If you need to check for a range, it's even more complicated.

> 

> But then, if the version is provided as separate components, nothing prevents you to re-encode it however you want after that.  You can do easily enough

> 

> set $my_gdb_version = ($_gdb_version_major * 100) + $_gdb_version_minor

> 

> if you want, and use the simple comparison.  And everyone is free to choose their own encoding :).  So, to me separate components sounds better.


Exactly my thoughts.  That's what the GCC manual describes, BTW:

  https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

Note that with the new gdb version numbering, going forward, for features,
you'll only really care about the major version number, since there won't
be new features added in minor releases.

Thanks,
Pedro Alves
Eli Zaretskii March 21, 2019, 6:38 p.m. | #50
> Cc: tom@tromey.com, gdb-patches@sourceware.org

> From: Pedro Alves <palves@redhat.com>

> Date: Thu, 21 Mar 2019 18:19:04 +0000

> 

> > if $_gdb_version_major >= 9 \

> >    || ($_gdb_version_major >= 8 && $gdb_version_minor >= 5

> > 

> > If you need to check for a range, it's even more complicated.

> > 

> > But then, if the version is provided as separate components, nothing prevents you to re-encode it however you want after that.  You can do easily enough

> > 

> > set $my_gdb_version = ($_gdb_version_major * 100) + $_gdb_version_minor

> > 

> > if you want, and use the simple comparison.  And everyone is free to choose their own encoding :).  So, to me separate components sounds better.

> 

> Exactly my thoughts.  That's what the GCC manual describes, BTW:

> 

>   https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html


And I've seen my share of erroneous version checks using the GCC
macros.

But if there's a consensus to go with 2 separate numbers, I'm okay
with redoing the patch that way.
Eli Zaretskii March 25, 2019, 5:31 p.m. | #51
Seems like there's a consensus to have 2 separate variables, so here's
a version of the patch that does that.  OK to commit?

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2de1adc..f841e0a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-03-06  Eli Zaretskii  <eliz@gnu.org>
+
+	* NEWS: Announce $_gdb_major and $_gdb_minor.
+
+	* top.c (init_gdb_version_vars): New function.
+	(gdb_init): Call init_gdb_version_vars.
+
 2019-03-25  Alan Hayward  <alan.hayward@arm.com>
 
 	* s390-linux-nat.c: Add include.
diff --git a/gdb/NEWS b/gdb/NEWS
index c45b313..676f0a0 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,12 @@
 
 *** Changes since GDB 8.3
 
+* New built-in convenience variables $_gdb_major and $_gdb_minor
+  provide the GDB version.  They are handy for conditionally using
+  features available only in or since specific GDB versions, in
+  scripts that should work error-free with many different versions,
+  such as in system-wide init files.
+
 *** Changes in GDB 8.3
 
 * GDB and GDBserver now support access to additional registers on
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 1f41498..c384267 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2019-03-06  Eli Zaretskii  <eliz@gnu.org>
+
+	* gdb.texinfo (Convenience Vars): Document $_gdb_version.
+
 2019-03-22  Alan Hayward  <alan.hayward@arm.com>
 	    Jiong Wang  <jiong.wang@arm.com>
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 4c1d5e8..115bcf7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -11211,6 +11211,17 @@
 @item $_gthread
 The global number of the current thread.  @xref{global thread numbers}.
 
+@item $_gdb_major
+@itemx $_gdb_minor
+@vindex $_gdb_major@{, convenience variable}
+@vindex $_gdb_minor@{, convenience variable}
+The major and minor version numbers of the running @value{GDBN}.
+Development snapshots and pretest versions have their minor version
+incremented by one; thus, @value{GDBN} pretest 9.11.90 will produce
+the value 12 for @code{$_gdb_minor}.  These variables allow you to
+write scripts that work with different versions of @value{GDBN}
+without errors caused by features unavailable in some of those
+versions.
 @end table
 
 @node Convenience Funs
diff --git a/gdb/top.c b/gdb/top.c
index b10b064..afb77c0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -2003,6 +2004,17 @@ set_history_filename (const char *args,
 }
 
 static void
+init_gdb_version_vars (void)
+{
+  struct internalvar *major_version_var = create_internalvar ("_gdb_major");
+  struct internalvar *minor_version_var = create_internalvar ("_gdb_minor");
+  int vmajor = 0, vminor = 0, vrevision = 0;
+  sscanf (version, "%d.%d.%d", &vmajor, &vminor, &vrevision);
+  set_internalvar_integer (major_version_var, vmajor);
+  set_internalvar_integer (minor_version_var, vminor + (vrevision > 0));
+}
+
+static void
 init_main (void)
 {
   struct cmd_list_element *c;
@@ -2206,4 +2218,7 @@ gdb_init (char *argv0)
      prefix to be installed.  Keep things simple and just do final
      script initialization here.  */
   finish_ext_lang_initialization ();
+
+  /* Create $_gdb_major and $_gdb_minor convenience variables.  */
+  init_gdb_version_vars ();
 }
Simon Marchi March 25, 2019, 5:58 p.m. | #52
On 2019-03-25 1:31 p.m., Eli Zaretskii wrote:
> --- a/gdb/doc/gdb.texinfo

> +++ b/gdb/doc/gdb.texinfo

> @@ -11211,6 +11211,17 @@

>   @item $_gthread

>   The global number of the current thread.  @xref{global thread numbers}.

>   

> +@item $_gdb_major

> +@itemx $_gdb_minor

> +@vindex $_gdb_major@{, convenience variable}

> +@vindex $_gdb_minor@{, convenience variable}

> +The major and minor version numbers of the running @value{GDBN}.

> +Development snapshots and pretest versions have their minor version

> +incremented by one; thus, @value{GDBN} pretest 9.11.90 will produce

> +the value 12 for @code{$_gdb_minor}.  These variables allow you to

> +write scripts that work with different versions of @value{GDBN}

> +without errors caused by features unavailable in some of those

> +versions.

>   @end table


I get this error when trying to "make html":

/home/smarchi/src/binutils-gdb/gdb/doc/gdb.texinfo:11216: misplaced }
/home/smarchi/src/binutils-gdb/gdb/doc/gdb.texinfo:11217: misplaced }

Is it worth having a test for that?  If we check that they have the 
expected value, it would be something to change when a new GDB branch is 
created, probably (as the master branch will now represent the next 
version).  Maybe our release manager should have a say in this :).

Otherwise, this LGTM.

Simon
Eli Zaretskii March 25, 2019, 6:10 p.m. | #53
> From: Simon Marchi <simark@simark.ca>

> Date: Mon, 25 Mar 2019 13:58:20 -0400

> 

> > +@item $_gdb_major

> > +@itemx $_gdb_minor

> > +@vindex $_gdb_major@{, convenience variable}

> > +@vindex $_gdb_minor@{, convenience variable}

> > +The major and minor version numbers of the running @value{GDBN}.

> > +Development snapshots and pretest versions have their minor version

> > +incremented by one; thus, @value{GDBN} pretest 9.11.90 will produce

> > +the value 12 for @code{$_gdb_minor}.  These variables allow you to

> > +write scripts that work with different versions of @value{GDBN}

> > +without errors caused by features unavailable in some of those

> > +versions.

> >   @end table

> 

> I get this error when trying to "make html":

> 

> /home/smarchi/src/binutils-gdb/gdb/doc/gdb.texinfo:11216: misplaced }

> /home/smarchi/src/binutils-gdb/gdb/doc/gdb.texinfo:11217: misplaced }


Thanks, fixed.  Too many r's...

> Is it worth having a test for that?


Hmm... how would you test that?

> If we check that they have the expected value, it would be something

> to change when a new GDB branch is created, probably (as the master

> branch will now represent the next version).  Maybe our release

> manager should have a say in this :).


I'm all ears.

> Otherwise, this LGTM.


Thanks for the review.
Simon Marchi March 25, 2019, 6:33 p.m. | #54
On 2019-03-25 14:10, Eli Zaretskii wrote:
>> Is it worth having a test for that?

> 

> Hmm... how would you test that?


Something like

gdb_test "print \$_gdb_major" " = 8"
gdb_test "print \$_gdb_minor" " = 4"

Simon
Simon Marchi March 25, 2019, 6:37 p.m. | #55
On 2019-03-25 14:33, Simon Marchi wrote:
> On 2019-03-25 14:10, Eli Zaretskii wrote:

>>> Is it worth having a test for that?

>> 

>> Hmm... how would you test that?

> 

> Something like

> 

> gdb_test "print \$_gdb_major" " = 8"

> gdb_test "print \$_gdb_minor" " = 4"

> 

> Simon


Or, the test could somehow automatically get the current version of GDB 
and compare $_gdb_major/$_gdb_minor against that, so that we don't need 
to modify it at every release.

Simon
Eli Zaretskii March 25, 2019, 6:43 p.m. | #56
> Date: Mon, 25 Mar 2019 14:33:55 -0400

> From: Simon Marchi <simark@simark.ca>

> Cc: gdb-patches@sourceware.org, brobecker@adacore.com

> 

> > Hmm... how would you test that?

> 

> Something like

> 

> gdb_test "print \$_gdb_major" " = 8"

> gdb_test "print \$_gdb_minor" " = 4"


This would require changes with each new version.  I thought you had
in mind comparison with output of "gdb --version", suitably edited.
Simon Marchi March 25, 2019, 6:51 p.m. | #57
On 2019-03-25 14:43, Eli Zaretskii wrote:
>> Date: Mon, 25 Mar 2019 14:33:55 -0400

>> From: Simon Marchi <simark@simark.ca>

>> Cc: gdb-patches@sourceware.org, brobecker@adacore.com

>> 

>> > Hmm... how would you test that?

>> 

>> Something like

>> 

>> gdb_test "print \$_gdb_major" " = 8"

>> gdb_test "print \$_gdb_minor" " = 4"

> 

> This would require changes with each new version.  I thought you had

> in mind comparison with output of "gdb --version", suitably edited.


Yes, that's why I wondered initially if it was worth adding a test like 
this, since it would impose an additional burden on the person doing the 
release.  However, comparing the output with gdb --version, or some 
other automated mean, would be perfect.

Simon
Eli Zaretskii March 25, 2019, 7:18 p.m. | #58
> Date: Mon, 25 Mar 2019 14:51:36 -0400

> From: Simon Marchi <simark@simark.ca>

> Cc: gdb-patches@sourceware.org, brobecker@adacore.com

> 

> comparing the output with gdb --version, or some other automated

> mean, would be perfect.


Well, I never wrote any tests for GDB, and I'm not sure I have a
system on which to run such a test.  So I hope someone else will add
this test.
Simon Marchi March 26, 2019, 2:46 p.m. | #59
On 2019-03-25 3:18 p.m., Eli Zaretskii wrote:
>> Date: Mon, 25 Mar 2019 14:51:36 -0400

>> From: Simon Marchi <simark@simark.ca>

>> Cc: gdb-patches@sourceware.org, brobecker@adacore.com

>>

>> comparing the output with gdb --version, or some other automated

>> mean, would be perfect.

> 

> Well, I never wrote any tests for GDB, and I'm not sure I have a

> system on which to run such a test.  So I hope someone else will add

> this test.

> 


Apparently, the test default.exp needs to be changed in any case, because
it adds new convenience variables.  Without modification, the
"show convenience" test fails because there is a new entry it doesn't know
about.

So for now, I would suggest adding the modification below to your patch to keep
the test passing.  It is not obvious to change the test to match the convenience
variable value against a variable or even against a regexp, because it uses
gdb_test_list_exact, which does straight string comparison.

Also, I am not sure if reading the output of "show version" and testing against
that would be a really good test, because the implementation and the test would
work essentially work the same way.  So if the version string erroneously becomes
0.0.0 because of some bug in some script, the test would still pass, even though
the values are wrong.

At least, the following is simple and robust.


diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp
index ece1428e617e..9ff5144448d8 100644
--- a/gdb/testsuite/gdb.base/default.exp
+++ b/gdb/testsuite/gdb.base/default.exp
@@ -602,6 +602,8 @@ set show_conv_list \
        {$_probe_arg10 = <error: No frame selected>} \
        {$_probe_arg11 = <error: No frame selected>} \
        {$_isvoid = <internal function _isvoid>} \
+       {$_gdb_major = 8} \
+       {$_gdb_minor = 4} \
     }
 if ![skip_python_tests] {
     append show_conv_list \


	
Simon
Joel Brobecker March 26, 2019, 8:57 p.m. | #60
> Apparently, the test default.exp needs to be changed in any case,

> because it adds new convenience variables.  Without modification, the

> "show convenience" test fails because there is a new entry it doesn't

> know about.

> 

> So for now, I would suggest adding the modification below to your

> patch to keep the test passing.  It is not obvious to change the test

> to match the convenience variable value against a variable or even

> against a regexp, because it uses gdb_test_list_exact, which does

> straight string comparison.

> 

> Also, I am not sure if reading the output of "show version" and

> testing against that would be a really good test, because the

> implementation and the test would work essentially work the same way.

> So if the version string erroneously becomes 0.0.0 because of some bug

> in some script, the test would still pass, even though the values are

> wrong.

> 

> At least, the following is simple and robust.


Hmmm, the direct string comparison thing is a bit annoying.

This is going to be one more manual test to do after each branch,
and I'm a bit concerned about that. We can start with that, as we want
the test to pass. But I'm wondering if, instead of getting the output
from "show version" to determine the expected values, we could parse
gdb/version.in instead. If needed, we could so the parsing as part of
the build process (ig do it in the makefile, and store the result in
a couple of files). Something like that.

> diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp

> index ece1428e617e..9ff5144448d8 100644

> --- a/gdb/testsuite/gdb.base/default.exp

> +++ b/gdb/testsuite/gdb.base/default.exp

> @@ -602,6 +602,8 @@ set show_conv_list \

>         {$_probe_arg10 = <error: No frame selected>} \

>         {$_probe_arg11 = <error: No frame selected>} \

>         {$_isvoid = <internal function _isvoid>} \

> +       {$_gdb_major = 8} \

> +       {$_gdb_minor = 4} \

>      }

>  if ![skip_python_tests] {

>      append show_conv_list \

> 

> 

> 	

> Simon


-- 
Joel
Eli Zaretskii March 27, 2019, 3:34 a.m. | #61
> Date: Tue, 26 Mar 2019 13:57:16 -0700

> From: Joel Brobecker <brobecker@adacore.com>

> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org

> 

> This is going to be one more manual test to do after each branch,

> and I'm a bit concerned about that. We can start with that, as we want

> the test to pass. But I'm wondering if, instead of getting the output

> from "show version" to determine the expected values, we could parse

> gdb/version.in instead. If needed, we could so the parsing as part of

> the build process (ig do it in the makefile, and store the result in

> a couple of files). Something like that.


Yes, if version.in is updated by some automated method, the same
method could update the test suite.
Joel Brobecker March 27, 2019, 12:56 p.m. | #62
> > This is going to be one more manual test to do after each branch,

> > and I'm a bit concerned about that. We can start with that, as we want

> > the test to pass. But I'm wondering if, instead of getting the output

> > from "show version" to determine the expected values, we could parse

> > gdb/version.in instead. If needed, we could so the parsing as part of

> > the build process (ig do it in the makefile, and store the result in

> > a couple of files). Something like that.

> 

> Yes, if version.in is updated by some automated method, the same

> method could update the test suite.


We're actually thinking of getting rid of that daily bump, and doing
this would make it more difficult. There has to be a way to parse
version.in during the build and make that information available to
the testsuite, somehow. If not, I'd rather adjust the testing to
allow regexps, and just verify that it prints a positive number for
the major version, and a non-zero number for the minor version.

-- 
Joel
Eli Zaretskii March 30, 2019, 10 a.m. | #63
> From: Simon Marchi <simark@simark.ca>

> Cc: gdb-patches@sourceware.org, brobecker@adacore.com

> Date: Tue, 26 Mar 2019 10:46:56 -0400

> 

> Apparently, the test default.exp needs to be changed in any case, because

> it adds new convenience variables.  Without modification, the

> "show convenience" test fails because there is a new entry it doesn't know

> about.

> 

> So for now, I would suggest adding the modification below to your patch to keep

> the test passing.  It is not obvious to change the test to match the convenience

> variable value against a variable or even against a regexp, because it uses

> gdb_test_list_exact, which does straight string comparison.

> 

> Also, I am not sure if reading the output of "show version" and testing against

> that would be a really good test, because the implementation and the test would

> work essentially work the same way.  So if the version string erroneously becomes

> 0.0.0 because of some bug in some script, the test would still pass, even though

> the values are wrong.

> 

> At least, the following is simple and robust.


Thanks, I used that, and pushed the changes including this one.
Simon Marchi March 30, 2019, 5:24 p.m. | #64
On 2019-03-27 8:56 a.m., Joel Brobecker wrote:
>>> This is going to be one more manual test to do after each branch,

>>> and I'm a bit concerned about that. We can start with that, as we want

>>> the test to pass. But I'm wondering if, instead of getting the output

>>> from "show version" to determine the expected values, we could parse

>>> gdb/version.in instead. If needed, we could so the parsing as part of

>>> the build process (ig do it in the makefile, and store the result in

>>> a couple of files). Something like that.

>>

>> Yes, if version.in is updated by some automated method, the same

>> method could update the test suite.

> 

> We're actually thinking of getting rid of that daily bump, and doing

> this would make it more difficult. There has to be a way to parse

> version.in during the build and make that information available to

> the testsuite, somehow. If not, I'd rather adjust the testing to

> allow regexps, and just verify that it prints a positive number for

> the major version, and a non-zero number for the minor version.

> 


I wrote a patch to make the test read the version number from version.in
directly, see here:

https://sourceware.org/ml/gdb-patches/2019-03/msg00762.html

Simon