[PATCH v2 0/7] Small cleanups in array_operation::evaluate

[PATCH v2 0/7] Small cleanups in array_operation::evaluate

[Tom Tromey]

While working on another, larger, series, I found a few small cleanups
to do in array_operation::evaluate.  I've made each one a separate
patch to try to keep each one reasonably obvious.

Regression tested on x86-64 Fedora 36.

---
Changes in v2:
- Added two more cleanup patches
- Link to v1: https://inbox.sourceware.org/gdb-patches/20230828-cleanup-array-op-v1-0-12ca00f20917@adacore.com

---
Tom Tromey (7):
      Use gdb::array_view for value_array
      Declare 'tem' in loop header in array_operation::evaluate
      Hoist array bounds check in array_operation::evaluate
      Remove redundant variable from array_operation::evaluate
      Remove another redundant variable from array_operation::evaluate
      Remove "highbound" parameter from value_array
      More renames in array_operation::evaluate

 gdb/eval.c      | 34 +++++++++++++---------------------
 gdb/rust-lang.c |  2 +-
 gdb/valops.c    | 21 ++++++++-------------
 gdb/value.h     |  4 ++--
 4 files changed, 24 insertions(+), 37 deletions(-)
---
base-commit: aa7b36b832a1475fad2f184e0b4b58fb2f12241f
change-id: 20230828-cleanup-array-op-49e3af3bd743

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH v2 1/7] Use gdb::array_view for value_array

[Tom Tromey]

This changes value_array to accept an array view.  I also replaced an
alloca with a std::vector in array_operation::evaluate.  This function
can work on any size of array, so it seems bad to use alloca.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>
---
 gdb/eval.c      | 2 +-
 gdb/rust-lang.c | 2 +-
 gdb/valops.c    | 3 ++-
 gdb/value.h     | 2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index 457a6697923..00b9231a5b9 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2515,7 +2515,7 @@ array_operation::evaluate (struct type *expect_type,
       return set;
     }
 
-  value **argvec = XALLOCAVEC (struct value *, nargs);
+  std::vector<value *> argvec (nargs);
   for (tem = 0; tem < nargs; tem++)
     {
       /* Ensure that array expressions are coerced into pointer
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 5eb33d0d3b7..f6e7d25f587 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -1344,7 +1344,7 @@ eval_op_rust_array (struct type *expect_type, struct expression *exp,
 
       for (i = 0; i < copies; ++i)
 	eltvec[i] = elt;
-      return value_array (0, copies - 1, eltvec.data ());
+      return value_array (0, copies - 1, eltvec);
     }
   else
     {
diff --git a/gdb/valops.c b/gdb/valops.c
index ea9d9b38e74..1133049c54a 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1692,7 +1692,8 @@ value_ind (struct value *arg1)
    don't currently enforce any restriction on their types).  */
 
 struct value *
-value_array (int lowbound, int highbound, struct value **elemvec)
+value_array (int lowbound, int highbound,
+	     gdb::array_view<struct value *> elemvec)
 {
   int nelem;
   int idx;
diff --git a/gdb/value.h b/gdb/value.h
index e5c63dccbe2..ccf52199146 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1226,7 +1226,7 @@ inline struct value *value_string (const char *ptr, ssize_t count,
 }
 
 extern struct value *value_array (int lowbound, int highbound,
-				  struct value **elemvec);
+				  gdb::array_view<struct value *> elemvec);
 
 extern struct value *value_concat (struct value *arg1, struct value *arg2);
 

-- 
2.40.1

[PATCH v2 2/7] Declare 'tem' in loop header in array_operation::evaluate

[Tom Tromey]

This changes array_operation::evaluate to declare the 'tem' variable
in the loop header, rather than at the top of the function.  This is
cleaner and easier to reason about.  I also changed 'nargs' to be
'const'.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>
---
 gdb/eval.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index 00b9231a5b9..63c414e546e 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2397,11 +2397,10 @@ array_operation::evaluate (struct type *expect_type,
 			   struct expression *exp,
 			   enum noside noside)
 {
-  int tem;
   int tem2 = std::get<0> (m_storage);
   int tem3 = std::get<1> (m_storage);
   const std::vector<operation_up> &in_args = std::get<2> (m_storage);
-  int nargs = tem3 - tem2 + 1;
+  const int nargs = tem3 - tem2 + 1;
   struct type *type = expect_type ? check_typedef (expect_type) : nullptr;
 
   if (expect_type != nullptr
@@ -2429,7 +2428,7 @@ array_operation::evaluate (struct type *expect_type,
 	}
       index = low_bound;
       memset (array->contents_raw ().data (), 0, expect_type->length ());
-      for (tem = nargs; --nargs >= 0;)
+      for (int tem = 0; tem < nargs; ++tem)
 	{
 	  struct value *element;
 
@@ -2467,7 +2466,7 @@ array_operation::evaluate (struct type *expect_type,
 	error (_("(power)set type with unknown size"));
       memset (valaddr, '\0', type->length ());
       int idx = 0;
-      for (tem = 0; tem < nargs; tem++)
+      for (int tem = 0; tem < nargs; tem++)
 	{
 	  LONGEST range_low, range_high;
 	  struct type *range_low_type, *range_high_type;
@@ -2516,7 +2515,7 @@ array_operation::evaluate (struct type *expect_type,
     }
 
   std::vector<value *> argvec (nargs);
-  for (tem = 0; tem < nargs; tem++)
+  for (int tem = 0; tem < nargs; tem++)
     {
       /* Ensure that array expressions are coerced into pointer
 	 objects.  */

-- 
2.40.1

[PATCH v2 3/7] Hoist array bounds check in array_operation::evaluate

[Tom Tromey]

This hoists the array bounds check in array_operation::evaluate to
before the loop.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>
---
 gdb/eval.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index 63c414e546e..6cf72545f62 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2426,6 +2426,8 @@ array_operation::evaluate (struct type *expect_type,
 	  low_bound = 0;
 	  high_bound = (type->length () / element_size) - 1;
 	}
+      if (low_bound + nargs - 1 > high_bound)
+	error (_("Too many array elements"));
       index = low_bound;
       memset (array->contents_raw ().data (), 0, expect_type->length ());
       for (int tem = 0; tem < nargs; ++tem)
@@ -2436,9 +2438,6 @@ array_operation::evaluate (struct type *expect_type,
 							  exp, noside);
 	  if (element->type () != element_type)
 	    element = value_cast (element_type, element);
-	  if (index > high_bound)
-	    /* To avoid memory corruption.  */
-	    error (_("Too many array elements"));
 	  memcpy (array->contents_raw ().data ()
 		  + (index - low_bound) * element_size,
 		  element->contents ().data (),

-- 
2.40.1

[PATCH v2 4/7] Remove redundant variable from array_operation::evaluate

[Tom Tromey]

In array_operation::evaluate, 'idx' and 'tem' are redundant in one
branch.  This patch merges them, using the clearer name.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>
---
 gdb/eval.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index 6cf72545f62..710506ef778 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2464,14 +2464,13 @@ array_operation::evaluate (struct type *expect_type,
       if (!get_discrete_bounds (element_type, &low_bound, &high_bound))
 	error (_("(power)set type with unknown size"));
       memset (valaddr, '\0', type->length ());
-      int idx = 0;
-      for (int tem = 0; tem < nargs; tem++)
+      for (int idx = 0; idx < nargs; idx++)
 	{
 	  LONGEST range_low, range_high;
 	  struct type *range_low_type, *range_high_type;
 	  struct value *elem_val;
 
-	  elem_val = in_args[idx++]->evaluate (element_type, exp, noside);
+	  elem_val = in_args[idx]->evaluate (element_type, exp, noside);
 	  range_low_type = range_high_type = elem_val->type ();
 	  range_low = range_high = value_as_long (elem_val);
 

-- 
2.40.1

[PATCH v2 5/7] Remove another redundant variable from array_operation::evaluate

[Tom Tromey]

This removes yet another redundant variable from
array_operation::evaluate -- only one index is needed.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>
---
 gdb/eval.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index 710506ef778..8dd1b530d06 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2419,7 +2419,7 @@ array_operation::evaluate (struct type *expect_type,
       struct type *element_type = type->target_type ();
       struct value *array = value::allocate (expect_type);
       int element_size = check_typedef (element_type)->length ();
-      LONGEST low_bound, high_bound, index;
+      LONGEST low_bound, high_bound;
 
       if (!get_discrete_bounds (range_type, &low_bound, &high_bound))
 	{
@@ -2428,21 +2428,17 @@ array_operation::evaluate (struct type *expect_type,
 	}
       if (low_bound + nargs - 1 > high_bound)
 	error (_("Too many array elements"));
-      index = low_bound;
       memset (array->contents_raw ().data (), 0, expect_type->length ());
-      for (int tem = 0; tem < nargs; ++tem)
+      for (int idx = 0; idx < nargs; ++idx)
 	{
 	  struct value *element;
 
-	  element = in_args[index - low_bound]->evaluate (element_type,
-							  exp, noside);
+	  element = in_args[idx]->evaluate (element_type, exp, noside);
 	  if (element->type () != element_type)
 	    element = value_cast (element_type, element);
-	  memcpy (array->contents_raw ().data ()
-		  + (index - low_bound) * element_size,
+	  memcpy (array->contents_raw ().data () + idx * element_size,
 		  element->contents ().data (),
 		  element_size);
-	  index++;
 	}
       return array;
     }

-- 
2.40.1

[PATCH v2 6/7] Remove "highbound" parameter from value_array

[Tom Tromey]

value_array requires the passed-in bounds to match the length of the
array_view it is given.  This patch removes the redundant "highbound"
parameter.
---
 gdb/eval.c      |  2 +-
 gdb/rust-lang.c |  2 +-
 gdb/valops.c    | 22 ++++++++--------------
 gdb/value.h     |  2 +-
 4 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index 8dd1b530d06..7c955a4e9eb 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2515,7 +2515,7 @@ array_operation::evaluate (struct type *expect_type,
 	 objects.  */
       argvec[tem] = in_args[tem]->evaluate_with_coercion (exp, noside);
     }
-  return value_array (tem2, tem3, argvec);
+  return value_array (tem2, argvec);
 }
 
 value *
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index f6e7d25f587..0e2ca090ba8 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -1344,7 +1344,7 @@ eval_op_rust_array (struct type *expect_type, struct expression *exp,
 
       for (i = 0; i < copies; ++i)
 	eltvec[i] = elt;
-      return value_array (0, copies - 1, eltvec);
+      return value_array (0, eltvec);
     }
   else
     {
diff --git a/gdb/valops.c b/gdb/valops.c
index 1133049c54a..081c621594e 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1684,18 +1684,16 @@ value_ind (struct value *arg1)
 /* Create a value for an array by allocating space in GDB, copying the
    data into that space, and then setting up an array value.
 
-   The array bounds are set from LOWBOUND and HIGHBOUND, and the array
-   is populated from the values passed in ELEMVEC.
+   The array bounds are set from LOWBOUND and the size of ELEMVEC, and
+   the array is populated from the values passed in ELEMVEC.
 
    The element type of the array is inherited from the type of the
    first element, and all elements must have the same size (though we
    don't currently enforce any restriction on their types).  */
 
 struct value *
-value_array (int lowbound, int highbound,
-	     gdb::array_view<struct value *> elemvec)
+value_array (int lowbound, gdb::array_view<struct value *> elemvec)
 {
-  int nelem;
   int idx;
   ULONGEST typelength;
   struct value *val;
@@ -1704,13 +1702,8 @@ value_array (int lowbound, int highbound,
   /* Validate that the bounds are reasonable and that each of the
      elements have the same size.  */
 
-  nelem = highbound - lowbound + 1;
-  if (nelem <= 0)
-    {
-      error (_("bad array bounds (%d, %d)"), lowbound, highbound);
-    }
   typelength = type_length_units (elemvec[0]->enclosing_type ());
-  for (idx = 1; idx < nelem; idx++)
+  for (idx = 1; idx < elemvec.size (); idx++)
     {
       if (type_length_units (elemvec[idx]->enclosing_type ())
 	  != typelength)
@@ -1720,12 +1713,13 @@ value_array (int lowbound, int highbound,
     }
 
   arraytype = lookup_array_range_type (elemvec[0]->enclosing_type (),
-				       lowbound, highbound);
+				       lowbound,
+				       lowbound + elemvec.size () - 1);
 
   if (!current_language->c_style_arrays_p ())
     {
       val = value::allocate (arraytype);
-      for (idx = 0; idx < nelem; idx++)
+      for (idx = 0; idx < elemvec.size (); idx++)
 	elemvec[idx]->contents_copy (val, idx * typelength, 0, typelength);
       return val;
     }
@@ -1734,7 +1728,7 @@ value_array (int lowbound, int highbound,
      copying in each element.  */
 
   val = value::allocate (arraytype);
-  for (idx = 0; idx < nelem; idx++)
+  for (idx = 0; idx < elemvec.size (); idx++)
     elemvec[idx]->contents_copy (val, idx * typelength, 0, typelength);
   return val;
 }
diff --git a/gdb/value.h b/gdb/value.h
index ccf52199146..c28b731cc42 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1225,7 +1225,7 @@ inline struct value *value_string (const char *ptr, ssize_t count,
   return value_string ((const gdb_byte *) ptr, count, char_type);
 }
 
-extern struct value *value_array (int lowbound, int highbound,
+extern struct value *value_array (int lowbound,
 				  gdb::array_view<struct value *> elemvec);
 
 extern struct value *value_concat (struct value *arg1, struct value *arg2);

-- 
2.40.1

Re: [PATCH v2 6/7] Remove "highbound" parameter from value_array

[Simon Marchi]

> @@ -1704,13 +1702,8 @@ value_array (int lowbound, int highbound,
>    /* Validate that the bounds are reasonable and that each of the
>       elements have the same size.  */
>  
> -  nelem = highbound - lowbound + 1;
> -  if (nelem <= 0)
> -    {
> -      error (_("bad array bounds (%d, %d)"), lowbound, highbound);
> -    }
>    typelength = type_length_units (elemvec[0]->enclosing_type ());
> -  for (idx = 1; idx < nelem; idx++)
> +  for (idx = 1; idx < elemvec.size (); idx++)
>      {
>        if (type_length_units (elemvec[idx]->enclosing_type ())
>  	  != typelength)

I don't know if you want to make that change, but that loop can become
a range for loop.

Simon

Re: [PATCH v2 6/7] Remove "highbound" parameter from value_array

[Tom Tromey]

>> -  for (idx = 1; idx < nelem; idx++)
>> +  for (idx = 1; idx < elemvec.size (); idx++)
>> {
>> if (type_length_units (elemvec[idx]->enclosing_type ())
>> != typelength)

Simon> I don't know if you want to make that change, but that loop can become
Simon> a range for loop.

I did this in v3.

Tom

[PATCH v2 7/7] More renames in array_operation::evaluate

[Tom Tromey]

array_operation::evaluate has variables named "tem2" and "tem3".  This
patch replaces one with a better name, and entirely removes the other.
---
 gdb/eval.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/eval.c b/gdb/eval.c
index 7c955a4e9eb..81b7aa0cb99 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2397,10 +2397,9 @@ array_operation::evaluate (struct type *expect_type,
 			   struct expression *exp,
 			   enum noside noside)
 {
-  int tem2 = std::get<0> (m_storage);
-  int tem3 = std::get<1> (m_storage);
+  const int provided_low_bound = std::get<0> (m_storage);
   const std::vector<operation_up> &in_args = std::get<2> (m_storage);
-  const int nargs = tem3 - tem2 + 1;
+  const int nargs = std::get<1> (m_storage) - provided_low_bound + 1;
   struct type *type = expect_type ? check_typedef (expect_type) : nullptr;
 
   if (expect_type != nullptr
@@ -2515,7 +2514,7 @@ array_operation::evaluate (struct type *expect_type,
 	 objects.  */
       argvec[tem] = in_args[tem]->evaluate_with_coercion (exp, noside);
     }
-  return value_array (tem2, argvec);
+  return value_array (provided_low_bound, argvec);
 }
 
 value *

-- 
2.40.1

Re: [PATCH v2 0/7] Small cleanups in array_operation::evaluate

[John Baldwin]

On 8/29/23 8:34 AM, Tom Tromey wrote:
> While working on another, larger, series, I found a few small cleanups
> to do in array_operation::evaluate.  I've made each one a separate
> patch to try to keep each one reasonably obvious.
> 
> Regression tested on x86-64 Fedora 36.
> 
> ---
> Changes in v2:
> - Added two more cleanup patches
> - Link to v1: https://inbox.sourceware.org/gdb-patches/20230828-cleanup-array-op-v1-0-12ca00f20917@adacore.com
> 
> ---
> Tom Tromey (7):
>        Use gdb::array_view for value_array
>        Declare 'tem' in loop header in array_operation::evaluate
>        Hoist array bounds check in array_operation::evaluate
>        Remove redundant variable from array_operation::evaluate
>        Remove another redundant variable from array_operation::evaluate
>        Remove "highbound" parameter from value_array
>        More renames in array_operation::evaluate
> 
>   gdb/eval.c      | 34 +++++++++++++---------------------
>   gdb/rust-lang.c |  2 +-
>   gdb/valops.c    | 21 ++++++++-------------
>   gdb/value.h     |  4 ++--
>   4 files changed, 24 insertions(+), 37 deletions(-)
> ---
> base-commit: aa7b36b832a1475fad2f184e0b4b58fb2f12241f
> change-id: 20230828-cleanup-array-op-49e3af3bd743
> 
> Best regards,

The new patches (6 and 7) look good to me.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>

-- 
John Baldwin