Re: [PATCH] Improve the fetch/store of general-purpose and floating-point PowerPC registers

["Sérgio Durigan Júnior" <sergiodj@linux.vnet.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 1665 bytes --]

Hi Joel,

On Tue, 2009-04-28 at 21:05 -0700, Joel Brobecker wrote:
> Actually, no. The comments are for the functions themselves. We're
> trying to make sure that every new function gets in with some
> documentation of what it does. It doesn't have to be very long,
> but sometimes writing what the return value is about is very useful.
> For instance, I remember that some of your functions will return
> zero if the operation failed, I think.  That's an interesting piece
> of information to put in the documentation.  When the function is
> obvious, or when it implements a routine that's part of the gdbarch
> vector, then what we've been doing, lately, is just say "Implements
> the "bla_bla_bla" gdbarch method." or somesuch (we try not to repeat
> the documentation to avoid maintenance issues).


Here goes the reviewed version of the patch, addressing your
observations.  Is it OK to check in now?

Thanks,

-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil

2009-05-05  Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>

	* ppc-linux-nat.c (have_ptrace_getsetregs): New variable.
	(have_ptrace_getsetfpregs): Likewise.
	fetch_all_gp_regs): New function.
	(fetch_gp_regs): New function.
	(fetch_all_fp_regs): Likewise.
	(fetch_fp_regs): New function.
	(fetch_ppc_registers): Using the new methods to fetch general-
	purpose and floating-pointer registers.
	(store_all_gp_regs): New function.
	(store_gp_regs): Likewise.
	(store_all_fp_regs): New function.
	(store_fp_regs): Likewise.
	(store_ppc_registers): Using the new methods to store general-
	purpose and floating-pointer registers.


[-- Attachment #2: read-regs-ppc.patch --]
[-- Type: text/x-patch, Size: 11130 bytes --]

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 8710d51..297365b 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -108,6 +108,21 @@
 #define PTRACE_GETSIGINFO    0x4202
 #endif
 
+/* Similarly for the general-purpose (gp0 -- gp31)
+   and floating-point registers (fp0 -- fp31).  */
+#ifndef PTRACE_GETREGS
+#define PTRACE_GETREGS 12
+#endif
+#ifndef PTRACE_SETREGS
+#define PTRACE_SETREGS 13
+#endif
+#ifndef PTRACE_GETFPREGS
+#define PTRACE_GETFPREGS 14
+#endif
+#ifndef PTRACE_SETFPREGS
+#define PTRACE_SETFPREGS 15
+#endif
+
 /* This oddity is because the Linux kernel defines elf_vrregset_t as
    an array of 33 16 bytes long elements.  I.e. it leaves out vrsave.
    However the PTRACE_GETVRREGS and PTRACE_SETVRREGS requests return
@@ -218,6 +233,18 @@ int have_ptrace_getvrregs = 1;
    error.  */
 int have_ptrace_getsetevrregs = 1;
 
+/* Non-zero if our kernel may support the PTRACE_GETREGS and
+   PTRACE_SETREGS requests, for reading and writing the
+   general-purpose registers.  Zero if we've tried one of
+   them and gotten an error.  */
+int have_ptrace_getsetregs = 1;
+
+/* Non-zero if our kernel may support the PTRACE_GETFPREGS and
+   PTRACE_SETFPREGS requests, for reading and writing the
+   floating-pointers registers.  Zero if we've tried one of
+   them and gotten an error.  */
+int have_ptrace_getsetfpregs = 1;
+
 /* *INDENT-OFF* */
 /* registers layout, as presented by the ptrace interface:
 PT_R0, PT_R1, PT_R2, PT_R3, PT_R4, PT_R5, PT_R6, PT_R7,
@@ -601,6 +628,112 @@ fetch_altivec_registers (struct regcache *regcache, int tid)
   supply_vrregset (regcache, &regs);
 }
 
+/* This function actually issues the request to ptrace, telling
+   it to get all general-purpose registers and put them into the
+   specified regset.
+   
+   If the ptrace request does not exist, this function returns 0
+   and properly sets the have_ptrace_* flag.  If the request fails,
+   this function calls perror_with_name.  Otherwise, if the request
+   succeeds, then the regcache gets filled and 1 is returned.  */
+static int
+fetch_all_gp_regs (struct regcache *regcache, int tid)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  gdb_gregset_t gregset;
+
+  if (ptrace (PTRACE_GETREGS, tid, 0, (void *) &gregset) < 0)
+    {
+      if (errno == EIO)
+        {
+          have_ptrace_getsetregs = 0;
+          return 0;
+        }
+      perror_with_name (_("Couldn't get general-purpose registers."));
+    }
+
+  supply_gregset (regcache, (const gdb_gregset_t *) &gregset);
+
+  return 1;
+}
+
+/* This is a wrapper for the fetch_all_gp_regs function.  It is
+   responsible for verifying if this target has the ptrace request
+   that can be used to fetch all general-purpose registers at one
+   shot.  If it doesn't, then we should fetch them using the
+   old-fashioned way, which is to iterate over the registers and
+   request them one by one.  */
+static void
+fetch_gp_regs (struct regcache *regcache, int tid)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  int i;
+
+  if (have_ptrace_getsetregs)
+    if (fetch_all_gp_regs (regcache, tid))
+      return;
+
+  /* If we've hit this point, it doesn't really matter which
+     architecture we are using.  We just need to read the
+     registers in the "old-fashioned way".  */
+  for (i = 0; i < ppc_num_gprs; i++)
+    fetch_register (regcache, tid, tdep->ppc_gp0_regnum + i);
+}
+
+/* This function actually issues the request to ptrace, telling
+   it to get all floating-point registers and put them into the
+   specified regset.
+   
+   If the ptrace request does not exist, this function returns 0
+   and properly sets the have_ptrace_* flag.  If the request fails,
+   this function calls perror_with_name.  Otherwise, if the request
+   succeeds, then the regcache gets filled and 1 is returned.  */
+static int
+fetch_all_fp_regs (struct regcache *regcache, int tid)
+{
+  gdb_fpregset_t fpregs;
+
+  if (ptrace (PTRACE_GETFPREGS, tid, 0, (void *) &fpregs) < 0)
+    {
+      if (errno == EIO)
+        {
+          have_ptrace_getsetfpregs = 0;
+          return 0;
+        }
+      perror_with_name (_("Couldn't get floating-point registers."));
+    }
+
+  supply_fpregset (regcache, (const gdb_fpregset_t *) &fpregs);
+
+  return 1;
+}
+
+/* This is a wrapper for the fetch_all_fp_regs function.  It is
+   responsible for verifying if this target has the ptrace request
+   that can be used to fetch all floating-point registers at one
+   shot.  If it doesn't, then we should fetch them using the
+   old-fashioned way, which is to iterate over the registers and
+   request them one by one.  */
+static void
+fetch_fp_regs (struct regcache *regcache, int tid)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  int i;
+
+  if (have_ptrace_getsetfpregs)
+    if (fetch_all_fp_regs (regcache, tid))
+      return;
+ 
+  /* If we've hit this point, it doesn't really matter which
+     architecture we are using.  We just need to read the
+     registers in the "old-fashioned way".  */
+  for (i = 0; i < ppc_num_fprs; i++)
+    fetch_register (regcache, tid, tdep->ppc_fp0_regnum + i);
+}
+
 static void 
 fetch_ppc_registers (struct regcache *regcache, int tid)
 {
@@ -608,11 +741,9 @@ fetch_ppc_registers (struct regcache *regcache, int tid)
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
-  for (i = 0; i < ppc_num_gprs; i++)
-    fetch_register (regcache, tid, tdep->ppc_gp0_regnum + i);
+  fetch_gp_regs (regcache, tid);
   if (tdep->ppc_fp0_regnum >= 0)
-    for (i = 0; i < ppc_num_fprs; i++)
-      fetch_register (regcache, tid, tdep->ppc_fp0_regnum + i);
+    fetch_fp_regs (regcache, tid);
   fetch_register (regcache, tid, gdbarch_pc_regnum (gdbarch));
   if (tdep->ppc_ps_regnum != -1)
     fetch_register (regcache, tid, tdep->ppc_ps_regnum);
@@ -970,18 +1101,142 @@ store_altivec_registers (const struct regcache *regcache, int tid)
     perror_with_name (_("Couldn't write AltiVec registers"));
 }
 
+/* This function actually issues the request to ptrace, telling
+   it to store all general-purpose registers present in the specified
+   regset.
+   
+   If the ptrace request does not exist, this function returns 0
+   and properly sets the have_ptrace_* flag.  If the request fails,
+   this function calls perror_with_name.  Otherwise, if the request
+   succeeds, then the regcache is stored and 1 is returned.  */
+static int
+store_all_gp_regs (const struct regcache *regcache, int tid, int regno)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  gdb_gregset_t gregset;
+
+  if (ptrace (PTRACE_GETREGS, tid, 0, (void *) &gregset) < 0)
+    {
+      if (errno == EIO)
+        {
+          have_ptrace_getsetregs = 0;
+          return 0;
+        }
+      perror_with_name (_("Couldn't get general-purpose registers."));
+    }
+
+  fill_gregset (regcache, &gregset, regno);
+
+  if (ptrace (PTRACE_SETREGS, tid, 0, (void *) &gregset) < 0)
+    {
+      if (errno == EIO)
+        {
+          have_ptrace_getsetregs = 0;
+          return 0;
+        }
+      perror_with_name (_("Couldn't set general-purpose registers."));
+    }
+
+  return 1;
+}
+
+/* This is a wrapper for the store_all_gp_regs function.  It is
+   responsible for verifying if this target has the ptrace request
+   that can be used to store all general-purpose registers at one
+   shot.  If it doesn't, then we should store them using the
+   old-fashioned way, which is to iterate over the registers and
+   store them one by one.  */
 static void
-store_ppc_registers (const struct regcache *regcache, int tid)
+store_gp_regs (const struct regcache *regcache, int tid, int regno)
 {
-  int i;
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
-  
+  int i;
+
+  if (have_ptrace_getsetregs)
+    if (store_all_gp_regs (regcache, tid, regno))
+      return;
+
+  /* If we hit this point, it doesn't really matter which
+     architecture we are using.  We just need to store the
+     registers in the "old-fashioned way".  */
   for (i = 0; i < ppc_num_gprs; i++)
     store_register (regcache, tid, tdep->ppc_gp0_regnum + i);
+}
+
+/* This function actually issues the request to ptrace, telling
+   it to store all floating-point registers present in the specified
+   regset.
+   
+   If the ptrace request does not exist, this function returns 0
+   and properly sets the have_ptrace_* flag.  If the request fails,
+   this function calls perror_with_name.  Otherwise, if the request
+   succeeds, then the regcache is stored and 1 is returned.  */
+static int
+store_all_fp_regs (const struct regcache *regcache, int tid, int regno)
+{
+  gdb_fpregset_t fpregs;
+
+  if (ptrace (PTRACE_GETFPREGS, tid, 0, (void *) &fpregs) < 0)
+    {
+      if (errno == EIO)
+        {
+          have_ptrace_getsetfpregs = 0;
+          return 0;
+        }
+      perror_with_name (_("Couldn't get floating-point registers."));
+    }
+
+  fill_fpregset (regcache, &fpregs, regno);
+
+  if (ptrace (PTRACE_SETFPREGS, tid, 0, (void *) &fpregs) < 0)
+    {
+      if (errno == EIO)
+        {
+          have_ptrace_getsetfpregs = 0;
+          return 0;
+        }
+      perror_with_name (_("Couldn't set floating-point registers."));
+    }
+
+  return 1;
+}
+
+/* This is a wrapper for the store_all_fp_regs function.  It is
+   responsible for verifying if this target has the ptrace request
+   that can be used to store all floating-point registers at one
+   shot.  If it doesn't, then we should store them using the
+   old-fashioned way, which is to iterate over the registers and
+   store them one by one.  */
+static void
+store_fp_regs (const struct regcache *regcache, int tid, int regno)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  int i;
+
+  if (have_ptrace_getsetfpregs)
+    if (store_all_fp_regs (regcache, tid, regno))
+      return;
+
+  /* If we hit this point, it doesn't really matter which
+     architecture we are using.  We just need to store the
+     registers in the "old-fashioned way".  */
+  for (i = 0; i < ppc_num_fprs; i++)
+    store_register (regcache, tid, tdep->ppc_fp0_regnum + i);
+}
+
+static void
+store_ppc_registers (const struct regcache *regcache, int tid)
+{
+  int i;
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+ 
+  store_gp_regs (regcache, tid, -1);
   if (tdep->ppc_fp0_regnum >= 0)
-    for (i = 0; i < ppc_num_fprs; i++)
-      store_register (regcache, tid, tdep->ppc_fp0_regnum + i);
+    store_fp_regs (regcache, tid, -1);
   store_register (regcache, tid, gdbarch_pc_regnum (gdbarch));
   if (tdep->ppc_ps_regnum != -1)
     store_register (regcache, tid, tdep->ppc_ps_regnum);