[PATCH 0/5] Small cleanups in array_operation::evaluate

[PATCH 0/5] 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.

---
Tom Tromey (5):
      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

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

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

[PATCH 1/5] 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.
---
 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

Re: [PATCH 1/5] Use gdb::array_view for value_array

[Simon Marchi]

On 8/28/23 15:59, Tom Tromey via Gdb-patches wrote:
> 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)
>  {

I suppose it is necessary that "highbound - lowbound + 1" be equal to
elemvec.size()?

If so, we could add an assert to make sure it's true, or better (IMO),
remove highbound.

Simon

Re: [PATCH 1/5] Use gdb::array_view for value_array

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> struct value *
>> -value_array (int lowbound, int highbound, struct value **elemvec)
>> +value_array (int lowbound, int highbound,
>> +	     gdb::array_view<struct value *> elemvec)
>> {

Simon> I suppose it is necessary that "highbound - lowbound + 1" be equal to
Simon> elemvec.size()?

Simon> If so, we could add an assert to make sure it's true, or better (IMO),
Simon> remove highbound.

I've added a patch to do this.

Tom

[PATCH 2/5] 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'.
---
 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 3/5] Hoist array bounds check in array_operation::evaluate

[Tom Tromey]

This hoists the array bounds check in array_operation::evaluate to
before the loop.
---
 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 4/5] 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.
---
 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 5/5] 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.
---
 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

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

[John Baldwin]

On 8/28/23 12:59 PM, Tom Tromey via Gdb-patches 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.
> 
> ---
> Tom Tromey (5):
>        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
> 
>   gdb/eval.c      | 29 +++++++++++------------------
>   gdb/rust-lang.c |  2 +-
>   gdb/valops.c    |  3 ++-
>   gdb/value.h     |  2 +-
>   4 files changed, 15 insertions(+), 21 deletions(-)
> ---
> base-commit: b8a175b415454df6a039ba0b5d2ff13c3c180275
> change-id: 20230828-cleanup-array-op-49e3af3bd743
> 
> Best regards,

All lgtm.

It might be nice as a further cleanup to rename tem2 and tem3.  Based on
the arguments passed to value_array, the ideal names for those would be
something like 'lowbound' and 'highbound' but that clashes with the
'low_bound' and 'high_bound' used for the nested array. :(

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

-- 
John Baldwin

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

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> It might be nice as a further cleanup to rename tem2 and tem3.  Based on
John> the arguments passed to value_array, the ideal names for those would be
John> something like 'lowbound' and 'highbound' but that clashes with the
John> 'low_bound' and 'high_bound' used for the nested array. :(

I added a patch to do this.

Tom