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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

Hi,

Thanks for the patch.

On 7/28/22 02:23, 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.
> ---
>   gdb/aarch64-linux-nat.c | 11 ++++++++---
>   gdb/aarch64-tdep.c      | 25 +++++++++++++++++++++++++
>   gdb/aarch64-tdep.h      |  2 ++
>   3 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index a457fcd48ad8..5963e246b43f 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);

Even though we're removing this code, we're still actively using info.id in aarch64_gdbarch_init (). Will that work correctly?

> +  info.target_desc = aarch64_create_target_description (features);

The current mechanism caches potentially multiple target description (to account for threads having different VL's). Instead of creating a fresh target description here, would it make
more sense to figure out the features from this particular thread and then check if we have the right target description already?

Maybe through aarch64_linux_nat_target::read_description ()?

See the code in gdb/aarch64-tdep.c:aarch64_read_description (), which fetches a cached target description and creates a fresh one if it doesn't exist.

>     return gdbarch_find_by_info (info);
>   }
>   
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 8670197a8889..8b89b877f8f0 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3352,6 +3352,31 @@ aarch64_read_description (const aarch64_features &features)
>     return tdesc;
>   }
>   
> +/* 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;
> +  const struct tdesc_feature *feature;
> +
> +  if (tdesc == nullptr)
> +    return features;
> +
> +  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
> +  if (feature != nullptr)
> +    features.vq = tdesc_register_bitsize (feature, "z0") / 128;
> +
> +  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;
> +}
> +
>   /* Return the VQ used when creating the target description TDESC.  */
>   
>   static uint64_t
> 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);