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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-03-01 9:45 a.m., Zoran Zaric via Gdb-patches wrote:
> 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
> +      || offset + length > regsize || numregs < regnum)

The last line is missing one column of indent.

Can `frame` really be NULL here?  Given that where write_to_register is
used, we have:

    struct frame_info *frame = frame_find_by_id (c->frame_id);
    struct gdbarch *arch = get_frame_arch (frame);

If frame was NULL, it would segfault in get_frame_arch.

Can regsize really be 0?

I don't understand the code and how it relates to the comment.  What
does it mean for a register to be inside an offset?  The expression
`offset + length > regsize` checks that the end of the portion we want
to read is beyond the end of the register.  But there could be a part of
the portion we want to read that is within the register.  The code might
be correct, but the comment needs to express the intention more clearly.

Is `numregs < regnum` really useful?  When would you encounter that?

Simon