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

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

[Alan Hayward]

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 }
+    };
+
+#define SVE_HEADER_SIZE (sve_header[6].offset)
+
+#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)
+
+/* 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)
+    {
+      /* 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;
+    }
+
+  return vq;
+}
+
 /* Implement the "regset_from_core_section" gdbarch method.  */
 
 static void
@@ -233,8 +302,7 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
       &aarch64_linux_fpregset, NULL, cb_data);
 }
 
-/* Implement the "core_read_description" gdbarch method.  SVE not yet
-   supported.  */
+/* Implement the "core_read_description" gdbarch method.  */
 
 static const struct target_desc *
 aarch64_linux_core_read_description (struct gdbarch *gdbarch,
@@ -245,7 +313,7 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
   if (target_auxv_search (target, AT_HWCAP, &aarch64_hwcap) != 1)
     return NULL;
 
-  return aarch64_read_description (0);
+  return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd));
 }
 
 /* Implementation of `gdbarch_stap_is_single_operand', as defined in
-- 
2.15.2 (Apple Git-101.1)

[PATCH v2 3/3] Parse SVE registers in aarch64 core file reading/writing

[Alan Hayward]

sve_regmap cannot be global static as the size is dependant on the current
vector length.

2018-07-30  Alan Hayward  <alan.hayward@arm.com>

	* aarch64-linux-tdep.c (aarch64_linux_supply_sve_regset): New function.
	(aarch64_linux_collect_sve_regset): Likewise.
	(aarch64_linux_iterate_over_regset_sections): Check for SVE.
	* regcache.h (regcache_map_entry_size): New function.
---
 gdb/aarch64-linux-tdep.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/regcache.h           |   8 ++++
 2 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index f9a95950da..bd61a2d722 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -288,6 +288,85 @@ 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)
+{
+  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 vg = sve_vg_from_vl (SVE_HEADER_READ (buf, 2, byte_order));
+  uint16_t flags = SVE_HEADER_READ (buf, 4, byte_order);
+
+  if (regnum == -1 || regnum == AARCH64_SVE_VG_REGNUM)
+    regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg);
+
+  if (flags & 1)
+    {
+      /* 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)
+{
+  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.  */
+  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);
+
+  /* The SVE register dump follows.  */
+  regcache->collect_regset (regset, regnum, (gdb_byte *) buf + SVE_HEADER_SIZE,
+			    size - SVE_HEADER_SIZE);
+}
+
 /* Implement the "regset_from_core_section" gdbarch method.  */
 
 static void
@@ -296,10 +375,39 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					    void *cb_data,
 					    const struct regcache *regcache)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
   cb (".reg", AARCH64_LINUX_SIZEOF_GREGSET, AARCH64_LINUX_SIZEOF_GREGSET,
       &aarch64_linux_gregset, NULL, cb_data);
-  cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, AARCH64_LINUX_SIZEOF_FPREGSET,
-      &aarch64_linux_fpregset, NULL, cb_data);
+
+  if (tdep->has_sve ())
+    {
+      /* Create this on the fly in order to handle vector register sizes.  */
+      const struct regcache_map_entry sve_regmap[] =
+	{
+	  { 32, AARCH64_SVE_Z0_REGNUM, tdep->vq * 16 },
+	  { 16, AARCH64_SVE_P0_REGNUM, tdep->vq * 16 / 8 },
+	  { 1, AARCH64_SVE_FFR_REGNUM, 4 },
+	  { 1, AARCH64_FPSR_REGNUM, 4 },
+	  { 1, AARCH64_FPCR_REGNUM, 4 },
+	  { 0 }
+	};
+
+      const struct regset aarch64_linux_sve_regset =
+	{
+	  sve_regmap,
+	  aarch64_linux_supply_sve_regset, aarch64_linux_collect_sve_regset,
+	  REGSET_VARIABLE_SIZE
+	};
+
+      cb (".reg-aarch-sve",
+	  SVE_HEADER_SIZE + regcache_map_entry_size (aarch64_linux_fpregmap),
+	  SVE_HEADER_SIZE + regcache_map_entry_size (sve_regmap),
+	  &aarch64_linux_sve_regset, "SVE registers", cb_data);
+    }
+  else
+    cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, AARCH64_LINUX_SIZEOF_FPREGSET,
+    	&aarch64_linux_fpregset, NULL, cb_data);
 }
 
 /* Implement the "core_read_description" gdbarch method.  */
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)
+{
+  int size = 0;
+  for (int i = 0; map[i].count != 0; i++)
+    size += (map[i].count * map[i].size);
+  return size;
+}
+
 /* Special value for the 'regno' field in the struct above.  */
 
 enum
-- 
2.15.2 (Apple Git-101.1)

[PATCH v2 1/3] Add min size to regset section iterations

[Alan Hayward]

When using the regset section iteration functions, the size parameter is used
in different ways.

With collect, size is used to create the buffer in which to write the regset.
(see linux-tdep.c::linux_collect_regset_section_cb).

With supply, size is used to confirm the existing regset is the correct size.
If REGSET_VARIABLE_SIZE is set then the regset can be bigger than size.
Effectively, size is the minimum possible size of the regset.
(see corelow.c::get_core_register_section).

There are currently no targets with both REGSET_VARIABLE_SIZE and a collect
function.

To allow support of collects for REGSET_VARIABLE_SIZE we need two sizes.
Min_size is the minimum allowed size for the regset, and size is used as the
size to use when creating new regsets. For all targets that are not
REGSET_VARIABLE_SIZE then these two sizes are equal.

I considered 1) renaming the sizes as supply_size and collect_size, 2) moving
the sizes into the regset structure and 3) replacing
_iterate_over_regset_sections funcs with two funcs -
_iterate_over_collect_regset and _iterate_over_supply_regset.
All of these required lots of moving around for not much gain.

2018-07-30  Alan Hayward  <alan.hayward@arm.com>

	* aarch64-fbsd-tdep.c
	(aarch64_fbsd_iterate_over_regset_sections): Add min size.
	* aarch64-linux-tdep.c
	(aarch64_linux_iterate_over_regset_sections): Likewise.
	* alpha-linux-tdep.c
	(alpha_linux_iterate_over_regset_sections):
	* alpha-nbsd-tdep.c
	(alphanbsd_iterate_over_regset_sections): Likewise.
	* amd64-fbsd-tdep.c
	(amd64fbsd_iterate_over_regset_sections): Likewise.
	* amd64-linux-tdep.c
	(amd64_linux_iterate_over_regset_sections): Likewise.
	* arm-bsd-tdep.c
	(armbsd_iterate_over_regset_sections): Likewise.
	* arm-fbsd-tdep.c
	(arm_fbsd_iterate_over_regset_sections): Likewise.
	* arm-linux-tdep.c
	(arm_linux_iterate_over_regset_sections): Likewise.
	* corelow.c (core_target::get_core_register_section): Add size.
	(get_core_registers_cb): Add min size.
	(core_target::fetch_registers): Likewise.
	* fbsd-tdep.c (fbsd_collect_regset_section_cb): Likewise.
	* frv-linux-tdep.c (frv_linux_iterate_over_regset_sections): Likewise.
	* gdbarch.h (void): Regenerate.
	* gdbarch.sh: Add min size.
	* hppa-linux-tdep.c (hppa_linux_iterate_over_regset_sections): Likewise.
	* hppa-nbsd-tdep.c (hppanbsd_iterate_over_regset_sections): Likewise.
	* hppa-obsd-tdep.c (hppaobsd_iterate_over_regset_sections): Likewise.
	* i386-fbsd-tdep.c (i386fbsd_iterate_over_regset_sections): Likewise.
	* i386-linux-tdep.c (i386_linux_iterate_over_regset_sections): Likewise.
	* i386-tdep.c (i386_iterate_over_regset_sections): Likewise.
	* ia64-linux-tdep.c (ia64_linux_iterate_over_regset_sections): Likewise.
	* linux-tdep.c (linux_collect_regset_section_cb): Likewise.
	* m32r-linux-tdep.c (m32r_linux_iterate_over_regset_sections): Likewise.
	* m68k-bsd-tdep.c (m68kbsd_iterate_over_regset_sections): Likewise.
	* m68k-linux-tdep.c (m68k_linux_iterate_over_regset_sections): Likewise.
	* mips-fbsd-tdep.c (mips_fbsd_iterate_over_regset_sections): Likewise.
	* mips-linux-tdep.c (mips_linux_iterate_over_regset_sections): Likewise.
	* mips-nbsd-tdep.c (mipsnbsd_iterate_over_regset_sections): Likewise.
	* mips64-obsd-tdep.c (mips64obsd_iterate_over_regset_sections): Likewise.
	* mn10300-linux-tdep.c (am33_iterate_over_regset_sections): Likewise.
	* nios2-linux-tdep.c (nios2_iterate_over_regset_sections): Likewise.
	* ppc-fbsd-tdep.c (ppcfbsd_iterate_over_regset_sections): Likewise.
	* ppc-linux-tdep.c (ppc_linux_iterate_over_regset_sections): Likewise.
	* ppc-nbsd-tdep.c (ppcnbsd_iterate_over_regset_sections): Likewise.
	* ppc-obsd-tdep.c (ppcobsd_iterate_over_regset_sections): Likewise.
	* rs6000-aix-tdep.c (rs6000_aix_iterate_over_regset_sections): Likewise.
	* s390-linux-tdep.c (s390_iterate_over_regset_sections): Likewise.
	* score-tdep.c (score7_linux_iterate_over_regset_sections): Likewise.
	* sh-tdep.c (sh_iterate_over_regset_sections): Likewise.
	* sparc-tdep.c (sparc_iterate_over_regset_sections): Likewise.
	* tilegx-linux-tdep.c (tilegx_iterate_over_regset_sections): Likewise.
	* vax-tdep.c (vax_iterate_over_regset_sections): Likewise.
	* xtensa-tdep.c (xtensa_iterate_over_regset_sections): Likewise.
---
 gdb/aarch64-fbsd-tdep.c  |  8 ++++----
 gdb/aarch64-linux-tdep.c |  8 ++++----
 gdb/alpha-linux-tdep.c   |  4 ++--
 gdb/alpha-nbsd-tdep.c    |  6 ++++--
 gdb/amd64-fbsd-tdep.c    |  8 +++++---
 gdb/amd64-linux-tdep.c   |  6 +++---
 gdb/arm-bsd-tdep.c       |  6 ++++--
 gdb/arm-fbsd-tdep.c      |  7 ++++---
 gdb/arm-linux-tdep.c     | 11 ++++++-----
 gdb/corelow.c            | 27 +++++++++++++++------------
 gdb/fbsd-tdep.c          |  2 +-
 gdb/frv-linux-tdep.c     |  8 ++++----
 gdb/gdbarch.h            |  2 +-
 gdb/gdbarch.sh           |  2 +-
 gdb/hppa-linux-tdep.c    |  6 +++---
 gdb/hppa-nbsd-tdep.c     |  3 ++-
 gdb/hppa-obsd-tdep.c     |  6 ++++--
 gdb/i386-fbsd-tdep.c     | 11 +++++++----
 gdb/i386-linux-tdep.c    |  9 +++++----
 gdb/i386-tdep.c          |  6 ++++--
 gdb/ia64-linux-tdep.c    |  6 ++++--
 gdb/linux-tdep.c         |  2 +-
 gdb/m32r-linux-tdep.c    |  3 ++-
 gdb/m68k-bsd-tdep.c      |  6 ++++--
 gdb/m68k-linux-tdep.c    |  6 ++++--
 gdb/mips-fbsd-tdep.c     |  8 ++++----
 gdb/mips-linux-tdep.c    | 14 ++++++++------
 gdb/mips-nbsd-tdep.c     |  8 ++++----
 gdb/mips64-obsd-tdep.c   |  3 ++-
 gdb/mn10300-linux-tdep.c |  8 ++++----
 gdb/nios2-linux-tdep.c   |  3 ++-
 gdb/ppc-fbsd-tdep.c      |  6 +++---
 gdb/ppc-linux-tdep.c     | 12 ++++++------
 gdb/ppc-nbsd-tdep.c      |  4 ++--
 gdb/ppc-obsd-tdep.c      |  2 +-
 gdb/rs6000-aix-tdep.c    |  4 ++--
 gdb/s390-linux-tdep.c    | 23 ++++++++++++-----------
 gdb/score-tdep.c         |  4 ++--
 gdb/sh-tdep.c            |  6 ++++--
 gdb/sparc-tdep.c         |  6 ++++--
 gdb/tilegx-linux-tdep.c  |  4 ++--
 gdb/vax-tdep.c           |  2 +-
 gdb/xtensa-tdep.c        |  4 ++--
 43 files changed, 163 insertions(+), 127 deletions(-)

diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
index 21beb5b77c..9f8b1366ce 100644
--- a/gdb/aarch64-fbsd-tdep.c
+++ b/gdb/aarch64-fbsd-tdep.c
@@ -169,10 +169,10 @@ aarch64_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					   void *cb_data,
 					   const struct regcache *regcache)
 {
-  cb (".reg", AARCH64_FBSD_SIZEOF_GREGSET, &aarch64_fbsd_gregset,
-      NULL, cb_data);
-  cb (".reg2", AARCH64_FBSD_SIZEOF_FPREGSET, &aarch64_fbsd_fpregset,
-      NULL, cb_data);
+  cb (".reg", AARCH64_FBSD_SIZEOF_GREGSET, AARCH64_FBSD_SIZEOF_GREGSET,
+      &aarch64_fbsd_gregset, NULL, cb_data);
+  cb (".reg2", AARCH64_FBSD_SIZEOF_FPREGSET, AARCH64_FBSD_SIZEOF_FPREGSET,
+      &aarch64_fbsd_fpregset, NULL, cb_data);
 }
 
 /* Implement the 'init_osabi' method of struct gdb_osabi_handler.  */
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 93b6d416a3..7b63cddbe6 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -227,10 +227,10 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					    void *cb_data,
 					    const struct regcache *regcache)
 {
-  cb (".reg", AARCH64_LINUX_SIZEOF_GREGSET, &aarch64_linux_gregset,
-      NULL, cb_data);
-  cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset,
-      NULL, cb_data);
+  cb (".reg", AARCH64_LINUX_SIZEOF_GREGSET, AARCH64_LINUX_SIZEOF_GREGSET,
+      &aarch64_linux_gregset, NULL, cb_data);
+  cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, AARCH64_LINUX_SIZEOF_FPREGSET,
+      &aarch64_linux_fpregset, NULL, cb_data);
 }
 
 /* Implement the "core_read_description" gdbarch method.  SVE not yet
diff --git a/gdb/alpha-linux-tdep.c b/gdb/alpha-linux-tdep.c
index 8bacb10aa1..913a420401 100644
--- a/gdb/alpha-linux-tdep.c
+++ b/gdb/alpha-linux-tdep.c
@@ -238,8 +238,8 @@ alpha_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					  void *cb_data,
 					  const struct regcache *regcache)
 {
-  cb (".reg", 32 * 8, &alpha_linux_gregset, NULL, cb_data);
-  cb (".reg2", 32 * 8, &alpha_linux_fpregset, NULL, cb_data);
+  cb (".reg", 32 * 8, 32 * 8, &alpha_linux_gregset, NULL, cb_data);
+  cb (".reg2", 32 * 8, 32 * 8, &alpha_linux_fpregset, NULL, cb_data);
 }
 
 /* Implementation of `gdbarch_gdb_signal_from_target', as defined in
diff --git a/gdb/alpha-nbsd-tdep.c b/gdb/alpha-nbsd-tdep.c
index dffab3fb37..694e3d3555 100644
--- a/gdb/alpha-nbsd-tdep.c
+++ b/gdb/alpha-nbsd-tdep.c
@@ -161,8 +161,10 @@ alphanbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					void *cb_data,
 					const struct regcache *regcache)
 {
-  cb (".reg", ALPHANBSD_SIZEOF_GREGS, &alphanbsd_gregset, NULL, cb_data);
-  cb (".reg2", ALPHANBSD_SIZEOF_FPREGS, &alphanbsd_fpregset, NULL, cb_data);
+  cb (".reg", ALPHANBSD_SIZEOF_GREGS, ALPHANBSD_SIZEOF_GREGS,
+      &alphanbsd_gregset, NULL, cb_data);
+  cb (".reg2", ALPHANBSD_SIZEOF_FPREGS, ALPHANBSD_SIZEOF_FPREGS,
+      &alphanbsd_fpregset, NULL, cb_data);
 }
 \f
 
diff --git a/gdb/amd64-fbsd-tdep.c b/gdb/amd64-fbsd-tdep.c
index f5bf1985b6..7f7ecfcf11 100644
--- a/gdb/amd64-fbsd-tdep.c
+++ b/gdb/amd64-fbsd-tdep.c
@@ -196,9 +196,11 @@ amd64fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", tdep->sizeof_gregset, &i386_gregset, NULL, cb_data);
-  cb (".reg2", tdep->sizeof_fpregset, &amd64_fpregset, NULL, cb_data);
-  cb (".reg-xstate", X86_XSTATE_SIZE(tdep->xcr0),
+  cb (".reg", tdep->sizeof_gregset, tdep->sizeof_gregset, &i386_gregset, NULL,
+      cb_data);
+  cb (".reg2", tdep->sizeof_fpregset, tdep->sizeof_fpregset, &amd64_fpregset,
+      NULL, cb_data);
+  cb (".reg-xstate", X86_XSTATE_SIZE (tdep->xcr0), X86_XSTATE_SIZE (tdep->xcr0),
       &amd64fbsd_xstateregset, "XSAVE extended state", cb_data);
 }
 
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 7fe37d83f6..7ab43897ab 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1651,9 +1651,9 @@ amd64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", 27 * 8, &i386_gregset, NULL, cb_data);
-  cb (".reg2", 512, &amd64_fpregset, NULL, cb_data);
-  cb (".reg-xstate",  X86_XSTATE_SIZE (tdep->xcr0),
+  cb (".reg", 27 * 8, 27 * 8, &i386_gregset, NULL, cb_data);
+  cb (".reg2", 512, 512, &amd64_fpregset, NULL, cb_data);
+  cb (".reg-xstate", X86_XSTATE_SIZE (tdep->xcr0), X86_XSTATE_SIZE (tdep->xcr0),
       &amd64_linux_xstateregset, "XSAVE extended state", cb_data);
 }
 
diff --git a/gdb/arm-bsd-tdep.c b/gdb/arm-bsd-tdep.c
index a2719caa4c..8025ab3dee 100644
--- a/gdb/arm-bsd-tdep.c
+++ b/gdb/arm-bsd-tdep.c
@@ -117,6 +117,8 @@ armbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				     void *cb_data,
 				     const struct regcache *regcache)
 {
-  cb (".reg", ARMBSD_SIZEOF_GREGS, &armbsd_gregset, NULL, cb_data);
-  cb (".reg2", ARMBSD_SIZEOF_FPREGS, &armbsd_fpregset, NULL, cb_data);
+  cb (".reg", ARMBSD_SIZEOF_GREGS, ARMBSD_SIZEOF_GREGS, &armbsd_gregset, NULL,
+      cb_data);
+  cb (".reg2", ARMBSD_SIZEOF_FPREGS, ARMBSD_SIZEOF_FPREGS, &armbsd_fpregset,
+      NULL, cb_data);
 }
diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
index 4ad3abdffa..d2af21bf2c 100644
--- a/gdb/arm-fbsd-tdep.c
+++ b/gdb/arm-fbsd-tdep.c
@@ -175,14 +175,15 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", ARM_FBSD_SIZEOF_GREGSET, &arm_fbsd_gregset, NULL, cb_data);
+  cb (".reg", ARM_FBSD_SIZEOF_GREGSET, ARM_FBSD_SIZEOF_GREGSET,
+      &arm_fbsd_gregset, NULL, cb_data);
 
   /* While FreeBSD/arm cores do contain a NT_FPREGSET / ".reg2"
      register set, it is not populated with register values by the
      kernel but just contains all zeroes.  */
   if (tdep->vfp_register_count > 0)
-    cb (".reg-arm-vfp", ARM_FBSD_SIZEOF_VFPREGSET, &arm_fbsd_vfpregset,
-	"VFP floating-point", cb_data);
+    cb (".reg-arm-vfp", ARM_FBSD_SIZEOF_VFPREGSET, ARM_FBSD_SIZEOF_VFPREGSET,
+	&arm_fbsd_vfpregset, "VFP floating-point", cb_data);
 }
 
 /* Lookup a target description from a target's AT_HWCAP auxiliary
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 264e8ca42b..36198b7cdc 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -712,14 +712,15 @@ arm_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", ARM_LINUX_SIZEOF_GREGSET, &arm_linux_gregset, NULL, cb_data);
+  cb (".reg", ARM_LINUX_SIZEOF_GREGSET, ARM_LINUX_SIZEOF_GREGSET,
+      &arm_linux_gregset, NULL, cb_data);
 
   if (tdep->vfp_register_count > 0)
-    cb (".reg-arm-vfp", ARM_LINUX_SIZEOF_VFP, &arm_linux_vfpregset,
-	"VFP floating-point", cb_data);
+    cb (".reg-arm-vfp", ARM_LINUX_SIZEOF_VFP, ARM_LINUX_SIZEOF_VFP,
+	&arm_linux_vfpregset, "VFP floating-point", cb_data);
   else if (tdep->have_fpa_registers)
-    cb (".reg2", ARM_LINUX_SIZEOF_NWFPE, &arm_linux_fpregset,
-	"FPA floating-point", cb_data);
+    cb (".reg2", ARM_LINUX_SIZEOF_NWFPE, ARM_LINUX_SIZEOF_NWFPE,
+    	&arm_linux_fpregset, "FPA floating-point", cb_data);
 }
 
 /* Determine target description from core file.  */
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 059ce2f6eb..32a054ee3e 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -107,6 +107,7 @@ public:
 				  const struct regset *regset,
 				  const char *name,
 				  int min_size,
+				  int size,
 				  int which,
 				  const char *human_name,
 				  bool required);
@@ -570,12 +571,13 @@ core_target::get_core_register_section (struct regcache *regcache,
 					const struct regset *regset,
 					const char *name,
 					int min_size,
+					int size,
 					int which,
 					const char *human_name,
 					bool required)
 {
   struct bfd_section *section;
-  bfd_size_type size;
+  bfd_size_type core_size;
   char *contents;
   bool variable_size_section = (regset != NULL
 				&& regset->flags & REGSET_VARIABLE_SIZE);
@@ -591,22 +593,22 @@ core_target::get_core_register_section (struct regcache *regcache,
       return;
     }
 
-  size = bfd_section_size (core_bfd, section);
-  if (size < min_size)
+  core_size = bfd_section_size (core_bfd, section);
+  if (core_size < min_size)
     {
       warning (_("Section `%s' in core file too small."),
 	       section_name.c_str ());
       return;
     }
-  if (size != min_size && !variable_size_section)
+  if (core_size != size && !variable_size_section)
     {
       warning (_("Unexpected size of section `%s' in core file."),
 	       section_name.c_str ());
     }
 
-  contents = (char *) alloca (size);
+  contents = (char *) alloca (core_size);
   if (! bfd_get_section_contents (core_bfd, section, contents,
-				  (file_ptr) 0, size))
+				  (file_ptr) 0, core_size))
     {
       warning (_("Couldn't read %s registers from `%s' section in core file."),
 	       human_name, section_name.c_str ());
@@ -615,12 +617,12 @@ core_target::get_core_register_section (struct regcache *regcache,
 
   if (regset != NULL)
     {
-      regset->supply_regset (regset, regcache, -1, contents, size);
+      regset->supply_regset (regset, regcache, -1, contents, core_size);
       return;
     }
 
   gdb_assert (m_core_vec != nullptr);
-  m_core_vec->core_read_registers (regcache, contents, size, which,
+  m_core_vec->core_read_registers (regcache, contents, core_size, which,
 				   ((CORE_ADDR)
 				    bfd_section_vma (core_bfd, section)));
 }
@@ -636,7 +638,7 @@ struct get_core_registers_cb_data
    register note section. */
 
 static void
-get_core_registers_cb (const char *sect_name, int size,
+get_core_registers_cb (const char *sect_name, int min_size, int size,
 		       const struct regset *regset,
 		       const char *human_name, void *cb_data)
 {
@@ -658,7 +660,8 @@ get_core_registers_cb (const char *sect_name, int size,
   /* The 'which' parameter is only used when no regset is provided.
      Thus we just set it to -1. */
   data->target->get_core_register_section (data->regcache, regset, sect_name,
-					   size, -1, human_name, required);
+					   min_size, size, -1, human_name,
+					   required);
 }
 
 /* Get the registers out of a core file.  This is the machine-
@@ -694,9 +697,9 @@ core_target::fetch_registers (struct regcache *regcache, int regno)
   else
     {
       get_core_register_section (regcache, NULL,
-				 ".reg", 0, 0, "general-purpose", 1);
+				 ".reg", 0, 0, 0, "general-purpose", 1);
       get_core_register_section (regcache, NULL,
-				 ".reg2", 0, 2, "floating-point", 0);
+				 ".reg2", 0, 0, 2, "floating-point", 0);
     }
 
   /* Mark all registers not found in the core as unavailable.  */
diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
index ac5b3bfa12..f7cef7cac4 100644
--- a/gdb/fbsd-tdep.c
+++ b/gdb/fbsd-tdep.c
@@ -428,7 +428,7 @@ struct fbsd_collect_regset_section_cb_data
 };
 
 static void
-fbsd_collect_regset_section_cb (const char *sect_name, int size,
+fbsd_collect_regset_section_cb (const char *sect_name, int min_size, int size,
 				const struct regset *regset,
 				const char *human_name, void *cb_data)
 {
diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c
index c47f1f24f0..655c48d787 100644
--- a/gdb/frv-linux-tdep.c
+++ b/gdb/frv-linux-tdep.c
@@ -445,10 +445,10 @@ frv_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					void *cb_data,
 					const struct regcache *regcache)
 {
-  cb (".reg", sizeof (frv_elf_gregset_t), &frv_linux_gregset,
-      NULL, cb_data);
-  cb (".reg2", sizeof (frv_elf_fpregset_t), &frv_linux_fpregset,
-      NULL, cb_data);
+  cb (".reg", sizeof (frv_elf_gregset_t), sizeof (frv_elf_gregset_t),
+      &frv_linux_gregset, NULL, cb_data);
+  cb (".reg2", sizeof (frv_elf_fpregset_t), sizeof (frv_elf_fpregset_t),
+      &frv_linux_fpregset, NULL, cb_data);
 }
 
 \f
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 0df1fd1692..205ed523ac 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -97,7 +97,7 @@ typedef int (iterate_over_objfiles_in_search_order_cb_ftype)
    CB_DATA should have been passed unchanged through the iterator.  */
 
 typedef void (iterate_over_regset_sections_cb)
-  (const char *sect_name, int size, const struct regset *regset,
+  (const char *sect_name, int min_size, int size, const struct regset *regset,
    const char *human_name, void *cb_data);
 
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 447394381e..a8af0ed3d2 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1321,7 +1321,7 @@ typedef int (iterate_over_objfiles_in_search_order_cb_ftype)
    CB_DATA should have been passed unchanged through the iterator.  */
 
 typedef void (iterate_over_regset_sections_cb)
-  (const char *sect_name, int size, const struct regset *regset,
+  (const char *sect_name, int min_size, int size, const struct regset *regset,
    const char *human_name, void *cb_data);
 EOF
 
diff --git a/gdb/hppa-linux-tdep.c b/gdb/hppa-linux-tdep.c
index 080fc0449b..c4c169fe98 100644
--- a/gdb/hppa-linux-tdep.c
+++ b/gdb/hppa-linux-tdep.c
@@ -479,9 +479,9 @@ hppa_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", 80 * tdep->bytes_per_address, &hppa_linux_regset,
-      NULL, cb_data);
-  cb (".reg2", 64 * 4, &hppa_linux_fpregset, NULL, cb_data);
+  cb (".reg", 80 * tdep->bytes_per_address, 80 * tdep->bytes_per_address,
+      &hppa_linux_regset, NULL, cb_data);
+  cb (".reg2", 64 * 4, 64 * 4, &hppa_linux_fpregset, NULL, cb_data);
 }
 
 static void
diff --git a/gdb/hppa-nbsd-tdep.c b/gdb/hppa-nbsd-tdep.c
index f9932b5b80..49bf17b1cc 100644
--- a/gdb/hppa-nbsd-tdep.c
+++ b/gdb/hppa-nbsd-tdep.c
@@ -190,7 +190,8 @@ hppanbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				       void *cb_data,
 				       const struct regcache *regcache)
 {
-  cb (".reg", HPPANBSD_SIZEOF_GREGS, &hppanbsd_gregset, NULL, cb_data);
+  cb (".reg", HPPANBSD_SIZEOF_GREGS, HPPANBSD_SIZEOF_GREGS, &hppanbsd_gregset,
+      NULL, cb_data);
 }
 \f
 static void
diff --git a/gdb/hppa-obsd-tdep.c b/gdb/hppa-obsd-tdep.c
index 403e9ddc82..97d5a2bf01 100644
--- a/gdb/hppa-obsd-tdep.c
+++ b/gdb/hppa-obsd-tdep.c
@@ -149,8 +149,10 @@ hppaobsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				       void *cb_data,
 				       const struct regcache *regcache)
 {
-  cb (".reg", HPPAOBSD_SIZEOF_GREGS, &hppaobsd_gregset, NULL, cb_data);
-  cb (".reg2", HPPAOBSD_SIZEOF_FPREGS, &hppaobsd_fpregset, NULL, cb_data);
+  cb (".reg", HPPAOBSD_SIZEOF_GREGS, HPPAOBSD_SIZEOF_GREGS, &hppaobsd_gregset,
+      NULL, cb_data);
+  cb (".reg2", HPPAOBSD_SIZEOF_FPREGS, HPPAOBSD_SIZEOF_FPREGS,
+      &hppaobsd_fpregset, NULL, cb_data);
 }
 \f
 
diff --git a/gdb/i386-fbsd-tdep.c b/gdb/i386-fbsd-tdep.c
index ebc75a8d58..22c8d4506b 100644
--- a/gdb/i386-fbsd-tdep.c
+++ b/gdb/i386-fbsd-tdep.c
@@ -309,12 +309,15 @@ i386fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", tdep->sizeof_gregset, &i386_gregset, NULL, cb_data);
-  cb (".reg2", tdep->sizeof_fpregset, &i386_fpregset, NULL, cb_data);
+  cb (".reg", tdep->sizeof_gregset, tdep->sizeof_gregset, &i386_gregset, NULL,
+      cb_data);
+  cb (".reg2", tdep->sizeof_fpregset, tdep->sizeof_fpregset, &i386_fpregset,
+      NULL, cb_data);
 
   if (tdep->xcr0 & X86_XSTATE_AVX)
-    cb (".reg-xstate", X86_XSTATE_SIZE(tdep->xcr0),
-	&i386fbsd_xstateregset, "XSAVE extended state", cb_data);
+    cb (".reg-xstate", X86_XSTATE_SIZE (tdep->xcr0),
+	X86_XSTATE_SIZE (tdep->xcr0), &i386fbsd_xstateregset,
+	"XSAVE extended state", cb_data);
 }
 
 static void
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index f5b039b794..802c41fe72 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -764,16 +764,17 @@ i386_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", 68, &i386_gregset, NULL, cb_data);
+  cb (".reg", 68, 68, &i386_gregset, NULL, cb_data);
 
   if (tdep->xcr0 & X86_XSTATE_AVX)
     cb (".reg-xstate", X86_XSTATE_SIZE (tdep->xcr0),
-	&i386_linux_xstateregset, "XSAVE extended state", cb_data);
+	X86_XSTATE_SIZE (tdep->xcr0), &i386_linux_xstateregset,
+	"XSAVE extended state", cb_data);
   else if (tdep->xcr0 & X86_XSTATE_SSE)
-    cb (".reg-xfp", 512, &i386_fpregset, "extended floating-point",
+    cb (".reg-xfp", 512, 512, &i386_fpregset, "extended floating-point",
 	cb_data);
   else
-    cb (".reg2", 108, &i386_fpregset, NULL, cb_data);
+    cb (".reg2", 108, 108, &i386_fpregset, NULL, cb_data);
 }
 
 /* Linux kernel shows PC value after the 'int $0x80' instruction even if
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index b1d502f482..a6994aaf12 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -3910,9 +3910,11 @@ i386_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", tdep->sizeof_gregset, &i386_gregset, NULL, cb_data);
+  cb (".reg", tdep->sizeof_gregset, tdep->sizeof_gregset, &i386_gregset, NULL,
+      cb_data);
   if (tdep->sizeof_fpregset)
-    cb (".reg2", tdep->sizeof_fpregset, tdep->fpregset, NULL, cb_data);
+    cb (".reg2", tdep->sizeof_fpregset, tdep->sizeof_fpregset, tdep->fpregset,
+	NULL, cb_data);
 }
 \f
 
diff --git a/gdb/ia64-linux-tdep.c b/gdb/ia64-linux-tdep.c
index 19d0cf2deb..0054f5a514 100644
--- a/gdb/ia64-linux-tdep.c
+++ b/gdb/ia64-linux-tdep.c
@@ -207,8 +207,10 @@ ia64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					 void *cb_data,
 					 const struct regcache *regcache)
 {
-  cb (".reg", IA64_LINUX_GREGS_SIZE, &ia64_linux_gregset, NULL, cb_data);
-  cb (".reg2", IA64_LINUX_FPREGS_SIZE, &ia64_linux_fpregset, NULL, cb_data);
+  cb (".reg", IA64_LINUX_GREGS_SIZE, IA64_LINUX_GREGS_SIZE, &ia64_linux_gregset,
+      NULL, cb_data);
+  cb (".reg2", IA64_LINUX_FPREGS_SIZE, IA64_LINUX_FPREGS_SIZE,
+      &ia64_linux_fpregset, NULL, cb_data);
 }
 
 static void
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 3cfa2a5aa4..765c3fe149 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1579,7 +1579,7 @@ struct linux_collect_regset_section_cb_data
    regset in the corefile note section.  */
 
 static void
-linux_collect_regset_section_cb (const char *sect_name, int size,
+linux_collect_regset_section_cb (const char *sect_name, int min_size, int size,
 				 const struct regset *regset,
 				 const char *human_name, void *cb_data)
 {
diff --git a/gdb/m32r-linux-tdep.c b/gdb/m32r-linux-tdep.c
index be35b699c2..aa441592a6 100644
--- a/gdb/m32r-linux-tdep.c
+++ b/gdb/m32r-linux-tdep.c
@@ -440,7 +440,8 @@ m32r_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					 void *cb_data,
 					 const struct regcache *regcache)
 {
-  cb (".reg", M32R_LINUX_GREGS_SIZE, &m32r_linux_gregset, NULL, cb_data);
+  cb (".reg", M32R_LINUX_GREGS_SIZE, M32R_LINUX_GREGS_SIZE, &m32r_linux_gregset,
+      NULL, cb_data);
 }
 
 static void
diff --git a/gdb/m68k-bsd-tdep.c b/gdb/m68k-bsd-tdep.c
index 2f53870b98..0991407557 100644
--- a/gdb/m68k-bsd-tdep.c
+++ b/gdb/m68k-bsd-tdep.c
@@ -123,8 +123,10 @@ m68kbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				      void *cb_data,
 				      const struct regcache *regcache)
 {
-  cb (".reg", M68KBSD_SIZEOF_GREGS, &m68kbsd_gregset, NULL, cb_data);
-  cb (".reg2", M68KBSD_SIZEOF_FPREGS, &m68kbsd_fpregset, NULL, cb_data);
+  cb (".reg", M68KBSD_SIZEOF_GREGS, M68KBSD_SIZEOF_GREGS, &m68kbsd_gregset,
+      NULL, cb_data);
+  cb (".reg2", M68KBSD_SIZEOF_FPREGS, M68KBSD_SIZEOF_FPREGS, &m68kbsd_fpregset,
+      NULL, cb_data);
 }
 \f
 
diff --git a/gdb/m68k-linux-tdep.c b/gdb/m68k-linux-tdep.c
index 11cde0b09f..8ebf7913e5 100644
--- a/gdb/m68k-linux-tdep.c
+++ b/gdb/m68k-linux-tdep.c
@@ -374,8 +374,10 @@ m68k_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					 void *cb_data,
 					 const struct regcache *regcache)
 {
-  cb (".reg", M68K_LINUX_GREGS_SIZE, &m68k_linux_gregset, NULL, cb_data);
-  cb (".reg2", M68K_LINUX_FPREGS_SIZE, &m68k_linux_fpregset, NULL, cb_data);
+  cb (".reg", M68K_LINUX_GREGS_SIZE, M68K_LINUX_GREGS_SIZE, &m68k_linux_gregset,
+      NULL, cb_data);
+  cb (".reg2", M68K_LINUX_FPREGS_SIZE, M68K_LINUX_FPREGS_SIZE,
+      &m68k_linux_fpregset, NULL, cb_data);
 }
 
 static void
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index c3075b23fc..e341088702 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -245,10 +245,10 @@ mips_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   size_t regsize = mips_abi_regsize (gdbarch);
 
-  cb (".reg", MIPS_FBSD_NUM_GREGS * regsize, &mips_fbsd_gregset,
-      NULL, cb_data);
-  cb (".reg2", MIPS_FBSD_NUM_FPREGS * regsize, &mips_fbsd_fpregset,
-      NULL, cb_data);
+  cb (".reg", MIPS_FBSD_NUM_GREGS * regsize, MIPS_FBSD_NUM_GREGS * regsize,
+      &mips_fbsd_gregset, NULL, cb_data);
+  cb (".reg2", MIPS_FBSD_NUM_FPREGS * regsize, MIPS_FBSD_NUM_FPREGS * regsize,
+      &mips_fbsd_fpregset, NULL, cb_data);
 }
 
 /* Signal trampoline support.  */
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 71183d7325..44b2b2e29b 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -537,16 +537,18 @@ mips_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   if (register_size (gdbarch, MIPS_ZERO_REGNUM) == 4)
     {
-      cb (".reg", sizeof (mips_elf_gregset_t), &mips_linux_gregset,
-	  NULL, cb_data);
-      cb (".reg2", sizeof (mips64_elf_fpregset_t), &mips64_linux_fpregset,
+      cb (".reg", sizeof (mips_elf_gregset_t), sizeof (mips_elf_gregset_t),
+	  &mips_linux_gregset, NULL, cb_data);
+      cb (".reg2", sizeof (mips64_elf_fpregset_t),
+	  sizeof (mips64_elf_fpregset_t), &mips64_linux_fpregset,
 	  NULL, cb_data);
     }
   else
     {
-      cb (".reg", sizeof (mips64_elf_gregset_t), &mips64_linux_gregset,
-	  NULL, cb_data);
-      cb (".reg2", sizeof (mips64_elf_fpregset_t), &mips64_linux_fpregset,
+      cb (".reg", sizeof (mips64_elf_gregset_t), sizeof (mips64_elf_gregset_t),
+	  &mips64_linux_gregset, NULL, cb_data);
+      cb (".reg2", sizeof (mips64_elf_fpregset_t),
+	  sizeof (mips64_elf_fpregset_t), &mips64_linux_fpregset,
 	  NULL, cb_data);
     }
 }
diff --git a/gdb/mips-nbsd-tdep.c b/gdb/mips-nbsd-tdep.c
index 5f4db87498..5005a176b5 100644
--- a/gdb/mips-nbsd-tdep.c
+++ b/gdb/mips-nbsd-tdep.c
@@ -123,10 +123,10 @@ mipsnbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   size_t regsize = mips_isa_regsize (gdbarch);
 
-  cb (".reg", MIPSNBSD_NUM_GREGS * regsize, &mipsnbsd_gregset,
-      NULL, cb_data);
-  cb (".reg2", MIPSNBSD_NUM_FPREGS * regsize, &mipsnbsd_fpregset,
-      NULL, cb_data);
+  cb (".reg", MIPSNBSD_NUM_GREGS * regsize, MIPSNBSD_NUM_GREGS * regsize,
+      &mipsnbsd_gregset, NULL, cb_data);
+  cb (".reg2", MIPSNBSD_NUM_FPREGS * regsize, MIPSNBSD_NUM_FPREGS * regsize,
+      &mipsnbsd_fpregset, NULL, cb_data);
 }
 \f
 
diff --git a/gdb/mips64-obsd-tdep.c b/gdb/mips64-obsd-tdep.c
index ab910edfc4..cbebe053f5 100644
--- a/gdb/mips64-obsd-tdep.c
+++ b/gdb/mips64-obsd-tdep.c
@@ -72,7 +72,8 @@ mips64obsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					 void *cb_data,
 					 const struct regcache *regcache)
 {
-  cb (".reg", MIPS64OBSD_NUM_REGS * 8, &mips64obsd_gregset, NULL, cb_data);
+  cb (".reg", MIPS64OBSD_NUM_REGS * 8, MIPS64OBSD_NUM_REGS * 8,
+      &mips64obsd_gregset, NULL, cb_data);
 }
 \f
 
diff --git a/gdb/mn10300-linux-tdep.c b/gdb/mn10300-linux-tdep.c
index 070fb205fc..76e9c6f15f 100644
--- a/gdb/mn10300-linux-tdep.c
+++ b/gdb/mn10300-linux-tdep.c
@@ -455,10 +455,10 @@ am33_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				   void *cb_data,
 				   const struct regcache *regcache)
 {
-  cb (".reg", sizeof (mn10300_elf_gregset_t), &am33_gregset,
-      NULL, cb_data);
-  cb (".reg2", sizeof(mn10300_elf_fpregset_t), &am33_fpregset,
-      NULL, cb_data);
+  cb (".reg", sizeof (mn10300_elf_gregset_t), sizeof (mn10300_elf_gregset_t),
+      &am33_gregset, NULL, cb_data);
+  cb (".reg2", sizeof (mn10300_elf_fpregset_t), sizeof (mn10300_elf_fpregset_t),
+      &am33_fpregset, NULL, cb_data);
 }
 \f
 static void
diff --git a/gdb/nios2-linux-tdep.c b/gdb/nios2-linux-tdep.c
index e7f4ecf16e..d7e97abf09 100644
--- a/gdb/nios2-linux-tdep.c
+++ b/gdb/nios2-linux-tdep.c
@@ -106,7 +106,8 @@ nios2_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				    void *cb_data,
 				    const struct regcache *regcache)
 {
-  cb (".reg", NIOS2_GREGS_SIZE, &nios2_core_regset, NULL, cb_data);
+  cb (".reg", NIOS2_GREGS_SIZE, NIOS2_GREGS_SIZE, &nios2_core_regset, NULL,
+      cb_data);
 }
 
 /* Initialize a trad-frame cache corresponding to the tramp-frame.
diff --git a/gdb/ppc-fbsd-tdep.c b/gdb/ppc-fbsd-tdep.c
index 495ccca8f1..e709c36d58 100644
--- a/gdb/ppc-fbsd-tdep.c
+++ b/gdb/ppc-fbsd-tdep.c
@@ -128,10 +128,10 @@ ppcfbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   if (tdep->wordsize == 4)
-    cb (".reg", 148, &ppc32_fbsd_gregset, NULL, cb_data);
+    cb (".reg", 148, 148, &ppc32_fbsd_gregset, NULL, cb_data);
   else
-    cb (".reg", 296, &ppc64_fbsd_gregset, NULL, cb_data);
-  cb (".reg2", 264, &ppc32_fbsd_fpregset, NULL, cb_data);
+    cb (".reg", 296, 296, &ppc64_fbsd_gregset, NULL, cb_data);
+  cb (".reg2", 264, 264, &ppc32_fbsd_fpregset, NULL, cb_data);
 }
 
 /* Default page size.  */
diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index 33a0b5a83b..1c0fb7a741 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -605,21 +605,21 @@ ppc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
   int have_vsx = tdep->ppc_vsr0_upper_regnum != -1;
 
   if (tdep->wordsize == 4)
-    cb (".reg", 48 * 4, &ppc32_linux_gregset, NULL, cb_data);
+    cb (".reg", 48 * 4, 48 * 4, &ppc32_linux_gregset, NULL, cb_data);
   else
-    cb (".reg", 48 * 8, &ppc64_linux_gregset, NULL, cb_data);
+    cb (".reg", 48 * 8, 48 * 8, &ppc64_linux_gregset, NULL, cb_data);
 
-  cb (".reg2", 264, &ppc32_linux_fpregset, NULL, cb_data);
+  cb (".reg2", 264, 264, &ppc32_linux_fpregset, NULL, cb_data);
 
   if (have_altivec)
     {
       const struct regset *vrregset = ppc_linux_vrregset (gdbarch);
-      cb (".reg-ppc-vmx", PPC_LINUX_SIZEOF_VRREGSET, vrregset,
-	  "ppc Altivec", cb_data);
+      cb (".reg-ppc-vmx", PPC_LINUX_SIZEOF_VRREGSET, PPC_LINUX_SIZEOF_VRREGSET,
+	  vrregset, "ppc Altivec", cb_data);
     }
 
   if (have_vsx)
-    cb (".reg-ppc-vsx", PPC_LINUX_SIZEOF_VSXREGSET,
+    cb (".reg-ppc-vsx", PPC_LINUX_SIZEOF_VSXREGSET, PPC_LINUX_SIZEOF_VSXREGSET,
 	&ppc32_linux_vsxregset, "POWER7 VSX", cb_data);
 }
 
diff --git a/gdb/ppc-nbsd-tdep.c b/gdb/ppc-nbsd-tdep.c
index c86aeb8a29..053ec49e6e 100644
--- a/gdb/ppc-nbsd-tdep.c
+++ b/gdb/ppc-nbsd-tdep.c
@@ -59,8 +59,8 @@ ppcnbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				      void *cb_data,
 				      const struct regcache *regcache)
 {
-  cb (".reg", 148, &ppcnbsd_gregset, NULL, cb_data);
-  cb (".reg2", 264, &ppcnbsd_fpregset, NULL, cb_data);
+  cb (".reg", 148, 148, &ppcnbsd_gregset, NULL, cb_data);
+  cb (".reg2", 264, 264, &ppcnbsd_fpregset, NULL, cb_data);
 }
 \f
 
diff --git a/gdb/ppc-obsd-tdep.c b/gdb/ppc-obsd-tdep.c
index 838783111a..c478324d38 100644
--- a/gdb/ppc-obsd-tdep.c
+++ b/gdb/ppc-obsd-tdep.c
@@ -88,7 +88,7 @@ ppcobsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				      void *cb_data,
 				      const struct regcache *regcache)
 {
-  cb (".reg", 412, &ppcobsd_gregset, NULL, cb_data);
+  cb (".reg", 412, 412, &ppcobsd_gregset, NULL, cb_data);
 }
 \f
 
diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
index 8e69c465fa..50a146a4f0 100644
--- a/gdb/rs6000-aix-tdep.c
+++ b/gdb/rs6000-aix-tdep.c
@@ -146,9 +146,9 @@ rs6000_aix_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					 const struct regcache *regcache)
 {
   if (gdbarch_tdep (gdbarch)->wordsize == 4)
-    cb (".reg", 592, &rs6000_aix32_regset, NULL, cb_data);
+    cb (".reg", 592, 592, &rs6000_aix32_regset, NULL, cb_data);
   else
-    cb (".reg", 576, &rs6000_aix64_regset, NULL, cb_data);
+    cb (".reg", 576, 576, &rs6000_aix64_regset, NULL, cb_data);
 }
 
 
diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 4561559a85..ee8fb01109 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -273,21 +273,22 @@ s390_iterate_over_regset_sections (struct gdbarch *gdbarch,
   const int gregset_size = (tdep->abi == ABI_LINUX_S390 ?
 			    s390_sizeof_gregset : s390x_sizeof_gregset);
 
-  cb (".reg", gregset_size, &s390_gregset, NULL, cb_data);
-  cb (".reg2", s390_sizeof_fpregset, &s390_fpregset, NULL, cb_data);
+  cb (".reg", gregset_size, gregset_size, &s390_gregset, NULL, cb_data);
+  cb (".reg2", s390_sizeof_fpregset, s390_sizeof_fpregset, &s390_fpregset, NULL,
+      cb_data);
 
   if (tdep->abi == ABI_LINUX_S390 && tdep->gpr_full_regnum != -1)
-    cb (".reg-s390-high-gprs", 16 * 4, &s390_upper_regset,
+    cb (".reg-s390-high-gprs", 16 * 4, 16 * 4, &s390_upper_regset,
 	"s390 GPR upper halves", cb_data);
 
   if (tdep->have_linux_v1)
-    cb (".reg-s390-last-break", 8,
+    cb (".reg-s390-last-break", 8, 8,
 	(gdbarch_ptr_bit (gdbarch) == 32
 	 ? &s390_last_break_regset : &s390x_last_break_regset),
 	"s390 last-break address", cb_data);
 
   if (tdep->have_linux_v2)
-    cb (".reg-s390-system-call", 4, &s390_system_call_regset,
+    cb (".reg-s390-system-call", 4, 4, &s390_system_call_regset,
 	"s390 system-call", cb_data);
 
   /* If regcache is set, we are in "write" (gcore) mode.  In this
@@ -297,14 +298,14 @@ s390_iterate_over_regset_sections (struct gdbarch *gdbarch,
       && (regcache == NULL
 	  || (REG_VALID
 	      == regcache->get_register_status (S390_TDB_DWORD0_REGNUM))))
-    cb (".reg-s390-tdb", s390_sizeof_tdbregset, &s390_tdb_regset,
-	"s390 TDB", cb_data);
+    cb (".reg-s390-tdb", s390_sizeof_tdbregset, s390_sizeof_tdbregset,
+	&s390_tdb_regset, "s390 TDB", cb_data);
 
   if (tdep->v0_full_regnum != -1)
     {
-      cb (".reg-s390-vxrs-low", 16 * 8, &s390_vxrs_low_regset,
+      cb (".reg-s390-vxrs-low", 16 * 8, 16 * 8, &s390_vxrs_low_regset,
 	  "s390 vector registers 0-15 lower half", cb_data);
-      cb (".reg-s390-vxrs-high", 16 * 16, &s390_vxrs_high_regset,
+      cb (".reg-s390-vxrs-high", 16 * 16, 16 * 16, &s390_vxrs_high_regset,
 	  "s390 vector registers 16-31", cb_data);
     }
 
@@ -314,12 +315,12 @@ s390_iterate_over_regset_sections (struct gdbarch *gdbarch,
     {
       if (regcache == NULL
 	  || REG_VALID == regcache->get_register_status (S390_GSD_REGNUM))
-	cb (".reg-s390-gs-cb", 4 * 8, &s390_gs_regset,
+	cb (".reg-s390-gs-cb", 4 * 8, 4 * 8, &s390_gs_regset,
 	    "s390 guarded-storage registers", cb_data);
 
       if (regcache == NULL
 	  || REG_VALID == regcache->get_register_status (S390_BC_GSD_REGNUM))
-	cb (".reg-s390-gs-bc", 4 * 8, &s390_gsbc_regset,
+	cb (".reg-s390-gs-bc", 4 * 8, 4 * 8, &s390_gsbc_regset,
 	    "s390 guarded-storage broadcast control", cb_data);
     }
 }
diff --git a/gdb/score-tdep.c b/gdb/score-tdep.c
index 16bf00ea02..b2887c5eae 100644
--- a/gdb/score-tdep.c
+++ b/gdb/score-tdep.c
@@ -1447,8 +1447,8 @@ score7_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					   void *cb_data,
 					   const struct regcache *regcache)
 {
-  cb (".reg", SCORE7_LINUX_SIZEOF_GREGSET, &score7_linux_gregset,
-      NULL, cb_data);
+  cb (".reg", SCORE7_LINUX_SIZEOF_GREGSET, SCORE7_LINUX_SIZEOF_GREGSET,
+      &score7_linux_gregset, NULL, cb_data);
 }
 
 static struct gdbarch *
diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
index d1a143414d..fe64cf979a 100644
--- a/gdb/sh-tdep.c
+++ b/gdb/sh-tdep.c
@@ -2232,10 +2232,12 @@ sh_iterate_over_regset_sections (struct gdbarch *gdbarch,
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   if (tdep->core_gregmap != NULL)
-    cb (".reg", tdep->sizeof_gregset, &sh_corefile_gregset, NULL, cb_data);
+    cb (".reg", tdep->sizeof_gregset, tdep->sizeof_gregset,
+	&sh_corefile_gregset, NULL, cb_data);
 
   if (tdep->core_fpregmap != NULL)
-    cb (".reg2", tdep->sizeof_fpregset, &sh_corefile_fpregset, NULL, cb_data);
+    cb (".reg2", tdep->sizeof_fpregset, tdep->sizeof_fpregset,
+	&sh_corefile_fpregset, NULL, cb_data);
 }
 
 /* This is the implementation of gdbarch method
diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c
index c638a6d20c..7a50a8d4a9 100644
--- a/gdb/sparc-tdep.c
+++ b/gdb/sparc-tdep.c
@@ -1777,8 +1777,10 @@ sparc_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  cb (".reg", tdep->sizeof_gregset, tdep->gregset, NULL, cb_data);
-  cb (".reg2", tdep->sizeof_fpregset, tdep->fpregset, NULL, cb_data);
+  cb (".reg", tdep->sizeof_gregset, tdep->sizeof_gregset, tdep->gregset, NULL,
+      cb_data);
+  cb (".reg2", tdep->sizeof_fpregset, tdep->sizeof_fpregset, tdep->fpregset,
+      NULL, cb_data);
 }
 \f
 
diff --git a/gdb/tilegx-linux-tdep.c b/gdb/tilegx-linux-tdep.c
index d5280f3e17..c44bbd15f3 100644
--- a/gdb/tilegx-linux-tdep.c
+++ b/gdb/tilegx-linux-tdep.c
@@ -99,8 +99,8 @@ tilegx_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				     void *cb_data,
 				     const struct regcache *regcache)
 {
-  cb (".reg", TILEGX_LINUX_SIZEOF_GREGSET, &tilegx_linux_regset,
-      NULL, cb_data);
+  cb (".reg", TILEGX_LINUX_SIZEOF_GREGSET, TILEGX_LINUX_SIZEOF_GREGSET,
+      &tilegx_linux_regset, NULL, cb_data);
 }
 
 /* OS specific initialization of gdbarch.  */
diff --git a/gdb/vax-tdep.c b/gdb/vax-tdep.c
index d07a477907..21f2066da2 100644
--- a/gdb/vax-tdep.c
+++ b/gdb/vax-tdep.c
@@ -96,7 +96,7 @@ vax_iterate_over_regset_sections (struct gdbarch *gdbarch,
 				  void *cb_data,
 				  const struct regcache *regcache)
 {
-  cb (".reg", VAX_NUM_REGS * 4, &vax_gregset, NULL, cb_data);
+  cb (".reg", VAX_NUM_REGS * 4, VAX_NUM_REGS * 4, &vax_gregset, NULL, cb_data);
 }
 \f
 /* The VAX UNIX calling convention uses R1 to pass a structure return
diff --git a/gdb/xtensa-tdep.c b/gdb/xtensa-tdep.c
index 60e34c3075..7dc3660210 100644
--- a/gdb/xtensa-tdep.c
+++ b/gdb/xtensa-tdep.c
@@ -903,8 +903,8 @@ xtensa_iterate_over_regset_sections (struct gdbarch *gdbarch,
 {
   DEBUGTRACE ("xtensa_iterate_over_regset_sections\n");
 
-  cb (".reg", sizeof (xtensa_elf_gregset_t), &xtensa_gregset,
-      NULL, cb_data);
+  cb (".reg", sizeof (xtensa_elf_gregset_t), sizeof (xtensa_elf_gregset_t),
+      &xtensa_gregset, NULL, cb_data);
 }
 
 
-- 
2.15.2 (Apple Git-101.1)

[PATCH v2 0/3] Core file support for Aarch64 SVE

[Alan Hayward]

Support both the reading and writing of core files on aarch64 SVE.

SVE core files are doumented here:
https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.txt
* A NT_ARM_SVE note will be added to each coredump for each thread of the
  dumped process.  The contents will be equivalent to the data that would have
  been read if a PTRACE_GETREGSET of NT_ARM_SVE were executed for each thread
  when the coredump was generated.

The first patch adds support allowing writing of core files for targets with
variable length core files. Currently all targets with variable length core
files do not allow the creation of core files.

The second patch detects SVE state in a core file.
The final patch adds the supply/collect functions for core file parsing.

Checked with make check on x86 (all targets) and aarch64.
Generated core files on SVE emulator both from gdb "generate-core-file"
command and a segfault. Loaded these back into gdb.


Alan Hayward (3):
  Add min size to regset section iterations
  Detect SVE when reading aarch64 core files
  Parse SVE registers in aarch64 core file reading/writing

 gdb/aarch64-fbsd-tdep.c  |   8 +-
 gdb/aarch64-linux-tdep.c | 190 +++++++++++++++++++++++++++++++++++++++++++++--
 gdb/alpha-linux-tdep.c   |   4 +-
 gdb/alpha-nbsd-tdep.c    |   6 +-
 gdb/amd64-fbsd-tdep.c    |   8 +-
 gdb/amd64-linux-tdep.c   |   6 +-
 gdb/arm-bsd-tdep.c       |   6 +-
 gdb/arm-fbsd-tdep.c      |   7 +-
 gdb/arm-linux-tdep.c     |  11 +--
 gdb/corelow.c            |  27 ++++---
 gdb/fbsd-tdep.c          |   2 +-
 gdb/frv-linux-tdep.c     |   8 +-
 gdb/gdbarch.h            |   2 +-
 gdb/gdbarch.sh           |   2 +-
 gdb/hppa-linux-tdep.c    |   6 +-
 gdb/hppa-nbsd-tdep.c     |   3 +-
 gdb/hppa-obsd-tdep.c     |   6 +-
 gdb/i386-fbsd-tdep.c     |  11 ++-
 gdb/i386-linux-tdep.c    |   9 ++-
 gdb/i386-tdep.c          |   6 +-
 gdb/ia64-linux-tdep.c    |   6 +-
 gdb/linux-tdep.c         |   2 +-
 gdb/m32r-linux-tdep.c    |   3 +-
 gdb/m68k-bsd-tdep.c      |   6 +-
 gdb/m68k-linux-tdep.c    |   6 +-
 gdb/mips-fbsd-tdep.c     |   8 +-
 gdb/mips-linux-tdep.c    |  14 ++--
 gdb/mips-nbsd-tdep.c     |   8 +-
 gdb/mips64-obsd-tdep.c   |   3 +-
 gdb/mn10300-linux-tdep.c |   8 +-
 gdb/nios2-linux-tdep.c   |   3 +-
 gdb/ppc-fbsd-tdep.c      |   6 +-
 gdb/ppc-linux-tdep.c     |  12 +--
 gdb/ppc-nbsd-tdep.c      |   4 +-
 gdb/ppc-obsd-tdep.c      |   2 +-
 gdb/regcache.h           |   8 ++
 gdb/rs6000-aix-tdep.c    |   4 +-
 gdb/s390-linux-tdep.c    |  23 +++---
 gdb/score-tdep.c         |   4 +-
 gdb/sh-tdep.c            |   6 +-
 gdb/sparc-tdep.c         |   6 +-
 gdb/tilegx-linux-tdep.c  |   4 +-
 gdb/vax-tdep.c           |   2 +-
 gdb/xtensa-tdep.c        |   4 +-
 44 files changed, 350 insertions(+), 130 deletions(-)

-- 
2.15.2 (Apple Git-101.1)

[PING][PATCH v2 0/3] Core file support for Aarch64 SVE

[Alan Hayward]

Ping.

> On 30 Jul 2018, at 10:25, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> Support both the reading and writing of core files on aarch64 SVE.
> 
> SVE core files are doumented here:
> https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.txt
> * A NT_ARM_SVE note will be added to each coredump for each thread of the
>  dumped process.  The contents will be equivalent to the data that would have
>  been read if a PTRACE_GETREGSET of NT_ARM_SVE were executed for each thread
>  when the coredump was generated.
> 
> The first patch adds support allowing writing of core files for targets with
> variable length core files. Currently all targets with variable length core
> files do not allow the creation of core files.
> 
> The second patch detects SVE state in a core file.
> The final patch adds the supply/collect functions for core file parsing.
> 
> Checked with make check on x86 (all targets) and aarch64.
> Generated core files on SVE emulator both from gdb "generate-core-file"
> command and a segfault. Loaded these back into gdb.
> 
> 
> Alan Hayward (3):
>  Add min size to regset section iterations
>  Detect SVE when reading aarch64 core files
>  Parse SVE registers in aarch64 core file reading/writing
> 
> gdb/aarch64-fbsd-tdep.c  |   8 +-
> gdb/aarch64-linux-tdep.c | 190 +++++++++++++++++++++++++++++++++++++++++++++--
> gdb/alpha-linux-tdep.c   |   4 +-
> gdb/alpha-nbsd-tdep.c    |   6 +-
> gdb/amd64-fbsd-tdep.c    |   8 +-
> gdb/amd64-linux-tdep.c   |   6 +-
> gdb/arm-bsd-tdep.c       |   6 +-
> gdb/arm-fbsd-tdep.c      |   7 +-
> gdb/arm-linux-tdep.c     |  11 +--
> gdb/corelow.c            |  27 ++++---
> gdb/fbsd-tdep.c          |   2 +-
> gdb/frv-linux-tdep.c     |   8 +-
> gdb/gdbarch.h            |   2 +-
> gdb/gdbarch.sh           |   2 +-
> gdb/hppa-linux-tdep.c    |   6 +-
> gdb/hppa-nbsd-tdep.c     |   3 +-
> gdb/hppa-obsd-tdep.c     |   6 +-
> gdb/i386-fbsd-tdep.c     |  11 ++-
> gdb/i386-linux-tdep.c    |   9 ++-
> gdb/i386-tdep.c          |   6 +-
> gdb/ia64-linux-tdep.c    |   6 +-
> gdb/linux-tdep.c         |   2 +-
> gdb/m32r-linux-tdep.c    |   3 +-
> gdb/m68k-bsd-tdep.c      |   6 +-
> gdb/m68k-linux-tdep.c    |   6 +-
> gdb/mips-fbsd-tdep.c     |   8 +-
> gdb/mips-linux-tdep.c    |  14 ++--
> gdb/mips-nbsd-tdep.c     |   8 +-
> gdb/mips64-obsd-tdep.c   |   3 +-
> gdb/mn10300-linux-tdep.c |   8 +-
> gdb/nios2-linux-tdep.c   |   3 +-
> gdb/ppc-fbsd-tdep.c      |   6 +-
> gdb/ppc-linux-tdep.c     |  12 +--
> gdb/ppc-nbsd-tdep.c      |   4 +-
> gdb/ppc-obsd-tdep.c      |   2 +-
> gdb/regcache.h           |   8 ++
> gdb/rs6000-aix-tdep.c    |   4 +-
> gdb/s390-linux-tdep.c    |  23 +++---
> gdb/score-tdep.c         |   4 +-
> gdb/sh-tdep.c            |   6 +-
> gdb/sparc-tdep.c         |   6 +-
> gdb/tilegx-linux-tdep.c  |   4 +-
> gdb/vax-tdep.c           |   2 +-
> gdb/xtensa-tdep.c        |   4 +-
> 44 files changed, 350 insertions(+), 130 deletions(-)
> 
> -- 
> 2.15.2 (Apple Git-101.1)
> 

Re: [PATCH v2 1/3] Add min size to regset section iterations

[Simon Marchi]

On 2018-07-30 05:25 AM, Alan Hayward wrote:
> When using the regset section iteration functions, the size parameter is used
> in different ways.
> 
> With collect, size is used to create the buffer in which to write the regset.
> (see linux-tdep.c::linux_collect_regset_section_cb).
> 
> With supply, size is used to confirm the existing regset is the correct size.
> If REGSET_VARIABLE_SIZE is set then the regset can be bigger than size.
> Effectively, size is the minimum possible size of the regset.
> (see corelow.c::get_core_register_section).
> 
> There are currently no targets with both REGSET_VARIABLE_SIZE and a collect
> function.
> 
> To allow support of collects for REGSET_VARIABLE_SIZE we need two sizes.
> Min_size is the minimum allowed size for the regset, and size is used as the
> size to use when creating new regsets. For all targets that are not
> REGSET_VARIABLE_SIZE then these two sizes are equal.

Hi Alan,

I am still a bit confused by how these two sizes are used.  Please document
carefully what they each mean, for both the collect and supply case.

Part of my confusion comes the fact that for Aarch64 SVE, we seem to know
in advance the exact size the SVE register section should have, based on
vq (which we read in aarch64_linux_core_read_vq).  In this case, the single
size parameter would be enough for collecting and supplying, since it's the
exact size (not a minimum size).

> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 059ce2f6eb..32a054ee3e 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -107,6 +107,7 @@ public:
>  				  const struct regset *regset,
>  				  const char *name,
>  				  int min_size,
> +				  int size,
>  				  int which,
>  				  const char *human_name,
>  				  bool required);
> @@ -570,12 +571,13 @@ core_target::get_core_register_section (struct regcache *regcache,
>  					const struct regset *regset,
>  					const char *name,
>  					int min_size,
> +					int size,
>  					int which,
>  					const char *human_name,
>  					bool required)
>  {>    struct bfd_section *section;
> -  bfd_size_type size;
> +  bfd_size_type core_size;

I would suggest naming this "section_size", or "reg_section_size".  At first I thought it meant
the size of the whole core file.  You could push this rename as an obvious patch right now to
reduce the noise in this patch, since it's a good change on its own.

Simon

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

[Simon Marchi]

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

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

[Simon Marchi]

On 2018-07-30 05:25 AM, Alan Hayward wrote:
> sve_regmap cannot be global static as the size is dependant on the current
> vector length.
> 
> 2018-07-30  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* aarch64-linux-tdep.c (aarch64_linux_supply_sve_regset): New function.
> 	(aarch64_linux_collect_sve_regset): Likewise.
> 	(aarch64_linux_iterate_over_regset_sections): Check for SVE.
> 	* regcache.h (regcache_map_entry_size): New function.
> ---
>  gdb/aarch64-linux-tdep.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++-
>  gdb/regcache.h           |   8 ++++
>  2 files changed, 118 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index f9a95950da..bd61a2d722 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -288,6 +288,85 @@ 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)
> +{
> +  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 vg = sve_vg_from_vl (SVE_HEADER_READ (buf, 2, byte_order));
> +  uint16_t flags = SVE_HEADER_READ (buf, 4, byte_order);
> +
> +  if (regnum == -1 || regnum == AARCH64_SVE_VG_REGNUM)
> +    regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg);

I think this raw_supply is wrong.  The vg local variable is in host byte order,
but the data in the buffer passed to raw_supply should be in target.  So you
may need to do a store_unsigned_integer in a buffer and supply that.  If this
is a pattern that happens often enough, maybe it would be worth having a
raw_supply that does this conversion from integer to register buffer, a bit
like "regcache::raw_write (int regnum, T val)" does.

> +
> +  if (flags & 1)
> +    {
> +      /* 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);

Just wondering, should they be made unavailable instead of cleared?

Simon

Re: [PATCH v2 1/3] Add min size to regset section iterations

[Alan Hayward]

Thanks for the reviews.

> On 6 Aug 2018, at 19:27, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> On 2018-07-30 05:25 AM, Alan Hayward wrote:
>> When using the regset section iteration functions, the size parameter is used
>> in different ways.
>> 
>> With collect, size is used to create the buffer in which to write the regset.
>> (see linux-tdep.c::linux_collect_regset_section_cb).
>> 
>> With supply, size is used to confirm the existing regset is the correct size.
>> If REGSET_VARIABLE_SIZE is set then the regset can be bigger than size.
>> Effectively, size is the minimum possible size of the regset.
>> (see corelow.c::get_core_register_section).
>> 
>> There are currently no targets with both REGSET_VARIABLE_SIZE and a collect
>> function.
>> 
>> To allow support of collects for REGSET_VARIABLE_SIZE we need two sizes.
>> Min_size is the minimum allowed size for the regset, and size is used as the
>> size to use when creating new regsets. For all targets that are not
>> REGSET_VARIABLE_SIZE then these two sizes are equal.
> 
> Hi Alan,
> 
> I am still a bit confused by how these two sizes are used.  Please document
> carefully what they each mean, for both the collect and supply case.
> 
> Part of my confusion comes the fact that for Aarch64 SVE, we seem to know
> in advance the exact size the SVE register section should have, based on
> vq (which we read in aarch64_linux_core_read_vq).  In this case, the single
> size parameter would be enough for collecting and supplying, since it's the
> exact size (not a minimum size).
> 

It’s probably clearer if I explain the SVE specific case:

With SVE when we read the section from the core file it will give us one of two things:
1) SVE header followed by a full SVE register dump (size dependant on register size)
Or
2) SVE header followed by a neon register dump (I usually refer to this as a fpsimd dump).

The second is a shortcut used by the kernel if the process hadn't run any SVE code, and 
all the SVE state is null. This structure is significantly smaller than the SVE dump.

In the common gdb supply code, it will assert if the size of the section read from the core
file is smaller than the given size. So for SVE I need to set the size to the fpsimd size
to prevent this.
Ok, I could peak ahead into the core file to see what type of dump it is. But, at the
point of the _iterate_over_regset_sections() we don’t have any access to the core file.

In the common collect code, it uses the size to allocate memory for writing the dump into.
For this we always want to write out a full SVE dump, so need the size of the first dump.


If that makes sense now, I can rework it into the comments.

>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index 059ce2f6eb..32a054ee3e 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -107,6 +107,7 @@ public:
>> 				  const struct regset *regset,
>> 				  const char *name,
>> 				  int min_size,
>> +				  int size,
>> 				  int which,
>> 				  const char *human_name,
>> 				  bool required);
>> @@ -570,12 +571,13 @@ core_target::get_core_register_section (struct regcache *regcache,
>> 					const struct regset *regset,
>> 					const char *name,
>> 					int min_size,
>> +					int size,
>> 					int which,
>> 					const char *human_name,
>> 					bool required)
>> {>    struct bfd_section *section;
>> -  bfd_size_type size;
>> +  bfd_size_type core_size;
> 
> I would suggest naming this "section_size", or "reg_section_size".  At first I thought it meant
> the size of the whole core file.  You could push this rename as an obvious patch right now to
> reduce the noise in this patch, since it's a good change on its own.

Agreed. Will do.

> 
> Simon

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

[Alan Hayward]

> On 6 Aug 2018, at 19:29, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> On 2018-07-30 05:25 AM, Alan Hayward wrote:
>> sve_regmap cannot be global static as the size is dependant on the current
>> vector length.
>> 
>> 2018-07-30  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* aarch64-linux-tdep.c (aarch64_linux_supply_sve_regset): New function.
>> 	(aarch64_linux_collect_sve_regset): Likewise.
>> 	(aarch64_linux_iterate_over_regset_sections): Check for SVE.
>> 	* regcache.h (regcache_map_entry_size): New function.
>> ---
>> gdb/aarch64-linux-tdep.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++-
>> gdb/regcache.h           |   8 ++++
>> 2 files changed, 118 insertions(+), 2 deletions(-)
>> 
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index f9a95950da..bd61a2d722 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -288,6 +288,85 @@ 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)
>> +{
>> +  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 vg = sve_vg_from_vl (SVE_HEADER_READ (buf, 2, byte_order));
>> +  uint16_t flags = SVE_HEADER_READ (buf, 4, byte_order);
>> +
>> +  if (regnum == -1 || regnum == AARCH64_SVE_VG_REGNUM)
>> +    regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg);
> 
> I think this raw_supply is wrong.  The vg local variable is in host byte order,
> but the data in the buffer passed to raw_supply should be in target.  So you
> may need to do a store_unsigned_integer in a buffer and supply that.  If this
> is a pattern that happens often enough, maybe it would be worth having a
> raw_supply that does this conversion from integer to register buffer, a bit
> like "regcache::raw_write (int regnum, T val)" does.
> 

Yes! I’ll consider adding the function too.

>> +
>> +  if (flags & 1)
>> +    {
>> +      /* 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);
> 
> Just wondering, should they be made unavailable instead of cleared?
> 

No. The supply_regset directly afterwards will write to the V registers.
With SVE, the V registers are pseudo registers built off the Z registers.
I do the clear to ensure the non overlapping parts of the Z registers are cleared.

Re: [PATCH v2 1/3] Add min size to regset section iterations

[Simon Marchi]

On 2018-08-07 07:01, Alan Hayward wrote:
> It’s probably clearer if I explain the SVE specific case:
> 
> With SVE when we read the section from the core file it will give us
> one of two things:
> 1) SVE header followed by a full SVE register dump (size dependant on
> register size)
> Or
> 2) SVE header followed by a neon register dump (I usually refer to
> this as a fpsimd dump).
> 
> The second is a shortcut used by the kernel if the process hadn't run
> any SVE code, and
> all the SVE state is null. This structure is significantly smaller
> than the SVE dump.
> 
> In the common gdb supply code, it will assert if the size of the
> section read from the core
> file is smaller than the given size. So for SVE I need to set the size
> to the fpsimd size
> to prevent this.
> 
> Ok, I could peak ahead into the core file to see what type of dump it
> is. But, at the
> point of the _iterate_over_regset_sections() we don’t have any access
> to the core file.
> 
> In the common collect code, it uses the size to allocate memory for
> writing the dump into.
> For this we always want to write out a full SVE dump, so need the size
> of the first dump.
> 
> 
> If that makes sense now, I can rework it into the comments.

Ok, it kind of make sense but I still find the naming (min_size and 
size) confusing.  I can't quickly tell what is used when.

 From what I understand, when supplying, you only care about min_size.  
If REGSET_VARIABLE_SIZE is not set, min_size is actually not a min_size, 
but the expected size (anything else than this size will be rejected).  
When collecting, you only care about "size", to allocate the memory for 
the dump.

Therefore, I am starting to think the semantic is more straightforward 
(to me at least) if we named them supply_size and collect_size (which 
you mentioned in the original patch message).  This would make it 
somewhat clear that if you are in a supply scenario, collect_size is 
meaningless (and vice versa).  It becomes a bit simpler to explain:

- When supplying fixed-size regsets (!REGSET_VARIABLE_SIZE), supply_size 
is the exact expected size.
- When supplying variable-size regsets (REGSET_VARIABLE_SIZE) 
supply_size is actually just a minumum, because we don't know what we 
will actually find in the section yet.
- When collecting, we know the size in advance (since we know what we 
will dump), so collect_size is always the exact size that will be 
dumped.

In core_target::get_core_register_section (or maybe somewhere else), we 
can assert that

   if (!variable_size_section)
     gdb_assert (supply_size == collect_size);


On a different track, did you consider keeping a single "size" parameter 
to gdbarch_iterate_over_regset_sections, but add one to indicate whether 
the caller intends to supply or collect registers?  And then, in 
aarch64's implementation, pass different sizes in the supply/collect 
cases?  Most other arch implementations would simply ignore this 
parameter and always pass the same size, as they do today.

Simon

Re: [PATCH v2 1/3] Add min size to regset section iterations

[Alan Hayward]

> On 7 Aug 2018, at 17:05, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
> On 2018-08-07 07:01, Alan Hayward wrote:
>> It’s probably clearer if I explain the SVE specific case:
>> With SVE when we read the section from the core file it will give us
>> one of two things:
>> 1) SVE header followed by a full SVE register dump (size dependant on
>> register size)
>> Or
>> 2) SVE header followed by a neon register dump (I usually refer to
>> this as a fpsimd dump).
>> The second is a shortcut used by the kernel if the process hadn't run
>> any SVE code, and
>> all the SVE state is null. This structure is significantly smaller
>> than the SVE dump.
>> In the common gdb supply code, it will assert if the size of the
>> section read from the core
>> file is smaller than the given size. So for SVE I need to set the size
>> to the fpsimd size
>> to prevent this.
>> Ok, I could peak ahead into the core file to see what type of dump it
>> is. But, at the
>> point of the _iterate_over_regset_sections() we don’t have any access
>> to the core file.
>> In the common collect code, it uses the size to allocate memory for
>> writing the dump into.
>> For this we always want to write out a full SVE dump, so need the size
>> of the first dump.
>> If that makes sense now, I can rework it into the comments.
> 
> Ok, it kind of make sense but I still find the naming (min_size and size) confusing.  I can't quickly tell what is used when.

Agreed.

> 
> From what I understand, when supplying, you only care about min_size.  If REGSET_VARIABLE_SIZE is not set, min_size is actually not a min_size, but the expected size (anything else than this size will be rejected).  When collecting, you only care about "size", to allocate the memory for the dump.
> 

Yes.

> Therefore, I am starting to think the semantic is more straightforward (to me at least) if we named them supply_size and collect_size (which you mentioned in the original patch message).  This would make it somewhat clear that if you are in a supply scenario, collect_size is meaningless (and vice versa).  It becomes a bit simpler to explain:
> 
> - When supplying fixed-size regsets (!REGSET_VARIABLE_SIZE), supply_size is the exact expected size.
> - When supplying variable-size regsets (REGSET_VARIABLE_SIZE) supply_size is actually just a minumum, because we don't know what we will actually find in the section yet.
> - When collecting, we know the size in advance (since we know what we will dump), so collect_size is always the exact size that will be dumped.

The only point I have against this is that I had always assumed that the _iterate_over_regset_sections function was designed so that in the future extra functions could get added to regset, alongside supply and collect. If that happened, I expected the new function to use either size or min size. Calling the sizes collect_size and supply_size would confuse it. However, I probably shouldn’t worry about that, given it’s doubtful another function would get added.

Happy to do it that way.


But, how about if I moved the two sizes into regset?

struct regset
{
  const void *regmap;
  supply_regset_ftype *supply_regset;
  int supply_size;
  collect_regset_ftype *collect_regset;
  int collect_size;
  unsigned flags;
};

Reducing the callback to:
cb (".reg", &aarch64_linux_gregset, NULL, cb_data);

For most targets the size will be fixed, so the regset structures can stay global.

But I’d have to be careful - for example s390_iterate_over_regset_sections sets size based on the current abi - instead I’d create both s390_gregset and s390x_gregset.

This is why I avoided doing it in the original patch :)

I could update using your suggestion, then maybe do a follow on patch with the above?


> 
> In core_target::get_core_register_section (or maybe somewhere else), we can assert that
> 
>  if (!variable_size_section)
>    gdb_assert (supply_size == collect_size);
> 

Will do.


> 
> On a different track, did you consider keeping a single "size" parameter to gdbarch_iterate_over_regset_sections, but add one to indicate whether the caller intends to supply or collect registers?  And then, in aarch64's implementation, pass different sizes in the supply/collect cases?  Most other arch implementations would simply ignore this parameter and always pass the same size, as they do today.

If it’s going to indicate whether to use supply or collect, then it would seem odd to pass back a structure with both collect and supply functions in it, when you know which one isn’t getting used.

If going down that route, I’d probably split _iterate_over_regset_sections into two functions, one for collect and one for supply. And then that gets rid of the regset structure, replacing it with collect_regset and supply_regset ? At this point it feels like a large code shuffle.


Alan.

Re: [PATCH v2 1/3] Add min size to regset section iterations

[Simon Marchi]

On 2018-08-08 04:18, Alan Hayward wrote:
>> Therefore, I am starting to think the semantic is more straightforward 
>> (to me at least) if we named them supply_size and collect_size (which 
>> you mentioned in the original patch message).  This would make it 
>> somewhat clear that if you are in a supply scenario, collect_size is 
>> meaningless (and vice versa).  It becomes a bit simpler to explain:
>> 
>> - When supplying fixed-size regsets (!REGSET_VARIABLE_SIZE), 
>> supply_size is the exact expected size.
>> - When supplying variable-size regsets (REGSET_VARIABLE_SIZE) 
>> supply_size is actually just a minumum, because we don't know what we 
>> will actually find in the section yet.
>> - When collecting, we know the size in advance (since we know what we 
>> will dump), so collect_size is always the exact size that will be 
>> dumped.
> 
> The only point I have against this is that I had always assumed that
> the _iterate_over_regset_sections function was designed so that in the
> future extra functions could get added to regset, alongside supply and
> collect. If that happened, I expected the new function to use either
> size or min size. Calling the sizes collect_size and supply_size would
> confuse it. However, I probably shouldn’t worry about that, given it’s
> doubtful another function would get added.
> 
> Happy to do it that way.

I understand the concern.  The min_size/size does indeed sound more 
generic/extensible, but at the expense of clarity.  My pragmatic side 
prefers supply_size/collect_size, because I think a reader would 
understand more easily.

> But, how about if I moved the two sizes into regset?
> 
> struct regset
> {
>   const void *regmap;
>   supply_regset_ftype *supply_regset;
>   int supply_size;
>   collect_regset_ftype *collect_regset;
>   int collect_size;
>   unsigned flags;
> };
> 
> Reducing the callback to:
> cb (".reg", &aarch64_linux_gregset, NULL, cb_data);
> 
> For most targets the size will be fixed, so the regset structures can
> stay global.
> 
> But I’d have to be careful - for example
> s390_iterate_over_regset_sections sets size based on the current abi -
> instead I’d create both s390_gregset and s390x_gregset.
> 
> This is why I avoided doing it in the original patch :)
> 
> I could update using your suggestion, then maybe do a follow on patch
> with the above?

I don't have a strong opinion.  It just moves the problem around, 
passing the info in the structure instead of as formal parameters.  I 
think your original solution is ok, as long as the parameters are 
clearly documented.

Just to put yet another option on the table: since "size" parameter is 
only used to allocate some space for the collect function to dump the 
register data in, what about making the collect functions allocate that 
space themselves.  For example, by making them return a 
gdb::byte_vector.

>> On a different track, did you consider keeping a single "size" 
>> parameter to gdbarch_iterate_over_regset_sections, but add one to 
>> indicate whether the caller intends to supply or collect registers?  
>> And then, in aarch64's implementation, pass different sizes in the 
>> supply/collect cases?  Most other arch implementations would simply 
>> ignore this parameter and always pass the same size, as they do today.
> 
> If it’s going to indicate whether to use supply or collect, then it
> would seem odd to pass back a structure with both collect and supply
> functions in it, when you know which one isn’t getting used.
> 
> If going down that route, I’d probably split
> _iterate_over_regset_sections into two functions, one for collect and
> one for supply. And then that gets rid of the regset structure,
> replacing it with collect_regset and supply_regset ? At this point it
> feels like a large code shuffle.

Indeed, I don't think either that's a good direction.

Simon

Re: [PATCH v2 1/3] Add min size to regset section iterations

[Simon Marchi]

On 2018-08-08 09:34, Simon Marchi wrote:
> Just to put yet another option on the table: since "size" parameter is
> only used to allocate some space for the collect function to dump the
> register data in, what about making the collect functions allocate
> that space themselves.  For example, by making them return a
> gdb::byte_vector.

Just to let you know, I've started to play with this idea, see this 
branch (still very crude):

https://github.com/simark/binutils-gdb/commits/regset

Please let me know what you think.  Does it actually help in your case, 
and is it a good way to go in general?

Simon

Re: [PATCH v2 1/3] Add min size to regset section iterations

[Simon Marchi]

On 2018-08-09 14:29, Simon Marchi wrote:
> On 2018-08-08 09:34, Simon Marchi wrote:
>> Just to put yet another option on the table: since "size" parameter is
>> only used to allocate some space for the collect function to dump the
>> register data in, what about making the collect functions allocate
>> that space themselves.  For example, by making them return a
>> gdb::byte_vector.
> 
> Just to let you know, I've started to play with this idea, see this
> branch (still very crude):
> 
> https://github.com/simark/binutils-gdb/commits/regset
> 
> Please let me know what you think.  Does it actually help in your
> case, and is it a good way to go in general?
> 
> Simon

Oh, something that looks like a blocker already is that collect 
functions need to be able to write a single register in an existing 
buffer (leaving the other ones untouched).  If they return an allocated 
gdb::byte_vector, that doesn't seem possible.

Simon