[PATCH 0/2] Array indexing, lazy values, and C-like languages

[PATCH 0/2] Array indexing, lazy values, and C-like languages

[Andrew Burgess]

Create fewer lazy values when performing array indexing for C-like
languages.

Thanks,
Andrew


---

Andrew Burgess (2):
  gdb: make value_subscripted_rvalue static
  gdb: improve reuse of value contents when fetching array elements

 gdb/testsuite/gdb.base/non-lazy-array-index.c | 31 ++++++++
 .../gdb.base/non-lazy-array-index.exp         | 78 +++++++++++++++++++
 gdb/valarith.c                                | 28 ++++---
 gdb/value.h                                   |  4 -
 4 files changed, 126 insertions(+), 15 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

-- 
2.25.4

[PATCH 1/2] gdb: make value_subscripted_rvalue static

[Andrew Burgess]

The function value_subscripted_rvalue is only used in valarith.c, so
lets make it a static function.

There should be no user visible change after this commit.
---
 gdb/valarith.c | 10 ++++++++--
 gdb/value.h    |  4 ----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/valarith.c b/gdb/valarith.c
index 07472ef7c8a..0e204135bf2 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -29,6 +29,11 @@
 #include "gdbsupport/byte-vector.h"
 #include "gdbarch.h"
 
+/* Forward declarations.  */
+static struct value *value_subscripted_rvalue (struct value *array,
+					       LONGEST index,
+					       LONGEST lowerbound);
+
 /* Define whether or not the C operator '/' truncates towards zero for
    differently signed operands (truncation direction is undefined in C).  */
 
@@ -190,8 +195,9 @@ value_subscript (struct value *array, LONGEST index)
    (eg, a vector register).  This routine used to promote floats
    to doubles, but no longer does.  */
 
-struct value *
-value_subscripted_rvalue (struct value *array, LONGEST index, LONGEST lowerbound)
+static struct value *
+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));
diff --git a/gdb/value.h b/gdb/value.h
index 45012372dbf..11c22ddf14f 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1165,10 +1165,6 @@ extern struct value *find_function_in_inferior (const char *,
 
 extern struct value *value_allocate_space_in_inferior (int);
 
-extern struct value *value_subscripted_rvalue (struct value *array,
-					       LONGEST index,
-					       LONGEST lowerbound);
-
 /* User function handler.  */
 
 typedef struct value *(*internal_function_fn) (struct gdbarch *gdbarch,
-- 
2.25.4

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

[Andrew Burgess]

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

Re: [PATCH 1/2] gdb: make value_subscripted_rvalue static

[Simon Marchi]

On 2021-10-15 11:39 a.m., Andrew Burgess wrote:
> The function value_subscripted_rvalue is only used in valarith.c, so
> lets make it a static function.
> 
> There should be no user visible change after this commit.
> ---
>  gdb/valarith.c | 10 ++++++++--
>  gdb/value.h    |  4 ----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/valarith.c b/gdb/valarith.c
> index 07472ef7c8a..0e204135bf2 100644
> --- a/gdb/valarith.c
> +++ b/gdb/valarith.c
> @@ -29,6 +29,11 @@
>  #include "gdbsupport/byte-vector.h"
>  #include "gdbarch.h"
>  
> +/* Forward declarations.  */
> +static struct value *value_subscripted_rvalue (struct value *array,
> +					       LONGEST index,
> +					       LONGEST lowerbound);
> +
>  /* Define whether or not the C operator '/' truncates towards zero for
>     differently signed operands (truncation direction is undefined in C).  */
>  
> @@ -190,8 +195,9 @@ value_subscript (struct value *array, LONGEST index)
>     (eg, a vector register).  This routine used to promote floats
>     to doubles, but no longer does.  */
>  
> -struct value *
> -value_subscripted_rvalue (struct value *array, LONGEST index, LONGEST lowerbound)
> +static struct value *
> +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));
> diff --git a/gdb/value.h b/gdb/value.h
> index 45012372dbf..11c22ddf14f 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -1165,10 +1165,6 @@ extern struct value *find_function_in_inferior (const char *,
>  
>  extern struct value *value_allocate_space_in_inferior (int);
>  
> -extern struct value *value_subscripted_rvalue (struct value *array,
> -					       LONGEST index,
> -					       LONGEST lowerbound);
> -
>  /* User function handler.  */
>  
>  typedef struct value *(*internal_function_fn) (struct gdbarch *gdbarch,
> -- 
> 2.25.4
> 


This one seems obvious to me.

Simon

Re: [PATCH 1/2] gdb: make value_subscripted_rvalue static

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2021-10-15 11:42:26 -0400]:

> On 2021-10-15 11:39 a.m., Andrew Burgess wrote:
> > The function value_subscripted_rvalue is only used in valarith.c, so
> > lets make it a static function.
> > 
> > There should be no user visible change after this commit.
> > ---
> >  gdb/valarith.c | 10 ++++++++--
> >  gdb/value.h    |  4 ----
> >  2 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/gdb/valarith.c b/gdb/valarith.c
> > index 07472ef7c8a..0e204135bf2 100644
> > --- a/gdb/valarith.c
> > +++ b/gdb/valarith.c
> > @@ -29,6 +29,11 @@
> >  #include "gdbsupport/byte-vector.h"
> >  #include "gdbarch.h"
> >  
> > +/* Forward declarations.  */
> > +static struct value *value_subscripted_rvalue (struct value *array,
> > +					       LONGEST index,
> > +					       LONGEST lowerbound);
> > +
> >  /* Define whether or not the C operator '/' truncates towards zero for
> >     differently signed operands (truncation direction is undefined in C).  */
> >  
> > @@ -190,8 +195,9 @@ value_subscript (struct value *array, LONGEST index)
> >     (eg, a vector register).  This routine used to promote floats
> >     to doubles, but no longer does.  */
> >  
> > -struct value *
> > -value_subscripted_rvalue (struct value *array, LONGEST index, LONGEST lowerbound)
> > +static struct value *
> > +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));
> > diff --git a/gdb/value.h b/gdb/value.h
> > index 45012372dbf..11c22ddf14f 100644
> > --- a/gdb/value.h
> > +++ b/gdb/value.h
> > @@ -1165,10 +1165,6 @@ extern struct value *find_function_in_inferior (const char *,
> >  
> >  extern struct value *value_allocate_space_in_inferior (int);
> >  
> > -extern struct value *value_subscripted_rvalue (struct value *array,
> > -					       LONGEST index,
> > -					       LONGEST lowerbound);
> > -
> >  /* User function handler.  */
> >  
> >  typedef struct value *(*internal_function_fn) (struct gdbarch *gdbarch,
> > -- 
> > 2.25.4
> > 
> 
> 
> This one seems obvious to me.

Thanks, I pushed this patch.

Andrew

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

[Andrew Burgess]

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
> 

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

[Andrew Burgess]

I've gone ahead and pushed this patch.  If anyone sees any problems,
then please just let me know.

Thanks,
Andrew

* Andrew Burgess <aburgess@redhat.com> [2021-12-03 11:11:42 +0000]:

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