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

[Andrew Burgess <aburgess@redhat.com>]

Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org>
writes:

> 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.

It doesn't seem right to me that the method to lookup a new gdbarch is
itself, a member function of gdbarch.   Looking at the implementation,
I'm still puzzled, I'll expand on this inline below...

> ---
>  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;

OK, this bit is AArch64 specific.  But do we actually expect this case
to trigger?  If the remote is running ARM (so bit_per_word == 32), the
surely we wouldn't be sending different target description ids, and so
we wouldn't end up in here?

And if, in some future (imaginary?) world there is some reason why we
want different architecture for different ARM threads, then does the
lookup below actually hurt?

> +
> +  struct gdbarch_info info;
> +
> +  info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);

Instead of hard-coding bfd_arch_aarch64 and bfd_mach_aarch64, could you
not use tdesc_architecture to get a bfd_arch_info?

I guess the point I'm driving towards is that maybe instead of a new
gdbarch method we should add something like gdbarch_from_tdesc into
arch-utils.c (like we have gdbarch_from_bfd and gdbarch_find_by_info),
which just does a lookup from tdesc.

Thanks,
Andrew


> +  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.  */

I think we could give a better comment here; say what the default
behaviour actually is.

> +extern struct gdbarch *
> +default_update_architecture (struct gdbarch *gdbarch, const target_desc *tdesc);

The GDB style here is to place the function name on the same line as the
return value, and then place the parameter list on the second line.
Check out default_gdbarch_return_value just above for an example.

> +
>  #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 ()
>  {