Re: [PATCH v3 3/3] Parse SVE registers in aarch64 core file reading/writing

[Simon Marchi <simon dot marchi at ericsson dot com>]

Hi Alan,

Just a few nits for this one.  The patch is approved with these fixed.  If
you prefer to send a new version for review, note that I'll only be able to
take a look on the 20th or after (after catching up on emails and stuff).

> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 73cb9ea714..bfcee8564c 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -287,6 +287,103 @@ aarch64_linux_core_read_vq (struct gdbarch *gdbarch, bfd *abfd)
>    return vq;
>  }
>  
> +/* Supply register REGNUM from BUF to REGCACHE, using the register map
> +   in REGSET.  If REGNUM is -1, do this for all registers in REGSET.
> +   If BUF is NULL, set the registers to "unavailable" status.  */
> +
> +static void
> +aarch64_linux_supply_sve_regset (const struct regset *regset,
> +				 struct regcache *regcache,
> +				 int regnum, const void *buf, size_t size)
> +{
> +  gdb_byte *header = (gdb_byte *) buf;
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +
> +  if (buf == nullptr)
> +    return regcache->supply_regset (regset, regnum, nullptr, size);
> +  gdb_assert (size > SVE_HEADER_SIZE);
> +
> +  /* BUF contains an SVE header followed by a register dump of either the
> +     passed in SVE regset or a NEON fpregset.  */
> +
> +  /* Extract required fields from the header.  */
> +  uint64_t vl = extract_unsigned_integer (header + SVE_HEADER_VL_OFFSET,
> +					  SVE_HEADER_VL_LENGTH, byte_order);
> +  uint16_t flags = extract_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET,
> +					     SVE_HEADER_FLAGS_LENGTH,
> +					     byte_order);
> +
> +  if (regnum == -1 || regnum == AARCH64_SVE_VG_REGNUM)
> +    {
> +      uint64_t vg_target;
> +      store_integer ((gdb_byte *)&vg_target, sizeof (uint64_t), byte_order,

Use

  gdb_byte vg_target[8];

instead of an uint64_t.  That value doesn't belong to an host integer variable,
putting it in an uint64_t may lead people to use it in a wrong way.

> +		     sve_vg_from_vl (vl));
> +      regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_target);
> +    }
> +
> +  if (flags & 1)

Can you replace this magic 1 with a define?

> +    {
> +      /* Register dump is a SVE structure.  */
> +      regcache->supply_regset (regset, regnum,
> +			       (gdb_byte *) buf + SVE_HEADER_SIZE,
> +			       size - SVE_HEADER_SIZE);
> +    }
> +  else
> +    {
> +      /* Register dump is a fpsimd structure.  First clear the SVE
> +	 registers.  */
> +      for (int i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
> +	regcache->raw_supply_zeroed (AARCH64_SVE_Z0_REGNUM + i);
> +      for (int i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
> +	regcache->raw_supply_zeroed (AARCH64_SVE_P0_REGNUM + i);
> +      regcache->raw_supply_zeroed (AARCH64_SVE_FFR_REGNUM);
> +
> +      /* Then supply the fpsimd registers.  */
> +      regcache->supply_regset (&aarch64_linux_fpregset, regnum,
> +			       (gdb_byte *) buf + SVE_HEADER_SIZE,
> +			       size - SVE_HEADER_SIZE);
> +    }
> +}
> +
> +/* Collect register REGNUM from REGCACHE to BUF, using the register
> +   map in REGSET.  If REGNUM is -1, do this for all registers in
> +   REGSET.  */
> +
> +static void
> +aarch64_linux_collect_sve_regset (const struct regset *regset,
> +				  const struct regcache *regcache,
> +				  int regnum, void *buf, size_t size)
> +{
> +  gdb_byte *header = (gdb_byte *) buf;
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  uint64_t vq = gdbarch_tdep (gdbarch)->vq;
> +
> +  gdb_assert (buf != NULL);
> +  gdb_assert (size > SVE_HEADER_SIZE);
> +
> +  /* BUF starts with a SVE header prior to the register dump.  */
> +
> +  store_unsigned_integer (header + SVE_HEADER_SIZE_OFFSET,
> +			  SVE_HEADER_SIZE_LENGTH, byte_order, size);
> +  store_unsigned_integer (header + SVE_HEADER_MAX_SIZE_OFFSET,
> +			  SVE_HEADER_MAX_SIZE_LENGTH, byte_order, size);
> +  store_unsigned_integer (header + SVE_HEADER_VL_OFFSET, SVE_HEADER_VL_LENGTH,
> +			  byte_order, sve_vl_from_vq (vq));
> +  store_unsigned_integer (header + SVE_HEADER_MAX_VL_OFFSET,
> +			  SVE_HEADER_MAX_VL_LENGTH, byte_order,
> +			  sve_vl_from_vq (vq));
> +  store_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET,
> +			  SVE_HEADER_FLAGS_LENGTH, byte_order, 1);

I guess this 1 is the same flag as up there, so could use the same define?

> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index ea692f38b8..ef2cc478e2 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -92,6 +92,14 @@ struct regcache_map_entry
>    int size;
>  };
>  
> +static inline int regcache_map_entry_size (const struct regcache_map_entry *map)

Please provide a comment and add a \n before the function name.

Also, we might want to name it "regcache_map_size", since this function provides
the size of a complete regcache_map (an array of regcache_map_entry), not the size
of a single regcache_map_entry.

Simon