RE: [PATCH v2 05/24] gdb: change regcache interface to use array_view

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Friday, November 24, 2023 10:26 PM, Simon Marchi wrote:
> New in v2:
> 
>  - remove dummy float parameters
> 
> Change most of regcache (and base classes) to use array_view when
> possible, instead of raw pointers.  By propagating the use of array_view
> further, it enables having some runtime checks to make sure the what we
> read from or write to regcaches has the expected length (such as the one
> in the `copy(array_view, array_view)` function.  It also integrates well
> when connecting with other APIs already using gdb::array_view.
> 
> Add some overloads of the methods using raw pointers to avoid having to
> change all call sites at once (which is both a lot of work and risky).
> 
> I tried to do this change in small increments, but since many of these
> functions use each other, it ended up simpler to do it in one shot than
> having a lot of intermediary / transient changes.
> 
> This change extends into gdbserver as well, because there is some part
> of the regcache interface that is shared.
> 
> Changing the reg_buffer_common interface to use array_view caused some
> build failures in nat/aarch64-scalable-linux-ptrace.c.  That file
> currently "takes advantage" of the fact that
> reg_buffer_common::{raw_supply,raw_collect} operates on `void *`, which
> IMO is dangerous.  It uses raw_supply/raw_collect directly on
> uint64_t's, which I guess is fine because it is expected that native
> code will have the same endianness as the debugged process.  To
> accomodate that, add some overloads of raw_collect and raw_supply that
> work on uint64_t.
> 
> This file also uses raw_collect and raw_supply on `char` pointers.
> Change it to use `gdb_byte` pointers instead.  Add overloads of
> raw_collect and raw_supply that work on `gdb_byte *` and make an
> array_view on the fly using the register's size.  Those call sites could
> be converted to use array_view with not much work, in which case these
> overloads could be removed, but I didn't want to do it in this patch, to
> avoid starting to dig in arch-specific code.
> 
> Change-Id: I9005f04114543ddff738949e12d85a31855304c2
> Reviewed-By: John Baldwin <jhb@FreeBSD.org>
> ---
...
> @@ -650,13 +677,13 @@ template<typename T, typename>
>  void
>  regcache::raw_write (int regnum, T val)
>  {
> -  gdb_byte *buf;
> -
>    assert_regnum (regnum);
> -  buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]);
> -  store_integer (buf, m_descr->sizeof_register[regnum],
> -		 gdbarch_byte_order (m_descr->gdbarch), val);
> -  raw_write (regnum, buf);
> +
> +  int len = m_descr->sizeof_register[regnum];
> +  gdb_byte *buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]);

We could use `len` as the argument to `alloca`.  Could you also consider naming
the variable `size` instead of `len`?  It seems to be used more often than `len`
in this patch, and also fits better to array_view's `size` method.

Regards
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928