PING: Re: [PATCH 1/3] gdb: New set/show max-value-size command.

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

Ping!  I believe I've addressed all review comments.

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2016-01-06 11:40:50 +0000]:

> * Eli Zaretskii <eliz@gnu.org> [2016-01-05 18:24:32 +0200]:
> 
> > > Date: Tue, 5 Jan 2016 14:12:41 +0000
> > > From: Andrew Burgess <andrew.burgess@embecosm.com>
> > > Cc: gdb-patches@sourceware.org
> > > 
> > > For languages with dynamic types, an incorrect program, or uninitialised
> > > variables within a program, could result in an incorrect, overly large
> > > type being associated with a value.  Currently, attempting to print such
> > > a variable will result in gdb trying to allocate an overly large buffer.
> > > 
> > > If this large memory allocation fails then the result can be gdb either
> > > terminating, or (due to memory contention) becoming unresponsive for the
> > > user.
> > > 
> > > A new user visible variable in gdb helps guard against such problems,
> > > two new commands are available:
> > > 
> > >    set max-value-size
> > >    show max-value-size
> > > 
> > > The 'max-value-size' is the maximum size in bytes that gdb will allocate
> > > for the contents of a value.  Any attempt to allocate a value with a
> > > size greater than this will result in an error.  The initial default for
> > > this limit is set at 64k, this is based on a similar limit that exists
> > > within the ada specific code.
> > > 
> > > It is possible for the user to set max-value-size to unlimited, in which
> > > case the old behaviour is restored.
> > 
> > Thanks.
> > 
> > > gdb/doc/ChangeLog:
> > > 
> > > 	* gdb.texinfo (Value Sizes): New section, also add the new section
> > > 	to the menu.
> > 
> > The addition to the menu is probably in a different node, so it needs
> > a separate entry in the ChangeLog.
> > 
> > > +set max-value-size
> > > +show max-value-size
> > > +  Control the maximum size, in bytes, that GDB will allocate for value
> > > +  contents.  Prevent incorrect programs from causing GDB to allocate
> > > +  overly large buffers.  Default is unlimited.
> > 
> > "Controls" and "Prevents".  Also "maximum size of memory" (we allocate
> > memory, not size).
> > 
> > > +@table @code
> > > +@kindex set max-value-size
> > > +@itemx set max-value-size @var{bytes}
> > > +@itemx set max-value-size unlimited
> > > +Set the maximum size, in bytes, that @value{GDBN} will allocate for
> > 
> > Same here:
> > 
> >   Set the maximum size of memory, in bytes, that @value{GDBN} will ...
> > 
> > Also, the "in bytes" part is redundant, since the parameter is called
> > "bytes", which is self-explanatory.
> > 
> > > +the contents of a value to @var{bytes}.  Any value whose contents
> > > +require more than this number of bytes can't be displayed by
> > > +@value{GDBN}, and trying to display the value will result in an error.
> > 
> > I would remove the "can't be displayed by @value{GDBN}" part.  It can
> > be interpreted to mean some limitation inherent in GDB, which is not
> > what you want to convey.  The rest of the sentence says it all: trying
> > to display a value that requires more memory than that will result in
> > an error.
> > 
> > > +Setting this variable does not effect values that have already been
> > > +allocated within gdb, only future allocations.
> >                     ^^^
> > @value{GDBN}
> > 
> > > +There's a minimum size that @code{max-value-size} can be set too in
> >                                                                 ^^^
> > "to"
> > 
> > OK with those fixed.
> 
> New revision resolves all of Eli's comments, and also addresses
> Pedro's comment.
> 
> OK to apply?
> 
> Thanks,
> Andrew
> 
> ---
> 
> For languages with dynamic types, an incorrect program, or uninitialised
> variables within a program, could result in an incorrect, overly large
> type being associated with a value.  Currently, attempting to print such
> a variable will result in gdb trying to allocate an overly large buffer.
> 
> If this large memory allocation fails then the result can be gdb either
> terminating, or (due to memory contention) becoming unresponsive for the
> user.
> 
> A new user visible variable in gdb helps guard against such problems,
> two new commands are available:
> 
>    set max-value-size
>    show max-value-size
> 
> The 'max-value-size' is the maximum size of memory in bytes that gdb
> will allocate for the contents of a value.  Any attempt to allocate a
> value with a size greater than this will result in an error.  The
> initial default for this limit is set at 64k, this is based on a similar
> limit that exists within the ada specific code.
> 
> It is possible for the user to set max-value-size to unlimited, in which
> case the old behaviour is restored.
> 
> gdb/ChangeLog:
> 
> 	* value.c (max_value_size): New variable.
> 	(MIN_VALUE_FOR_MAX_VALUE_SIZE): New define.
> 	(show_max_value_size): New function.
> 	(check_type_length_before_alloc): New function.
> 	(allocate_value_contents): Call check_type_length_before_alloc.
> 	(set_value_enclosing_type): Likewise.
> 	(_initialize_values): Add set/show handler for max-value-size.
> 	* NEWS: Mention new set/show command.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Value Sizes): New section.
> 	(Data): Add the 'Value Sizes' node to the menu.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/max-value-size.c: New file.
> 	* gdb.base/max-value-size.exp: New file.
> ---
>  gdb/ChangeLog                             | 12 ++++
>  gdb/NEWS                                  |  6 ++
>  gdb/doc/ChangeLog                         |  5 ++
>  gdb/doc/gdb.texinfo                       | 42 +++++++++++++
>  gdb/testsuite/ChangeLog                   |  5 ++
>  gdb/testsuite/gdb.base/max-value-size.c   | 26 +++++++++
>  gdb/testsuite/gdb.base/max-value-size.exp | 97 +++++++++++++++++++++++++++++++
>  gdb/value.c                               | 97 +++++++++++++++++++++++++++++--
>  8 files changed, 286 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/max-value-size.c
>  create mode 100644 gdb/testsuite/gdb.base/max-value-size.exp
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 3d8923b..42633d2 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,15 @@
> +2016-01-05  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* value.c (max_value_size): New variable.
> +	(MIN_VALUE_FOR_MAX_VALUE_SIZE): New define.
> +	(set_max_value_size): New function.
> +	(show_max_value_size): New function.
> +	(check_type_length_before_alloc): New function.
> +	(allocate_value_contents): Call check_type_length_before_alloc.
> +	(set_value_enclosing_type): Likewise.
> +	(_initialize_values): Add set/show handler for max-value-size.
> +	* NEWS: Mention new set/show command.
> +
>  2016-01-04  Markus Metzger  <markus.t.metzger@intel.com>
>  
>  	* btrace.c (btrace_pt_readmem_callback): Do not return in TRY/CATCH.
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 484d98d..861b125 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -60,6 +60,12 @@ show ada print-signatures"
>    Control whether parameter types and return types are displayed in overloads
>    selection menus.  It is activaled (@code{on}) by default.
>  
> +set max-value-size
> +show max-value-size
> +  Controls the maximum size of memory, in bytes, that GDB will
> +  allocate for value contents.  Prevents incorrect programs from
> +  causing GDB to allocate overly large buffers.  Default is 64k.
> +
>  * The "disassemble" command accepts a new modifier: /s.
>    It prints mixed source+disassembly like /m with two differences:
>    - disassembled instructions are now printed in program order, and
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index ef3bdaa..97a1b25 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-01-05  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* gdb.texinfo (Value Sizes): New section.
> +	(Data): Add the 'Value Sizes' node to the menu.
> +
>  2015-12-11  Don Breazeal  <donb@codesourcery.com>
>  
>  	* gdb.texinfo (Forks): Correct Linux kernel version where
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 0778383..9dea873 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -8531,6 +8531,7 @@ being passed the type of @var{arg} as the argument.
>                                  character set than GDB does
>  * Caching Target Data::         Data caching for targets
>  * Searching Memory::            Searching memory for a sequence of bytes
> +* Value Sizes::                 Managing memory allocated for values
>  @end menu
>  
>  @node Expressions
> @@ -11596,6 +11597,47 @@ $1 = 1
>  $2 = (void *) 0x8049560
>  @end smallexample
>  
> +@node Value Sizes
> +@section Value Sizes
> +
> +Whenever @value{GDBN} prints a value memory will be allocated within
> +@value{GDBN} to hold the contents of the value.  It is possible in
> +some languages with dynamic typing systems, that an invalid program
> +may indicate a value that is incorrectly large, this in turn may cause
> +@value{GDBN} to try and allocate an overly large ammount of memory.
> +
> +@table @code
> +@kindex set max-value-size
> +@itemx set max-value-size @var{bytes}
> +@itemx set max-value-size unlimited
> +Set the maximum size of memory that @value{GDBN} will allocate for the
> +contents of a value to @var{bytes}, trying to display a value that
> +requires more memory than that will result in an error.
> +
> +Setting this variable does not effect values that have already been
> +allocated within @value{GDBN}, only future allocations.
> +
> +There's a minimum size that @code{max-value-size} can be set to in
> +order that @value{GDBN} can still operate correctly, this minimum is
> +currently 16 bytes.
> +
> +The limit applies to the results of some subexpressions as well as to
> +complete expressions.  For example, an expression denoting a simple
> +integer component, such as @code{x.y.z}, may fail if the size of
> +@var{x.y} is dynamic and exceeds @var{bytes}.  On the other hand,
> +@value{GDBN} is sometimes clever; the expression @code{A[i]}, where
> +@var{A} is an array variable with non-constant size, will generally
> +succeed regardless of the bounds on @var{A}, as long as the component
> +size is less than @var{bytes}.
> +
> +The default value of @code{max-value-size} is currently 64k.
> +
> +@kindex show max-value-size
> +@item show max-value-size
> +Show the maximum size of memory, in bytes, that @value{GDBN} will
> +allocate for the contents of a value.
> +@end table
> +
>  @node Optimized Code
>  @chapter Debugging Optimized Code
>  @cindex optimized code, debugging
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 45a6c6a..b10da8c 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-01-05  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* gdb.base/max-value-size.c: New file.
> +	* gdb.base/max-value-size.exp: New file.
> +
>  2016-01-04  Markus Metzger  <markus.t.metzger@intel.com>
>  
>  	* gdb.btrace/dlopen.exp: New.
> diff --git a/gdb/testsuite/gdb.base/max-value-size.c b/gdb/testsuite/gdb.base/max-value-size.c
> new file mode 100644
> index 0000000..b6a6fe5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/max-value-size.c
> @@ -0,0 +1,26 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright (C) 2016 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/>.  */
> +
> +char one;
> +char ten [10];
> +char one_hundred [100];
> +
> +int
> +main (void)
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/max-value-size.exp b/gdb/testsuite/gdb.base/max-value-size.exp
> new file mode 100644
> index 0000000..63a97a0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/max-value-size.exp
> @@ -0,0 +1,97 @@
> +# Copyright (C) 2016 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/>.
> +
> +standard_testfile
> +
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
> +    untested $testfile.exp
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    fail "Can't run to main"
> +    return 0
> +}
> +
> +# Run "show max-value-size" and return the interesting bit of the
> +# result.  This is either the maximum size in bytes, or the string
> +# "unlimited".
> +proc get_max_value_size {} {
> +    global gdb_prompt
> +    global decimal
> +
> +    gdb_test_multiple "show max-value-size" "" {
> +	-re "Maximum value size is ($decimal) bytes.*$gdb_prompt $" {
> +	    return $expect_out(1,string)
> +	}
> +	-re "Maximum value size is unlimited.*$gdb_prompt $" {
> +	    return "unlimited"
> +	}
> +    }
> +}
> +
> +# Assuming that MAX_VALUE_SIZE is the current value for
> +# max-value-size, print the test values.  Use TEST_PREFIX to make the
> +# test names unique.
> +proc do_value_printing { max_value_size test_prefix } {
> +    with_test_prefix ${test_prefix} {
> +	gdb_test "p/d one" " = 0"
> +	if { $max_value_size != "unlimited" && $max_value_size < 100 } then {
> +	    gdb_test "p/d one_hundred" \
> +		"value requires 100 bytes, which is more than max-value-size"
> +	} else {
> +	    gdb_test "p/d one_hundred" " = \\{0 <repeats 100 times>\\}"
> +	}
> +	gdb_test "p/d one_hundred \[99\]" " = 0"
> +    }
> +}
> +
> +# Install SET_VALUE as the value for max-value-size, then print the
> +# test values.
> +proc set_and_check_max_value_size { set_value } {
> +    if { $set_value == "unlimited" } then {
> +	set check_pattern "unlimited"
> +    } else {
> +	set check_pattern "${set_value} bytes"
> +    }
> +
> +    gdb_test_no_output "set max-value-size ${set_value}"
> +    gdb_test "show max-value-size" \
> +	"Maximum value size is ${check_pattern}." \
> +	"check that the value shows as ${check_pattern}"
> +
> +    do_value_printing ${set_value} "max-value-size is '${set_value}'"
> +}
> +
> +# Check the default value is sufficient.
> +do_value_printing [get_max_value_size] "using initial max-value-size"
> +
> +# Check some values for max-value-size that should prevent some
> +# allocations.
> +set_and_check_max_value_size 25
> +set_and_check_max_value_size 99
> +
> +# Check values for max-value-size that should allow all allocations.
> +set_and_check_max_value_size 100
> +set_and_check_max_value_size 200
> +set_and_check_max_value_size "unlimited"
> +
> +# Check that we can't set the maximum size stupidly low.
> +gdb_test "set max-value-size 1" \
> +    "max-value-size set too low, increasing to \[0-9\]+ bytes"
> +gdb_test "set max-value-size 0" \
> +    "max-value-size set too low, increasing to \[0-9\]+ bytes"
> +gdb_test "set max-value-size -5" \
> +    "only -1 is allowed to set as unlimited"
> diff --git a/gdb/value.c b/gdb/value.c
> index eeb2b7d..41f44c8 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -955,13 +955,86 @@ allocate_value_lazy (struct type *type)
>    return val;
>  }
>  
> +/* The maximum size, in bytes, that GDB will try to allocate for a value.
> +   The initial value of 64k was not selected for any specific reason, it is
> +   just a reasonable starting point.  */
> +
> +static int max_value_size = 65536; /* 64k bytes */
> +
> +/* It is critical that the MAX_VALUE_SIZE is at least as bit as the size of
> +   LONGEST, otherwise GDB will not be able to parse integer values from the
> +   CLI; for example if the MAX_VALUE_SIZE could be set to 1 then GDB would
> +   be unable to parse "set max-value-size 2".
> +
> +   As we want a consistent GDB experience across hosts with different sizes
> +   of LONGEST, this arbitrary minimum value was selected, so long as this
> +   is bigger than LONGEST of all GDB supported hosts we're fine.  */
> +
> +#define MIN_VALUE_FOR_MAX_VALUE_SIZE 16
> +gdb_static_assert (sizeof (LONGEST) <= MIN_VALUE_FOR_MAX_VALUE_SIZE);
> +
> +/* Implement the "set max-value-size" command.  */
> +
> +static void
> +set_max_value_size (char *args, int from_tty,
> +		    struct cmd_list_element *c)
> +{
> +  gdb_assert (max_value_size == -1 || max_value_size >= 0);
> +
> +  if (max_value_size > -1 && max_value_size < MIN_VALUE_FOR_MAX_VALUE_SIZE)
> +    {
> +      max_value_size = MIN_VALUE_FOR_MAX_VALUE_SIZE;
> +      error (_("max-value-size set too low, increasing to %d bytes"),
> +	     max_value_size);
> +    }
> +}
> +
> +/* Implement the "show max-value-size" command.  */
> +
> +static void
> +show_max_value_size (struct ui_file *file, int from_tty,
> +		     struct cmd_list_element *c, const char *value)
> +{
> +  if (max_value_size == -1)
> +    fprintf_filtered (file, _("Maximum value size is unlimited.\n"));
> +  else
> +    fprintf_filtered (file, _("Maximum value size is %d bytes.\n"),
> +		      max_value_size);
> +}
> +
> +/* Called before we attempt to allocate or reallocate a buffer for the
> +   contents of a value.  TYPE is the type of the value for which we are
> +   allocating the buffer.  If the buffer is too large (based on the user
> +   controllable setting) then throw an error.  If this function returns
> +   then we should attempt to allocate the buffer.  */
> +
> +static void
> +check_type_length_before_alloc (const struct type *type)
> +{
> +  unsigned int length = TYPE_LENGTH (type);
> +
> +  if (max_value_size > -1 && length > max_value_size)
> +    {
> +      if (TYPE_NAME (type) != NULL)
> +	error (_("value of type `%s' requires %d bytes, which is more "
> +		 "than max-value-size"), TYPE_NAME (type), length);
> +      else
> +	error (_("value requires %d bytes, which is more than "
> +		 "max-value-size"), length);
> +    }
> +}
> +
>  /* Allocate the contents of VAL if it has not been allocated yet.  */
>  
>  static void
>  allocate_value_contents (struct value *val)
>  {
>    if (!val->contents)
> -    val->contents = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
> +    {
> +      check_type_length_before_alloc (val->enclosing_type);
> +      val->contents
> +	= (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
> +    }
>  }
>  
>  /* Allocate a  value  and its contents for type TYPE.  */
> @@ -2986,9 +3059,12 @@ value_static_field (struct type *type, int fieldno)
>  void
>  set_value_enclosing_type (struct value *val, struct type *new_encl_type)
>  {
> -  if (TYPE_LENGTH (new_encl_type) > TYPE_LENGTH (value_enclosing_type (val))) 
> -    val->contents =
> -      (gdb_byte *) xrealloc (val->contents, TYPE_LENGTH (new_encl_type));
> +  if (TYPE_LENGTH (new_encl_type) > TYPE_LENGTH (value_enclosing_type (val)))
> +    {
> +      check_type_length_before_alloc (new_encl_type);
> +      val->contents
> +	= (gdb_byte *) xrealloc (val->contents, TYPE_LENGTH (new_encl_type));
> +    }
>  
>    val->enclosing_type = new_encl_type;
>  }
> @@ -4013,4 +4089,17 @@ Check whether an expression is void.\n\
>  Usage: $_isvoid (expression)\n\
>  Return 1 if the expression is void, zero otherwise."),
>  			 isvoid_internal_fn, NULL);
> +
> +  add_setshow_zuinteger_unlimited_cmd ("max-value-size",
> +				       class_support, &max_value_size, _("\
> +Set maximum sized value gdb will load from the inferior."), _("\
> +Show maximum sized value gdb will load from the inferior."), _("\
> +Use this to control the maximum size, in bytes, of a value that gdb\n\
> +will load from the inferior.  Setting this value to 'unlimited'\n\
> +disables checking.\n\
> +Setting this does not invalidate already allocated values, it only\n\
> +prevents future values, larger than this size, from being allocated."),
> +			    set_max_value_size,
> +			    show_max_value_size,
> +			    &setlist, &showlist);
>  }
> -- 
> 2.5.1
>