Re: [PATCH v2 2/3] Detect SVE when reading aarch64 core files

[Simon Marchi <simon.marchi@ericsson.com>]

On 2018-07-30 05:25 AM, Alan Hayward wrote:
> Add a function which reads the vector length from the SVE section within an
> aarch64 core file.
> 
> The SVE section in a core file contains a header followed by the registers.
> All macros to parse access this structure.
> 
> 2018-07-30  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* aarch64-linux-tdep.c (struct struct_contents): Add struct.
> 	(SVE_HEADER_SIZE): Add Macro.
> 	(SVE_HEADER_WRITE): Likewise.
> 	(SVE_HEADER_READ): Likewise.
> 	(aarch64_linux_core_read_vq): Add function.
> 	(aarch64_linux_core_read_description): Check for SVE section.
> ---
>  gdb/aarch64-linux-tdep.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 7b63cddbe6..f9a95950da 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -219,6 +219,75 @@ const struct regset aarch64_linux_fpregset =
>      regcache_supply_regset, regcache_collect_regset
>    };
>  
> +/* Description of the fields in an SVE header at the start of a SVE regset.  */
> +
> +struct struct_contents {
> +  int len;
> +  int offset;
> +} sve_header[] =
> +    {
> +      { 4, 0 },		/* __u32 size.  */
> +      { 4, 4 },		/* __u32 max_size.  */
> +      { 2, 8 },		/* __u16 vl.  */
> +      { 2, 10 },	/* __u16 max_vl.  */
> +      { 2, 12 },	/* __u16 flags.  */
> +      { 2, 14 },	/* __u16 reserved.  */
> +      { -1, 16 }
> +    };

I don't think the resulting code is very readable, for example this bit from the next
patch:

  SVE_HEADER_WRITE (buf, 0, byte_order, size);
  SVE_HEADER_WRITE (buf, 1, byte_order, size);
  SVE_HEADER_WRITE (buf, 2, byte_order, sve_vl_from_vq (vq));
  SVE_HEADER_WRITE (buf, 3, byte_order, sve_vl_from_vq (vq));
  SVE_HEADER_WRITE (buf, 4, byte_order, 1);
  SVE_HEADER_WRITE (buf, 5, byte_order, 0);

Maybe have some macros to access the index of the fields in the structure, like this?

#define SVE_HEADER_SIZE_FIELD 0
#define SVE_HEADER_MAX_SIZE_FIELD 1
#define SVE_HEADER_VL_FIELD 2
...

> +
> +#define SVE_HEADER_SIZE (sve_header[6].offset)

Perhaps use

  ARRAY_SIZE (sve_header) - 1

instead of 6?

> +
> +#define SVE_HEADER_WRITE(header, entry, byte_order, value) \
> +  store_unsigned_integer ((gdb_byte *) header + sve_header[entry].offset, \
> +			  sve_header[entry].len, byte_order, value)
> +
> +#define SVE_HEADER_READ(header, entry, byte_order) \
> +  extract_unsigned_integer ((gdb_byte *) header + sve_header[entry].offset, \
> +			    sve_header[entry].len, byte_order)

I'm not a big fan of macros when the same work can be done with a function, which
seems to be the case here.

> +
> +/* Get VQ value from SVE section in the core dump.  */
> +
> +static uint64_t
> +aarch64_linux_core_read_vq (struct gdbarch *gdbarch, bfd *abfd)
> +{
> +  gdb_byte header[SVE_HEADER_SIZE];
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  asection *sve_section = bfd_get_section_by_name (abfd, ".reg-aarch-sve");
> +
> +  if (!sve_section)

== nullptr

> +    {
> +      /* No SVE state.  */
> +      return 0;
> +    }
> +
> +  size_t size = bfd_section_size (abfd, sve_section);
> +
> +  /* Check extended state size.  */
> +  if (size < SVE_HEADER_SIZE)
> +    {
> +      warning (_("'.reg-aarch-sve' section in core file too small."));
> +      return 0;
> +    }
> +
> +  if (!bfd_get_section_contents (abfd, sve_section, header, 0, SVE_HEADER_SIZE))
> +    {
> +      warning (_("Couldn't read sve header from "
> +		 "'.reg-aarch-sve' section in core file."));
> +      return 0;
> +    }
> +
> +  uint64_t vq = sve_vq_from_vl (SVE_HEADER_READ (header, 2, byte_order));
> +
> +  if (vq > AARCH64_MAX_SVE_VQ)
> +    {
> +      warning (_("sve header invalid in "
> +		 "'.reg-aarch-sve' section in core file."));
> +      return 0;
> +    }

Is 0 a valid value for vq (in other words, should it show a warning)?

Simon