This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]