Re: [PATCH 3/3] Dynamic core regset sections support

[Andreas Arnez <arnez@linux.vnet.ibm.com>]

Luis Machado <lgustavo@codesourcery.com> writes:

> Just a general thought. If a core file section must not always be
> generated, would it make more sense to use dynamic section information
> for s390 instead of forcing all the other targets to use a mechanism
> that is not required for them?

The reason for removing the old interface was to avoid increasing the
number of gdbarch interfaces for this.  In addition, the change reduces
the amount of target-dependent code for most targets, which I consider
beneficial.

> I'm not necessarily against this modification, but i was wondering
> about other options.

Sure, I'm open for suggestions.

> Did you maybe forget to handle ppc64 core file sections in this
> change?

Phew, you're right.  I guess we can use tdep->wordsize instead, like
this (right?):

  if ((*cb) (".reg", 48 * tdep->wordsize, "general-purpose", cb_data))

It would be great if you could verify this change -- I'm providing an
updated patch below.

> Another general comment/suggestion. Removing static definitions of
> core file register sets and replacing them with hardcoded arguments
> for a pointer-based call to a callback function seems to
> scramble/confuse things a little.
>
> It may be nicer to keep the static definitions around, but iterate
> through them in the new iterator function. What do you think?

I thought about something like that as well.  However, my implementation
attempt didn't result in less (or simpler) code.

Updated patch for linux-tdep.c:

--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -256,53 +256,26 @@ ppc_linux_return_value (struct gdbarch *gdbarch, struct value *function,
 				      readbuf, writebuf);
 }
 
-static struct core_regset_section ppc_linux_vsx_regset_sections[] =
-{
-  { ".reg", 48 * 4, "general-purpose" },
-  { ".reg2", 264, "floating-point" },
-  { ".reg-ppc-vmx", 544, "ppc Altivec" },
-  { ".reg-ppc-vsx", 256, "POWER7 VSX" },
-  { NULL, 0}
-};
-
-static struct core_regset_section ppc_linux_vmx_regset_sections[] =
-{
-  { ".reg", 48 * 4, "general-purpose" },
-  { ".reg2", 264, "floating-point" },
-  { ".reg-ppc-vmx", 544, "ppc Altivec" },
-  { NULL, 0}
-};
-
-static struct core_regset_section ppc_linux_fp_regset_sections[] =
-{
-  { ".reg", 48 * 4, "general-purpose" },
-  { ".reg2", 264, "floating-point" },
-  { NULL, 0}
-};
-
-static struct core_regset_section ppc64_linux_vsx_regset_sections[] =
-{
-  { ".reg", 48 * 8, "general-purpose" },
-  { ".reg2", 264, "floating-point" },
-  { ".reg-ppc-vmx", 544, "ppc Altivec" },
-  { ".reg-ppc-vsx", 256, "POWER7 VSX" },
-  { NULL, 0}
-};
-
-static struct core_regset_section ppc64_linux_vmx_regset_sections[] =
-{
-  { ".reg", 48 * 8, "general-purpose" },
-  { ".reg2", 264, "floating-point" },
-  { ".reg-ppc-vmx", 544, "ppc Altivec" },
-  { NULL, 0}
-};
-
-static struct core_regset_section ppc64_linux_fp_regset_sections[] =
+static void
+ppc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
+					 iterate_over_regset_sections_cb *cb,
+					 void *cb_data,
+					 const struct regcache *regcache)
 {
-  { ".reg", 48 * 8, "general-purpose" },
-  { ".reg2", 264, "floating-point" },
-  { NULL, 0}
-};
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  int have_vsx = tdep->ppc_vr0_regnum != -1;
+  int have_altivec = tdep->ppc_vsr0_upper_regnum != -1;
+
+  if ((*cb) (".reg", 48 * tdep->wordsize, "general-purpose", cb_data))
+    return;
+  if ((*cb) (".reg2", 264, "floating-point", cb_data))
+    return;
+  if (have_altivec)
+    if ((*cb) (".reg-ppc-vmx", 544, "ppc Altivec", cb_data))
+      return;
+  if (have_vsx)
+    (*cb) (".reg-ppc-vsx", 256, "POWER7 VSX", cb_data);
+}
 
 /* PLT stub in executable.  */
 static struct ppc_insn_pattern powerpc32_plt_stub[] =
@@ -1305,18 +1278,9 @@ ppc_linux_init_abi (struct gdbarch_info info,
       else
 	set_gdbarch_gcore_bfd_target (gdbarch, "elf32-powerpc");
 
-      /* Supported register sections.  */
-      if (tdesc_find_feature (info.target_desc,
-			      "org.gnu.gdb.power.vsx"))
-	set_gdbarch_core_regset_sections (gdbarch,
-					  ppc_linux_vsx_regset_sections);
-      else if (tdesc_find_feature (info.target_desc,
-			       "org.gnu.gdb.power.altivec"))
-	set_gdbarch_core_regset_sections (gdbarch,
-					  ppc_linux_vmx_regset_sections);
-      else
-	set_gdbarch_core_regset_sections (gdbarch,
-					  ppc_linux_fp_regset_sections);
+      /* Iterator for supported register sections.  */
+      set_gdbarch_iterate_over_regset_sections
+	(gdbarch, ppc_linux_iterate_over_regset_sections);
 
       if (powerpc_so_ops.in_dynsym_resolve_code == NULL)
 	{
@@ -1360,18 +1324,9 @@ ppc_linux_init_abi (struct gdbarch_info info,
       else
 	set_gdbarch_gcore_bfd_target (gdbarch, "elf64-powerpc");
 
-      /* Supported register sections.  */
-      if (tdesc_find_feature (info.target_desc,
-			      "org.gnu.gdb.power.vsx"))
-	set_gdbarch_core_regset_sections (gdbarch,
-					  ppc64_linux_vsx_regset_sections);
-      else if (tdesc_find_feature (info.target_desc,
-			       "org.gnu.gdb.power.altivec"))
-	set_gdbarch_core_regset_sections (gdbarch,
-					  ppc64_linux_vmx_regset_sections);
-      else
-	set_gdbarch_core_regset_sections (gdbarch,
-					  ppc64_linux_fp_regset_sections);
+      /* Iterator for supported register sections.  */
+      set_gdbarch_iterate_over_regset_sections
+	(gdbarch, ppc_linux_iterate_over_regset_sections);
     }
 
   /* PPC32 uses a different prpsinfo32 compared to most other Linux