* [PATCH] Fixed pretty printing max depth behavior
@ 2020-07-20 10:49 Kent Cheung
2020-07-20 17:21 ` Andrew Burgess
0 siblings, 1 reply; 3+ messages in thread
From: Kent Cheung @ 2020-07-20 10:49 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, richard.bunt, Kent Cheung
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.
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;
+ 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fixed pretty printing max depth behavior
2020-07-20 10:49 [PATCH] Fixed pretty printing max depth behavior Kent Cheung
@ 2020-07-20 17:21 ` Andrew Burgess
2020-07-21 13:29 ` Kent Cheung
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Burgess @ 2020-07-20 17:21 UTC (permalink / raw)
To: Kent Cheung; +Cc: gdb-patches, nd
* 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
> + 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fixed pretty printing max depth behavior
2020-07-20 17:21 ` Andrew Burgess
@ 2020-07-21 13:29 ` Kent Cheung
0 siblings, 0 replies; 3+ messages in thread
From: Kent Cheung @ 2020-07-21 13:29 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, nd
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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-21 13:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-20 10:49 [PATCH] Fixed pretty printing max depth behavior Kent Cheung
2020-07-20 17:21 ` Andrew Burgess
2020-07-21 13:29 ` Kent Cheung
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).