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

Simon Marchi via Gdb-patches
2023-02-01T15:44:03Z

On 1/31/23 14:49, Thiago Jung Bauermann via Gdb-patches wrote:
>
> 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.

Probably, I will take a look (Andrew has left a comment already though).

Simon