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

Simon Marchi via Gdb-patches
2023-01-31T17:05:55Z

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