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

  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(-)
  

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 ());

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

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

Simon
  

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!
  

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