Re: [PATCH v2 1/5] GDB: Ignore `max-value-size' setting with value history accesses

[Andrew Burgess <aburgess@redhat.com>]

"Maciej W. Rozycki" <macro@embecosm.com> writes:

> We have an inconsistency in value history accesses where array element 
> accesses cause an error for entries exceeding the currently selected 
> `max-value-size' setting even where such accesses successfully complete 
> for elements located in the inferior, e.g.:
>
>   (gdb) p/d one
>   $1 = 0
>   (gdb) p/d one_hundred
>   $2 = {0 <repeats 100 times>}
>   (gdb) p/d one_hundred[99]
>   $3 = 0
>   (gdb) set max-value-size 25
>   (gdb) p/d one_hundred
>   value requires 100 bytes, which is more than max-value-size
>   (gdb) p/d one_hundred[99]
>   $7 = 0
>   (gdb) p/d $2
>   value requires 100 bytes, which is more than max-value-size
>   (gdb) p/d $2[99]
>   value requires 100 bytes, which is more than max-value-size
>   (gdb) 
>
> According to our documentation the `max-value-size' setting is a safety 
> guard against allocating an overly large amount of memory.  Moreover a 
> statement in documentation says, concerning this setting, that: "Setting 
> this variable does not effect values that have already been allocated 
> within GDB, only future allocations."  While in the implementer-speak 
> the sentence may be unambiguous I think the outside user may well infer 
> that the setting only applies to values that need to be retrieved from 
> the debuggee.
>
> Therefore rather than just fixing this inconsistency it seems reasonable 
> to lift the setting for value history accesses, under an implication 
> that by having been retrieved from the debuggee they have already passed 
> the safety check.  Do it then, making the last two commands succeed:
>
>   (gdb) p/d $2
>   $8 = {0 <repeats 100 times>}
>   (gdb) p/d $2 [99]
>   $9 = 0
>   (gdb) 
>
> Expand the testsuite accordingly.
> ---
>  gdb/testsuite/gdb.base/max-value-size.exp |    3 +
>  gdb/value.c                               |   56 ++++++++++++++++++++----------
>  2 files changed, 42 insertions(+), 17 deletions(-)
>
> gdb-value-history-size.diff
> Index: src/gdb/testsuite/gdb.base/max-value-size.exp
> ===================================================================
> --- src.orig/gdb/testsuite/gdb.base/max-value-size.exp
> +++ src/gdb/testsuite/gdb.base/max-value-size.exp
> @@ -53,6 +53,9 @@ proc do_value_printing { max_value_size
>  	    gdb_test "p/d one_hundred" " = \\{0 <repeats 100 times>\\}"
>  	}
>  	gdb_test "p/d one_hundred \[99\]" " = 0"
> +	# Verify that accessing value history is undisturbed.
> +	gdb_test "p/d \$2" " = \\{0 <repeats 100 times>\\}"
> +	gdb_test "p/d \$2 \[99\]" " = 0"
>      }
>  }
>  
> Index: src/gdb/value.c
> ===================================================================
> --- src.orig/gdb/value.c
> +++ src/gdb/value.c
> @@ -1034,31 +1034,42 @@ check_type_length_before_alloc (const st
>      }
>  }
>  
> -/* Allocate the contents of VAL if it has not been allocated yet.  */
> +/* Allocate the contents of VAL if it has not been allocated yet.
> +   If CHECK_SIZE is true, then apply the usual max-value-size checks.  */
>  
>  static void
> -allocate_value_contents (struct value *val)
> +allocate_value_contents (struct value *val, bool check_size)
>  {
>    if (!val->contents)
>      {
> -      check_type_length_before_alloc (val->enclosing_type);
> +      if (check_size)
> +	check_type_length_before_alloc (val->enclosing_type);
>        val->contents.reset
>  	((gdb_byte *) xzalloc (val->enclosing_type->length ()));
>      }
>  }
>  
> -/* Allocate a  value  and its contents for type TYPE.  */
> +/* Allocate a value and its contents for type TYPE.  If CHECK_SIZE is true,
> +   then apply the usual max-value-size checks.  */
>  
> -struct value *
> -allocate_value (struct type *type)
> +static struct value *
> +allocate_value (struct type *type, bool check_size)
>  {
>    struct value *val = allocate_value_lazy (type);
>  
> -  allocate_value_contents (val);
> +  allocate_value_contents (val, check_size);
>    val->lazy = 0;
>    return val;
>  }
>  
> +/* Allocate a value and its contents for type TYPE.  */
> +
> +struct value *
> +allocate_value (struct type *type)
> +{
> +  return allocate_value (type, true);
> +}
> +
>  /* Allocate a  value  that has the correct length
>     for COUNT repetitions of type TYPE.  */
>  
> @@ -1169,7 +1180,7 @@ value_contents_raw (struct value *value)
>    struct gdbarch *arch = get_value_arch (value);
>    int unit_size = gdbarch_addressable_memory_unit_size (arch);
>  
> -  allocate_value_contents (value);
> +  allocate_value_contents (value, true);
>  
>    ULONGEST length = value_type (value)->length ();
>    return gdb::make_array_view
> @@ -1179,7 +1190,7 @@ value_contents_raw (struct value *value)
>  gdb::array_view<gdb_byte>
>  value_contents_all_raw (struct value *value)
>  {
> -  allocate_value_contents (value);
> +  allocate_value_contents (value, true);
>  
>    ULONGEST length = value_enclosing_type (value)->length ();
>    return gdb::make_array_view (value->contents.get (), length);
> @@ -1752,12 +1763,14 @@ value_release_to_mark (const struct valu
>    return result;
>  }
>  
> -/* Return a copy of the value ARG.
> -   It contains the same contents, for same memory address,
> -   but it's a different block of storage.  */
> +/* Return a copy of the value ARG.  It contains the same contents,
> +   for the same memory address, but it's a different block of storage.
> +   If CHECK_SIZE is true, then throw an exception whenever the size
> +   of memory allocated for the contents of the value would exceed
> +   max-value-size.  */
>  
> -struct value *
> -value_copy (const value *arg)
> +static struct value *
> +value_copy (const value *arg, bool check_size)
>  {
>    struct type *encl_type = value_enclosing_type (arg);
>    struct value *val;
> @@ -1765,7 +1778,7 @@ value_copy (const value *arg)
>    if (value_lazy (arg))
>      val = allocate_value_lazy (encl_type);
>    else
> -    val = allocate_value (encl_type);
> +    val = allocate_value (encl_type, check_size);

I wonder, maybe value_copy should never check the max-value-size.  As
you point out above, the max-value-size was introduced to catch cases
where attempting to read a value from the inferior would cause GDB to
try an allocate a stupid amount of memory.

We don't currently have any mechanism in GDB to try an cap the
cumulative memory usage across all values, which suggests that
currently, once a value has been read into GDB we assume we can safely
make as many copies as we want.

And so, is there any reason why value_copy shouldn't always disable the
size check?

Thanks,
Andrew



>    val->type = arg->type;
>    VALUE_LVAL (val) = arg->lval;
>    val->location = arg->location;
> @@ -1802,6 +1815,15 @@ value_copy (const value *arg)
>    return val;
>  }
>  
> +/* Return a copy of the value ARG.  It contains the same contents,
> +   for the same memory address, but it's a different block of storage.  */
> +
> +struct value *
> +value_copy (const value *arg)
> +{
> +  return value_copy (arg, true);
> +}
> +
>  /* Return a "const" and/or "volatile" qualified version of the value V.
>     If CNST is true, then the returned value will be qualified with
>     "const".
> @@ -1965,7 +1987,7 @@ access_value_history (int num)
>  
>    absnum--;
>  
> -  return value_copy (value_history[absnum].get ());
> +  return value_copy (value_history[absnum].get (), false);
>  }
>  
>  /* See value.h.  */
> @@ -4162,7 +4184,7 @@ void
>  value_fetch_lazy (struct value *val)
>  {
>    gdb_assert (value_lazy (val));
> -  allocate_value_contents (val);
> +  allocate_value_contents (val, true);
>    /* A value is either lazy, or fully fetched.  The
>       availability/validity is only established as we try to fetch a
>       value.  */