Re: [PATCH 18/43] Add new register access interface to expr.c

[Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>]

Hi,

I have minor style related comments above.

Le Mon, Mar 01, 2021 at 02:45:55PM +0000, Zoran Zaric via Gdb-patches a écrit :
> DWARF expression evaluator is currently using get_frame_register_bytes
> and put_frame_register_bytes interface for register access.
> 
> The problem with evaluator using this interface is that it allows a
> bleed out register access. This means that if the caller specifies a
> larger amount of data then the size of a specified register, the
> operation will continue accessing the neighboring registers until a
> full amount of data has been reached.
> 
> DWARF specification does not define this behavior, so a new simplified
> register access interface is needed instead.
> 
> 	* dwarf2/expr.c (read_from_register): New function.
> 	(write_to_register): New function.
> 	(rw_pieced_value): Now calls the read_from_register and
> 	write_to_register functions.
> ---
>  gdb/dwarf2/expr.c | 128 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 106 insertions(+), 22 deletions(-)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index c50bb3c8d90..5a1fd5b941f 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -106,6 +106,96 @@ read_addr_from_reg (struct frame_info *frame, int reg)
>    return address_from_register (regnum, frame);
>  }
>  
> +/* Read register REGNUM's contents in a given FRAME context.
> +
> +   The data read is offsetted by OFFSET, and the number of bytes read
> +   is defined by LENGTH.  The data is then copied into the
> +   caller-managed buffer BUF.
> +
> +   If the register is optimized out or unavailable for the given
> +   FRAME, the OPTIMIZED and UNAVAILABLE outputs are set
> +   accordingly  */
> +
> +static void
> +read_from_register (struct frame_info *frame, int regnum,
> +		    CORE_ADDR offset, gdb::array_view<gdb_byte> buf,
> +		    int *optimized, int *unavailable)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  int regsize = register_size (gdbarch, regnum);
> +  int numregs = gdbarch_num_cooked_regs (gdbarch);
> +  int length = buf.size ();
> +
> +  /* If a register is wholly inside the OFFSET, skip it.  */
> +  if (frame == NULL || !regsize

Gdb coding standard asks for explicit comparison of numbers
(https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_NULL_And_Zero).

!regsize should be regsize == 0.

I guess NULL could also be changed to nullptr.

> +      || offset + length > regsize || numregs < regnum)
> +    {
> +      *optimized = 0;
> +      *unavailable = 1;
> +      return;
> +    }
> +
> +  gdb::byte_vector temp_buf (regsize);
> +  enum lval_type lval;
> +  CORE_ADDR address;
> +  int realnum;
> +
> +  frame_register (frame, regnum, optimized, unavailable,
> +		  &lval, &address, &realnum, temp_buf.data ());
> +
> +  if (!*optimized && !*unavailable)
> +     memcpy (buf.data (), (char *) temp_buf.data () + offset, length);
> +
> +  return;

I changes nothing, but I find it odd to have a return; as last
statement of a void returning function.

> +}
> +
> +/* Write register REGNUM's contents in a given FRAME context.
> +
> +   The data written is offsetted by OFFSET, and the number of bytes
> +   written is defined by LENGTH.  The data is copied from
> +   caller-managed buffer BUF.
> +
> +   If the register is optimized out or unavailable for the given
> +   FRAME, the OPTIMIZED and UNAVAILABLE outputs are set
> +   accordingly. */
> +
> +static void
> +write_to_register (struct frame_info *frame, int regnum,
> +		   CORE_ADDR offset, gdb::array_view<gdb_byte> buf,
> +		   int *optimized, int *unavailable)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  int regsize = register_size (gdbarch, regnum);
> +  int numregs = gdbarch_num_cooked_regs (gdbarch);
> +  int length = buf.size ();
> +
> +  /* If a register is wholly inside of OFFSET, skip it.  */
> +  if (frame == NULL || !regsize

Same remark for regsize.

Thanks,
Lancelot.

> +     || offset + length > regsize || numregs < regnum)
> +    {
> +      *optimized = 0;
> +      *unavailable = 1;
> +      return;
> +    }
> +
> +  gdb::byte_vector temp_buf (regsize);
> +  enum lval_type lval;
> +  CORE_ADDR address;
> +  int realnum;
> +
> +  frame_register (frame, regnum, optimized, unavailable,
> +		  &lval, &address, &realnum, temp_buf.data ());
> +
> +  if (!*optimized && !*unavailable)
> +    {
> +      memcpy ((chr *) temp_buf.data () + offset, buf.data (), length);
> +
> +      put_frame_register (frame, regnum, temp_buf.data ());
> +    }
> +
> +  return;
> +}
> +
>  struct piece_closure
>  {
>    /* Reference count.  */
> @@ -243,22 +333,18 @@ rw_pieced_value (struct value *v, struct value *from)
>  	    if (from == NULL)
>  	      {
>  		/* Read mode.  */
> -		if (!get_frame_register_bytes (frame, gdb_regnum,
> -					       bits_to_skip / 8,
> -					       buffer, &optim, &unavail))
> -		  {
> -		    if (optim)
> -		      mark_value_bits_optimized_out (v, offset,
> -						     this_size_bits);
> -		    if (unavail)
> -		      mark_value_bits_unavailable (v, offset,
> -						   this_size_bits);
> -		    break;
> -		  }
> -
> -		copy_bitwise (v_contents, offset,
> -			      buffer.data (), bits_to_skip % 8,
> -			      this_size_bits, bits_big_endian);
> +		read_from_register (frame, gdb_regnum, bits_to_skip / 8,
> +				    buffer, &optim, &unavail);
> +
> +		if (optim)
> +		  mark_value_bits_optimized_out (v, offset, this_size_bits);
> +		if (unavail)
> +		  mark_value_bits_unavailable (v, offset, this_size_bits);
> +		/* Only copy data if valid.  */
> +		if (!optim && !unavail)
> +		  copy_bitwise (v_contents, offset,
> +				buffer.data (), bits_to_skip % 8,
> +				this_size_bits, bits_big_endian);
>  	      }
>  	    else
>  	      {
> @@ -267,9 +353,8 @@ rw_pieced_value (struct value *v, struct value *from)
>  		  {
>  		    /* Data is copied non-byte-aligned into the register.
>  		       Need some bits from original register value.  */
> -		    get_frame_register_bytes (frame, gdb_regnum,
> -					      bits_to_skip / 8,
> -					      buffer, &optim, &unavail);
> +		    read_from_register (frame, gdb_regnum, bits_to_skip / 8,
> +					buffer, &optim, &unavail);
>  		    if (optim)
>  		      throw_error (OPTIMIZED_OUT_ERROR,
>  				   _("Can't do read-modify-write to "
> @@ -285,9 +370,8 @@ rw_pieced_value (struct value *v, struct value *from)
>  		copy_bitwise (buffer.data (), bits_to_skip % 8,
>  			      from_contents, offset,
>  			      this_size_bits, bits_big_endian);
> -		put_frame_register_bytes (frame, gdb_regnum,
> -					  bits_to_skip / 8,
> -					  buffer);
> +		write_to_register (frame, gdb_regnum, bits_to_skip / 8,
> +				   buffer, &optim, &unavail);
>  	      }
>  	  }
>  	  break;
> -- 
> 2.17.1
> 

-- 
Lancelot SIX