Re: [PATCH 17/24] gdb: add gdbarch_pseudo_register_write that takes a frame

Andrew Burgess
2023-11-14T12:20:40Z

Simon Marchi <simon.marchi@polymtl.ca> writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> Add a new variant of gdbarch_pseudo_register_write that takes a
> frame_info in order to write raw registers. Use this new method when
> available:
>
> - in put_frame_register, when trying to write a pseudo register to a given frame
> - in regcache::cooked_write
>
> No implementation is migrated to use this new method (that will come in
> subsequent patches), so no behavior change is expected here.
>
> The objective is to fix writing pseudo registers to non-current
> frames. See previous commit "gdb: read pseudo register through
> frame" for a more detailed explanation.
>
> Change-Id: Ie7fe364a15a4d86c2ecb09de2b4baa08c45555ac
> ---
> gdb/frame.c | 9 +++++-
> gdb/gdbarch-gen.h | 15 +++++++++-
> gdb/gdbarch.c | 32 ++++++++++++++++++++
> gdb/gdbarch_components.py | 19 ++++++++++++
> gdb/regcache.c | 4 +++
> gdb/value.c | 63 +++++++++++++++++++++++++++++++++++++++
> gdb/value.h | 22 ++++++++++++++
> 7 files changed, 162 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 3035f87369ca..81836d6d5357 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1452,7 +1452,14 @@ put_frame_register (frame_info_ptr next_frame, int regnum,
> break;
> }
> case lval_register:
> - get_current_regcache ()->cooked_write (realnum, buf, 1.0f);
> + /* Not sure if that's always true... but we have a problem if not. */
> + gdb_assert (size == register_size (gdbarch, realnum));
> +
> + if (realnum < gdbarch_num_regs (gdbarch)
> + || !gdbarch_pseudo_register_write_p (gdbarch))
> + get_current_regcache ()->cooked_write (realnum, buf, 1.0f);
> + else
> + gdbarch_pseudo_register_write (gdbarch, next_frame, realnum, buf);
> break;
> default:
> error (_("Attempt to assign to an unmodifiable value."));
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index 3160aa8a9613..9cf58490d0b6 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -200,12 +200,25 @@ typedef struct value * (gdbarch_pseudo_register_read_value_ftype) (struct gdbarc
> extern struct value * gdbarch_pseudo_register_read_value (struct gdbarch *gdbarch, frame_info_ptr next_frame, int cookednum);
> extern void set_gdbarch_pseudo_register_read_value (struct gdbarch *gdbarch, gdbarch_pseudo_register_read_value_ftype *pseudo_register_read_value);
>
> +/* Write bytes in BUF to pseudo register with number PSEUDO_REG_NUM.
> +
> + Raw registers backing the pseudo register should be written to using
> + NEXT_FRAME. */
> +
> +extern bool gdbarch_pseudo_register_write_p (struct gdbarch *gdbarch);
> +
> +typedef void (gdbarch_pseudo_register_write_ftype) (struct gdbarch *gdbarch, frame_info_ptr next_frame, int pseudo_reg_num, gdb::array_view<const gdb_byte> buf);
> +extern void gdbarch_pseudo_register_write (struct gdbarch *gdbarch, frame_info_ptr next_frame, int pseudo_reg_num, gdb::array_view<const gdb_byte> buf);
> +extern void set_gdbarch_pseudo_register_write (struct gdbarch *gdbarch, gdbarch_pseudo_register_write_ftype *pseudo_register_write);
> +
> /* Write bytes to a pseudo register.
>
> This is marked as deprecated because it gets passed a regcache for
> implementations to write raw registers in. This doesn't work for unwound
> frames, where the raw registers backing the pseudo registers may have been
> - saved elsewhere. */
> + saved elsewhere.
> +
> + Implementations should be migrated to implement pseudo_register_write instead. */
>
> extern bool gdbarch_deprecated_pseudo_register_write_p (struct gdbarch *gdbarch);
>
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index e198d339f6ba..d584305fefb2 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -74,6 +74,7 @@ struct gdbarch
> gdbarch_virtual_frame_pointer_ftype *virtual_frame_pointer = legacy_virtual_frame_pointer;
> gdbarch_pseudo_register_read_ftype *pseudo_register_read = nullptr;
> gdbarch_pseudo_register_read_value_ftype *pseudo_register_read_value = nullptr;
> + gdbarch_pseudo_register_write_ftype *pseudo_register_write = nullptr;
> gdbarch_deprecated_pseudo_register_write_ftype *deprecated_pseudo_register_write = nullptr;
> int num_regs = -1;
> int num_pseudo_regs = 0;
> @@ -330,6 +331,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
> /* Skip verify of virtual_frame_pointer, invalid_p == 0 */
> /* Skip verify of pseudo_register_read, has predicate. */
> /* Skip verify of pseudo_register_read_value, has predicate. */
> + /* Skip verify of pseudo_register_write, has predicate. */
> /* Skip verify of deprecated_pseudo_register_write, has predicate. */
> if (gdbarch->num_regs == -1)
> log.puts ("\n\tnum_regs");
> @@ -649,6 +651,12 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
> gdb_printf (file,
> "gdbarch_dump: pseudo_register_read_value = <%s>\n",
> host_address_to_string (gdbarch->pseudo_register_read_value));
> + gdb_printf (file,
> + "gdbarch_dump: gdbarch_pseudo_register_write_p() = %d\n",
> + gdbarch_pseudo_register_write_p (gdbarch));
> + gdb_printf (file,
> + "gdbarch_dump: pseudo_register_write = <%s>\n",
> + host_address_to_string (gdbarch->pseudo_register_write));
> gdb_printf (file,
> "gdbarch_dump: gdbarch_deprecated_pseudo_register_write_p() = %d\n",
> gdbarch_deprecated_pseudo_register_write_p (gdbarch));
> @@ -1902,6 +1910,30 @@ set_gdbarch_pseudo_register_read_value (struct gdbarch *gdbarch,
> gdbarch->pseudo_register_read_value = pseudo_register_read_value;
> }
>
> +bool
> +gdbarch_pseudo_register_write_p (struct gdbarch *gdbarch)
> +{
> + gdb_assert (gdbarch != NULL);
> + return gdbarch->pseudo_register_write != NULL;
> +}
> +
> +void
> +gdbarch_pseudo_register_write (struct gdbarch *gdbarch, frame_info_ptr next_frame, int pseudo_reg_num, gdb::array_view<const gdb_byte> buf)
> +{
> + gdb_assert (gdbarch != NULL);
> + gdb_assert (gdbarch->pseudo_register_write != NULL);
> + if (gdbarch_debug >= 2)
> + gdb_printf (gdb_stdlog, "gdbarch_pseudo_register_write called\n");
> + gdbarch->pseudo_register_write (gdbarch, next_frame, pseudo_reg_num, buf);
> +}
> +
> +void
> +set_gdbarch_pseudo_register_write (struct gdbarch *gdbarch,
> + gdbarch_pseudo_register_write_ftype pseudo_register_write)
> +{
> + gdbarch->pseudo_register_write = pseudo_register_write;
> +}
> +
> bool
> gdbarch_deprecated_pseudo_register_write_p (struct gdbarch *gdbarch)
> {
> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index 1100da160550..ce8169b19a57 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -418,6 +418,23 @@ never be called.
> predicate=True,
> )
>
> +Method(
> + comment="""
> +Write bytes in BUF to pseudo register with number PSEUDO_REG_NUM.
> +
> +Raw registers backing the pseudo register should be written to using
> +NEXT_FRAME.
> +""",
> + type="void",
> + name="pseudo_register_write",
> + params=[
> + ("frame_info_ptr", "next_frame"),
> + ("int", "pseudo_reg_num"),
> + ("gdb::array_view<const gdb_byte>", "buf"),
> + ],
> + predicate=True,
> +)
> +
> Method(
> comment="""
> Write bytes to a pseudo register.
> @@ -426,6 +443,8 @@ This is marked as deprecated because it gets passed a regcache for
> implementations to write raw registers in. This doesn't work for unwound
> frames, where the raw registers backing the pseudo registers may have been
> saved elsewhere.
> +
> +Implementations should be migrated to implement pseudo_register_write instead.
> """,
> type="void",
> name="deprecated_pseudo_register_write",
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index dadc949434ee..16b117e767be 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -929,6 +929,10 @@ regcache::cooked_write (int regnum, gdb::array_view<const gdb_byte> src, float)
>
> if (regnum < num_raw_registers ())
> raw_write (regnum, src, 1.0f);
> + else if (gdbarch_pseudo_register_write_p (m_descr->gdbarch))
> + gdbarch_pseudo_register_write
> + (m_descr->gdbarch, get_next_frame_sentinel_okay (get_current_frame ()),
> + regnum, src);
> else
> gdbarch_deprecated_pseudo_register_write (m_descr->gdbarch, this, regnum,
> src.data ());
> diff --git a/gdb/value.c b/gdb/value.c
> index 27a79dc62fc1..51dca972a587 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -4057,6 +4057,22 @@ pseudo_from_raw_part (frame_info_ptr next_frame, int pseudo_reg_num,
>
> /* See value.h. */
>
> +void
> +pseudo_to_raw_part (frame_info_ptr next_frame,
> + gdb::array_view<const gdb_byte> pseudo_buf,
> + int raw_reg_num, int raw_offset)
> +{
> + int raw_reg_size = register_size (get_frame_arch (next_frame), raw_reg_num);
> +
> + /* When overflowing a register, put_frame_register_bytes writes to the
> + subsequent registers. We don't want that behavior here, so make sure
> + the write is wholly within register RAW_REG_NUM. */
> + gdb_assert (raw_offset + pseudo_buf.size () <= raw_reg_size);
> + put_frame_register_bytes (next_frame, raw_reg_num, raw_offset, pseudo_buf);
> +}
> +
> +/* See value.h. */
> +
> value *
> pseudo_from_concat_raw (frame_info_ptr next_frame, int pseudo_reg_num,
> int raw_reg_1_num, int raw_reg_2_num)
> @@ -4080,6 +4096,27 @@ pseudo_from_concat_raw (frame_info_ptr next_frame, int pseudo_reg_num,
> return pseudo_reg_val;
> }
>
> +void
> +pseudo_to_concat_raw (frame_info_ptr next_frame,
> + gdb::array_view<const gdb_byte> pseudo_buf,
> + int raw_reg_1_num, int raw_reg_2_num)

This function, and its overload are missing a /* See value.h. */ comment.

> +{
> + int src_offset = 0;
> + gdbarch *arch = frame_unwind_arch (next_frame);
> +
> + int raw_reg_1_size = register_size (arch, raw_reg_1_num);
> + put_frame_register_bytes (next_frame, raw_reg_1_num, 0,
> + pseudo_buf.slice (src_offset, raw_reg_1_size));
> + src_offset += raw_reg_1_size;
> +
> + int raw_reg_2_size = register_size (arch, raw_reg_2_num);
> + put_frame_register_bytes (next_frame, raw_reg_2_num, 0,
> + pseudo_buf.slice (src_offset, raw_reg_2_size));
> + src_offset += raw_reg_2_size;
> +
> + gdb_assert (src_offset == pseudo_buf.size ());
> +}
> +
> /* See value.h. */
>
> value *
> @@ -4111,6 +4148,32 @@ pseudo_from_concat_raw (frame_info_ptr next_frame, int pseudo_reg_num,
> return pseudo_reg_val;
> }
>
> +void
> +pseudo_to_concat_raw (frame_info_ptr next_frame,
> + gdb::array_view<const gdb_byte> pseudo_buf,
> + int raw_reg_1_num, int raw_reg_2_num, int raw_reg_3_num)
> +{
> + int src_offset = 0;
> + gdbarch *arch = frame_unwind_arch (next_frame);
> +
> + int raw_reg_1_size = register_size (arch, raw_reg_1_num);
> + put_frame_register_bytes (next_frame, raw_reg_1_num, 0,
> + pseudo_buf.slice (src_offset, raw_reg_1_size));
> + src_offset += raw_reg_1_size;
> +
> + int raw_reg_2_size = register_size (arch, raw_reg_2_num);
> + put_frame_register_bytes (next_frame, raw_reg_2_num, 0,
> + pseudo_buf.slice (src_offset, raw_reg_2_size));
> + src_offset += raw_reg_2_size;
> +
> + int raw_reg_3_size = register_size (arch, raw_reg_3_num);
> + put_frame_register_bytes (next_frame, raw_reg_3_num, 0,
> + pseudo_buf.slice (src_offset, raw_reg_3_size));
> + src_offset += raw_reg_3_size;
> +
> + gdb_assert (src_offset == pseudo_buf.size ());
> +}
> +
> /* Implementation of the convenience function $_isvoid. */
>
> static struct value *
> diff --git a/gdb/value.h b/gdb/value.h
> index b5c097ad58bf..6a74d4e2c2ee 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -1661,6 +1661,13 @@ struct scoped_array_length_limiting
> value *pseudo_from_raw_part (frame_info_ptr next_frame, int pseudo_reg_num,
> int raw_reg_num, int raw_offset);
>
> +/* Write PSEUDO_BUF, the contents of a pseudo register, to part of raw register
> + RAW_REG_NUM starting at RAW_OFFSET. */
> +
> +void pseudo_to_raw_part (frame_info_ptr this_frame,
> + gdb::array_view<const gdb_byte> pseudo_buf,
> + int raw_reg_num, int raw_offset);
> +
> /* Create a value for pseudo register PSEUDO_REG_NUM by concatenating raw
> registers RAW_REG_1_NUM and RAW_REG_2_NUM.
>
> @@ -1670,10 +1677,25 @@ value *pseudo_from_raw_part (frame_info_ptr next_frame, int pseudo_reg_num,
> value *pseudo_from_concat_raw (frame_info_ptr next_frame, int pseudo_reg_num,
> int raw_reg_1_num, int raw_reg_2_num);
>
> +/* Write PSEUDO_BUF, the contents of a pseudo register, to the two raw registers
> + RAW_REG_1_NUM and RAW_REG_2_NUM. */
> +
> +void pseudo_to_concat_raw (frame_info_ptr this_frame,
> + gdb::array_view<const gdb_byte> pseudo_buf,
> + int raw_reg_1_num, int raw_reg_2_num);
> +
> /* Same as the above, but with three raw registers. */
>
> value *pseudo_from_concat_raw (frame_info_ptr next_frame, int pseudo_reg_num,
> int raw_reg_1_num, int raw_reg_2_num,
> int raw_reg_3_num);
>
> +/* Write PSEUDO_BUF, the contents of a pseudo register, to the tthreewo raw
> + registers RAW_REG_1_NUM, RAW_REG_2_NUM and RAW_REG_3_NUM. */

typo s/tthreewo/three/.

Thanks,
Andrew
> +
> +void pseudo_to_concat_raw (frame_info_ptr this_frame,
> + gdb::array_view<const gdb_byte> pseudo_buf,
> + int raw_reg_1_num, int raw_reg_2_num,
> + int raw_reg_3_num);
> +
> #endif /* !defined (VALUE_H) */
> --
> 2.42.1