[v2,1/6] gdbserver: Add asserts in register_size and register_data functions

  These helped me during development, catching bugs closer to when they
actually happened.
---
 gdbserver/regcache.cc | 4 ++++
 1 file changed, 4 insertions(+)
  

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> These helped me during development, catching bugs closer to when they
> actually happened.
> ---
>   gdbserver/regcache.cc | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 5cbcea978a05..14236069f712 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>   int
>   register_size (const struct target_desc *tdesc, int n)
>   {
> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
> +
>     return find_register_by_number (tdesc, n).size / 8;
>   }
>   
> @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>   static unsigned char *
>   register_data (const struct regcache *regcache, int n)
>   {
> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
> +
>     return (regcache->registers
>   	  + find_register_by_number (regcache->tdesc, n).offset / 8);
>   }

LGTM.

Reviewed-by: Luis Machado <luis.machado@arm.com>
  

On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
> These helped me during development, catching bugs closer to when they
> actually happened.
> ---
>  gdbserver/regcache.cc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 5cbcea978a05..14236069f712 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>  int
>  register_size (const struct target_desc *tdesc, int n)
>  {
> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
> +
>    return find_register_by_number (tdesc, n).size / 8;
>  }
>  
> @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>  static unsigned char *
>  register_data (const struct regcache *regcache, int n)
>  {
> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());

Missing space before parenthesis.

I don't know if that would have helped you, but given that
find_register_by_number is implemented as an std::vector lookup, it
would probably have been caught if building with -D_GLIBCXX_DEBUG.  I
recommend using that for development, it's really handy.

https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html

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

Simon
  

On 11/28/22 09:48, Simon Marchi wrote:
> 
> 
> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> These helped me during development, catching bugs closer to when they
>> actually happened.
>> ---
>>  gdbserver/regcache.cc | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 5cbcea978a05..14236069f712 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>>  int
>>  register_size (const struct target_desc *tdesc, int n)
>>  {
>> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
>> +
>>    return find_register_by_number (tdesc, n).size / 8;
>>  }
>>  
>> @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>>  static unsigned char *
>>  register_data (const struct regcache *regcache, int n)
>>  {
>> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
> 
> Missing space before parenthesis.
> 
> I don't know if that would have helped you, but given that
> find_register_by_number is implemented as an std::vector lookup, it
> would probably have been caught if building with -D_GLIBCXX_DEBUG.  I
> recommend using that for development, it's really handy.
> 
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Actually, I would perhaps suggest moving the assertion checks to
find_register_by_number, the place that actually accesses reg_defs.

And we could perhaps remove the equivalent gdb_assert in
regcache_raw_read_unsigned, since it's checking the same a few frames
above.

Simon
  

Hello Simon,

Thank you for your review!

Simon Marchi <simark@simark.ca> writes:

> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> These helped me during development, catching bugs closer to when they
>> actually happened.
>> ---
>>  gdbserver/regcache.cc | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 5cbcea978a05..14236069f712 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>>  int
>>  register_size (const struct target_desc *tdesc, int n)
>>  {
>> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
>> +
>>    return find_register_by_number (tdesc, n).size / 8;
>>  }
>>  
>> @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>>  static unsigned char *
>>  register_data (const struct regcache *regcache, int n)
>>  {
>> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
>
> Missing space before parenthesis.
>
> I don't know if that would have helped you, but given that
> find_register_by_number is implemented as an std::vector lookup, it
> would probably have been caught if building with -D_GLIBCXX_DEBUG.  I
> recommend using that for development, it's really handy.
>
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html

Nice, I will start using that flag on my development builds. Thank you
for the hint!
  

Simon Marchi <simark@simark.ca> writes:

> On 11/28/22 09:48, Simon Marchi wrote:
>> 
>> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> Actually, I would perhaps suggest moving the assertion checks to
> find_register_by_number, the place that actually accesses reg_defs.

Good idea. I'll do that in v3.

> And we could perhaps remove the equivalent gdb_assert in
> regcache_raw_read_unsigned, since it's checking the same a few frames
> above.

Good point. Will do.
  

Hello Luis,

Thank you for the quick review!

Luis Machado <luis.machado@arm.com> writes:

> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> These helped me during development, catching bugs closer to when they
>> actually happened.
>> ---
>>   gdbserver/regcache.cc | 4 ++++
>>   1 file changed, 4 insertions(+)
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 5cbcea978a05..14236069f712 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>>   int
>>   register_size (const struct target_desc *tdesc, int n)
>>   {
>> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
>> +
>>     return find_register_by_number (tdesc, n).size / 8;
>>   }
>>   @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>>   static unsigned char *
>>   register_data (const struct regcache *regcache, int n)
>>   {
>> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
>> +
>>     return (regcache->registers
>>   	  + find_register_by_number (regcache->tdesc, n).offset / 8);
>>   }
>
> LGTM.
>
> Reviewed-by: Luis Machado <luis.machado@arm.com>

With Simon's suggestion, this patch will be different in v3 so
unfortunately I won't be able to add this Reviewed-by.