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.
> 
> 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.
> +
> +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 recalled this. One deficiency of the current SVE implementation is that it doesn't have a proper way to hide the Z register when they don't have any meaningful state (SVE not active, so we have fpsimd state).

Given the VL will always be reported as > 0, even if there is no active SVE state, gdb will always attempt to show Z registers. And those are quite verbose.

In this sense, the implementations of the the thread_architecture methods for both native aarch64 and remote differ. While native aarch64's implementation is capable of detecting the lack of active SVE state, the
remote implementation can't. If gdbserver decided to drop the Z registers mid-execution (before the SVE state is not active), there wouldn't be vg for gdb to make a decision.

For example, there isn't a way to return a value of 0 for vg, as vg == 0 would mean no SVE feature. If there isn't a SVE feature, vg wouldn't exist.

Looking ahead, it would be the same for SME with its SVL. If we have to show both Z and ZA contexts, it will be quite a verbose exchange if we're listing all the possible registers.

I wonder if there is a better way to handle this that would still allow us to hide these scalable registers when there is no active state.