[PATCH] Fixed pretty printing max depth behavior

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fixed pretty printing max depth behavior

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  <[hidden email]>

        * 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  <[hidden email]>

        * 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  <[hidden email]>
+
+ * 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  <[hidden email]>
 
  * 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  <[hidden email]>
+
+ * 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  <[hidden email]>
 
  * 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixed pretty printing max depth behavior

Andrew Burgess
* Kent Cheung <[hidden email]> [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  <[hidden email]>
>
>         * 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  <[hidden email]>
>
>         * 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  <[hidden email]>
> +
> + * 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  <[hidden email]>
>  
>   * 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  <[hidden email]>
> +
> + * 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  <[hidden email]>
>  
>   * 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
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixed pretty printing max depth behavior

Kent Cheung

On 7/20/20 6:21 PM, Andrew Burgess wrote:

> * Kent Cheung <[hidden email]> [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  <[hidden email]>
>>
>>          * 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  <[hidden email]>
>>
>>          * 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  <[hidden email]>
>> +
>> + * 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  <[hidden email]>
>>  
>>   * 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  <[hidden email]>
>> +
>> + * 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  <[hidden email]>
>>  
>>   * 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
>>