This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


H Andreas,

Thanks for the patch.

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?

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

See comments below...

> Index: gdb/gdb/ppc-linux-tdep.c
===================================================================
--- gdb.orig/gdb/ppc-linux-tdep.c
+++ gdb/gdb/ppc-linux-tdep.c
@@ -256,53 +256,26 @@ ppc_linux_return_value (struct gdbarch *
  				      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[] =
+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" },
-  { ".reg-ppc-vmx", 544, "ppc Altivec" },
-  { 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;

-static struct core_regset_section ppc64_linux_fp_regset_sections[] =
-{
-  { ".reg", 48 * 8, "general-purpose" },
-  { ".reg2", 264, "floating-point" },
-  { NULL, 0}
-};
+  if ((*cb) (".reg", 48 * 4, "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
        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
        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


I am commenting explicitly for the ppc bits, but this may apply to the other backends as well.

Did you maybe forget to handle ppc64 core file sections in this change? I see a core file register set entry for ppc64 being removed, like so:

-  { ".reg", 48 * 8, "general-purpose" },

... but it is not being checked by the new code. Though the name is the same, the size of the section is different compared to ppc32.

-  { ".reg", 48 * 4, "general-purpose" },

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?

Regards,
Luis


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]