Re: [PATCH v2 1/2] gdb/aarch64: Fix thread's gdbarch when SVE vector length changes

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

Hi,

On 8/5/22 01:46, Thiago Jung Bauermann via Gdb-patches wrote:
> When the inferior program changes the SVE length, GDB can stop tracking
> some registers as it obtains the new gdbarch that corresponds to the
> updated length:
> 
>    Breakpoint 1, do_sve_ioctl_test () at sve-ioctls.c:44
>    44              res = prctl(PR_SVE_SET_VL, i, 0, 0, 0, 0);
>    (gdb) print i
>    $2 = 32
>    (gdb) info registers
>            ⋮
>    [ snip registers x0 to x30 ]
>            ⋮
>    sp             0xffffffffeff0      0xffffffffeff0
>    pc             0xaaaaaaaaa8ac      0xaaaaaaaaa8ac <do_sve_ioctl_test+112>
>    cpsr           0x60000000          [ EL=0 BTYPE=0 C Z ]
>    fpsr           0x0                 0
>    fpcr           0x0                 0
>    vg             0x8                 8
>    tpidr          0xfffff7fcb320      0xfffff7fcb320
>    (gdb) next
>    45              if (res < 0) {
>    (gdb) info registers
>            ⋮
>    [ snip registers x0 to x30 ]
>            ⋮
>    sp             0xffffffffeff0      0xffffffffeff0
>    pc             0xaaaaaaaaa8cc      0xaaaaaaaaa8cc <do_sve_ioctl_test+144>
>    cpsr           0x200000            [ EL=0 BTYPE=0 SS ]
>    fpsr           0x0                 0
>    fpcr           0x0                 0
>    vg             0x4                 4
>    (gdb)
> 
> Notice that register tpidr disappeared when vg (which holds the vector
> length) changed from 8 to 4.  The tpidr register is provided by the
> org.gnu.gdb.aarch64.tls feature.
> 
> This happens because the code that searches for a new gdbarch to match the
> new vector length in aarch64_linux_nat_target::thread_architecture doesn't
> take into account the features present in the target description associated
> with the previous gdbarch.  This patch makes it do that.
> 
> Since the id member of struct gdbarch_info is now unused, it's removed.
> ---
>   gdb/aarch64-linux-nat.c | 11 ++++++++---
>   gdb/aarch64-tdep.c      | 41 ++++++++++++++++++++++++-----------------
>   gdb/aarch64-tdep.h      |  2 ++
>   gdb/gdbarch.h           | 13 ++-----------
>   4 files changed, 36 insertions(+), 31 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index a457fcd48ad8..eda79ec6d35c 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -900,11 +900,16 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>   
>     /* We reach here if the vector length for the thread is different from its
>        value at process start.  Lookup gdbarch via info (potentially creating a
> -     new one), stashing the vector length inside id.  Use -1 for when SVE
> -     unavailable, to distinguish from an unset value of 0.  */
> +     new one) by using a target description that corresponds to the new vq value
> +     and the current architecture features.  */
> +
> +  const struct target_desc *tdesc = gdbarch_target_desc (inf->gdbarch);
> +  aarch64_features features = aarch64_features_from_target_desc (tdesc);
> +  features.vq = vq;
> +
>     struct gdbarch_info info;
>     info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
> -  info.id = (int *) (vq == 0 ? -1 : vq);
> +  info.target_desc = aarch64_read_description (features);
>     return gdbarch_find_by_info (info);
>   }
>   
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 8670197a8889..c9ce4f95447c 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3372,6 +3372,27 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>     return sve_vq_from_vl (vl);
>   }
>   
> +/* Get the AArch64 features present in the given target description. */
> +
> +aarch64_features
> +aarch64_features_from_target_desc (const struct target_desc *tdesc)
> +{
> +  aarch64_features features;
> +
> +  if (tdesc == nullptr)
> +    return features;
> +
> +  features.vq = aarch64_get_tdesc_vq (tdesc);
> +  features.pauth
> +      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth") != nullptr);
> +  features.mte
> +      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte") != nullptr);
> +  features.tls
> +      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.tls") != nullptr);
> +
> +  return features;
> +}
> +
>   /* Implement the "cannot_store_register" gdbarch method.  */
>   
>   static int
> @@ -3422,17 +3443,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     int i, num_regs = 0, num_pseudo_regs = 0;
>     int first_pauth_regnum = -1, ra_sign_state_offset = -1;
>     int first_mte_regnum = -1, tls_regnum = -1;
> -
> -  /* Use the vector length passed via the target info.  Here -1 is used for no
> -     SVE, and 0 is unset.  If unset then use the vector length from the existing
> -     tdesc.  */
> -  uint64_t vq = 0;
> -  if (info.id == (int *) -1)
> -    vq = 0;
> -  else if (info.id != 0)
> -    vq = (uint64_t) info.id;
> -  else
> -    vq = aarch64_get_tdesc_vq (info.target_desc);
> +  uint64_t vq = aarch64_get_tdesc_vq (info.target_desc);
>   
>     if (vq > AARCH64_MAX_SVE_VQ)
>       internal_error (__FILE__, __LINE__, _("VQ out of bounds: %s (max %d)"),
> @@ -3452,12 +3463,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     /* Ensure we always have a target descriptor, and that it is for the given VQ
>        value.  */
>     const struct target_desc *tdesc = info.target_desc;
> -  if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
> -    {
> -      aarch64_features features;
> -      features.vq = vq;
> -      tdesc = aarch64_read_description (features);
> -    }
> +  if (!tdesc_has_registers (tdesc))
> +    tdesc = aarch64_read_description ({});
>     gdb_assert (tdesc);
>   
>     feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 5bdd733dce32..d8513023c376 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -121,6 +121,8 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
>   };
>   
>   const target_desc *aarch64_read_description (const aarch64_features &features);
> +aarch64_features
> +aarch64_features_from_target_desc (const struct target_desc *tdesc);
>   
>   extern int aarch64_process_record (struct gdbarch *gdbarch,
>   			       struct regcache *regcache, CORE_ADDR addr);
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 324958469a7e..1287f0214885 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -250,17 +250,8 @@ struct gdbarch_info
>   
>     bfd *abfd = nullptr;
>   
> -  union
> -    {
> -      /* Architecture-specific target description data.  Numerous targets
> -	 need only this, so give them an easy way to hold it.  */
> -      struct tdesc_arch_data *tdesc_data;
> -
> -      /* SPU file system ID.  This is a single integer, so using the
> -	 generic form would only complicate code.  Other targets may
> -	 reuse this member if suitable.  */
> -      int *id;
> -    };
> +  /* Architecture-specific target description data.  */
> +  struct tdesc_arch_data *tdesc_data;
>   
>     enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
>   

This version looks good to me. Thanks for addressing the comments.

I think the gdbarch.h change is reasonably obvious that folks wouldn't mind it going in
along with this fix.