Re: [PATCH v3 7/8] gdb/aarch64: Detect vector length changes when debugging remotely

Luis Machado via Gdb-patches
2023-02-01T09:59:13Z

On 1/30/23 04:45, Thiago Jung Bauermann via Gdb-patches wrote:
> If the remote target provides target description IDs in their threads
> list and stop reply packets, use them to update the thread's gdbarch.
> This allows debugging programs remotely that change their SVE vector
> length during runtime.
>
> GDB already supports different vector lengths in native debugging by
> calling target_ops::thread_architecture, and aarch64_linux_nat_target
> provides an implementation of that method.
>
> So to provide the same feature in remote debugging, implement the
> thread_architecture method in remote_target, so that the same
> mechanism can be used for both the native and remote cases. This
> method returns the gdbarch corresponding to the target description
> provided by the last threads list or stop reply packet.
>
> To allow changing the architecture based on the target description,
> add a new gdbarch method to allow arch-specific code to make the
> adjustment.
> ---
> gdb/aarch64-tdep.c | 20 ++++++++++
> gdb/arch-utils.c | 8 ++++
> gdb/arch-utils.h | 4 ++
> gdb/gdbarch-components.py | 15 +++++++
> gdb/gdbarch-gen.h | 10 +++++
> gdb/gdbarch.c | 22 ++++++++++
> gdb/remote.c | 84 +++++++++++++++++++++++++++++++++++++++
> 7 files changed, 163 insertions(+)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index b576d3b9d99f..1e6e7116ed8a 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3502,6 +3502,25 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
> || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
> }
>
> +/* Implement the "update_architecture" gdbarch method. */
> +
> +static struct gdbarch *
> +aarch64_update_architecture (struct gdbarch *gdbarch, const target_desc *tdesc)
> +{
> + /* If this is a 32-bit architecture, then this is ARM, not AArch64.
> + There are no SVE registers here, so just return the inferior
> + architecture. */
> + if (gdbarch_bfd_arch_info (gdbarch)->bits_per_word == 32)
> + return gdbarch;
> +
> + struct gdbarch_info info;
> +
> + info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
> + info.target_desc = tdesc;
> +
> + return gdbarch_find_by_info (info);
> +}
> +
> /* Implement the stack_frame_destroyed_p gdbarch method. */
>
> static int
> @@ -3748,6 +3767,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> set_tdesc_pseudo_register_reggroup_p (gdbarch,
> aarch64_pseudo_register_reggroup_p);
> set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
> + set_gdbarch_update_architecture (gdbarch, aarch64_update_architecture);
>
> /* ABI */
> set_gdbarch_short_bit (gdbarch, 16);
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 67968126488e..e8ad0aed6eb4 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1098,6 +1098,14 @@ default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
> return 0;
> }
>
> +/* See arch-utils.h. */
> +
> +struct gdbarch *
> +default_update_architecture (struct gdbarch *gdbarch, const target_desc *tdesc)
> +{
> + return gdbarch;
> +}
> +
> /* Non-zero if we want to trace architecture code. */
>
> #ifndef GDBARCH_DEBUG
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 56690f0fd435..aeee7f51ad49 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -314,4 +314,8 @@ extern enum return_value_convention default_gdbarch_return_value
> struct regcache *regcache, struct value **read_value,
> const gdb_byte *writebuf);
>
> +/* Default implementation of gdbarch update_architecture method. */
> +extern struct gdbarch *
> +default_update_architecture (struct gdbarch *gdbarch, const target_desc *tdesc);
> +
> #endif /* ARCH_UTILS_H */
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 76ad2832d8a2..68b7521c9966 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -2744,3 +2744,18 @@ Read core file mappings
> predefault="default_read_core_file_mappings",
> invalid=False,
> )
> +
> +Method(
> + comment="""
> +An architecture may change while the inferior is running. For instance, the
> +length of the vector registers in AArch64's Scalable Vector Extension is given
> +by the contents of the VG pseudo-register.
> +
> +Return a gdbarch corresponding to the given target description.
> +""",
> + type="struct gdbarch *",
> + name="update_architecture",
> + params=[("const target_desc *", "tdesc")],
> + predefault="default_update_architecture",
> + invalid=False,
> +)
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index 32b2d96fbe01..d6068c2bc24f 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1672,3 +1672,13 @@ extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_g
> typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
> extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
> extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarch_read_core_file_mappings_ftype *read_core_file_mappings);
> +
> +/* An architecture may change while the inferior is running. For instance, the
> + length of the vector registers in AArch64's Scalable Vector Extension is given
> + by the contents of the VG pseudo-register.
> +
> + Return a gdbarch corresponding to the given target description. */
> +
> +typedef struct gdbarch * (gdbarch_update_architecture_ftype) (struct gdbarch *gdbarch, const target_desc *tdesc);
> +extern struct gdbarch * gdbarch_update_architecture (struct gdbarch *gdbarch, const target_desc *tdesc);
> +extern void set_gdbarch_update_architecture (struct gdbarch *gdbarch, gdbarch_update_architecture_ftype *update_architecture);
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 46baca9c4793..07d3e060fe1f 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -256,6 +256,7 @@ struct gdbarch
> gdbarch_type_align_ftype *type_align = default_type_align;
> gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = default_get_pc_address_flags;
> gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = default_read_core_file_mappings;
> + gdbarch_update_architecture_ftype *update_architecture = default_update_architecture;
> };
>
> /* Create a new ``struct gdbarch'' based on information provided by
> @@ -517,6 +518,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
> /* Skip verify of type_align, invalid_p == 0 */
> /* Skip verify of get_pc_address_flags, invalid_p == 0 */
> /* Skip verify of read_core_file_mappings, invalid_p == 0 */
> + /* Skip verify of update_architecture, invalid_p == 0 */
> if (!log.empty ())
> internal_error (_("verify_gdbarch: the following are invalid ...%s"),
> log.c_str ());
> @@ -1355,6 +1357,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
> gdb_printf (file,
> "gdbarch_dump: read_core_file_mappings = <%s>\n",
> host_address_to_string (gdbarch->read_core_file_mappings));
> + gdb_printf (file,
> + "gdbarch_dump: update_architecture = <%s>\n",
> + host_address_to_string (gdbarch->update_architecture));
> if (gdbarch->dump_tdep != NULL)
> gdbarch->dump_tdep (gdbarch, file);
> }
> @@ -5317,3 +5322,20 @@ set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch,
> {
> gdbarch->read_core_file_mappings = read_core_file_mappings;
> }
> +
> +struct gdbarch *
> +gdbarch_update_architecture (struct gdbarch *gdbarch, const target_desc *tdesc)
> +{
> + gdb_assert (gdbarch != NULL);
> + gdb_assert (gdbarch->update_architecture != NULL);
> + if (gdbarch_debug >= 2)
> + gdb_printf (gdb_stdlog, "gdbarch_update_architecture called\n");
> + return gdbarch->update_architecture (gdbarch, tdesc);
> +}
> +
> +void
> +set_gdbarch_update_architecture (struct gdbarch *gdbarch,
> + gdbarch_update_architecture_ftype update_architecture)
> +{
> + gdbarch->update_architecture = update_architecture;
> +}
> diff --git a/gdb/remote.c b/gdb/remote.c
> index f1d1944414c3..a17d65220408 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -506,6 +506,8 @@ class remote_target : public process_stratum_target
> gdb::byte_vector thread_info_to_thread_handle (struct thread_info *tp)
> override;
>
> + struct gdbarch *thread_architecture (ptid_t ptid) override;
> +
> void stop (ptid_t) override;
>
> void interrupt () override;
> @@ -1168,6 +1170,10 @@ struct remote_thread_info : public private_thread_info
> to stop for a watchpoint. */
> CORE_ADDR watch_data_address = 0;
>
> + /* The architecture corresponding to the target description returned
> + in the last stop reply or threads list. */
> + struct gdbarch *arch = nullptr;
> +
> /* Get the thread's resume state. */
> enum resume_state get_resume_state () const
> {
> @@ -1186,6 +1192,8 @@ struct remote_thread_info : public private_thread_info
> m_resume_state = resume_state::RESUMED_PENDING_VCONT;
> m_resumed_pending_vcont_info.step = step;
> m_resumed_pending_vcont_info.sig = sig;
> +
> + arch = nullptr;
> }
>
> /* Get the information this thread's pending vCont-resumption.
> @@ -1203,6 +1211,8 @@ struct remote_thread_info : public private_thread_info
> void set_resumed ()
> {
> m_resume_state = resume_state::RESUMED;
> +
> + arch = nullptr;
> }
>
> private:
> @@ -4046,6 +4056,29 @@ remote_target::update_thread_list ()
> info->extra = std::move (item.extra);
> info->name = std::move (item.name);
> info->thread_handle = std::move (item.thread_handle);
> +
> + /* If the threads list indicated a target description for this
> + thread, add it to the thread information. */
> + if (item.tdesc_id)
> + {
> + const target_desc *tdesc
> + = get_remote_state ()->get_tdesc (*item.tdesc_id);
> +
> + gdb_assert (tdesc != nullptr);
> +
> + /* If there's no thread-specific gdbarch, use the one from the
> + whole inferior. */
> + if (info->arch == nullptr)
> + info->arch = tp->inf->gdbarch;
> +
> + gdbarch *new_arch = gdbarch_update_architecture (info->arch,
> + tdesc);
> + if (new_arch != info->arch)
> + {
> + registers_changed_thread (tp);
> + info->arch = new_arch;
> + }
> + }
> }
> }
> }
> @@ -8152,6 +8185,36 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
> && status->kind () != TARGET_WAITKIND_SIGNALLED
> && status->kind () != TARGET_WAITKIND_NO_RESUMED)
> {
> + thread_info *thr = find_thread_ptid (this, ptid);
> +
> + /* If GDB already knows about this thread, we can give the
> + architecture-specific code a chance to update the gdbarch based on the
> + provided target description. */
> + if (stop_reply->tdesc_id && thr != nullptr)
> + {
> + const target_desc *tdesc
> + = stop_reply->rs->get_tdesc (*stop_reply->tdesc_id);
> +
> + gdb_assert (tdesc != nullptr);
> +
> + /* If there's no gdbarch associated with the stop reply, use the one
> + from the whole inferior. */
> + if (stop_reply->arch == nullptr)
> + stop_reply->arch = thr->inf->gdbarch;
> +
> + stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
> + tdesc);
> +
> + /* Save stop_reply->arch so that it can be returned by the
> + thread_architecture method. */
> + remote_thread_info *remote_thr = get_remote_thread_info (thr);
> + if (remote_thr->arch != stop_reply->arch)
> + {
> + registers_changed_thread (thr);
> + remote_thr->arch = stop_reply->arch;
> + }
> + }
> +
> /* Expedited registers. */
> if (!stop_reply->regcache.empty ())
> {
> @@ -14469,6 +14532,27 @@ remote_target::thread_info_to_thread_handle (struct thread_info *tp)
> return priv->thread_handle;
> }
>
> +/* Implementation of the thread_architecture method for the remote target. */
> +
> +struct gdbarch *
> +remote_target::thread_architecture (ptid_t ptid)
> +{
> + thread_info *thr = find_thread_ptid (this, ptid);
> + remote_thread_info *remote_thr;
> +
> + if (thr == nullptr)
> + remote_thr = nullptr;
> + else
> + remote_thr = get_remote_thread_info (thr);
> +
> + if (remote_thr == nullptr || remote_thr->arch == nullptr)
> + /* The default thread_architecture implementation is the one from
> + process_stratum_target. */
> + return process_stratum_target::thread_architecture (ptid);
> +
> + return remote_thr->arch;
> +}
> +
> bool
> remote_target::can_async_p ()
> {

I have no comments on this patch. It looks sane to me.

Reviewed-by: Luis Machado <luis.machado@arm.com>