Ping: [PATCH 2/2] gdb: improve reuse of value contents when fetching array elements

Andrew Burgess aburgess@redhat.com
Fri Dec 3 11:11:42 GMT 2021


Ping!

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2021-10-15 16:39:07 +0100]:

> While working on a Python script, which was interacting with a remote
> target, I noticed some weird slowness in GDB.  In my program I had a
> structure something like this:
> 
>   struct foo_t
>   {
>     int array[5];
>   };
> 
>   struct foo_t global_foo;
> 
> Then in the Python script I was fetching a complete copy of global
> foo, like:
> 
>   val = gdb.parse_and_eval('global_foo')
>   val.fetch_lazy()
> 
> Then I would work with items in foo_t.array, like:
> 
>   print(val['array'][1])
> 
> I called the fetch_lazy method specifically because I knew I was going
> to end up accessing almost all of the contents of val, and so I wanted
> GDB to do a single remote protocol call to fetch all the contents in
> one go, rather than trying to do lazy fetches for a couple of bytes at
> a time.
> 
> What I observed was that, after the fetch_lazy call, GDB does,
> correctly, fetch the entire contents of global_foo, including all of
> the contents of array, however, when I access val.array[1], GDB still
> goes and fetches the value of this element from the remote target.
> 
> What's going on is that in valarith.c, in value_subscript, for C like
> languages, we always end up treating the array value as a pointer, and
> then doing value_ptradd, and value_ind, the second of these calls
> always returns a lazy value.
> 
> My guess is that this approach allows us to handle indexing off the
> end of an array, when working with zero element arrays, or when
> indexing a raw pointer as an array.  And, I agree, that in these
> cases, where, even when the original value is non-lazy, we still will
> not have the content of the array loaded, we should be using the
> value_ind approach.
> 
> However, for cases where we do have the array contents loaded, and we
> do know the bounds of the array, I think we should be using
> value_subscripted_rvalue, which is what we use for non C like
> languages.
> 
> One problem I did run into, exposed by gdb.base/charset.exp, was that
> value_subscripted_rvalue stripped typedefs from the element type of
> the array, which means the value returned will not have the same type
> as an element of the array, but would be the raw, non-typedefed,
> type.  In charset.exp we got back an 'int' instead of a
> 'wchar_t' (which is a typedef of 'int'), and this impacts how we print
> the value.  Removing typedefs from the resulting value just seems
> wrong, so I got rid of that, and I don't see any test regressions.
> 
> With this change in place, my original Python script is now doing no
> additional memory accesses, and its performance increases about 10x!
> ---
>  gdb/testsuite/gdb.base/non-lazy-array-index.c | 31 ++++++++
>  .../gdb.base/non-lazy-array-index.exp         | 78 +++++++++++++++++++
>  gdb/valarith.c                                | 18 ++---
>  3 files changed, 118 insertions(+), 9 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/non-lazy-array-index.c
>  create mode 100644 gdb/testsuite/gdb.base/non-lazy-array-index.exp
> 
> diff --git a/gdb/testsuite/gdb.base/non-lazy-array-index.c b/gdb/testsuite/gdb.base/non-lazy-array-index.c
> new file mode 100644
> index 00000000000..4140b67d262
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/non-lazy-array-index.c
> @@ -0,0 +1,31 @@
> +/* Copyright 2021 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +struct foo_t
> +{
> +  float f;
> +
> +  int array[5];
> +};
> +
> +struct foo_t global_foo = { 1.0f, { 1, 2, 3, 4, 5 } };
> +
> +int
> +main ()
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/non-lazy-array-index.exp b/gdb/testsuite/gdb.base/non-lazy-array-index.exp
> new file mode 100644
> index 00000000000..c837f9a48b2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/non-lazy-array-index.exp
> @@ -0,0 +1,78 @@
> +# Copyright 2021 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/>.
> +
> +# Check that GDB does not do excess accesses to the inferior memory
> +# when fetching elements from an array in the C language.
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    return -1
> +}
> +
> +# Load 'global_foo' into a history variable.
> +gdb_test "p global_foo" "\\{f = 1, array = \\{1, 2, 3, 4, 5\\}\\}"
> +
> +gdb_test_no_output "set debug target 1"
> +
> +# Now print an array element from within 'global_foo', but accessed
> +# via the history element.  The history element should be non-lazy,
> +# and so there should be no reason for GDB to fetch the array element
> +# from the inferior memory, we should be able to grab the contents
> +# directory from the history value.
> +#
> +# To check this we 'set debug target 1' (above), and then look for any
> +# xfer_partial calls; there shouldn't be any.
> +set saw_memory_access false
> +gdb_test_multiple "p \$.array\[1\]" "" {
> +    -re "^p \\\$\\.array\\\[1\\\]\r\n" {
> +	exp_continue
> +    }
> +    -re "^->\[^\r\n\]+xfer_partial\[^\r\n\]+\r\n" {
> +	set saw_memory_access true
> +	exp_continue
> +    }
> +    -re "^->\[^\r\n\]+\r\n" {
> +	exp_continue
> +    }
> +    -re "^<-\[^\r\n\]+\r\n" {
> +	exp_continue
> +    }
> +    -re "^\[^\$\]\[^\r\n\]+\r\n" {
> +	exp_continue
> +    }
> +    -re "^\\\$${decimal} = 2\r\n$gdb_prompt " {
> +	gdb_assert { ! $saw_memory_access }
> +    }
> +}
> +
> +gdb_test "set debug target 0" ".*"
> +
> +if { ! [skip_python_tests] } {
> +    gdb_test_no_output "python val = gdb.parse_and_eval('global_foo')"
> +    gdb_test "python print('val = %s' % val)" "val = \\{f = 1, array = \\{1, 2, 3, 4, 5\\}\\}"
> +    gdb_test "python print('val.is_lazy = %s' % val.is_lazy)" "val\\.is_lazy = False"
> +
> +    # Grab an element from the array within VAL.  The element should
> +    # immediately be non-lazy as the value contents can be copied
> +    # directly from VAL.
> +    gdb_test_no_output "python elem = val\['array'\]\[1\]"
> +    gdb_test "python print('elem.is_lazy = %s' % elem.is_lazy)" "elem\\.is_lazy = False"
> +    gdb_test "python print('elem = %s' % elem)" "elem = 2"
> +}
> diff --git a/gdb/valarith.c b/gdb/valarith.c
> index 0e204135bf2..a93940620cb 100644
> --- a/gdb/valarith.c
> +++ b/gdb/valarith.c
> @@ -162,17 +162,17 @@ value_subscript (struct value *array, LONGEST index)
>        if (VALUE_LVAL (array) != lval_memory)
>  	return value_subscripted_rvalue (array, index, *lowerbound);
>  
> -      if (!c_style)
> -	{
> -	  gdb::optional<LONGEST> upperbound
> -	    = get_discrete_high_bound (range_type);
> +      gdb::optional<LONGEST> upperbound
> +	= get_discrete_high_bound (range_type);
>  
> -	  if (!upperbound.has_value ())
> -	    upperbound = 0;
> +      if (!upperbound.has_value ())
> +	upperbound = -1;
>  
> -	  if (index >= *lowerbound && index <= *upperbound)
> -	    return value_subscripted_rvalue (array, index, *lowerbound);
> +      if (index >= *lowerbound && index <= *upperbound)
> +	return value_subscripted_rvalue (array, index, *lowerbound);
>  
> +      if (!c_style)
> +	{
>  	  /* Emit warning unless we have an array of unknown size.
>  	     An array of unknown size has lowerbound 0 and upperbound -1.  */
>  	  if (*upperbound > -1)
> @@ -200,7 +200,7 @@ value_subscripted_rvalue (struct value *array, LONGEST index,
>  			  LONGEST lowerbound)
>  {
>    struct type *array_type = check_typedef (value_type (array));
> -  struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (array_type));
> +  struct type *elt_type = TYPE_TARGET_TYPE (array_type);
>    LONGEST elt_size = type_length_units (elt_type);
>  
>    /* Fetch the bit stride and convert it to a byte stride, assuming 8 bits
> -- 
> 2.25.4
> 



More information about the Gdb-patches mailing list