Re: [PATCHv7 1/4] gdb: allow gdb.Color to work correctly with pagination

[Tom de Vries <tdevries@suse.de>]

On 8/24/25 18:13, Andrew Burgess wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
> 
>> This commit allows gdb.Color objects to be used to style output from
>> GDB commands written in Python, and the styled output should work
>> correctly with pagination.
> 
> I've gone ahead and pushed this fix.
> 

The new test-case fails for me on aarch64-linux.  I've filed a PR ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=33321 ) to track this.

Thanks,
- Tom

> I believe that the change I've made here is inline with how the same
> problem is handled in the TUI, parsing the escape sequence and then
> using the resulting ui_file_style object.  And the second part of the
> fix, changing gdb_printf to gdb_puts falls out from making the first
> change.
> 
> Given that the gdb.Color API was added since GDB 16, it seems sensible
> to get this bug fixed before GDB 17 branches.
> 
> The other patches in this series I've not pushed, pending review.
> 
> Thanks,
> Andrew
> 
> 
> 
>>
>> There are two parts to fixing this:
>>
>> First, GDB needs to be able to track the currently applied style
>> within the page_file class.  This means that style changes need to be
>> achieved with calls to pager_file::emit_style_escape.
>>
>> Now usually, GDB does this by calling something like fprintf_styled,
>> which takes care to apply the style for us.  However, that's not
>> really an option here as a gdb.Color isn't a full style, and as the
>> gdb.Color object is designed to be converted directly into escape
>> sequences that can then be printed, we really need a solution that
>> works with this approach.
>>
>> However pager_file::puts already has code in place to handle escape
>> sequences.  Right now all this code does is spot the escape sequence
>> and append it to the m_wrap_buffer.  But in this commit I propose that
>> we go one step further, parse the escape sequence back into a
>> ui_file_style object in pager_file::puts, and then we can call
>> pager_file::emit_style_escape.
>>
>> If the parsing doesn't work then we can just add the escape sequence
>> to m_wrap_buffer as we did before.
>>
>> But wait, how can this work if a gdb.Color isn't a full style?  Turns
>> out that's not a problem.  We only ever emit the escape sequence for
>> those parts of a style that need changing, so a full style that sets
>> the foreground color will emit the same escape sequence as a gdb.Color
>> for the foreground.  When we convert the escape sequence back into a
>> ui_file_style, then we get a style with everything set to default,
>> except the foreground color.
>>
>> I had hoped that this would be all that was needed.  But unfortunately
>> this doesn't work because of the second problem...
>>
>> ... the implementation of the Python function gdb.write() calls
>> gdb_printf(), which calls gdb_vprintf(), which calls ui_file::vprintf,
>> which calls ui_out::vmessage, which calls ui_out::call_do_message, and
>> finally we reach cli_ui_out::do_message.  This final do_message
>> function does this:
>>
>>    ui_file *stream = m_streams.back ();
>>    stream->emit_style_escape (style);
>>    stream->puts (str.c_str ());
>>    stream->emit_style_escape (ui_file_style ());
>>
>> If we imagine the case where we are emitting a style, triggered from
>> Python like this:
>>
>>    gdb.write(gdb.Color('red').escape_sequence(True))
>>
>> the STYLE in this case will be the default ui_file_style(), and STR
>> will hold the escape sequence we are writing.
>>
>> After the first change, where pager_file::puts now calls
>> pager_file::emit_style_escape, the current style of STREAM will have
>> been updated.  But this means that the final emit_style_escape will
>> now restore the default style.
>>
>> The fix for this is to avoid using the high level gdb_printf from
>> gdb.write(), and instead use gdb_puts instead.  The gdb_puts function
>> doesn't restore the default style, which means our style modification
>> survives.
>>
>> There's a new test included.  This test includes what appears like a
>> pointless extra loop (looping over a single value), but later commits
>> in this series will add more values to this list.
>> ---
>>   gdb/python/python.c                           |  18 +--
>>   .../gdb.python/py-color-pagination.exp        | 122 ++++++++++++++++++
>>   .../gdb.python/py-color-pagination.py         |  46 +++++++
>>   gdb/utils.c                                   |  21 ++-
>>   4 files changed, 195 insertions(+), 12 deletions(-)
>>   create mode 100644 gdb/testsuite/gdb.python/py-color-pagination.exp
>>   create mode 100644 gdb/testsuite/gdb.python/py-color-pagination.py
>>
>> diff --git a/gdb/python/python.c b/gdb/python/python.c
>> index cb0d642a67d..1af7896eb08 100644
>> --- a/gdb/python/python.c
>> +++ b/gdb/python/python.c
>> @@ -1570,21 +1570,21 @@ gdbpy_write (PyObject *self, PyObject *args, PyObject *kw)
>>   
>>     try
>>       {
>> +      ui_file *stream;
>>         switch (stream_type)
>>   	{
>>   	case 1:
>> -	  {
>> -	    gdb_printf (gdb_stderr, "%s", arg);
>> -	    break;
>> -	  }
>> +	  stream = gdb_stderr;
>> +	  break;
>>   	case 2:
>> -	  {
>> -	    gdb_printf (gdb_stdlog, "%s", arg);
>> -	    break;
>> -	  }
>> +	  stream = gdb_stdlog;
>> +	  break;
>>   	default:
>> -	  gdb_printf (gdb_stdout, "%s", arg);
>> +	  stream = gdb_stdout;
>> +	  break;
>>   	}
>> +
>> +      gdb_puts (arg, stream);
>>       }
>>     catch (const gdb_exception &except)
>>       {
>> diff --git a/gdb/testsuite/gdb.python/py-color-pagination.exp b/gdb/testsuite/gdb.python/py-color-pagination.exp
>> new file mode 100644
>> index 00000000000..3235fffe5cc
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-color-pagination.exp
>> @@ -0,0 +1,122 @@
>> +# Copyright (C) 2025 Free Software Foundation, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# This file is part of the GDB testsuite.  It tests gdb.Color and how this
>> +# interacts with GDB's pagination system.
>> +
>> +load_lib gdb-python.exp
>> +
>> +require allow_python_tests
>> +
>> +standard_testfile
>> +
>> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
>> +
>> +set str "<[string repeat - 78]>"
>> +
>> +# These define all the default attributes for a style: background
>> +# color, intensity, italics, and underlined.
>> +set other_attr ";49;22;23;24;27"
>> +
>> +# These colors set the foreground color only.  Everything else is the
>> +# default.
>> +set black "(?:\033\\\[30${other_attr}m)"
>> +set red "(?:\033\\\[31${other_attr}m)"
>> +set green "(?:\033\\\[32${other_attr}m)"
>> +set yellow "(?:\033\\\[33${other_attr}m)"
>> +set blue "(?:\033\\\[34${other_attr}m)"
>> +set magenta "(?:\033\\\[35${other_attr}m)"
>> +set cyan "(?:\033\\\[36${other_attr}m)"
>> +set white "(?:\033\\\[37${other_attr}m)"
>> +
>> +set any_color "(?:${black}|${red}|${green}|${yellow}|${blue}|${magenta}|${cyan}|${white})"
>> +
>> +# Run the command 'TYPE-fill MODE' which fills the screen with output and
>> +# triggers the pagination prompt.  Check that styling is applied correctly
>> +# to the output.
>> +proc test_pagination { type mode } {
>> +
>> +    # Start with a fresh GDB, but enable color support.
>> +    with_ansi_styling_terminal {
>> +	clean_restart
>> +    }
>> +
>> +    gdb_test_no_output "source $::pyfile" "source the script"
>> +
>> +    gdb_test_no_output "set width 80"
>> +    gdb_test_no_output "set height 15"
>> +
>> +    set saw_bad_color_handling false
>> +    set expected_restore_color ""
>> +    set last_color ""
>> +    gdb_test_multiple "$type-fill $mode" "" {
>> +	-re "^$type-fill $mode\r\n" {
>> +	    exp_continue
>> +	}
>> +
>> +	-re "^(${::any_color}?)(${::any_color})$::str" {
>> +	    # After a continuation prompt GDB will restore the previous
>> +	    # color, and then we immediately switch to a new color.
>> +	    set restored_color $expect_out(1,string)
>> +	    if { $restored_color ne ""
>> +		 && $restored_color ne $expected_restore_color } {
>> +		set saw_bad_color_handling true
>> +	    }
>> +	    set last_color $expect_out(2,string)
>> +	    exp_continue
>> +	}
>> +
>> +	-re "^\033\\\[${::decimal}m$::str" {
>> +	    # This catches the case where the color's escape sequence has
>> +	    # not been converted back into a full style.  This indicates
>> +	    # something went wrong in the pager_file::puts function.
>> +	    set saw_bad_color_handling true
>> +	    exp_continue
>> +	}
>> +
>> +	-re "^((?:\033\\\[m)?)$::pagination_prompt$" {
>> +	    # After a pagination prompt we expect GDB to restore the last
>> +	    # color.
>> +	    set expected_restore_color $last_color
>> +
>> +	    # If we didn't see a color reset sequence then the pagination
>> +	    # prompt will have been printed in the wrong color, this is a
>> +	    # GDB bug.
>> +	    set color_reset $expect_out(1,string)
>> +	    if { $color_reset eq "" } {
>> +		set saw_bad_color_handling true
>> +	    }
>> +
>> +	    # Send '\n' to view more output.
>> +	    send_gdb "\n"
>> +	    exp_continue
>> +	}
>> +
>> +	-re "^\r\n" {
>> +	    # The matches the newline sent to the continuation prompt.
>> +	    exp_continue
>> +	}
>> +
>> +	-re "^\033\\\[m\r\n$::gdb_prompt $" {
>> +	    gdb_assert { !$saw_bad_color_handling } $gdb_test_name
>> +	}
>> +    }
>> +}
>> +
>> +foreach_with_prefix type { color } {
>> +    foreach_with_prefix mode { write print } {
>> +	test_pagination $type $mode
>> +    }
>> +}
>> diff --git a/gdb/testsuite/gdb.python/py-color-pagination.py b/gdb/testsuite/gdb.python/py-color-pagination.py
>> new file mode 100644
>> index 00000000000..efd501eedf5
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-color-pagination.py
>> @@ -0,0 +1,46 @@
>> +# Copyright (C) 2025 Free Software Foundation, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +import gdb
>> +
>> +basic_colors = ["black", "red", "green", "yellow", "blue", "magenta", "cyan", "white"]
>> +
>> +
>> +def write(mode, text):
>> +    if mode == "write":
>> +        gdb.write(text)
>> +    else:
>> +        print(text, end="")
>> +
>> +
>> +class ColorTester(gdb.Command):
>> +    def __init__(self):
>> +        super().__init__("color-fill", gdb.COMMAND_USER)
>> +
>> +    def invoke(self, args, from_tty):
>> +        mode = args
>> +        str = "<" + "-" * 78 + ">"
>> +        for i in range(0, 20):
>> +            for color_name in basic_colors:
>> +                c = gdb.Color(color_name)
>> +                write(mode, c.escape_sequence(True))
>> +                write(mode, str)
>> +
>> +        default = gdb.Color("none")
>> +        write(mode, default.escape_sequence(True))
>> +        write(mode, "\n")
>> +
>> +
>> +ColorTester()
>> diff --git a/gdb/utils.c b/gdb/utils.c
>> index 10d3d51e481..92e626a9c75 100644
>> --- a/gdb/utils.c
>> +++ b/gdb/utils.c
>> @@ -1702,10 +1702,25 @@ pager_file::puts (const char *linebuffer)
>>   	  else if (*linebuffer == '\033'
>>   		   && skip_ansi_escape (linebuffer, &skip_bytes))
>>   	    {
>> -	      m_wrap_buffer.append (linebuffer, skip_bytes);
>> -	      /* Note that we don't consider this a character, so we
>> +	      /* We don't consider escape sequences as characters, so we
>>   		 don't increment chars_printed here.  */
>> -	      linebuffer += skip_bytes;
>> +
>> +	      size_t style_len;
>> +	      ui_file_style style;
>> +	      if (style.parse (linebuffer, &style_len)
>> +		  && style_len <= skip_bytes)
>> +		{
>> +		  this->emit_style_escape (style);
>> +
>> +		  linebuffer += style_len;
>> +		  skip_bytes -= style_len;
>> +		}
>> +
>> +	      if (skip_bytes > 0)
>> +		{
>> +		  m_wrap_buffer.append (linebuffer, skip_bytes);
>> +		  linebuffer += skip_bytes;
>> +		}
>>   	    }
>>   	  else if (*linebuffer == '\r')
>>   	    {
>> -- 
>> 2.47.1
>