Re: [PATCH v8 6/6] GDB/testsuite: Expand for character string limiting options

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi,

I noted a few minor comments, once they are fixed this LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

From my point of view, there's not need to send a new version.

> Index: src/gdb/testsuite/gdb.base/printcmds.exp
> ===================================================================
> --- src.orig/gdb/testsuite/gdb.base/printcmds.exp
> +++ src/gdb/testsuite/gdb.base/printcmds.exp
> @@ -451,11 +451,11 @@ proc test_print_all_chars {} {
>  # Test interaction of the number of print elements to print and the
>  # repeat count, set to the default of 10.
>  
> -proc test_print_repeats_10 {} {
> +proc test_print_repeats_10_one { setting } {

Please expand the doc to say what SETTING is.

> Index: src/gdb/testsuite/gdb.pascal/str-chars.exp
> ===================================================================
> --- /dev/null
> +++ src/gdb/testsuite/gdb.pascal/str-chars.exp
> @@ -0,0 +1,56 @@
> +# Copyright 2022 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/>.
> +
> +load_lib "pascal.exp"
> +
> +standard_testfile .pas
> +
> +if {[gdb_compile_pascal "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug ]] != "" } {
> +  return -1
> +}
> +
> +clean_restart ${testfile}
> +gdb_breakpoint ${srcfile}:[gdb_get_line_number "set breakpoint 1 here"]
> +
> +# Verify that "start" lands inside the right procedure.
> +if { [gdb_start_cmd] < 0 } {
> +    untested start
> +    return -1
> +}

Can you update this to use runto_main, mirroring what was done in this
commit?

https://gitlab.com/gnutools/binutils-gdb/-/commit/4a6bdfb9baa27e29151c7e97ae2abbe902f53638

> Index: src/gdb/testsuite/gdb.python/py-format-string.exp
> ===================================================================
> --- src.orig/gdb/testsuite/gdb.python/py-format-string.exp
> +++ src/gdb/testsuite/gdb.python/py-format-string.exp
> @@ -705,13 +705,13 @@ proc_with_prefix test_static_members {}
>  }
>  
>  # Test the max_elements option for gdb.Value.format_string.
> -proc_with_prefix test_max_elements {} {
> +proc test_max_string_one { setting unlimited } {

The comment above should be expanded a bit.

> @@ -722,8 +722,10 @@ proc_with_prefix test_max_elements {} {
>      check_format_string "a_binary_string" $opts
>      check_format_string "a_binary_string_array" $opts
>      check_format_string "a_big_string" $opts
> -    check_format_string "an_array" $opts
> -    check_format_string "an_array_with_repetition" $opts
> +    if { $setting == "elements"} {

Space before closing bracket (just for consistency with the opening
one).

> @@ -785,15 +791,24 @@ proc_with_prefix test_max_elements {} {
>      }
>    }
>  
> -  with_temp_option "set print elements 4" "set print elements 200" {
> +  with_temp_option "set print $setting 4" "set print $setting 200" {
>      check_format_string "a_string" "" \
>        "${default_pointer_regexp} \"hell\"..."
>      check_format_string "a_binary_string" "" \
>        "${default_pointer_regexp} \"hell\"..."
>      check_format_string "a_binary_string_array" "" \
>        "\"hell\"..."
> -    check_format_string "an_array_with_repetition" "" \
> -      "\\{1, 3 <repeats 12 times>...\\}"
> +    if { $setting == "elements"} {
> +      check_format_string "an_array_with_repetition" "" \
> +	"\\{1, 3 <repeats 12 times>...\\}"
> +    }
> +  }
> +}
> +
> +proc_with_prefix test_max_string {} {

Is the name (and the name of test_max_string_one) accurate?  Does it
test only strings, or strings and arrays?  Just wondering if
test_max_strings_and_arrays would be more accurate.

> +  foreach_with_prefix setting { "elements" "characters" } {

The foreach_with_prefix here is not really necessary, because all tests
under test_max_string_one are under a proc_with_prefix that depends on
the setting passed.  You can remove it if you want to make test names a
bit shorter, but I'm fine with leaving it as well for extra clarity.

> +    test_max_string_one $setting \
> +      [string map {elements 0 characters 4294967295} $setting]

Can you put a comment here and/or in test_max_string_one's comment,
explaining what the last parameter does?  It's not clear to me what is
done.

Simon