Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Luis Machado <luis.machado@arm.com>]

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> If the remote target provides an expedited VG register, use it to update
> the thread's gdbarch. This allows debugging programs that change their SVE
> vector length during runtime.

debugging programs -> debugging programs remotely
> 
> This is accomplished by implementing the thread_architecture method in
> remote_target, which returns the gdbarch corresponding to the expedited
> registers provided by the last stop reply.
> 
> To allow changing the architecture based on the expedited registers, add a
> new gdbarch method to allow arch-specific code to make the adjustment.
> ---
>   gdb/aarch64-tdep.c        | 26 +++++++++++++++++++++++-
>   gdb/arch-utils.c          |  9 +++++++++
>   gdb/arch-utils.h          |  5 +++++
>   gdb/gdbarch-components.py | 16 +++++++++++++++
>   gdb/gdbarch-gen.h         | 11 ++++++++++
>   gdb/gdbarch.c             | 22 ++++++++++++++++++++
>   gdb/remote.c              | 42 +++++++++++++++++++++++++++++++++++++++
>   7 files changed, 130 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index ffc128d91f60..764f693aee8d 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3486,7 +3486,8 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
>   	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
>   }
>   
> -/* Helper function for the "thread_architecture" target_ops method.
> +/* Helper function for both the "update_architecture" gdbarch method and the
> +   "thread_architecture" target_ops method.
>   
>      Returns a new gdbarch that is equivalent to the given gdbarch, but with SVE
>      registers reflecting the given vq value.  */
> @@ -3521,6 +3522,28 @@ aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq)
>     return gdbarch_find_by_info (info);
>   }
>   
> +/* Implement the "update_architecture" gdbarch method.  */
> +
> +static struct gdbarch *
> +aarch64_update_architecture (struct gdbarch *gdbarch,
> +			     const std::vector<cached_reg_t> &regcache)
> +{
> +  /* Look for the VG register.  */
> +  auto it = find_if (regcache.cbegin (), regcache.cend (),
> +		     [] (const cached_reg_t &reg) {
> +		       return reg.num == AARCH64_SVE_VG_REGNUM;
> +		     });
> +
> +  /* No VG register was provided.  Don't change gdbarch.  */
> +  if (it == regcache.cend ())
> +    return gdbarch;
> +
> +  ULONGEST vg = extract_unsigned_integer (it->data, 8,
> +					  gdbarch_byte_order (gdbarch));
> +
> +  return aarch64_update_gdbarch (gdbarch, sve_vq_from_vg (vg));
> +}
> +
>   /* Implement the stack_frame_destroyed_p gdbarch method.  */
>   
>   static int
> @@ -3742,6 +3765,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 7b84daf046e2..2315aacb4bbe 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1090,6 +1090,15 @@ 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 std::vector<cached_reg_t> &regcache)
> +{
> +  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 46018a6fbbb6..e68a91f6753d 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -304,4 +304,9 @@ extern void default_read_core_file_mappings
>   /* Default implementation of gdbarch default_get_return_buf_addr method.  */
>   extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
>   					      frame_info_ptr cur_frame);
> +
> +/* Default implementation of gdbarch update_architecture method.  */
> +extern struct gdbarch *
> +default_update_architecture (struct gdbarch *gdbarch,
> +			     const std::vector<cached_reg_t> &regcache);
>   #endif /* ARCH_UTILS_H */
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 9b688998a7bd..fbb32c5e5853 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -2698,3 +2698,19 @@ Read core file mappings
>       predefault="default_read_core_file_mappings",
>       invalid=False,
>   )
> +
> +Method(
> +    comment="""
> +An architecture may change based on the current contents of its registers.
> +For instance, the length of the vector registers in AArch64's Scalable Vector
> +Extension is given by the contents of the VL pseudo-register.

VL -> VG

> +
> +This method allows an architecture to provide a new gdbarch corresponding to
> +the registers in the given regcache.
> +""",
> +    type="struct gdbarch *",
> +    name="update_architecture",
> +    params=[("const std::vector<cached_reg_t> &", "regcache")],
> +    predefault="default_update_architecture",
> +    invalid=False,
> +)
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index a663316df166..a65699e7241f 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1649,3 +1649,14 @@ 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 based on the current contents of its registers.
> +   For instance, the length of the vector registers in AArch64's Scalable Vector
> +   Extension is given by the contents of the VL pseudo-register.
> +
> +   This method allows an architecture to provide a new gdbarch corresponding to
> +   the registers in the given regcache. */
> +
> +typedef struct gdbarch * (gdbarch_update_architecture_ftype) (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache);
> +extern struct gdbarch * gdbarch_update_architecture (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache);
> +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 3227e9458801..1b0a2c70db4c 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -255,6 +255,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
> @@ -514,6 +515,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 ());
> @@ -1352,6 +1354,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);
>   }
> @@ -5314,3 +5319,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 std::vector<cached_reg_t> &regcache)
> +{
> +  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, regcache);
> +}
> +
> +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 5118ecd0a312..eb60ed51585b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -491,6 +491,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;
> @@ -1150,6 +1152,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 expedited registers returned
> +     in the last stop reply.  */
> +  struct gdbarch *expedited_arch = nullptr;
> +
>     /* Get the thread's resume state.  */
>     enum resume_state get_resume_state () const
>     {
> @@ -1168,6 +1174,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;
> +
> +    expedited_arch = nullptr;
>     }
>   
>     /* Get the information this thread's pending vCont-resumption.
> @@ -1185,6 +1193,8 @@ struct remote_thread_info : public private_thread_info
>     void set_resumed ()
>     {
>       m_resume_state = resume_state::RESUMED;
> +
> +    expedited_arch = nullptr;
>     }
>   
>   private:
> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>         /* Expedited registers.  */
>         if (!stop_reply->regcache.empty ())
>   	{
> +	  /* If GDB already knows about this thread, we can give the
> +	     architecture-specific code a chance to update the gdbarch based on
> +	     the expedited registers.  */
> +	  if (find_thread_ptid (this, ptid) != nullptr)
> +	    {
> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
> +							      stop_reply->regcache);
> +
> +	      /* Save stop_reply->arch so that it can be returned by the
> +		 thread_architecture method.  */
> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
> +								       ptid);
> +	      remote_thr->expedited_arch = stop_reply->arch;
> +	    }
> +
>   	  struct regcache *regcache
>   	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>   
> @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct thread_info *tp)
>     return priv->thread_handle;
>   }
>   
> +struct gdbarch *
> +remote_target::thread_architecture (ptid_t ptid)
> +{
> +  thread_info *thr = find_thread_ptid (this, ptid);
> +  remote_thread_info *remote_thr = nullptr;
> +
> +  if (thr != nullptr)
> +    remote_thr = get_remote_thread_info (thr);
> +
> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
> +    /* The default thread_architecture implementation is the one from
> +       process_stratum_target.  */
> +    return process_stratum_target::thread_architecture(ptid);
> +
> +  return remote_thr->expedited_arch;
> +}
> +
>   bool
>   remote_target::can_async_p ()
>   {

Just to confirm, as I don't exactly remember how this went. Is the case where we switch threads during remote debugging covered in case we have threads with different vector lengths?

I seem to recall we'd call thread_architecture in those cases, but I don't know how the expedited register comes into play for this case. Do we get expedited registers for each thread that has stopped?

Otherwise LGTM.

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