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

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

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>