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

[Simon Marchi <simark@simark.ca>]

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