Re: [PATCH v3 1/8] gdbserver: Add assert in find_register_by_number

Thiago Jung Bauermann via Gdb-patches
2023-01-31T19:50:19Z

Hello Simon,

Simon Marchi <simon.marchi@efficios.com> writes:

> On 1/29/23 23:45, Thiago Jung Bauermann wrote:
>> It helped me during development, catching bugs closer to when they actually
>> happened.
>>
>> Also remove the equivalent gdb_assert in regcache_raw_read_unsigned, since
>> it's checking the same condition a few frames above.
>>
>> Suggested-By: Simon Marchi <simon.marchi@efficios.com>
>> ---
>> gdbserver/regcache.cc | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 3aeefcc79a37..7b896a19767d 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -199,6 +199,8 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
>> static const struct gdb::reg &
>> find_register_by_number (const struct target_desc *tdesc, int n)
>> {
>> + gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
>
> Since you're moving this assertion, I would suggest breaking it up in
> two. I general, I suggest avoiding multiple checks in a single
> gdb_assert. It makes it a little less obvious from the crash report
> which condition failed exactly. So:
>
> gdb_assert (n >= 0);
> gdb_assert (n < tdesc->reg_defs.size ());

Good point. I made this change.

> The patch is fine to push right away in any case, it's good
> independently from the rest of the series:

Indeed. I will do that. Is it OK if I push patch 2 as well? You approved
it in v2, and the only changes in v3 are to implement your review
comments.

> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Thanks for your review!

--
Thiago