From e6582e1b3c194b01a2461f5c5326fcf358303607 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Fri, 15 Oct 2021 15:16:20 +0100 Subject: [PATCH] gdb: improve reuse of value contents when fetching array elements 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 . */ + +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 . + +# 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 5ce931395cb..a0d0c602522 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 upperbound - = get_discrete_high_bound (range_type); + gdb::optional 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.30.2