[PATCH 2/2] GDB: Use gdb::array_view for buffers used in register reading and unwinding

[Simon Marchi]

On 2025-01-10 11:44, Thiago Jung Bauermann wrote:
> This allows checking the size of the given buffer.  Changes
> frame_register_unwind (), frame_unwind_register (), get_frame_register ()
> and deprecated_frame_register_read ().
> 
> As pointed out by Baris, in the case of MIPS target code this is best
> done by changing a couple of alloca-based buffers in
> mips_read_fp_register_single and mips_print_fp_register to
> gdb::byte_vector instances.
> ---
>  gdb/amd64-windows-tdep.c |  4 ++--
>  gdb/frame.c              | 22 ++++++++++--------
>  gdb/frame.h              | 10 ++++-----
>  gdb/mips-tdep.c          | 48 ++++++++++++++++++++++------------------
>  4 files changed, 47 insertions(+), 37 deletions(-)
> 
> Changes from RFCv4 patch 2:
> 
> - Allocated gdb::byte_vectors instead of making array_views in mips-tdep.c,
>   as suggested by Baris. This required changing raw_buffer in
>   mips_print_fp_register (the caller of mips_read_fp_register_single and
>   mips_read_fp_register_double) to a gdb::byte_vector as well, and also
>   changing the rare_buffer arguments to array views.
> 
> - Don't use std::optional for the buffer argument in frame_register_unwind,
>   as suggested by Simon.
> 
> - Changed the gdb_assert in frame_register_unwind to require the buffer
>   argument to have the same size as the register.
> 
> diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
> index 9e255bb2d433..ca7b7d96bc22 100644
> --- a/gdb/amd64-windows-tdep.c
> +++ b/gdb/amd64-windows-tdep.c
> @@ -801,7 +801,7 @@ amd64_windows_frame_decode_insns (const frame_info_ptr &this_frame,
>  	  std::array<gdb_byte, 8> buf;
>  	  int frreg = amd64_windows_w2gdb_regnum[frame_reg];
>  
> -	  get_frame_register (this_frame, frreg, buf.data ());
> +	  get_frame_register (this_frame, frreg, buf);
>  	  save_addr = extract_unsigned_integer (buf, byte_order);
>  
>  	  frame_debug_printf ("   frame_reg=%s, val=%s",
> @@ -1097,7 +1097,7 @@ amd64_windows_frame_cache (const frame_info_ptr &this_frame, void **this_cache)
>  
>    /* Get current PC and SP.  */
>    pc = get_frame_pc (this_frame);
> -  get_frame_register (this_frame, AMD64_RSP_REGNUM, buf.data ());
> +  get_frame_register (this_frame, AMD64_RSP_REGNUM, buf);
>    cache->sp = extract_unsigned_integer (buf, byte_order);
>    cache->pc = pc;
>  
> diff --git a/gdb/frame.c b/gdb/frame.c
> index ba4a07179f64..b64f9067ab9e 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1112,7 +1112,7 @@ frame_save_as_regcache (const frame_info_ptr &this_frame)
>  {
>    auto cooked_read = [this_frame] (int regnum, gdb::array_view<gdb_byte> buf)
>      {
> -      if (!deprecated_frame_register_read (this_frame, regnum, buf.data ()))
> +      if (!deprecated_frame_register_read (this_frame, regnum, buf))
>  	return REG_UNAVAILABLE;
>        else
>  	return REG_VALID;
> @@ -1177,7 +1177,8 @@ void
>  frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
>  		       int *optimizedp, int *unavailablep,
>  		       enum lval_type *lvalp, CORE_ADDR *addrp,
> -		       int *realnump, gdb_byte *bufferp)
> +		       int *realnump,
> +		       gdb::array_view<gdb_byte> buffer)
>  {
>    struct value *value;
>  

There are some comments referring to `bufferp` that would need to be
updated.  IMO the line:

 /* gdb_assert (bufferp != NULL); */

... can just be removed.

> @@ -1202,13 +1203,15 @@ frame_register_unwind (const frame_info_ptr &next_frame, int regnum,
>    else
>      *realnump = -1;
>  
> -  if (bufferp)
> +  if (!buffer.empty ())
>      {
> +      gdb_assert (buffer.size () == value->type ()->length ());

Regarding this, I think I'm going to backtrack.  I started to survey the
callers (and the callers of callers) of this function, and found one
case where this doesn't hold, that looks legitimate:

 - frame_register_unwind, called by
 - frame_unwind_register, called by
 - i386_unwind_pc

i386_unwind_pc has an 8 bytes static array and asks to unwind the PC
register.  This function is used for both amd64 (8 bytes PC) and i386 (4
bytes PC).  This function would now have to create a view of the right
size, something like:

    static CORE_ADDR
    i386_unwind_pc (struct gdbarch *gdbarch, const frame_info_ptr &next_frame)
    {
      gdb_byte buf[8];
      auto view = gdb::make_array_view
        (buf, builtin_type (gdbarch)->builtin_func_ptr->length ());

      frame_unwind_register (next_frame, gdbarch_pc_regnum (gdbarch), view);
      return extract_typed_address (view.data (),
    				builtin_type (gdbarch)->builtin_func_ptr);
    }

Without doing changes like this all over the place, things will break.
I'm not saying it can't be done, but it would require a lot of effort
and careful review (and testing when possible).

So, I think it would be safer for now to revert to what you had before,
assert that the buffer is large enough for the value.

If you agree, then this LGTM with those fixed.

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

Simon