[PATCH] Fixed pretty printing max depth behavior
Kent Cheung
kent.cheung@arm.com
Tue Jul 21 13:29:26 GMT 2020
On 7/20/20 6:21 PM, Andrew Burgess wrote:
> * Kent Cheung <kent.cheung@arm.com> [2020-07-20 11:49:34 +0100]:
>
>> The 'print max-depth' feature incorrectly causes GDB to skip printing
>> the string representation of pretty printed variables if the variable is
>> stored at a nested depth corresponding to the set max-depth value. This
>> change ensures that it is always printed before checking whether the
>> maximum print depth has been reached.
>>
>> Regression tested with GCC 7.3.0 on x86_64, ppc64le, aarch64.
>>
>> gdb/ChangeLog:
>>
>> 2020-07-17 Kent Cheung <kent.cheung@arm.com>
>>
>> * cp-valprint.c (cp_print_value): Replaced duplicate code.
>> * guile/scm-pretty-print.c (ppscm_print_children): Check
>> max_depth just before printing child values.
>> (gdbscm_apply_val_pretty_printer): Don't check max_depth before
>> printing string representation.
>> * python/py-prettyprint.c (print_children): Check max_depth just
>> before printing child values.
>> (gdbpy_apply_val_pretty_printer): Don't check max_depth before
>> printing string representation.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 2020-07-17 Kent Cheung <kent.cheung@arm.com>
>>
>> * gdb.python/py-format-string.c: Added a variable to test.
>> * gdb.python/py-format-string.exp: Check string representation
>> is printed at appropriate max_depth settings.
>> * gdb.python/py-nested-maps.exp: Same.
> Thanks for looking at this. I just had one query...
>
>> Change-Id: Ic4f8734361ab9d262c9468f3db929a8d18462136
>> ---
>> gdb/ChangeLog | 12 ++++++++
>> gdb/cp-valprint.c | 10 ++-----
>> gdb/guile/scm-pretty-print.c | 30 ++++++++++---------
>> gdb/python/py-prettyprint.c | 28 +++++++++--------
>> gdb/testsuite/ChangeLog | 7 +++++
>> gdb/testsuite/gdb.python/py-format-string.c | 6 ++++
>> gdb/testsuite/gdb.python/py-format-string.exp | 9 ++++++
>> gdb/testsuite/gdb.python/py-nested-maps.exp | 6 ++--
>> 8 files changed, 71 insertions(+), 37 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 6d231f5d94..70f1dc8a7c 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,15 @@
>> +2020-07-17 Kent Cheung <kent.cheung@arm.com>
>> +
>> + * cp-valprint.c (cp_print_value): Replaced duplicate code.
>> + * guile/scm-pretty-print.c (ppscm_print_children): Check max_depth
>> + just before printing child values.
>> + (gdbscm_apply_val_pretty_printer): Don't check max_depth before
>> + printing string representation.
>> + * python/py-prettyprint.c (print_children): Check max_depth just
>> + before printing child values.
>> + (gdbpy_apply_val_pretty_printer): Don't check max_depth before
>> + printing string representation.
>> +
>> 2020-07-18 Tom Tromey <tom@tromey.com>
>>
>> * linux-nat.c (linux_multi_process): Remove.
>> diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
>> index a02fee6b55..aae8041c85 100644
>> --- a/gdb/cp-valprint.c
>> +++ b/gdb/cp-valprint.c
>> @@ -495,14 +495,8 @@ cp_print_value (struct value *val, struct ui_file *stream,
>> {
>> int result = 0;
>>
>> - if (options->max_depth > -1
>> - && recurse >= options->max_depth)
>> - {
>> - const struct language_defn *language = current_language;
>> - gdb_assert (language->la_struct_too_deep_ellipsis != NULL);
>> - fputs_filtered (language->la_struct_too_deep_ellipsis, stream);
>> - }
>> - else
>> + if (!val_print_check_max_depth (stream, recurse, options,
>> + current_language))
>> {
>> struct value *baseclass_val = value_primitive_field (val, 0,
>> i, type);
>> diff --git a/gdb/guile/scm-pretty-print.c b/gdb/guile/scm-pretty-print.c
>> index ccc6164451..ec94d4d53c 100644
>> --- a/gdb/guile/scm-pretty-print.c
>> +++ b/gdb/guile/scm-pretty-print.c
>> @@ -818,21 +818,29 @@ ppscm_print_children (SCM printer, enum display_hint hint,
>> gdb::unique_xmalloc_ptr<char> name
>> = gdbscm_scm_to_c_string (scm_name);
>>
>> - /* Print initial "{". For other elements, there are three cases:
>> + /* Print initial "=" to separate print_string_repr output and
>> + children. For other elements, there are three cases:
>> 1. Maps. Print a "," after each value element.
>> 2. Arrays. Always print a ",".
>> 3. Other. Always print a ",". */
>> if (i == 0)
>> - {
>> - if (printed_nothing)
>> - fputs_filtered ("{", stream);
>> - else
>> - fputs_filtered (" = {", stream);
>> - }
>> -
>> + {
>> + if (!printed_nothing)
>> + fputs_filtered (" = ", stream);
>> + }
>> else if (! is_map || i % 2 == 0)
>> fputs_filtered (pretty ? "," : ", ", stream);
>>
>> + /* Skip printing children if max_depth has been reached. This check
>> + is performed after print_string_repr and the "=" separator so that
>> + these steps are not skipped if the variable is located within the
>> + permitted depth. */
>> + if (val_print_check_max_depth (stream, recurse, options, language))
>> + return;
> Unlike the python version, all the paths through this function seem to
> pass through the 'done' block at the end. I can't pretend I really
> know what's going on there, but I wonder if it would be better if we
> also made this path go through that block too?
>
> Looking at the following 'In summary mode...' logic, I'd be tempted to
> write:
>
> if (val_print_check_max_depth (stream, recurse, options, language))
> {
> /* Setting I to 0 tricks the post loop logic to not print
> anything. */
> i = 0;
> break;
> }
>
> What do you think?
>
> Otherwise, looks good.
>
> Thanks
> Andrew
Thanks for the comment. I think a goto is more appropriate here given
its use in the rest of this function. I will post v2 shortly.
Many thanks,
Kent
>
>> + else if (i == 0)
>> + /* Print initial "{" to bookend children. */
>> + fputs_filtered ("{", stream);
>> +
>> /* In summary mode, we just want to print "= {...}" if there is
>> a value. */
>> if (options->summary)
>> @@ -991,12 +999,6 @@ gdbscm_apply_val_pretty_printer (const struct extension_language_defn *extlang,
>> }
>> gdb_assert (ppscm_is_pretty_printer_worker (printer));
>>
>> - if (val_print_check_max_depth (stream, recurse, options, language))
>> - {
>> - result = EXT_LANG_RC_OK;
>> - goto done;
>> - }
>> -
>> /* If we are printing a map, we want some special formatting. */
>> hint = ppscm_get_display_hint_enum (printer);
>> if (hint == HINT_ERROR)
>> diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
>> index 7cb20df7f2..8da6b83036 100644
>> --- a/gdb/python/py-prettyprint.c
>> +++ b/gdb/python/py-prettyprint.c
>> @@ -431,22 +431,29 @@ print_children (PyObject *printer, const char *hint,
>> continue;
>> }
>>
>> - /* Print initial "{". For other elements, there are three
>> - cases:
>> + /* Print initial "=" to separate print_string_repr output and
>> + children. For other elements, there are three cases:
>> 1. Maps. Print a "," after each value element.
>> 2. Arrays. Always print a ",".
>> 3. Other. Always print a ",". */
>> if (i == 0)
>> - {
>> - if (is_py_none)
>> - fputs_filtered ("{", stream);
>> - else
>> - fputs_filtered (" = {", stream);
>> - }
>> -
>> + {
>> + if (!is_py_none)
>> + fputs_filtered (" = ", stream);
>> + }
>> else if (! is_map || i % 2 == 0)
>> fputs_filtered (pretty ? "," : ", ", stream);
>>
>> + /* Skip printing children if max_depth has been reached. This check
>> + is performed after print_string_repr and the "=" separator so that
>> + these steps are not skipped if the variable is located within the
>> + permitted depth. */
>> + if (val_print_check_max_depth (stream, recurse, options, language))
>> + return;
>> + else if (i == 0)
>> + /* Print initial "{" to bookend children. */
>> + fputs_filtered ("{", stream);
>> +
>> /* In summary mode, we just want to print "= {...}" if there is
>> a value. */
>> if (options->summary)
>> @@ -597,9 +604,6 @@ gdbpy_apply_val_pretty_printer (const struct extension_language_defn *extlang,
>> if (printer == Py_None)
>> return EXT_LANG_RC_NOP;
>>
>> - if (val_print_check_max_depth (stream, recurse, options, language))
>> - return EXT_LANG_RC_OK;
>> -
>> /* If we are printing a map, we want some special formatting. */
>> gdb::unique_xmalloc_ptr<char> hint (gdbpy_get_display_hint (printer.get ()));
>>
>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
>> index 045ac01745..0c7716ca2b 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,3 +1,10 @@
>> +2020-07-17 Kent Cheung <kent.cheung@arm.com>
>> +
>> + * gdb.python/py-format-string.c: Added a variable to test.
>> + * gdb.python/py-format-string.exp: Check string representation is
>> + printed at appropriate max_depth settings.
>> + * gdb.python/py-nested-maps.exp: Same.
>> +
>> 2020-07-20 Tom de Vries <tdevries@suse.de>
>>
>> * gdb.base/valgrind-infcall-2.exp: Handle printf unknown return type.
>> diff --git a/gdb/testsuite/gdb.python/py-format-string.c b/gdb/testsuite/gdb.python/py-format-string.c
>> index a771370fde..99b7982ebf 100644
>> --- a/gdb/testsuite/gdb.python/py-format-string.c
>> +++ b/gdb/testsuite/gdb.python/py-format-string.c
>> @@ -23,6 +23,11 @@ typedef struct point
>> int y;
>> } point_t;
>>
>> +typedef struct
>> +{
>> + point_t the_point;
>> +} struct_point_t;
>> +
>> typedef union
>> {
>> int an_int;
>> @@ -84,6 +89,7 @@ main ()
>> point_t &a_point_t_ref = a_point_t;
>> #endif
>> struct point another_point = { 123, 456 };
>> + struct_point_t a_struct_with_point = { a_point_t };
>>
>> struct_union_t a_struct_with_union;
>> /* Fill the union in an endianness-independent way. */
>> diff --git a/gdb/testsuite/gdb.python/py-format-string.exp b/gdb/testsuite/gdb.python/py-format-string.exp
>> index 792d60c09d..59a1eb94e2 100644
>> --- a/gdb/testsuite/gdb.python/py-format-string.exp
>> +++ b/gdb/testsuite/gdb.python/py-format-string.exp
>> @@ -126,6 +126,7 @@ set default_regexp_dict [dict create \
>> "a_point_t_pointer" $default_pointer_regexp \
>> "a_point_t_ref" "Pretty Point \\(42, 12\\)" \
>> "another_point" "Pretty Point \\(123, 456\\)" \
>> + "a_struct_with_point" "\\{the_point = Pretty Point \\(42, 12\\)\\}" \
>> "a_struct_with_union" "\\{the_union = \\{an_int = 707406378, a_char = 42 '\\*'\\}\\}" \
>> "an_enum" "ENUM_BAR" \
>> "a_string" "${default_pointer_regexp} \"hello world\"" \
>> @@ -679,18 +680,26 @@ proc test_max_depth {} {
>> set opts "max_depth=-1"
>> with_test_prefix $opts {
>> check_format_string "a_struct_with_union" $opts
>> + check_format_string "a_point_t" $opts "Pretty Point \\(42, 12\\)"
>> + check_format_string "a_struct_with_point" $opts
>> }
>> set opts "max_depth=0"
>> with_test_prefix $opts {
>> check_format_string "a_struct_with_union" $opts "\\{\.\.\.\\}"
>> + check_format_string "a_point_t" $opts "Pretty Point \\(42, 12\\)"
>> + check_format_string "a_struct_with_point" $opts "\\{\.\.\.\\}"
>> }
>> set opts "max_depth=1"
>> with_test_prefix $opts {
>> check_format_string "a_struct_with_union" $opts "\\{the_union = \\{\.\.\.\\}\\}"
>> + check_format_string "a_point_t" $opts "Pretty Point \\(42, 12\\)"
>> + check_format_string "a_struct_with_point" $opts
>> }
>> set opts "max_depth=2"
>> with_test_prefix $opts {
>> check_format_string "a_struct_with_union" $opts
>> + check_format_string "a_point_t" $opts "Pretty Point \\(42, 12\\)"
>> + check_format_string "a_struct_with_point" $opts
>> }
>> }
>>
>> diff --git a/gdb/testsuite/gdb.python/py-nested-maps.exp b/gdb/testsuite/gdb.python/py-nested-maps.exp
>> index 9e1fca58bf..c8717e7b90 100644
>> --- a/gdb/testsuite/gdb.python/py-nested-maps.exp
>> +++ b/gdb/testsuite/gdb.python/py-nested-maps.exp
>> @@ -222,15 +222,15 @@ with_test_prefix "headers=on" {
>> with_test_prefix "pretty=off" {
>> gdb_print_expr_at_depths "*m1" \
>> [list \
>> - "\{\\.\\.\\.\}" \
>> + "pp_map = \{\\.\\.\\.\}" \
>> "pp_map = \{\\\[\{a = 3, b = 4\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 4, b = 5\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 5, b = 6\}\\\] = \{\\.\\.\\.\}\}" \
>> "pp_map = \{\\\[\{a = 3, b = 4\}\\\] = \{x = 0, y = 1, z = 2\}, \\\[\{a = 4, b = 5\}\\\] = \{x = 3, y = 4, z = 5\}, \\\[\{a = 5, b = 6\}\\\] = \{x = 6, y = 7, z = 8\}\}" \
>> ]
>>
>> gdb_print_expr_at_depths "*mm" \
>> [list \
>> - "\{\\.\\.\\.\}" \
>> - "pp_map_map = \{\\\[$hex \"m1\"\\\] = \{\\.\\.\\.\}, \\\[$hex \"m2\"\\\] = \{\\.\\.\\.\}\}" \
>> + "pp_map_map = \{\\.\\.\\.\}" \
>> + "pp_map_map = \{\\\[$hex \"m1\"\\\] = pp_map = \{\\.\\.\\.\}, \\\[$hex \"m2\"\\\] = pp_map = \{\\.\\.\\.\}\}" \
>> "pp_map_map = \{\\\[$hex \"m1\"\\\] = pp_map = \{\\\[\{a = 3, b = 4\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 4, b = 5\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 5, b = 6\}\\\] = \{\\.\\.\\.\}\}, \\\[$hex \"m2\"\\\] = pp_map = \{\\\[\{a = 6, b = 7\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 7, b = 8\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 8, b = 9\}\\\] = \{\\.\\.\\.\}\}\}" \
>> "pp_map_map = \{\\\[$hex \"m1\"\\\] = pp_map = \{\\\[\{a = 3, b = 4\}\\\] = \{x = 0, y = 1, z = 2\}, \\\[\{a = 4, b = 5\}\\\] = \{x = 3, y = 4, z = 5\}, \\\[\{a = 5, b = 6\}\\\] = \{x = 6, y = 7, z = 8\}\}, \\\[$hex \"m2\"\\\] = pp_map = \{\\\[\{a = 6, b = 7\}\\\] = \{x = 9, y = 0, z = 1\}, \\\[\{a = 7, b = 8\}\\\] = \{x = 2, y = 3, z = 4\}, \\\[\{a = 8, b = 9\}\\\] = \{x = 5, y = 6, z = 7\}\}\}" \
>> ]
>> --
>> 2.23.0
>>
More information about the Gdb-patches
mailing list