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