* [PATCH 0/2] Style related fixes
@ 2021-01-14 9:56 Andrew Burgess
2021-01-14 9:56 ` [PATCH 1/2] gdb: don't print escape characters when a style is disabled Andrew Burgess
2021-01-14 9:56 ` [PATCH 2/2] gdb: add new version style Andrew Burgess
0 siblings, 2 replies; 13+ messages in thread
From: Andrew Burgess @ 2021-01-14 9:56 UTC (permalink / raw)
To: gdb-patches
Patch #1 - fixes some places where GDB would print escape characters
even when it was not going to change colour.
Patch #2 - Adds a new 'version' style, for styling the GDB version
number.
Thanks,
Andrew
---
Andrew Burgess (2):
gdb: don't print escape characters when a style is disabled
gdb: add new version style
gdb/ChangeLog | 19 ++
gdb/NEWS | 5 +
gdb/cli/cli-style.c | 17 +-
gdb/cli/cli-style.h | 6 +-
gdb/doc/ChangeLog | 4 +
gdb/doc/gdb.texinfo | 12 +
gdb/testsuite/ChangeLog | 16 ++
gdb/testsuite/gdb.base/style.exp | 449 +++++++++++++++++++------------
gdb/testsuite/lib/gdb-utils.exp | 1 +
gdb/top.c | 11 +-
gdb/utils.c | 18 +-
11 files changed, 371 insertions(+), 187 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/2] gdb: don't print escape characters when a style is disabled 2021-01-14 9:56 [PATCH 0/2] Style related fixes Andrew Burgess @ 2021-01-14 9:56 ` Andrew Burgess 2021-01-20 19:34 ` Tom Tromey 2021-01-14 9:56 ` [PATCH 2/2] gdb: add new version style Andrew Burgess 1 sibling, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2021-01-14 9:56 UTC (permalink / raw) To: gdb-patches While working on another patch I noticed that if I disable a single style with, for example: set style filename background none set style filename foreground none set style filename intensity normal Then in some places escape characters are still injected into the output stream. This is a bit of an edge case, and I can't think when this would actually cause problems, but it still felt like a bit of an annoyance. It's especially annoying because depending on how something is printed then GDB might, or might not, add escape characters. So this would not add escape characters if the filename style was disabled: fprintf_filtered (file, "%ps", styled_string (file_name_style.style (), "This is a test")); But this would add escape characters: fprintf_styled (file, file_name_style.style (), "%s", "This is a test"); I tracked this down to some unguarded calls to set_output_style in utils.c. In order to test this I have rewritten the gdb.base/style.exp test script. The core of the script is now run multiple times. The first time the test is run things are as they were before, all styles are on. After that though the test is rerun multiple times. Each time through a single style is disabled using the 3 explicit set calls listed above. I then repeat all the tests, however, I arrange so that the patterns that expected the now disabled styling expect no styling. gdb/ChangeLog: * utils.c (fputs_highlighted): Only call set_output_style when the requested style is not the default. (fprintf_styled): Likewise. (vfprintf_styled): Likewise. gdb/testsuite/ChangeLog: * gdb.base/style.exp (limited_style): New proc. (clean_restart_and_disable): New proc. (run_style_tests): New proc. Most of the old tests from this file are now in this proc. (test_startup_version_string): New proc. Reamining test from the old file is in this proc. (run_all_tests): New proc. --- gdb/ChangeLog | 7 + gdb/testsuite/ChangeLog | 10 + gdb/testsuite/gdb.base/style.exp | 443 +++++++++++++++++++------------ gdb/utils.c | 18 +- 4 files changed, 302 insertions(+), 176 deletions(-) diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp index aec1d0f42ed..58c8da0a10e 100644 --- a/gdb/testsuite/gdb.base/style.exp +++ b/gdb/testsuite/gdb.base/style.exp @@ -17,203 +17,306 @@ standard_testfile -save_vars { env(TERM) } { - # We need an ANSI-capable terminal to get the output. - setenv TERM ansi - - set test_macros 0 - set options debug - get_compiler_info - if { [test_compiler_info "gcc-*"] || [test_compiler_info "clang-*"] } { - lappend options additional_flags=-g3 - set test_macros 1 - } +# Compile the test executable. +set test_macros 0 +set options debug +get_compiler_info +if { [test_compiler_info "gcc-*"] || [test_compiler_info "clang-*"] } { + lappend options additional_flags=-g3 + set test_macros 1 +} + +if {[build_executable "failed to build" $testfile $srcfile $options]} { + return -1 +} + +# The tests in this file are run multiple times with GDB's styles +# disabled one at a time. This variable is the style that is +# currently disabled. +set currently_disabled_style "" - if {[prepare_for_testing "failed to prepare" $testfile $srcfile \ - $options]} { - return -1 +# A replacement for the 'style' function found in gdb-utils.exp, +# filter out requests for the disabled style. +proc limited_style { str style } { + global currently_disabled_style + + if { $style != $currently_disabled_style } { + return [global_style_func $str $style] } - set readnow [readnow] + return $str +} + +# A wrapper around clean_restart from gdb.exp, this performs the +# normal clean_restart, but then disables the currently disabled +# style. +proc clean_restart_and_disable { args } { + global currently_disabled_style + + eval "global_clean_restart $args" - if {![runto_main]} { - fail "style tests failed" - return + if { $currently_disabled_style != "" } { + set st $currently_disabled_style + gdb_test_no_output "set style $st background none" "" + gdb_test_no_output "set style $st foreground none" "" + gdb_test_no_output "set style $st intensity normal" "" } +} + +# The core of this test script. Run some tests of different aspects +# of GDB's styling. +proc run_style_tests { } { + global testfile srcfile hex binfile test_macros + global currently_disabled_style + + save_vars { env(TERM) } { + # We need an ANSI-capable terminal to get the output. + setenv TERM ansi - # Check that the source highlighter has not stripped away the leading - # newlines. - set main_line [gdb_get_line_number "break here"] - gdb_test "list $main_line,$main_line" "return.*some_called_function.*" + # Restart GDB with the correct TERM variable setting, this + # means that GDB will enable styling. + clean_restart ${binfile} - gdb_test_no_output "set style enabled off" + set readnow [readnow] - set argv "" - gdb_test_multiple "frame" "frame without styling" { - -re -wrap "main \\(argc=.*, (argv=$hex)\\).*style\\.c:\[0-9\].*" { - set argv $expect_out(1,string) - pass $gdb_test_name + if {![runto_main]} { + fail "style tests failed" + return } - } - gdb_test_no_output "set style enabled on" + # Check that the source highlighter has not stripped away the + # leading newlines. + set main_line [gdb_get_line_number "break here"] + gdb_test "list $main_line,$main_line" "return.*some_called_function.*" - set main_expr [style main function] - set base_file_expr [style ".*style\\.c" file] - set file_expr "$base_file_expr:\[0-9\]" - set arg_expr [style "arg." variable] + gdb_test_no_output "set style enabled off" - gdb_test "frame" \ - "$main_expr.*$arg_expr.*$arg_expr.*$file_expr.*" - gdb_test "info breakpoints" "$main_expr at $file_expr.*" + set argv "" + gdb_test_multiple "frame" "frame without styling" { + -re -wrap "main \\(argc=.*, (argv=$hex)\\).*style\\.c:\[0-9\].*" { + set argv $expect_out(1,string) + pass $gdb_test_name + } + } - gdb_test_no_output "set style sources off" - gdb_test "frame" \ - "\r\n\[^\033\]*break here.*" \ - "frame without sources styling" - gdb_test_no_output "set style sources on" + gdb_test_no_output "set style enabled on" + + set main_expr [style main function] + set base_file_expr [style ".*style\\.c" file] + set file_expr "$base_file_expr:\[0-9\]" + set arg_expr [style "arg." variable] + + gdb_test "frame" \ + "$main_expr.*$arg_expr.*$arg_expr.*$file_expr.*" + gdb_test "info breakpoints" "$main_expr at $file_expr.*" + + gdb_test_no_output "set style sources off" + gdb_test "frame" \ + "\r\n\[^\033\]*break here.*" \ + "frame without sources styling" + gdb_test_no_output "set style sources on" + + gdb_test "break -q main" "file $base_file_expr.*" + + gdb_test "print &main" " = .* [style $hex address] <$main_expr>" + + # Regression test for a bug where line-wrapping would occur at + # the wrong spot with styling. There were different bugs at + # different widths, so try two. + foreach width {20 30} { + set argv_len [string length $argv] + if { $argv_len == 0 } { + continue + } + + # There was also a bug where the styling could be wrong in + # the line listing; this is why the words from the source + # code are spelled out in the final result line of the + # test. + set re1_styled \ + [multi_line \ + "#0 *$main_expr.*$arg_expr.*" \ + ".*$arg_expr.*" \ + ".* at .*$file_expr.*" \ + "\[0-9\]+.*return.* break here .*"] + set re2_styled \ + [multi_line \ + "#0 *$main_expr.*$arg_expr.*" \ + ".*$arg_expr.* at .*$file_expr.*" \ + "\[0-9\]+.*return.* break here .*"] + + # The length of the line containing argv containing: + # - 4 leading spaces + # - argv string + # - closing parenthesis + set line_len [expr 4 + $argv_len + 1] + + if { $line_len > $width } { + # At on the next line. + set re_styled $re1_styled + } else { + # At on the same line as argv. + set re_styled $re2_styled + } + + gdb_test_no_output "set width $width" + gdb_test "frame" $re_styled "frame when width=$width" + } - gdb_test "break -q main" "file $base_file_expr.*" + # Reset width back to 0. + gdb_test_no_output "set width 0" "" - gdb_test "print &main" " = .* [style $hex address] <$main_expr>" + if {$test_macros} { + set macro_line [gdb_get_line_number "\#define SOME_MACRO"] + gdb_test "info macro SOME_MACRO" \ + "Defined at $base_file_expr:$macro_line\r\n#define SOME_MACRO 23" + } - # Regression test for a bug where line-wrapping would occur at the - # wrong spot with styling. There were different bugs at different - # widths, so try two. - foreach width {20 30} { - set argv_len [string length $argv] - if { $argv_len == 0 } { - continue + gdb_test_no_output "set width 0" + + set main [style main function] + set func [style some_called_function function] + # Somewhere should see the call to the function. + gdb_test "disassemble main" \ + [concat "Dump of assembler code for function $main:.*" \ + "[style $hex address].*$func.*"] + + set ifield [style int_field variable] + set sfield [style string_field variable] + set efield [style e_field variable] + set evalue [style VALUE_TWO variable] + gdb_test "print struct_value" \ + "\{$ifield = 23,.*$sfield = .*,.*$efield = $evalue.*" + + set address_style_expr [style ".*\".*address.*\".*style.*" address] + set color "blue" + if { $currently_disabled_style == "address" } { + set color "none" } + gdb_test "show style address foreground" \ + "The ${address_style_expr} foreground color is: ${color}" \ + "style name and style word styled using its own style in show style" - # There was also a bug where the styling could be wrong in the - # line listing; this is why the words from the source code are - # spelled out in the final result line of the test. - set re1_styled \ + set aliases_expr [style ".*aliases.*" title] + set breakpoints_expr [style ".*breakpoints.*" title] + gdb_test "help" \ [multi_line \ - "#0 *$main_expr.*$arg_expr.*" \ - ".*$arg_expr.*" \ - ".* at .*$file_expr.*" \ - "\[0-9\]+.*return.* break here .*"] - set re2_styled \ + "List of classes of commands:" \ + "" \ + "${aliases_expr} -- User-defined aliases of other commands\." \ + "${breakpoints_expr} -- Making program stop at certain points\." \ + ".*" \ + ] \ + "help classes of commands styled with title" + + set taas_expr [style ".*taas.*" title] + set tfaas_expr [style ".*tfaas.*" title] + set cut_for_thre_expr [style "cut for 'thre" highlight] + gdb_test "apropos -v cut for 'thre" \ [multi_line \ - "#0 *$main_expr.*$arg_expr.*" \ - ".*$arg_expr.* at .*$file_expr.*" \ - "\[0-9\]+.*return.* break here .*"] - - # The length of the line containing argv containing: - # - 4 leading spaces - # - argv string - # - closing parenthesis - set line_len [expr 4 + $argv_len + 1] - - if { $line_len > $width } { - # At on the next line. - set re_styled $re1_styled - } else { - # At on the same line as argv. - set re_styled $re2_styled + "" \ + "${taas_expr}" \ + "Apply a command to all .*" \ + "Usage:.*" \ + "short${cut_for_thre_expr}ad apply.*" \ + "" \ + "${tfaas_expr}" \ + "Apply a command to all .*" \ + "Usage:.*" \ + "short${cut_for_thre_expr}ad apply.*" \ + ] + + clean_restart + + set quoted [string_to_regexp $binfile] + set pass_re "Reading symbols from [style $quoted file]\.\.\." + if { $readnow } { + set pass_re \ + [multi_line \ + $pass_re \ + "Expanding full symbols from [style $quoted file]\.\.\."] } - - gdb_test_no_output "set width $width" - gdb_test "frame" $re_styled "frame when width=$width" + gdb_test "file $binfile" \ + $pass_re \ + "filename is styled when loading symbol file" \ + "Are you sure you want to change the file.*" \ + "y" + + gdb_test "pwd" "Working directory [style .*? file].*" + + gdb_test_no_output "set print repeat 3" + gdb_test "print {0,0,0,0,0,0,0,0}" \ + " = \\{0 [style {<repeats.*8.*times>} metadata]\\}" + + gdb_test "show logging file" \ + "The current logfile is \"[style .*? file]\"\\..*" + + # Check warnings are styled by setting a rubbish data + # directory. + gdb_test "set data-directory Makefile" \ + "warning: [style .*? file] is not a directory\\..*" + gdb_test "show data-directory" \ + "GDB's data directory is \"[style .*? file]\"\\..*" + + # Check that deprecation styles command names. + gdb_test_no_output "maintenance deprecate p \"new_p\"" \ + "maintenance deprecate p \"new_p\" /1/" + gdb_test "p 5" \ + "Warning: '[style p title]', an alias for the command '[style print title]', is deprecated.*Use '[style new_p title]'.*" \ + "p deprecated warning, with replacement" } +} - # Reset width back to 0. - gdb_test_no_output "set width 0" +# A separate test from the above as the styled text this checks can't +# currently be disabled (the text is printed too early in GDB's +# startup process). +proc test_startup_version_string { } { + gdb_exit + gdb_spawn - if {$test_macros} { - set macro_line [gdb_get_line_number "\#define SOME_MACRO"] - gdb_test "info macro SOME_MACRO" \ - "Defined at $base_file_expr:$macro_line\r\n#define SOME_MACRO 23" - } + set vers [style "GNU gdb.*" "35;1"] + gdb_test "" "${vers}.*" "version is styled at startup" +} - set main [style main function] - set func [style some_called_function function] - # Somewhere should see the call to the function. - gdb_test "disassemble main" \ - [concat "Dump of assembler code for function $main:.*" \ - "[style $hex address].*$func.*"] +# A test driver. Calls RUN_STYLE_TESTS multiple times with each GDB +# style disabled one at a time. +proc run_all_tests { } { + global currently_disabled_style - set ifield [style int_field variable] - set sfield [style string_field variable] - set efield [style e_field variable] - set evalue [style VALUE_TWO variable] - gdb_test "print struct_value" \ - "\{$ifield = 23,.*$sfield = .*,.*$efield = $evalue.*" + # Run tests with all styles in their default state. + with_test_prefix "all styles enabled" { + run_style_tests + } - gdb_exit - gdb_spawn + # Now, for each style in turn. Disable that style only and run the + # test again. Things in that style should NOT now be styled. + foreach style { title file function highlight variable \ + address metadata } { - set vers [style "GNU gdb.*" "35;1"] - gdb_test "" "${vers}.*" \ - "version is styled" - - set address_style_expr [style ".*\".*address.*\".*style.*" address] - gdb_test "show style address foreground" \ - "The ${address_style_expr} foreground color is: blue" \ - "style name and style word styled using its own style in show style" - - set aliases_expr [style ".*aliases.*" title] - set breakpoints_expr [style ".*breakpoints.*" title] - gdb_test "help" \ - [multi_line \ - "List of classes of commands:" \ - "" \ - "${aliases_expr} -- User-defined aliases of other commands\." \ - "${breakpoints_expr} -- Making program stop at certain points\." \ - ".*" \ - ] \ - "help classes of commands styled with title" - - set taas_expr [style ".*taas.*" title] - set tfaas_expr [style ".*tfaas.*" title] - set cut_for_thre_expr [style "cut for 'thre" highlight] - gdb_test "apropos -v cut for 'thre" \ - [multi_line \ - "" \ - "${taas_expr}" \ - "Apply a command to all .*" \ - "Usage:.*" \ - "short${cut_for_thre_expr}ad apply.*" \ - "" \ - "${tfaas_expr}" \ - "Apply a command to all .*" \ - "Usage:.*" \ - "short${cut_for_thre_expr}ad apply.*" \ - ] - - set quoted [string_to_regexp $binfile] - set pass_re "Reading symbols from [style $quoted file]\.\.\." - if { $readnow } { - set pass_re \ - [multi_line \ - $pass_re \ - "Expanding full symbols from [style $quoted file]\.\.\."] + set currently_disabled_style $style + with_test_prefix "disable style $style" { + run_style_tests + } } - gdb_test "file $binfile" \ - $pass_re \ - "filename is styled when loading symbol file" - - gdb_test "pwd" "Working directory [style .*? file].*" - - gdb_test_no_output "set print repeat 3" - gdb_test "print {0,0,0,0,0,0,0,0}" \ - " = \\{0 [style {<repeats.*8.*times>} metadata]\\}" - - gdb_test "show logging file" \ - "The current logfile is \"[style .*? file]\"\\..*" - - # Check warnings are styled by setting a rubbish data directory. - gdb_test "set data-directory Makefile" \ - "warning: [style .*? file] is not a directory\\..*" - gdb_test "show data-directory" \ - "GDB's data directory is \"[style .*? file]\"\\..*" - - # Check that deprecation styles command names. - gdb_test_no_output "maintenance deprecate p \"new_p\"" \ - "maintenance deprecate p \"new_p\" /1/" - gdb_test "p 5" \ - "Warning: '[style p title]', an alias for the command '[style print title]', is deprecated.*Use '[style new_p title]'.*" \ - "p deprecated warning, with replacement" } + +# Override the 'style' function. +rename style global_style_func +rename limited_style style + +# Override the 'clean_restart' function. +rename clean_restart global_clean_restart +rename clean_restart_and_disable clean_restart + +run_all_tests + +# Restore the 'clean_restart' function. +rename clean_restart clean_restart_and_disable +rename global_clean_restart clean_restart + +# Restore the 'style' function. +rename style limited_style +rename global_style_func style + +# Finally, check the styling of the version string during startup. +test_startup_version_string diff --git a/gdb/utils.c b/gdb/utils.c index 414e7b153a0..3aad9c7af74 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1946,14 +1946,16 @@ fputs_highlighted (const char *str, const compiled_regex &highlight, } /* Output pmatch with the highlight style. */ - set_output_style (stream, highlight_style.style ()); + if (!highlight_style.style ().is_default ()) + set_output_style (stream, highlight_style.style ()); while (n_highlight > 0) { fputc_filtered (*str, stream); n_highlight--; str++; } - set_output_style (stream, ui_file_style ()); + if (!highlight_style.style ().is_default ()) + set_output_style (stream, ui_file_style ()); } /* Output the trailing part of STR not matching HIGHLIGHT. */ @@ -2195,11 +2197,13 @@ fprintf_styled (struct ui_file *stream, const ui_file_style &style, { va_list args; - set_output_style (stream, style); + if (!style.is_default ()) + set_output_style (stream, style); va_start (args, format); vfprintf_filtered (stream, format, args); va_end (args); - set_output_style (stream, ui_file_style ()); + if (!style.is_default ()) + set_output_style (stream, ui_file_style ()); } /* See utils.h. */ @@ -2208,9 +2212,11 @@ void vfprintf_styled (struct ui_file *stream, const ui_file_style &style, const char *format, va_list args) { - set_output_style (stream, style); + if (!style.is_default ()) + set_output_style (stream, style); vfprintf_filtered (stream, format, args); - set_output_style (stream, ui_file_style ()); + if (!style.is_default ()) + set_output_style (stream, ui_file_style ()); } /* See utils.h. */ -- 2.25.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] gdb: don't print escape characters when a style is disabled 2021-01-14 9:56 ` [PATCH 1/2] gdb: don't print escape characters when a style is disabled Andrew Burgess @ 2021-01-20 19:34 ` Tom Tromey 2021-01-21 12:06 ` Andrew Burgess 0 siblings, 1 reply; 13+ messages in thread From: Tom Tromey @ 2021-01-20 19:34 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: Andrew> +# A replacement for the 'style' function found in gdb-utils.exp, Andrew> +# filter out requests for the disabled style. Andrew> +proc limited_style { str style } { Rather than rename the global 'style', I think it would be fine to just have a wrapper proc that is used locally. That seems a lot less tricky. Same with clean_restart. Andrew> /* Output pmatch with the highlight style. */ Andrew> - set_output_style (stream, highlight_style.style ()); Andrew> + if (!highlight_style.style ().is_default ()) Andrew> + set_output_style (stream, highlight_style.style ()); set_output_style ends up setting applied_style. So avoiding this call seems iffy to me. Maybe another approach would be to change emit_style_escape so that it does nothing if the currently-applied style is equal to the passed-in style? Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] gdb: don't print escape characters when a style is disabled 2021-01-20 19:34 ` Tom Tromey @ 2021-01-21 12:06 ` Andrew Burgess 2021-01-21 16:08 ` Andrew Burgess 2021-01-22 15:21 ` Tom Tromey 0 siblings, 2 replies; 13+ messages in thread From: Andrew Burgess @ 2021-01-21 12:06 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches * Tom Tromey <tom@tromey.com> [2021-01-20 12:34:43 -0700]: > >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: > > Andrew> +# A replacement for the 'style' function found in gdb-utils.exp, > Andrew> +# filter out requests for the disabled style. > Andrew> +proc limited_style { str style } { > > Rather than rename the global 'style', I think it would be fine to just > have a wrapper proc that is used locally. That seems a lot less tricky. > Same with clean_restart. > > Andrew> /* Output pmatch with the highlight style. */ > Andrew> - set_output_style (stream, highlight_style.style ()); > Andrew> + if (!highlight_style.style ().is_default ()) > Andrew> + set_output_style (stream, highlight_style.style ()); > > set_output_style ends up setting applied_style. > So avoiding this call seems iffy to me. Tom, Thanks for the feedback. It's not clear to me if your feedback is about this one particular use of the 'if (!STYLE.is_default ()) { ... }' pattern, or if you are suggesting all uses of this pattern are suspect. It's just that you added the original uses of this pattern... So I guess my question would be, if its this one particular case then I don't understand why this cases is different to all the others, or... ... if it's all uses of this pattern then I guess my questions is, does this mean you think the original uses were a mistake? Or has something else changed ? > Maybe another approach would be to change emit_style_escape so that it > does nothing if the currently-applied style is equal to the passed-in > style? I'll give this a try and see if I can get this working. Thanks, Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] gdb: don't print escape characters when a style is disabled 2021-01-21 12:06 ` Andrew Burgess @ 2021-01-21 16:08 ` Andrew Burgess 2021-01-22 15:26 ` Tom Tromey 2021-01-22 15:21 ` Tom Tromey 1 sibling, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2021-01-21 16:08 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches * Andrew Burgess <andrew.burgess@embecosm.com> [2021-01-21 12:06:18 +0000]: > * Tom Tromey <tom@tromey.com> [2021-01-20 12:34:43 -0700]: > > > >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: > > > > Andrew> +# A replacement for the 'style' function found in gdb-utils.exp, > > Andrew> +# filter out requests for the disabled style. > > Andrew> +proc limited_style { str style } { > > > > Rather than rename the global 'style', I think it would be fine to just > > have a wrapper proc that is used locally. That seems a lot less tricky. > > Same with clean_restart. > > > > Andrew> /* Output pmatch with the highlight style. */ > > Andrew> - set_output_style (stream, highlight_style.style ()); > > Andrew> + if (!highlight_style.style ().is_default ()) > > Andrew> + set_output_style (stream, highlight_style.style ()); > > > > set_output_style ends up setting applied_style. > > So avoiding this call seems iffy to me. > > Tom, > > Thanks for the feedback. > > It's not clear to me if your feedback is about this one particular use > of the 'if (!STYLE.is_default ()) { ... }' pattern, or if you are > suggesting all uses of this pattern are suspect. > > It's just that you added the original uses of this pattern... > > So I guess my question would be, if its this one particular case then > I don't understand why this cases is different to all the others, > or... > > ... if it's all uses of this pattern then I guess my questions is, > does this mean you think the original uses were a mistake? Or has > something else changed ? > > > Maybe another approach would be to change emit_style_escape so that it > > does nothing if the currently-applied style is equal to the passed-in > > style? > > I'll give this a try and see if I can get this working. OK, I think I have something working. There were some issues with line wrapping, but I think that the final state is even better than the original patch. Looking at some of the gdb.base/style.exp tests the set of escape sequences emitted now seems to be minimal. Let me know if this is what you were thinking of. Thanks, Andrew --- commit c57874a4887009dee46687a895ea57157deb18b6 Author: Andrew Burgess <andrew.burgess@embecosm.com> Date: Wed Jan 13 20:08:42 2021 +0000 gdb: don't print escape characters when a style is disabled While working on another patch I noticed that if I disable a single style with, for example: set style filename background none set style filename foreground none set style filename intensity normal Then in some places escape characters are still injected into the output stream. This is a bit of an edge case, and I can't think when this would actually cause problems, but it still felt like a bit of an annoyance. One place where this does impact is in testing, where it becomes harder to write tight test patterns if it is not obvious when GDB will decide to inject escape sequences. It's especially annoying because depending on how something is printed then GDB might, or might not, add escape characters. So this would not add escape characters if the filename style was disabled: fprintf_filtered (file, "%ps", styled_string (file_name_style.style (), "This is a test")); But this would add escape characters: fprintf_styled (file, file_name_style.style (), "%s", "This is a test"); I tracked this down to some calls to set_output_style in utils.c. Currently some calls to set_output_style (in utils.c) are guarded like this: if (!STYLE.is_default ()) set_output_style (stream, STYLE); But some calls are not. It is the calls that are NOT guarded that cause the extra escape sequences to be emitted. My initial proposal to resolve this issue was simply to ensure that all calls to set_output_style were guarded. The patch I posted for this can be found here: https://sourceware.org/pipermail/gdb-patches/2021-January/175096.html The feedback on this proposal was that it might be better to guard against the escape sequences being emitted at a later lever, right down at emit_style_escape. So this is what this version does. In emit_style_escape we already track the currently applied style, so if the style we are being asked to switch to is the same as the currently applied style then no escape sequence needs to be emitted. Making this change immediately exposed some issues in fputs_maybe_filtered related to line wrapping. The best place to start to understand what's going on with the styling and wrapping is look at the test: gdb.base/style.exp: all styles enabled: frame when width=20 If you run this test and then examine the output in an editor so the escape sequences can be seen you'll see the duplicate escape sequences that are emitted before this patch, the compare to after this patch you'll see the set of escape sequences should be the minimum required. In order to test these changes I have rewritten the gdb.base/style.exp test script. The core of the script is now run multiple times. The first time the test is run things are as they were before, all styles are on. After that the test is rerun multiple times. Each time through a single style is disabled using the 3 explicit set calls listed above. I then repeat all the tests, however, I arrange so that the patterns for the disabled style now require no escape sequences. gdb/ChangeLog: * utils.c (emit_style_escape): Only emit an escape sequence if the requested style is different than the current applied style. (fputs_maybe_filtered): Adjust the juggling of the wrap_style, and current applied_style. (fputs_styled): Remove is_default check. (fputs_styled_unfiltered): Likewise. (vfprintf_styled_no_gdbfmt): Likewise. gdb/testsuite/ChangeLog: * gdb.base/style.exp (limited_style): New proc. (clean_restart_and_disable): New proc. (run_style_tests): New proc. Most of the old tests from this file are now in this proc. (test_startup_version_string): New proc. Reamining test from the old file is in this proc. diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp index aec1d0f42ed..08cd63546bd 100644 --- a/gdb/testsuite/gdb.base/style.exp +++ b/gdb/testsuite/gdb.base/style.exp @@ -17,203 +17,290 @@ standard_testfile -save_vars { env(TERM) } { - # We need an ANSI-capable terminal to get the output. - setenv TERM ansi - - set test_macros 0 - set options debug - get_compiler_info - if { [test_compiler_info "gcc-*"] || [test_compiler_info "clang-*"] } { - lappend options additional_flags=-g3 - set test_macros 1 - } +# Compile the test executable. +set test_macros 0 +set options debug +get_compiler_info +if { [test_compiler_info "gcc-*"] || [test_compiler_info "clang-*"] } { + lappend options additional_flags=-g3 + set test_macros 1 +} - if {[prepare_for_testing "failed to prepare" $testfile $srcfile \ - $options]} { - return -1 - } +if {[build_executable "failed to build" $testfile $srcfile $options]} { + return -1 +} - set readnow [readnow] +# The tests in this file are run multiple times with GDB's styles +# disabled one at a time. This variable is the style that is +# currently disabled. +set currently_disabled_style "" - if {![runto_main]} { - fail "style tests failed" - return +# A wrapper around the 'style' function found in gdb-utils.exp, +# filter out requests for the disabled style. +proc limited_style { str style } { + global currently_disabled_style + + if { $style != $currently_disabled_style } { + return [style $str $style] } - # Check that the source highlighter has not stripped away the leading - # newlines. - set main_line [gdb_get_line_number "break here"] - gdb_test "list $main_line,$main_line" "return.*some_called_function.*" + return $str +} - gdb_test_no_output "set style enabled off" +# A wrapper around 'clean_restart' from gdb.exp, this performs the +# normal clean_restart, but then disables the currently disabled +# style. +proc clean_restart_and_disable { args } { + global currently_disabled_style - set argv "" - gdb_test_multiple "frame" "frame without styling" { - -re -wrap "main \\(argc=.*, (argv=$hex)\\).*style\\.c:\[0-9\].*" { - set argv $expect_out(1,string) - pass $gdb_test_name - } - } + eval "clean_restart $args" - gdb_test_no_output "set style enabled on" + if { $currently_disabled_style != "" } { + set st $currently_disabled_style + gdb_test_no_output "set style $st background none" "" + gdb_test_no_output "set style $st foreground none" "" + gdb_test_no_output "set style $st intensity normal" "" + } +} - set main_expr [style main function] - set base_file_expr [style ".*style\\.c" file] - set file_expr "$base_file_expr:\[0-9\]" - set arg_expr [style "arg." variable] +# The core of this test script. Run some tests of different aspects +# of GDB's styling. +# +# Within this proc always use LIMITED_STYLE instead of STYLE, and +# CLEAN_RESTART_AND_DISABLE instead of CLEAN_RESTART, this ensures +# that the test operates as expected as styles are disabled. +proc run_style_tests { } { + global testfile srcfile hex binfile test_macros + global currently_disabled_style decimal hex + + save_vars { env(TERM) } { + # We need an ANSI-capable terminal to get the output. + setenv TERM ansi + + # Restart GDB with the correct TERM variable setting, this + # means that GDB will enable styling. + clean_restart_and_disable ${binfile} + + set readnow [readnow] + + if {![runto_main]} { + fail "style tests failed" + return + } - gdb_test "frame" \ - "$main_expr.*$arg_expr.*$arg_expr.*$file_expr.*" - gdb_test "info breakpoints" "$main_expr at $file_expr.*" + # Check that the source highlighter has not stripped away the + # leading newlines. + set main_line [gdb_get_line_number "break here"] + gdb_test "list $main_line,$main_line" "return.*some_called_function.*" - gdb_test_no_output "set style sources off" - gdb_test "frame" \ - "\r\n\[^\033\]*break here.*" \ - "frame without sources styling" - gdb_test_no_output "set style sources on" + gdb_test_no_output "set style enabled off" - gdb_test "break -q main" "file $base_file_expr.*" + set argv "" + gdb_test_multiple "frame" "frame without styling" { + -re -wrap "main \\(argc=.*, (argv=$hex)\\).*style\\.c:\[0-9\].*" { + set argv $expect_out(1,string) + pass $gdb_test_name + } + } - gdb_test "print &main" " = .* [style $hex address] <$main_expr>" + gdb_test_no_output "set style enabled on" - # Regression test for a bug where line-wrapping would occur at the - # wrong spot with styling. There were different bugs at different - # widths, so try two. - foreach width {20 30} { - set argv_len [string length $argv] - if { $argv_len == 0 } { - continue - } + set main_expr [limited_style main function] + set base_file_expr [limited_style ".*style\\.c" file] + set file_expr "$base_file_expr:\[0-9\]+" + set arg_expr [limited_style "arg." variable] - # There was also a bug where the styling could be wrong in the - # line listing; this is why the words from the source code are - # spelled out in the final result line of the test. - set re1_styled \ + gdb_test "frame" \ [multi_line \ - "#0 *$main_expr.*$arg_expr.*" \ - ".*$arg_expr.*" \ - ".* at .*$file_expr.*" \ - "\[0-9\]+.*return.* break here .*"] - set re2_styled \ - [multi_line \ - "#0 *$main_expr.*$arg_expr.*" \ - ".*$arg_expr.* at .*$file_expr.*" \ - "\[0-9\]+.*return.* break here .*"] - - # The length of the line containing argv containing: - # - 4 leading spaces - # - argv string - # - closing parenthesis - set line_len [expr 4 + $argv_len + 1] - - if { $line_len > $width } { - # At on the next line. - set re_styled $re1_styled - } else { - # At on the same line as argv. - set re_styled $re2_styled + "#0\\s+$main_expr\\s+\\($arg_expr=$decimal,\\s+$arg_expr=$hex\\)\\s+at\\s+$file_expr" \ + "\[0-9\]+\\s+.*return.* break here .*"] + gdb_test "info breakpoints" "$main_expr at $file_expr.*" + + gdb_test_no_output "set style sources off" + gdb_test "frame" \ + "\r\n\[^\033\]*break here.*" \ + "frame without sources styling" + gdb_test_no_output "set style sources on" + + gdb_test "break -q main" "file $base_file_expr.*" + + gdb_test "print &main" " = .* [limited_style $hex address] <$main_expr>" + + # Regression test for a bug where line-wrapping would occur at + # the wrong spot with styling. There were different bugs at + # different widths, so try two. + foreach width {20 30} { + set argv_len [string length $argv] + if { $argv_len == 0 } { + continue + } + + # There was also a bug where the styling could be wrong in + # the line listing; this is why the words from the source + # code are spelled out in the final result line of the + # test. + set re1_styled \ + [multi_line \ + "#0\\s+$main_expr\\s+\\($arg_expr=$decimal,\\s+" \ + "\\s+$arg_expr=$hex\\)" \ + "\\s+at\\s+$file_expr" \ + "\[0-9\]+\\s+.*return.* break here .*"] + set re2_styled \ + [multi_line \ + "#0\\s+$main_expr\\s+\\($arg_expr=.*" \ + "\\s+$arg_expr=$hex\\)\\s+at\\s+$file_expr" \ + "\[0-9\]+\\s+.*return.* break here .*"] + + # The length of the line containing argv containing: + # - 4 leading spaces + # - argv string + # - closing parenthesis + set line_len [expr 4 + $argv_len + 1] + + if { $line_len > $width } { + # At on the next line. + set re_styled $re1_styled + } else { + # At on the same line as argv. + set re_styled $re2_styled + } + + gdb_test_no_output "set width $width" + gdb_test "frame" $re_styled "frame when width=$width" } - gdb_test_no_output "set width $width" - gdb_test "frame" $re_styled "frame when width=$width" - } - - # Reset width back to 0. - gdb_test_no_output "set width 0" + # Reset width back to 0. + gdb_test_no_output "set width 0" "" - if {$test_macros} { - set macro_line [gdb_get_line_number "\#define SOME_MACRO"] - gdb_test "info macro SOME_MACRO" \ - "Defined at $base_file_expr:$macro_line\r\n#define SOME_MACRO 23" - } + if {$test_macros} { + set macro_line [gdb_get_line_number "\#define SOME_MACRO"] + gdb_test "info macro SOME_MACRO" \ + "Defined at $base_file_expr:$macro_line\r\n#define SOME_MACRO 23" + } - set main [style main function] - set func [style some_called_function function] - # Somewhere should see the call to the function. - gdb_test "disassemble main" \ - [concat "Dump of assembler code for function $main:.*" \ - "[style $hex address].*$func.*"] + gdb_test_no_output "set width 0" + + set main [limited_style main function] + set func [limited_style some_called_function function] + # Somewhere should see the call to the function. + gdb_test "disassemble main" \ + [concat "Dump of assembler code for function $main:.*" \ + "[limited_style $hex address].*$func.*"] + + set ifield [limited_style int_field variable] + set sfield [limited_style string_field variable] + set efield [limited_style e_field variable] + set evalue [limited_style VALUE_TWO variable] + gdb_test "print struct_value" \ + "\{$ifield = 23,.*$sfield = .*,.*$efield = $evalue.*" + + set address_style_expr [limited_style ".*\".*address.*\".*style.*" address] + set color "blue" + if { $currently_disabled_style == "address" } { + set color "none" + } + gdb_test "show style address foreground" \ + "The ${address_style_expr} foreground color is: ${color}" \ + "style name and style word styled using its own style in show style" - set ifield [style int_field variable] - set sfield [style string_field variable] - set efield [style e_field variable] - set evalue [style VALUE_TWO variable] - gdb_test "print struct_value" \ - "\{$ifield = 23,.*$sfield = .*,.*$efield = $evalue.*" + set aliases_expr [limited_style ".*aliases.*" title] + set breakpoints_expr [limited_style ".*breakpoints.*" title] + gdb_test "help" \ + [multi_line \ + "List of classes of commands:" \ + "" \ + "${aliases_expr} -- User-defined aliases of other commands\." \ + "${breakpoints_expr} -- Making program stop at certain points\." \ + ".*" \ + ] \ + "help classes of commands styled with title" + + set taas_expr [limited_style ".*taas.*" title] + set tfaas_expr [limited_style ".*tfaas.*" title] + set cut_for_thre_expr [limited_style "cut for 'thre" highlight] + gdb_test "apropos -v cut for 'thre" \ + [multi_line \ + "" \ + "${taas_expr}" \ + "Apply a command to all .*" \ + "Usage:.*" \ + "short${cut_for_thre_expr}ad apply.*" \ + "" \ + "${tfaas_expr}" \ + "Apply a command to all .*" \ + "Usage:.*" \ + "short${cut_for_thre_expr}ad apply.*" \ + ] + + clean_restart_and_disable + + set quoted [string_to_regexp $binfile] + set pass_re "Reading symbols from [limited_style $quoted file]\.\.\." + if { $readnow } { + set pass_re \ + [multi_line \ + $pass_re \ + "Expanding full symbols from [limited_style $quoted file]\.\.\."] + } + gdb_test "file $binfile" \ + $pass_re \ + "filename is styled when loading symbol file" \ + "Are you sure you want to change the file.*" \ + "y" + + gdb_test "pwd" "Working directory [limited_style .*? file].*" + + gdb_test_no_output "set print repeat 3" + gdb_test "print {0,0,0,0,0,0,0,0}" \ + " = \\{0 [limited_style {<repeats.*8.*times>} metadata]\\}" + + gdb_test "show logging file" \ + "The current logfile is \"[limited_style .*? file]\"\\..*" + + # Check warnings are styled by setting a rubbish data + # directory. + gdb_test "set data-directory Makefile" \ + "warning: [limited_style .*? file] is not a directory\\..*" + gdb_test "show data-directory" \ + "GDB's data directory is \"[limited_style .*? file]\"\\..*" + + # Check that deprecation styles command names. + gdb_test_no_output "maintenance deprecate p \"new_p\"" \ + "maintenance deprecate p \"new_p\" /1/" + gdb_test "p 5" \ + "Warning: '[limited_style p title]', an alias for the command '[limited_style print title]', is deprecated.*Use '[limited_style new_p title]'.*" \ + "p deprecated warning, with replacement" + } +} +# A separate test from the above as the styled text this checks can't +# currently be disabled (the text is printed too early in GDB's +# startup process). +proc test_startup_version_string { } { gdb_exit gdb_spawn + # Deliberate use of base STYLE proc here as the style of the + # startup version string can't (currently) be controlled. set vers [style "GNU gdb.*" "35;1"] - gdb_test "" "${vers}.*" \ - "version is styled" - - set address_style_expr [style ".*\".*address.*\".*style.*" address] - gdb_test "show style address foreground" \ - "The ${address_style_expr} foreground color is: blue" \ - "style name and style word styled using its own style in show style" - - set aliases_expr [style ".*aliases.*" title] - set breakpoints_expr [style ".*breakpoints.*" title] - gdb_test "help" \ - [multi_line \ - "List of classes of commands:" \ - "" \ - "${aliases_expr} -- User-defined aliases of other commands\." \ - "${breakpoints_expr} -- Making program stop at certain points\." \ - ".*" \ - ] \ - "help classes of commands styled with title" - - set taas_expr [style ".*taas.*" title] - set tfaas_expr [style ".*tfaas.*" title] - set cut_for_thre_expr [style "cut for 'thre" highlight] - gdb_test "apropos -v cut for 'thre" \ - [multi_line \ - "" \ - "${taas_expr}" \ - "Apply a command to all .*" \ - "Usage:.*" \ - "short${cut_for_thre_expr}ad apply.*" \ - "" \ - "${tfaas_expr}" \ - "Apply a command to all .*" \ - "Usage:.*" \ - "short${cut_for_thre_expr}ad apply.*" \ - ] - - set quoted [string_to_regexp $binfile] - set pass_re "Reading symbols from [style $quoted file]\.\.\." - if { $readnow } { - set pass_re \ - [multi_line \ - $pass_re \ - "Expanding full symbols from [style $quoted file]\.\.\."] + gdb_test "" "${vers}.*" "version is styled at startup" +} + + +# Run tests with all styles in their default state. +with_test_prefix "all styles enabled" { + run_style_tests +} + +# Now, for each style in turn. Disable that style only and run the +# test again. Things in that style should NOT now be styled. +foreach style { title file function highlight variable \ + address metadata } { + set currently_disabled_style $style + with_test_prefix "disable style $style" { + run_style_tests } - gdb_test "file $binfile" \ - $pass_re \ - "filename is styled when loading symbol file" - - gdb_test "pwd" "Working directory [style .*? file].*" - - gdb_test_no_output "set print repeat 3" - gdb_test "print {0,0,0,0,0,0,0,0}" \ - " = \\{0 [style {<repeats.*8.*times>} metadata]\\}" - - gdb_test "show logging file" \ - "The current logfile is \"[style .*? file]\"\\..*" - - # Check warnings are styled by setting a rubbish data directory. - gdb_test "set data-directory Makefile" \ - "warning: [style .*? file] is not a directory\\..*" - gdb_test "show data-directory" \ - "GDB's data directory is \"[style .*? file]\"\\..*" - - # Check that deprecation styles command names. - gdb_test_no_output "maintenance deprecate p \"new_p\"" \ - "maintenance deprecate p \"new_p\" /1/" - gdb_test "p 5" \ - "Warning: '[style p title]', an alias for the command '[style print title]', is deprecated.*Use '[style new_p title]'.*" \ - "p deprecated warning, with replacement" } + +# Finally, check the styling of the version string during startup. +test_startup_version_string diff --git a/gdb/utils.c b/gdb/utils.c index 414e7b153a0..b9f8997ec80 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1426,12 +1426,15 @@ static void emit_style_escape (const ui_file_style &style, struct ui_file *stream = nullptr) { - applied_style = style; + if (applied_style != style) + { + applied_style = style; - if (stream == nullptr) - wrap_buffer.append (style.to_ansi ()); - else - stream->puts (style.to_ansi ().c_str ()); + if (stream == nullptr) + wrap_buffer.append (style.to_ansi ()); + else + stream->puts (style.to_ansi ().c_str ()); + } } /* Set the current output style. This will affect future uses of the @@ -1800,14 +1803,20 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream, prompt is given; and to avoid emitting style sequences in the middle of a run of text, we track this as well. */ - ui_file_style save_style; + ui_file_style save_style = applied_style; bool did_paginate = false; chars_printed = 0; lines_printed++; if (wrap_column) { - save_style = wrap_style; + /* We are about to insert a newline at an historic + location in the WRAP_BUFFER. Before we do we want to + restore the default style. To know if we actually + need to insert an escape sequence we must restore the + current applied style to how it was at the WRAP_COLUMN + location. */ + applied_style = wrap_style; if (stream->can_emit_style_escape ()) emit_style_escape (ui_file_style (), stream); /* If we aren't actually wrapping, don't output @@ -1822,10 +1831,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream, stream->puts ("\n"); } else - { - save_style = applied_style; - flush_wrap_buffer (stream); - } + flush_wrap_buffer (stream); /* Possible new page. Note that PAGINATION_DISABLED_FOR_COMMAND might be set during @@ -1841,8 +1847,19 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream, if (wrap_column) { stream->puts (wrap_indent); + + /* Having finished inserting the wrapping we should + restore the style as it was at the WRAP_COLUMN. */ if (stream->can_emit_style_escape ()) - emit_style_escape (save_style, stream); + emit_style_escape (wrap_style, stream); + + /* The WRAP_BUFFER will still contain content, and that + content might set some alternative style. Restore + APPLIED_STYLE as it was before we started wrapping, + this reflects the current style for the last character + in WRAP_BUFFER. */ + applied_style = save_style; + /* FIXME, this strlen is what prevents wrap_indent from containing tabs. However, if we recurse to print it and count its chars, we risk trouble if wrap_indent is @@ -1895,16 +1912,9 @@ void fputs_styled (const char *linebuffer, const ui_file_style &style, struct ui_file *stream) { - /* This just makes it so we emit somewhat fewer escape - sequences. */ - if (style.is_default ()) - fputs_maybe_filtered (linebuffer, stream, 1); - else - { - set_output_style (stream, style); - fputs_maybe_filtered (linebuffer, stream, 1); - set_output_style (stream, ui_file_style ()); - } + set_output_style (stream, style); + fputs_maybe_filtered (linebuffer, stream, 1); + set_output_style (stream, ui_file_style ()); } /* See utils.h. */ @@ -1913,16 +1923,9 @@ void fputs_styled_unfiltered (const char *linebuffer, const ui_file_style &style, struct ui_file *stream) { - /* This just makes it so we emit somewhat fewer escape - sequences. */ - if (style.is_default ()) - fputs_maybe_filtered (linebuffer, stream, 0); - else - { - set_output_style (stream, style); - fputs_maybe_filtered (linebuffer, stream, 0); - set_output_style (stream, ui_file_style ()); - } + set_output_style (stream, style); + fputs_maybe_filtered (linebuffer, stream, 0); + set_output_style (stream, ui_file_style ()); } /* See utils.h. */ @@ -2222,11 +2225,9 @@ vfprintf_styled_no_gdbfmt (struct ui_file *stream, const ui_file_style &style, std::string str = string_vprintf (format, args); if (!str.empty ()) { - if (!style.is_default ()) - set_output_style (stream, style); + set_output_style (stream, style); fputs_maybe_filtered (str.c_str (), stream, filter); - if (!style.is_default ()) - set_output_style (stream, ui_file_style ()); + set_output_style (stream, ui_file_style ()); } } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] gdb: don't print escape characters when a style is disabled 2021-01-21 16:08 ` Andrew Burgess @ 2021-01-22 15:26 ` Tom Tromey 0 siblings, 0 replies; 13+ messages in thread From: Tom Tromey @ 2021-01-22 15:26 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: Andrew> OK, I think I have something working. There were some issues with Andrew> line wrapping, but I think that the final state is even better than Andrew> the original patch. Looking at some of the gdb.base/style.exp tests Andrew> the set of escape sequences emitted now seems to be minimal. Andrew> Let me know if this is what you were thinking of. Thank you for doing this. This looks good to me. Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] gdb: don't print escape characters when a style is disabled 2021-01-21 12:06 ` Andrew Burgess 2021-01-21 16:08 ` Andrew Burgess @ 2021-01-22 15:21 ` Tom Tromey 1 sibling, 0 replies; 13+ messages in thread From: Tom Tromey @ 2021-01-22 15:21 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches Andrew> It's not clear to me if your feedback is about this one particular use Andrew> of the 'if (!STYLE.is_default ()) { ... }' pattern, or if you are Andrew> suggesting all uses of this pattern are suspect. Well, I was suspicious of all of them. But maybe I was mistaken about this. Andrew> It's just that you added the original uses of this pattern... I didn't remember, but sorry about that. Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] gdb: add new version style 2021-01-14 9:56 [PATCH 0/2] Style related fixes Andrew Burgess 2021-01-14 9:56 ` [PATCH 1/2] gdb: don't print escape characters when a style is disabled Andrew Burgess @ 2021-01-14 9:56 ` Andrew Burgess 2021-01-14 10:35 ` Andrew Burgess ` (3 more replies) 1 sibling, 4 replies; 13+ messages in thread From: Andrew Burgess @ 2021-01-14 9:56 UTC (permalink / raw) To: gdb-patches This commit adds a new 'version' style, which replaces the hard coded styling currently used for GDB's version string. GDB's version number is displayed: 1. In the output of 'show version', and 2. When GDB starts up (without the --quiet option). This new style can only ever affect the first of these two cases as the second case is printed before GDB has processed any initialization files, or processed any GDB commands passed on the command line. However, because the first case exists I think this commit makes sense, it means the style is no longer hard coded into GDB, and we can add some tests that the style can be enabled/disabled correctly. This commit is an alternative to a patch Tom posted here: https://sourceware.org/pipermail/gdb-patches/2020-June/169820.html I've used the style name 'version' instead of 'startup' to reflect what the style is actually used for. If other parts of the startup text end up being highlighted I imagine they would get their own styles based on what is being highlighted. I feel this is more inline with the other style names that are already in use within GDB. I also decoupled adding this style from the idea of startup options, and the possibility of auto-saving startup options. Those ideas can be explored in later patches. gdb/ChangeLog: * cli/cli-style.c: Add 'cli/cli-setshow.h' include. (version_style): Define. (cli_style_option::cli_style_option): Add intensity parameter, and use as appropriate. (_initialize_cli_style): Register version style set/show commands. * cli/cli-style.h (cli_style_option): Add intensity parameter. (version_style): Declare. * top.c (print_gdb_version): Use version_stype, and styled_string to print the GDB version string. gdb/doc/ChangeLog: * gdb.texinfo (Output Styling): Document version style. gdb/testsuite/ChangeLog: * gdb.base/style.exp (run_style_tests): Add version string test. (test_startup_version_string): Use version style name. * lib/gdb-utils.exp (style): Handle version style name. --- gdb/ChangeLog | 12 ++++++++++++ gdb/NEWS | 5 +++++ gdb/cli/cli-style.c | 17 +++++++++++++++-- gdb/cli/cli-style.h | 6 +++++- gdb/doc/ChangeLog | 4 ++++ gdb/doc/gdb.texinfo | 12 ++++++++++++ gdb/testsuite/ChangeLog | 6 ++++++ gdb/testsuite/gdb.base/style.exp | 8 +++++++- gdb/testsuite/lib/gdb-utils.exp | 1 + gdb/top.c | 11 +++-------- 10 files changed, 70 insertions(+), 12 deletions(-) diff --git a/gdb/NEWS b/gdb/NEWS index 66702862efb..0ba0c0b5ea2 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -171,6 +171,11 @@ show exec-file-mismatch -- Show exec-file-mismatch handling (ask|warn|off). executable file; if 'warn', just display a warning; if 'off', don't attempt to detect a mismatch. +set style version foreground COLOR +set style version background COLOR +set style version intensity VALUE + Control the styling of GDB's version number text. + tui new-layout NAME WINDOW WEIGHT [WINDOW WEIGHT]... Define a new TUI layout, specifying its name and the windows that will be displayed. diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c index cbedd30ea74..8b4b6b24cda 100644 --- a/gdb/cli/cli-style.c +++ b/gdb/cli/cli-style.c @@ -19,6 +19,7 @@ #include "defs.h" #include "cli/cli-cmds.h" +#include "cli/cli-setshow.h" #include "cli/cli-style.h" #include "source-cache.h" #include "observable.h" @@ -98,13 +99,19 @@ cli_style_option metadata_style ("metadata", ui_file_style::DIM); /* See cli-style.h. */ +cli_style_option version_style ("version", ui_file_style::MAGENTA, + ui_file_style::BOLD); + +/* See cli-style.h. */ + cli_style_option::cli_style_option (const char *name, - ui_file_style::basic_color fg) + ui_file_style::basic_color fg, + ui_file_style::intensity intensity) : changed (name), m_name (name), m_foreground (cli_colors[fg - ui_file_style::NONE]), m_background (cli_colors[0]), - m_intensity (cli_intensities[ui_file_style::NORMAL]) + m_intensity (cli_intensities[intensity]) { } @@ -382,4 +389,10 @@ TUI window that does have the focus."), &style_set_list, &style_show_list, true); + + version_style.add_setshow_commands (no_class, _("\ +Version string display styling.\n\ +Configure colors used to display the GDB version string."), + &style_set_list, &style_show_list, + false); } diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h index cd51bf4aa94..187e1d07ce7 100644 --- a/gdb/cli/cli-style.h +++ b/gdb/cli/cli-style.h @@ -30,7 +30,8 @@ class cli_style_option public: /* Construct a CLI style option with a foreground color. */ - cli_style_option (const char *name, ui_file_style::basic_color fg); + cli_style_option (const char *name, ui_file_style::basic_color fg, + ui_file_style::intensity = ui_file_style::NORMAL); /* Construct a CLI style option with an intensity. */ cli_style_option (const char *name, ui_file_style::intensity i); @@ -124,6 +125,9 @@ extern cli_style_option tui_border_style; /* The border style of a TUI window that does have the focus. */ extern cli_style_option tui_active_border_style; +/* The style to use for the GDB version string. */ +extern cli_style_option version_style; + /* True if source styling is enabled. */ extern bool source_styling; diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 69fa6b709bf..37f97bfb2b2 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -25872,6 +25872,18 @@ @code{set style address} family of commands. By default, this style's foreground color is blue. +@item version +Control the styling of @value{GDBN}'s version number text. By +default, this style's foreground color is magenta and it has bold +intensity. The version number is displayed in two placed, the output +of @command{show version}, and when @value{GDBN} starts up. + +Currently the version string displayed at startup is printed before +@value{GDBN} has parsed any command line options, or parsed any +command files, so there is currently no way to control the styling of +this string. However, @value{GDBN}'s @code{--quiet} command line option +can be used to disable printing of the version string on startup. + @item title Control the styling of titles. These are managed with the @code{set style title} family of commands. By default, this style's diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp index 58c8da0a10e..dfbadd6c1af 100644 --- a/gdb/testsuite/gdb.base/style.exp +++ b/gdb/testsuite/gdb.base/style.exp @@ -264,6 +264,12 @@ proc run_style_tests { } { gdb_test "p 5" \ "Warning: '[style p title]', an alias for the command '[style print title]', is deprecated.*Use '[style new_p title]'.*" \ "p deprecated warning, with replacement" + + # Check that the version string is styled in the output of 'show + # version', and that this styling can be disabled. + set vers [style "GNU gdb.*" version] + gdb_test "show version" "${vers}.*" \ + "version is styled in 'show version'" } } @@ -274,7 +280,7 @@ proc test_startup_version_string { } { gdb_exit gdb_spawn - set vers [style "GNU gdb.*" "35;1"] + set vers [style "GNU gdb.*" version] gdb_test "" "${vers}.*" "version is styled at startup" } diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp index 000e3800cd9..ad7d2884aae 100644 --- a/gdb/testsuite/lib/gdb-utils.exp +++ b/gdb/testsuite/lib/gdb-utils.exp @@ -55,6 +55,7 @@ proc style {str style} { variable { set style 36 } address { set style 34 } metadata { set style 2 } + version { set style "35;1" } } return "\033\\\[${style}m${str}\033\\\[m" } diff --git a/gdb/top.c b/gdb/top.c index 2c13864e120..92090bccbf4 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -1395,14 +1395,9 @@ print_gdb_version (struct ui_file *stream, bool interactive) program to parse, and is just canonical program name and version number, which starts after last space. */ - ui_file_style style; - if (interactive) - { - ui_file_style nstyle = { ui_file_style::MAGENTA, ui_file_style::NONE, - ui_file_style::BOLD }; - style = nstyle; - } - fprintf_styled (stream, style, "GNU gdb %s%s\n", PKGVERSION, version); + std::string v_str = string_printf ("GNU gdb %s%s", PKGVERSION, version); + fprintf_filtered (stream, "%ps\n", + styled_string (version_style.style (), v_str.c_str ())); /* Second line is a copyright notice. */ -- 2.25.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] gdb: add new version style 2021-01-14 9:56 ` [PATCH 2/2] gdb: add new version style Andrew Burgess @ 2021-01-14 10:35 ` Andrew Burgess 2021-01-14 11:15 ` Lancelot SIX ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2021-01-14 10:35 UTC (permalink / raw) To: gdb-patches * Andrew Burgess <andrew.burgess@embecosm.com> [2021-01-14 09:56:43 +0000]: > This commit adds a new 'version' style, which replaces the hard coded > styling currently used for GDB's version string. GDB's version number > is displayed: > > 1. In the output of 'show version', and > > 2. When GDB starts up (without the --quiet option). > > This new style can only ever affect the first of these two cases as > the second case is printed before GDB has processed any initialization > files, or processed any GDB commands passed on the command line. > > However, because the first case exists I think this commit makes > sense, it means the style is no longer hard coded into GDB, and we can > add some tests that the style can be enabled/disabled correctly. > > This commit is an alternative to a patch Tom posted here: > > https://sourceware.org/pipermail/gdb-patches/2020-June/169820.html > > I've used the style name 'version' instead of 'startup' to reflect > what the style is actually used for. If other parts of the startup > text end up being highlighted I imagine they would get their own > styles based on what is being highlighted. I feel this is more inline > with the other style names that are already in use within GDB. > > I also decoupled adding this style from the idea of startup options, > and the possibility of auto-saving startup options. Those ideas can > be explored in later patches. > > gdb/ChangeLog: > > * cli/cli-style.c: Add 'cli/cli-setshow.h' include. > (version_style): Define. > (cli_style_option::cli_style_option): Add intensity parameter, and > use as appropriate. > (_initialize_cli_style): Register version style set/show commands. > * cli/cli-style.h (cli_style_option): Add intensity parameter. > (version_style): Declare. > * top.c (print_gdb_version): Use version_stype, and styled_string > to print the GDB version string. I realise I'm missing a ChangeLog entry for NEWS here. I also realise that the change I made to ChangeLog is in the wrong place. The rest of the patch should be OK though. Thanks, Andrew > > gdb/doc/ChangeLog: > > * gdb.texinfo (Output Styling): Document version style. > > gdb/testsuite/ChangeLog: > > * gdb.base/style.exp (run_style_tests): Add version string test. > (test_startup_version_string): Use version style name. > * lib/gdb-utils.exp (style): Handle version style name. > --- > gdb/ChangeLog | 12 ++++++++++++ > gdb/NEWS | 5 +++++ > gdb/cli/cli-style.c | 17 +++++++++++++++-- > gdb/cli/cli-style.h | 6 +++++- > gdb/doc/ChangeLog | 4 ++++ > gdb/doc/gdb.texinfo | 12 ++++++++++++ > gdb/testsuite/ChangeLog | 6 ++++++ > gdb/testsuite/gdb.base/style.exp | 8 +++++++- > gdb/testsuite/lib/gdb-utils.exp | 1 + > gdb/top.c | 11 +++-------- > 10 files changed, 70 insertions(+), 12 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index 66702862efb..0ba0c0b5ea2 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -171,6 +171,11 @@ show exec-file-mismatch -- Show exec-file-mismatch handling (ask|warn|off). > executable file; if 'warn', just display a warning; if 'off', don't > attempt to detect a mismatch. > > +set style version foreground COLOR > +set style version background COLOR > +set style version intensity VALUE > + Control the styling of GDB's version number text. > + > tui new-layout NAME WINDOW WEIGHT [WINDOW WEIGHT]... > Define a new TUI layout, specifying its name and the windows that > will be displayed. > diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c > index cbedd30ea74..8b4b6b24cda 100644 > --- a/gdb/cli/cli-style.c > +++ b/gdb/cli/cli-style.c > @@ -19,6 +19,7 @@ > > #include "defs.h" > #include "cli/cli-cmds.h" > +#include "cli/cli-setshow.h" > #include "cli/cli-style.h" > #include "source-cache.h" > #include "observable.h" > @@ -98,13 +99,19 @@ cli_style_option metadata_style ("metadata", ui_file_style::DIM); > > /* See cli-style.h. */ > > +cli_style_option version_style ("version", ui_file_style::MAGENTA, > + ui_file_style::BOLD); > + > +/* See cli-style.h. */ > + > cli_style_option::cli_style_option (const char *name, > - ui_file_style::basic_color fg) > + ui_file_style::basic_color fg, > + ui_file_style::intensity intensity) > : changed (name), > m_name (name), > m_foreground (cli_colors[fg - ui_file_style::NONE]), > m_background (cli_colors[0]), > - m_intensity (cli_intensities[ui_file_style::NORMAL]) > + m_intensity (cli_intensities[intensity]) > { > } > > @@ -382,4 +389,10 @@ TUI window that does have the focus."), > &style_set_list, > &style_show_list, > true); > + > + version_style.add_setshow_commands (no_class, _("\ > +Version string display styling.\n\ > +Configure colors used to display the GDB version string."), > + &style_set_list, &style_show_list, > + false); > } > diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h > index cd51bf4aa94..187e1d07ce7 100644 > --- a/gdb/cli/cli-style.h > +++ b/gdb/cli/cli-style.h > @@ -30,7 +30,8 @@ class cli_style_option > public: > > /* Construct a CLI style option with a foreground color. */ > - cli_style_option (const char *name, ui_file_style::basic_color fg); > + cli_style_option (const char *name, ui_file_style::basic_color fg, > + ui_file_style::intensity = ui_file_style::NORMAL); > > /* Construct a CLI style option with an intensity. */ > cli_style_option (const char *name, ui_file_style::intensity i); > @@ -124,6 +125,9 @@ extern cli_style_option tui_border_style; > /* The border style of a TUI window that does have the focus. */ > extern cli_style_option tui_active_border_style; > > +/* The style to use for the GDB version string. */ > +extern cli_style_option version_style; > + > /* True if source styling is enabled. */ > extern bool source_styling; > > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 69fa6b709bf..37f97bfb2b2 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -25872,6 +25872,18 @@ > @code{set style address} family of commands. By default, this style's > foreground color is blue. > > +@item version > +Control the styling of @value{GDBN}'s version number text. By > +default, this style's foreground color is magenta and it has bold > +intensity. The version number is displayed in two placed, the output > +of @command{show version}, and when @value{GDBN} starts up. > + > +Currently the version string displayed at startup is printed before > +@value{GDBN} has parsed any command line options, or parsed any > +command files, so there is currently no way to control the styling of > +this string. However, @value{GDBN}'s @code{--quiet} command line option > +can be used to disable printing of the version string on startup. > + > @item title > Control the styling of titles. These are managed with the > @code{set style title} family of commands. By default, this style's > diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp > index 58c8da0a10e..dfbadd6c1af 100644 > --- a/gdb/testsuite/gdb.base/style.exp > +++ b/gdb/testsuite/gdb.base/style.exp > @@ -264,6 +264,12 @@ proc run_style_tests { } { > gdb_test "p 5" \ > "Warning: '[style p title]', an alias for the command '[style print title]', is deprecated.*Use '[style new_p title]'.*" \ > "p deprecated warning, with replacement" > + > + # Check that the version string is styled in the output of 'show > + # version', and that this styling can be disabled. > + set vers [style "GNU gdb.*" version] > + gdb_test "show version" "${vers}.*" \ > + "version is styled in 'show version'" > } > } > > @@ -274,7 +280,7 @@ proc test_startup_version_string { } { > gdb_exit > gdb_spawn > > - set vers [style "GNU gdb.*" "35;1"] > + set vers [style "GNU gdb.*" version] > gdb_test "" "${vers}.*" "version is styled at startup" > } > > diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp > index 000e3800cd9..ad7d2884aae 100644 > --- a/gdb/testsuite/lib/gdb-utils.exp > +++ b/gdb/testsuite/lib/gdb-utils.exp > @@ -55,6 +55,7 @@ proc style {str style} { > variable { set style 36 } > address { set style 34 } > metadata { set style 2 } > + version { set style "35;1" } > } > return "\033\\\[${style}m${str}\033\\\[m" > } > diff --git a/gdb/top.c b/gdb/top.c > index 2c13864e120..92090bccbf4 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -1395,14 +1395,9 @@ print_gdb_version (struct ui_file *stream, bool interactive) > program to parse, and is just canonical program name and version > number, which starts after last space. */ > > - ui_file_style style; > - if (interactive) > - { > - ui_file_style nstyle = { ui_file_style::MAGENTA, ui_file_style::NONE, > - ui_file_style::BOLD }; > - style = nstyle; > - } > - fprintf_styled (stream, style, "GNU gdb %s%s\n", PKGVERSION, version); > + std::string v_str = string_printf ("GNU gdb %s%s", PKGVERSION, version); > + fprintf_filtered (stream, "%ps\n", > + styled_string (version_style.style (), v_str.c_str ())); > > /* Second line is a copyright notice. */ > > -- > 2.25.4 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] gdb: add new version style 2021-01-14 9:56 ` [PATCH 2/2] gdb: add new version style Andrew Burgess 2021-01-14 10:35 ` Andrew Burgess @ 2021-01-14 11:15 ` Lancelot SIX 2021-01-20 19:39 ` Tom Tromey 2021-01-14 14:08 ` Eli Zaretskii 2021-01-20 19:39 ` Tom Tromey 3 siblings, 1 reply; 13+ messages in thread From: Lancelot SIX @ 2021-01-14 11:15 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches Le Thu, Jan 14, 2021 at 09:56:43AM +0000, Andrew Burgess a écrit : > This commit adds a new 'version' style, which replaces the hard coded > styling currently used for GDB's version string. GDB's version number > is displayed: > > 1. In the output of 'show version', and > > 2. When GDB starts up (without the --quiet option). > > This new style can only ever affect the first of these two cases as > the second case is printed before GDB has processed any initialization > files, or processed any GDB commands passed on the command line. > > However, because the first case exists I think this commit makes > sense, it means the style is no longer hard coded into GDB, and we can > add some tests that the style can be enabled/disabled correctly. > > This commit is an alternative to a patch Tom posted here: > > https://sourceware.org/pipermail/gdb-patches/2020-June/169820.html > > I've used the style name 'version' instead of 'startup' to reflect > what the style is actually used for. If other parts of the startup > text end up being highlighted I imagine they would get their own > styles based on what is being highlighted. I feel this is more inline > with the other style names that are already in use within GDB. > > I also decoupled adding this style from the idea of startup options, > and the possibility of auto-saving startup options. Those ideas can > be explored in later patches. > > gdb/ChangeLog: > > * cli/cli-style.c: Add 'cli/cli-setshow.h' include. > (version_style): Define. > (cli_style_option::cli_style_option): Add intensity parameter, and > use as appropriate. > (_initialize_cli_style): Register version style set/show commands. > * cli/cli-style.h (cli_style_option): Add intensity parameter. > (version_style): Declare. > * top.c (print_gdb_version): Use version_stype, and styled_string > to print the GDB version string. > > gdb/doc/ChangeLog: > > * gdb.texinfo (Output Styling): Document version style. > > gdb/testsuite/ChangeLog: > > * gdb.base/style.exp (run_style_tests): Add version string test. > (test_startup_version_string): Use version style name. > * lib/gdb-utils.exp (style): Handle version style name. > --- > gdb/ChangeLog | 12 ++++++++++++ > gdb/NEWS | 5 +++++ > gdb/cli/cli-style.c | 17 +++++++++++++++-- > gdb/cli/cli-style.h | 6 +++++- > gdb/doc/ChangeLog | 4 ++++ > gdb/doc/gdb.texinfo | 12 ++++++++++++ > gdb/testsuite/ChangeLog | 6 ++++++ > gdb/testsuite/gdb.base/style.exp | 8 +++++++- > gdb/testsuite/lib/gdb-utils.exp | 1 + > gdb/top.c | 11 +++-------- > 10 files changed, 70 insertions(+), 12 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index 66702862efb..0ba0c0b5ea2 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -171,6 +171,11 @@ show exec-file-mismatch -- Show exec-file-mismatch handling (ask|warn|off). > executable file; if 'warn', just display a warning; if 'off', don't > attempt to detect a mismatch. > > +set style version foreground COLOR > +set style version background COLOR > +set style version intensity VALUE > + Control the styling of GDB's version number text. > + > tui new-layout NAME WINDOW WEIGHT [WINDOW WEIGHT]... > Define a new TUI layout, specifying its name and the windows that > will be displayed. > diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c > index cbedd30ea74..8b4b6b24cda 100644 > --- a/gdb/cli/cli-style.c > +++ b/gdb/cli/cli-style.c > @@ -19,6 +19,7 @@ > > #include "defs.h" > #include "cli/cli-cmds.h" > +#include "cli/cli-setshow.h" > #include "cli/cli-style.h" > #include "source-cache.h" > #include "observable.h" > @@ -98,13 +99,19 @@ cli_style_option metadata_style ("metadata", ui_file_style::DIM); > > /* See cli-style.h. */ > > +cli_style_option version_style ("version", ui_file_style::MAGENTA, > + ui_file_style::BOLD); > + > +/* See cli-style.h. */ > + > cli_style_option::cli_style_option (const char *name, > - ui_file_style::basic_color fg) > + ui_file_style::basic_color fg, > + ui_file_style::intensity intensity) > : changed (name), > m_name (name), > m_foreground (cli_colors[fg - ui_file_style::NONE]), > m_background (cli_colors[0]), > - m_intensity (cli_intensities[ui_file_style::NORMAL]) > + m_intensity (cli_intensities[intensity]) > { > } > > @@ -382,4 +389,10 @@ TUI window that does have the focus."), > &style_set_list, > &style_show_list, > true); > + > + version_style.add_setshow_commands (no_class, _("\ > +Version string display styling.\n\ > +Configure colors used to display the GDB version string."), > + &style_set_list, &style_show_list, > + false); > } > diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h > index cd51bf4aa94..187e1d07ce7 100644 > --- a/gdb/cli/cli-style.h > +++ b/gdb/cli/cli-style.h > @@ -30,7 +30,8 @@ class cli_style_option > public: > > /* Construct a CLI style option with a foreground color. */ > - cli_style_option (const char *name, ui_file_style::basic_color fg); > + cli_style_option (const char *name, ui_file_style::basic_color fg, > + ui_file_style::intensity = ui_file_style::NORMAL); > > /* Construct a CLI style option with an intensity. */ > cli_style_option (const char *name, ui_file_style::intensity i); > @@ -124,6 +125,9 @@ extern cli_style_option tui_border_style; > /* The border style of a TUI window that does have the focus. */ > extern cli_style_option tui_active_border_style; > > +/* The style to use for the GDB version string. */ > +extern cli_style_option version_style; > + > /* True if source styling is enabled. */ > extern bool source_styling; > > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 69fa6b709bf..37f97bfb2b2 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -25872,6 +25872,18 @@ > @code{set style address} family of commands. By default, this style's > foreground color is blue. > > +@item version > +Control the styling of @value{GDBN}'s version number text. By > +default, this style's foreground color is magenta and it has bold > +intensity. The version number is displayed in two placed, the output s/placed/places/ > +of @command{show version}, and when @value{GDBN} starts up. > + > +Currently the version string displayed at startup is printed before > +@value{GDBN} has parsed any command line options, or parsed any > +command files, so there is currently no way to control the styling of > +this string. However, @value{GDBN}'s @code{--quiet} command line option > +can be used to disable printing of the version string on startup. > + > @item title > Control the styling of titles. These are managed with the > @code{set style title} family of commands. By default, this style's > diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp > index 58c8da0a10e..dfbadd6c1af 100644 > --- a/gdb/testsuite/gdb.base/style.exp > +++ b/gdb/testsuite/gdb.base/style.exp > @@ -264,6 +264,12 @@ proc run_style_tests { } { > gdb_test "p 5" \ > "Warning: '[style p title]', an alias for the command '[style print title]', is deprecated.*Use '[style new_p title]'.*" \ > "p deprecated warning, with replacement" > + > + # Check that the version string is styled in the output of 'show > + # version', and that this styling can be disabled. > + set vers [style "GNU gdb.*" version] > + gdb_test "show version" "${vers}.*" \ > + "version is styled in 'show version'" > } > } > > @@ -274,7 +280,7 @@ proc test_startup_version_string { } { > gdb_exit > gdb_spawn > > - set vers [style "GNU gdb.*" "35;1"] > + set vers [style "GNU gdb.*" version] > gdb_test "" "${vers}.*" "version is styled at startup" > } > > diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp > index 000e3800cd9..ad7d2884aae 100644 > --- a/gdb/testsuite/lib/gdb-utils.exp > +++ b/gdb/testsuite/lib/gdb-utils.exp > @@ -55,6 +55,7 @@ proc style {str style} { > variable { set style 36 } > address { set style 34 } > metadata { set style 2 } > + version { set style "35;1" } > } > return "\033\\\[${style}m${str}\033\\\[m" > } > diff --git a/gdb/top.c b/gdb/top.c > index 2c13864e120..92090bccbf4 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -1395,14 +1395,9 @@ print_gdb_version (struct ui_file *stream, bool interactive) > program to parse, and is just canonical program name and version > number, which starts after last space. */ > > - ui_file_style style; > - if (interactive) > - { > - ui_file_style nstyle = { ui_file_style::MAGENTA, ui_file_style::NONE, > - ui_file_style::BOLD }; > - style = nstyle; > - } > - fprintf_styled (stream, style, "GNU gdb %s%s\n", PKGVERSION, version); > + std::string v_str = string_printf ("GNU gdb %s%s", PKGVERSION, version); > + fprintf_filtered (stream, "%ps\n", > + styled_string (version_style.style (), v_str.c_str ())); > > /* Second line is a copyright notice. */ > > -- > 2.25.4 > It looks like this patch could be enough to closes the following ticket: https://sourceware.org/bugzilla/show_bug.cgi?id=25956 (with the limitation that this patch makes the version number configurable… after it has been displayed ; but at least the style is not hard-coded anymore). I tried to go through the remaining code and did not find other hard-coded styles (but I might have missed some). Lancelot. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] gdb: add new version style 2021-01-14 11:15 ` Lancelot SIX @ 2021-01-20 19:39 ` Tom Tromey 0 siblings, 0 replies; 13+ messages in thread From: Tom Tromey @ 2021-01-20 19:39 UTC (permalink / raw) To: Lancelot SIX via Gdb-patches; +Cc: Andrew Burgess, Lancelot SIX Lancelot> It looks like this patch could be enough to closes the following ticket: Lancelot> https://sourceware.org/bugzilla/show_bug.cgi?id=25956 (with the Lancelot> limitation that this patch makes the version number configurable… after Lancelot> it has been displayed ; but at least the style is not hard-coded anymore). Lancelot> I tried to go through the remaining code and did not find other Lancelot> hard-coded styles (but I might have missed some). I believe this is the only one. Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] gdb: add new version style 2021-01-14 9:56 ` [PATCH 2/2] gdb: add new version style Andrew Burgess 2021-01-14 10:35 ` Andrew Burgess 2021-01-14 11:15 ` Lancelot SIX @ 2021-01-14 14:08 ` Eli Zaretskii 2021-01-20 19:39 ` Tom Tromey 3 siblings, 0 replies; 13+ messages in thread From: Eli Zaretskii @ 2021-01-14 14:08 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches > From: Andrew Burgess <andrew.burgess@embecosm.com> > Date: Thu, 14 Jan 2021 09:56:43 +0000 > > +@item version > +Control the styling of @value{GDBN}'s version number text. By > +default, this style's foreground color is magenta and it has bold > +intensity. The version number is displayed in two placed, the output ^^^^^^ "places" The documentation parts are okay with this nit fixed. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] gdb: add new version style 2021-01-14 9:56 ` [PATCH 2/2] gdb: add new version style Andrew Burgess ` (2 preceding siblings ...) 2021-01-14 14:08 ` Eli Zaretskii @ 2021-01-20 19:39 ` Tom Tromey 3 siblings, 0 replies; 13+ messages in thread From: Tom Tromey @ 2021-01-20 19:39 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: Andrew> gdb/ChangeLog: Andrew> * cli/cli-style.c: Add 'cli/cli-setshow.h' include. Andrew> (version_style): Define. Andrew> (cli_style_option::cli_style_option): Add intensity parameter, and Andrew> use as appropriate. Andrew> (_initialize_cli_style): Register version style set/show commands. Andrew> * cli/cli-style.h (cli_style_option): Add intensity parameter. Andrew> (version_style): Declare. Andrew> * top.c (print_gdb_version): Use version_stype, and styled_string Andrew> to print the GDB version string. Andrew> gdb/doc/ChangeLog: Andrew> * gdb.texinfo (Output Styling): Document version style. Andrew> gdb/testsuite/ChangeLog: Andrew> * gdb.base/style.exp (run_style_tests): Add version string test. Andrew> (test_startup_version_string): Use version style name. Andrew> * lib/gdb-utils.exp (style): Handle version style name. This looks good, thanks. Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-01-22 15:26 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-14 9:56 [PATCH 0/2] Style related fixes Andrew Burgess 2021-01-14 9:56 ` [PATCH 1/2] gdb: don't print escape characters when a style is disabled Andrew Burgess 2021-01-20 19:34 ` Tom Tromey 2021-01-21 12:06 ` Andrew Burgess 2021-01-21 16:08 ` Andrew Burgess 2021-01-22 15:26 ` Tom Tromey 2021-01-22 15:21 ` Tom Tromey 2021-01-14 9:56 ` [PATCH 2/2] gdb: add new version style Andrew Burgess 2021-01-14 10:35 ` Andrew Burgess 2021-01-14 11:15 ` Lancelot SIX 2021-01-20 19:39 ` Tom Tromey 2021-01-14 14:08 ` Eli Zaretskii 2021-01-20 19:39 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).