[binutils-gdb] gdb: improve reuse of value contents when fetching array elements

[binutils-gdb] gdb: improve reuse of value contents when fetching array elements

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e6582e1b3c194b01a2461f5c5326fcf358303607

commit e6582e1b3c194b01a2461f5c5326fcf358303607
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Fri Oct 15 15:16:20 2021 +0100

    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!

Diff:
---
 gdb/testsuite/gdb.base/non-lazy-array-index.c   | 31 ++++++++++
 gdb/testsuite/gdb.base/non-lazy-array-index.exp | 78 +++++++++++++++++++++++++
 gdb/valarith.c                                  | 18 +++---
 3 files changed, 118 insertions(+), 9 deletions(-)

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