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

  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
  

"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.  */
  

On Fri, 13 Jan 2023, Andrew Burgess wrote:

> > @@ -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?

 I had to investigate that and found out that in addition to value history 
accesses `value_copy' is only used for internal variables (which is fine; 
I guess that's one reason for the minimum of 16 set for `max-value-size'), 
and then an obscure piece of code called `make_cv_value'.

 Said function is only referred to from Python and Scheme support code, 
and its interaction with `max-value-size' is not covered by the testsuite.  
I have added a suitable test case then and that has verified it is indeed 
fine for `value_copy' to always disable the size check.

 Thank you for the suggestion then.  Since I missed a couple of copyright 
year updates with the original submission and also Tom's recent conversion 
requires the use of `allow_ada_tests' rather than `skip_ada_tests' in Ada 
test cases, I'll respin the patch series with all the necessary updates.

  Maciej