This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] gdb: Introduce 'print max-depth' feature
Noice!
Overall looks good to me.
I think you're missing tests for C++ inheritance (all of regular, multiple and
virtual).
E.g,. with:
struct A { int a; int x; };
struct B : A { int b; int x; };
struct C : B { int c; int x; };
C c;
Today we print:
(gdb) p c
$1 = {<B> = {<A> = {a = 0, x = 0}, b = 0, x = 0}, c = 0, x = 0}
I suppose max-depth affects this as well. But given no tests,
can't be sure. :-)
Some nits pointed out below.
On 03/27/2019 09:53 PM, Andrew Burgess wrote:
> Introduce a new print setting max-depth which can be set with 'set
> print max-depth DEPTH'. The default value of DEPTH is 20, but this
> can also be set to unlimited.
>
> When GDB is printing a value containing nested structures GDB will
> stop descending at depth DEPTH. Here is a small example:
>
> typedef struct s1 { int a; } s1;
> typedef struct s2 { s1 b; } s2;
> typedef struct s3 { s2 c; } s3;
> typedef struct s4 { s3 d; } s4;
>
> s4 var = { { { { 3 } } } };
>
> The following table shows how various depth settings effect printing
> of 'var':
s/effect/affect/
>
> | Depth Setting | Result of 'p var' |
> |---------------+--------------------------------|
> | Unlimited | $1 = {d = {c = {b = {a = 3}}}} |
> | 4 | $1 = {d = {c = {b = {a = 3}}}} |
> | 3 | $1 = {d = {c = {b = {...}}}} |
> | 2 | $1 = {d = {c = {...}}} |
> | 1 | $1 = {d = {...}} |
> | 0 | $1 = {...} |
>
> Only structures, unions, and arrays are replaced in this way, scalars
> and strings are not replaced.
>
> The replacement is counted from the level at which you print, not from
> the top level of the structure. So, consider the above example and
> this GDB session:
>
> (gdb) set print max-depth 2
> (gdb) p var
> $1 = {d = {c = {...}}}
> (gdb) p var.d
> $2 = {c = {b = {...}}}
> (gdb) p var.d.c
> $3 = {b = {a = 3}}
>
> Setting the max-depth to 2 doesn't prevent the user from exploring
> deeper into 'var' by asking for specific sub-fields to be printed.
>
> The motivation behind this feature is to try and give the user more
> control over how much is printed when examining large, complex data
> structures.
>
> The default max-depth of 20 means that there is a change in GDB's
> default behaviour. Someone printing a data structure with 20 levels
> of nesting will now see '{...}' instead of their data, they would need
> to adjust the max depth, or call print again naming a specific field
> in order to dig deeper into their data structure. If this is
> considered a problem then we could increase the default, or even make
> the default unlimited.
>
> This commit relies on the previous commit, which added a new field to
> the language structure, this new field was a string that contained the
> pattern that should be used when a structure/union/array is replaced
> in the output, this allows languages to use a syntax that is more
> appropriate, mostly this will be selecting the correct types of
> bracket '(...)' or '{...}', both of which are currently in use.
Could you say something about if/how this affects Python, Guile, and MI ?
> * GDB and GDBserver now support access to additional registers on
> diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
> index 443c3b06dac..71da2b19755 100644
> --- a/gdb/cp-valprint.c
> +++ b/gdb/cp-valprint.c
> @@ -272,10 +272,23 @@ cp_print_value_fields (struct type *type, struct type *real_type,
> current_language->la_language,
> DMGL_PARAMS | DMGL_ANSI);
> annotate_field_name_end ();
> +
> + /* We tweak various options in a few cases below. */
> + struct value_print_options options_copy = *options;
> + struct value_print_options *opts = &options_copy;
You could drop the redundant "struct".
> +
> /* Do not print leading '=' in case of anonymous
> unions. */
> if (strcmp (TYPE_FIELD_NAME (type, i), ""))
> fputs_filtered (" = ", stream);
> + else
> + {
> + /* If this is an anonymous field then we want to consider it
> + as though it is at its parents depth when it comes to the
"parent's"
> + max print depth. */
> + if (opts->max_depth != -1 && opts->max_depth < INT_MAX)
> + ++opts->max_depth;
> + }
> @defun pretty_printer.display_hint (self)
> diff --git a/gdb/guile/scm-pretty-print.c b/gdb/guile/scm-pretty-print.c
> index e5096944b68..219e0ef63ad 100644
> --- a/gdb/guile/scm-pretty-print.c
> +++ b/gdb/guile/scm-pretty-print.c
> @@ -744,6 +744,16 @@ ppscm_print_children (SCM printer, enum display_hint hint,
> return;
> }
>
> + if (options->max_depth > -1
> + && recurse >= options->max_depth)
> + {
> + if (language->la_language == language_fortran)
> + fputs_filtered ("(...)", stream);
> + else
> + fputs_filtered ("{...}", stream);
Shouldn't this be using la_struct_too_deep_ellipsis here too?
> + return;
> + }
> diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
> index e64d1f88af8..504644143f3 100644
> --- a/gdb/python/py-prettyprint.c
> +++ b/gdb/python/py-prettyprint.c
> @@ -361,6 +361,16 @@ print_children (PyObject *printer, const char *hint,
> if (! PyObject_HasAttr (printer, gdbpy_children_cst))
> return;
>
> + if (options->max_depth > -1
> + && recurse >= options->max_depth)
> + {
> + if (language->la_language == language_fortran)
> + fputs_filtered ("(...)", stream);
> + else
> + fputs_filtered ("{...}", stream);
Ditto.
> + return;
> + }
> +
> diff --git a/gdb/testsuite/gdb.base/max-depth.exp b/gdb/testsuite/gdb.base/max-depth.exp
> new file mode 100644
> index 00000000000..777f66fab4e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/max-depth.exp
> +proc compile_and_run_tests { lang } {
> + global testfile
> + global srcfile
> + global binfile
> + global subdir
> + global srcdir
> +
> + standard_testfile .c
> + set dir "$lang"
> +
> + # Create the additional flags
Add period.
> + set flags "debug"
> + lappend flags $lang
> +
> + set binfile [standard_output_file ${dir}/${testfile}]
> + if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "${flags}"] != "" } {
Spurious double space after "if".
Wonder if this could use build_executable or prepare_for_testing (eliminating
the clean_restart below).
> + unresolved "failed to compile"
> + return 0
> + }
> +
> + # Start with a fresh gdb.
> + clean_restart ${binfile}
> +
> + # Advance to main
Add period.
> + if { ![runto_main] } then {
> + fail "can't run to main"
> + return 0
> + }
> + # Check handling of typedefs.
> + gdb_print_expr_at_depths "s6" {"{...}" \
> + "{{raw = {...}, {x = 0, y = 0, z = 0}}}" \
> + "{{raw = \\\{0, 0, 0\\\}, {x = 0, y = 0, z = 0}}}"}
> +
> + # Multiple anonymous structures in parallel.
> + gdb_print_expr_at_depths "s7" {"{...}" \
> + "{{x = 0, y = 0}, {z = 0, a = 0}, {b = 0, c = 0}}" \
> + "{{x = 0, y = 0}, {z = 0, a = 0}, {b = 0, c = 0}}"}
> +
> + # Flip flop between named and anonymous. Expected to unfurl to the
Double space before "Expected".
> +++ b/gdb/testsuite/gdb.python/py-nested-maps.c
> @@ -0,0 +1,122 @@
> +
> +struct map_map_t *
> +create_map_map ()
create_map_map (void)
> +{
> diff --git a/gdb/testsuite/gdb.python/py-nested-maps.exp b/gdb/testsuite/gdb.python/py-nested-maps.exp
> new file mode 100644
> index 00000000000..146b9cab075
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-nested-maps.exp
> @@ -0,0 +1,211 @@
> +# Copyright (C) 2019 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/>.
> +
> +# This tests GDB's python pretty printing of nested map like
> +# structures using structures as keys and values.
> +
Should this say something about depth?
> +gdb_breakpoint [gdb_get_line_number "Break here"]
> +gdb_continue_to_breakpoint "run to testing point" ".*Break here.*"
> +
> +set remote_python_file [gdb_remote_download host \
> + ${srcdir}/${subdir}/${testfile}.py]
> +gdb_test_no_output "source ${remote_python_file}" "load python file"
> +
> +# Test printing with 'set print pretty off'
Missing period.
> +gdb_test_no_output "set print pretty off"
> +with_test_prefix "pretty=off" {
> + gdb_print_expr_at_depths "*m1" \
> + [list \
> + "\{\\.\\.\\.\}" \
> + "\{\\\[\{a = 3, b = 4\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 4, b = 5\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 5, b = 6\}\\\] = \{\\.\\.\\.\}\}" \
> + "\{\\\[\{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 \
> + "\{\\.\\.\\.\}" \
> + "\{\\\[$hex \"m1\"\\\] = \{\\.\\.\\.\}, \\\[$hex \"m2\"\\\] = \{\\.\\.\\.\}\}" \
> + "\{\\\[$hex \"m1\"\\\] = \{\\\[\{a = 3, b = 4\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 4, b = 5\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 5, b = 6\}\\\] = \{\\.\\.\\.\}\}, \\\[$hex \"m2\"\\\] = \{\\\[\{a = 6, b = 7\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 7, b = 8\}\\\] = \{\\.\\.\\.\}, \\\[\{a = 8, b = 9\}\\\] = \{\\.\\.\\.\}\}\}" \
> + "\{\\\[$hex \"m1\"\\\] = \{\\\[\{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\"\\\] = \{\\\[\{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\}\}\}" \
> + ]
> +}
> +
> +# Now again, but with 'set print pretty on'
Missing period.
>
> # Issue a PASS and return true if evaluating CONDITION in the caller's
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index 10020901c2d..e47ac981ce8 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -28,6 +28,7 @@
> #include "annotate.h"
> #include "valprint.h"
> #include "target-float.h"
> +#include "c-lang.h" /* For c_textual_element_type. */
> #include "extension.h"
> #include "ada-lang.h"
> #include "gdb_obstack.h"
> @@ -88,6 +89,7 @@ static void val_print_type_code_flags (struct type *type,
> struct ui_file *stream);
>
> #define PRINT_MAX_DEFAULT 200 /* Start print_max off at this value. */
> +#define PRINT_MAX_DEPTH_DEFAULT 20 /* Start print_max_depth off at this value. */
>
> struct value_print_options user_print_options =
> {
> @@ -109,7 +111,8 @@ struct value_print_options user_print_options =
> 1, /* pascal_static_field_print */
> 0, /* raw */
> 0, /* summary */
> - 1 /* symbol_print */
> + 1, /* symbol_print */
> + PRINT_MAX_DEPTH_DEFAULT /* max_depth */
> };
>
> /* Initialize *OPTS to be a copy of the user print options. */
> @@ -281,6 +284,39 @@ val_print_scalar_type_p (struct type *type)
> }
> }
>
> +/* A helper function for val_print. When printing with limited depth we
> + want to print string and scalar arguments, but not aggregate arguments.
> + This function distinguishes between the two. */
> +
> +static bool
> +val_print_scalar_or_string_type_p (struct type *type)
> +{
> + /* Resolve the real type. */
> + type = check_typedef (type);
> + while (TYPE_CODE (type) == TYPE_CODE_REF)
> + {
> + type = TYPE_TARGET_TYPE (type);
> + type = check_typedef (type);
> + }
> +
> + switch (TYPE_CODE (type))
> + {
> + case TYPE_CODE_ARRAY:
> + {
> + /* See if target type looks like a string. */
> + struct type * array_target_type = TYPE_TARGET_TYPE (type);
Spurious space after "*".
> + return (TYPE_LENGTH (type) > 0
> + && TYPE_LENGTH (array_target_type) > 0
> + && c_textual_element_type (array_target_type, 0));
Is using c_textual_element_type safe / the right thing to do
for all languages?
> + }
> + case TYPE_CODE_STRING:
> + return true;
> + default:
> + /* Check for scalar. */
> + return val_print_scalar_type_p(type);
> + }
> +}
> +
> /* See its definition in value.h. */
>
> int
> @@ -1054,6 +1090,15 @@ val_print (struct type *type, LONGEST embedded_offset,
> return;
> }
>
> + if (!val_print_scalar_or_string_type_p (type)
> + && options->max_depth > -1
> + && recurse >= options->max_depth)
> + {
> + gdb_assert (language->la_struct_too_deep_ellipsis != NULL);
Maybe put this assertion / validation where we register languages
instead, so you'd do this only once and in one place, instead of
potentially at all usage spots?
> + fputs_filtered (language->la_struct_too_deep_ellipsis, stream);
> + return;
> + }
> +
Thanks,
Pedro Alves