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

Simon Marchi via Gdb-patches
2021-04-28T03:25:34Z

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