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

[Simon Marchi <simark@simark.ca>]

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