* [PATCH 0/5] Small cleanups in array_operation::evaluate
@ 2023-08-28 19:59 Tom Tromey
2023-08-28 19:59 ` [PATCH 1/5] Use gdb::array_view for value_array Tom Tromey
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Tom Tromey @ 2023-08-28 19:59 UTC (permalink / raw)
To: gdb-patches
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/5] Use gdb::array_view for value_array
2023-08-28 19:59 [PATCH 0/5] Small cleanups in array_operation::evaluate Tom Tromey
@ 2023-08-28 19:59 ` Tom Tromey
2023-08-28 21:10 ` Simon Marchi
2023-08-28 19:59 ` [PATCH 2/5] Declare 'tem' in loop header in array_operation::evaluate Tom Tromey
` (4 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Tom Tromey @ 2023-08-28 19:59 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/5] Declare 'tem' in loop header in array_operation::evaluate
2023-08-28 19:59 [PATCH 0/5] Small cleanups in array_operation::evaluate Tom Tromey
2023-08-28 19:59 ` [PATCH 1/5] Use gdb::array_view for value_array Tom Tromey
@ 2023-08-28 19:59 ` Tom Tromey
2023-08-28 19:59 ` [PATCH 3/5] Hoist array bounds check " Tom Tromey
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2023-08-28 19:59 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/5] Hoist array bounds check in array_operation::evaluate
2023-08-28 19:59 [PATCH 0/5] Small cleanups in array_operation::evaluate Tom Tromey
2023-08-28 19:59 ` [PATCH 1/5] Use gdb::array_view for value_array Tom Tromey
2023-08-28 19:59 ` [PATCH 2/5] Declare 'tem' in loop header in array_operation::evaluate Tom Tromey
@ 2023-08-28 19:59 ` Tom Tromey
2023-08-28 19:59 ` [PATCH 4/5] Remove redundant variable from array_operation::evaluate Tom Tromey
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2023-08-28 19:59 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4/5] Remove redundant variable from array_operation::evaluate
2023-08-28 19:59 [PATCH 0/5] Small cleanups in array_operation::evaluate Tom Tromey
` (2 preceding siblings ...)
2023-08-28 19:59 ` [PATCH 3/5] Hoist array bounds check " Tom Tromey
@ 2023-08-28 19:59 ` Tom Tromey
2023-08-28 19:59 ` [PATCH 5/5] Remove another " Tom Tromey
2023-08-28 21:12 ` [PATCH 0/5] Small cleanups in array_operation::evaluate John Baldwin
5 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2023-08-28 19:59 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 5/5] Remove another redundant variable from array_operation::evaluate
2023-08-28 19:59 [PATCH 0/5] Small cleanups in array_operation::evaluate Tom Tromey
` (3 preceding siblings ...)
2023-08-28 19:59 ` [PATCH 4/5] Remove redundant variable from array_operation::evaluate Tom Tromey
@ 2023-08-28 19:59 ` Tom Tromey
2023-08-28 21:12 ` [PATCH 0/5] Small cleanups in array_operation::evaluate John Baldwin
5 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2023-08-28 19:59 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] Use gdb::array_view for value_array
2023-08-28 19:59 ` [PATCH 1/5] Use gdb::array_view for value_array Tom Tromey
@ 2023-08-28 21:10 ` Simon Marchi
2023-08-29 15:13 ` Tom Tromey
0 siblings, 1 reply; 10+ messages in thread
From: Simon Marchi @ 2023-08-28 21:10 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/5] Small cleanups in array_operation::evaluate
2023-08-28 19:59 [PATCH 0/5] Small cleanups in array_operation::evaluate Tom Tromey
` (4 preceding siblings ...)
2023-08-28 19:59 ` [PATCH 5/5] Remove another " Tom Tromey
@ 2023-08-28 21:12 ` John Baldwin
2023-08-29 15:35 ` Tom Tromey
5 siblings, 1 reply; 10+ messages in thread
From: John Baldwin @ 2023-08-28 21:12 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] Use gdb::array_view for value_array
2023-08-28 21:10 ` Simon Marchi
@ 2023-08-29 15:13 ` Tom Tromey
0 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2023-08-29 15:13 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/5] Small cleanups in array_operation::evaluate
2023-08-28 21:12 ` [PATCH 0/5] Small cleanups in array_operation::evaluate John Baldwin
@ 2023-08-29 15:35 ` Tom Tromey
0 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2023-08-29 15:35 UTC (permalink / raw)
To: John Baldwin; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-08-29 15:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-28 19:59 [PATCH 0/5] Small cleanups in array_operation::evaluate Tom Tromey
2023-08-28 19:59 ` [PATCH 1/5] Use gdb::array_view for value_array Tom Tromey
2023-08-28 21:10 ` Simon Marchi
2023-08-29 15:13 ` Tom Tromey
2023-08-28 19:59 ` [PATCH 2/5] Declare 'tem' in loop header in array_operation::evaluate Tom Tromey
2023-08-28 19:59 ` [PATCH 3/5] Hoist array bounds check " Tom Tromey
2023-08-28 19:59 ` [PATCH 4/5] Remove redundant variable from array_operation::evaluate Tom Tromey
2023-08-28 19:59 ` [PATCH 5/5] Remove another " Tom Tromey
2023-08-28 21:12 ` [PATCH 0/5] Small cleanups in array_operation::evaluate John Baldwin
2023-08-29 15:35 ` Tom Tromey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).