Another patch to aix-thread.c

Another patch to aix-thread.c

[Michael Snyder]

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

Hi Kevin, 

In aix-thread.c there are some obsolete and/or dubious uses of the
register cache (including direct reading/writing to the register cache).
This patch (which I emphasize, I have not tested!)
cleans them up and/or re-writes some of them.

Short summary: target_store_registers should copy the register cache
into the child's register state, but not by reading from the registers
array directly, and not by calling read_register.  The API for the 
target layer to get the contents of the cache is register_collect().
I also replaced a call to read_register_bytes from ops_prepare_to_store.

Michael

[-- Attachment #2: fetchstore.patch --]
[-- Type: text/plain, Size: 11481 bytes --]

2002-07-18  Michael Snyder  <msnyder@redhat.com>

	*  aix-thread.c (supply_sprs64): Cosmetic change.
	(supply_sprs32): Cosmetic change.
	(fill_gprs64, fill_gprs32, fill_fprs, fill_sprs32): New funcs.
	(fill_sprs64): Use regcache_collect instead of read_register.
	(store_regs_lib): Use regcache_collect instead of 
	read_register.  Use fill_sprs32 instead of fill_sprs64,
	if debugging a 32-bit architecture.
	(store_regs_kern): Use fill_gprs64 etc. to pull the values
	out of the register cache, instead of passing a pointer into
	the register cache directly to ptrace.  Use register_collect
	insteaad of read_register.
	(ops_prepare_to_store): Use target_read_registers instead
	of read_register_bytes.

Index: aix-thread.c
===================================================================
RCS file: /es/scratch/msnyder/cvsroot/aix/threads/aix-thread.c,v
retrieving revision 1.2
diff -p -r1.2 aix-thread.c
*** aix-thread.c	2002/07/17 15:25:34	1.2
--- aix-thread.c	2002/07/18 18:54:38
*************** supply_sprs64 (uint64_t iar, uint64_t ms
*** 1038,1044 ****
  	       uint64_t lr, uint64_t ctr, uint32_t xer)
  {
    int regno = FIRST_UISA_SP_REGNUM;
!   supply_register (regno, (char *) &iar);
    supply_register (regno + 1, (char *) &msr);
    supply_register (regno + 2, (char *) &cr);
    supply_register (regno + 3, (char *) &lr);
--- 1038,1045 ----
  	       uint64_t lr, uint64_t ctr, uint32_t xer)
  {
    int regno = FIRST_UISA_SP_REGNUM;
! 
!   supply_register (regno,     (char *) &iar);
    supply_register (regno + 1, (char *) &msr);
    supply_register (regno + 2, (char *) &cr);
    supply_register (regno + 3, (char *) &lr);
*************** supply_sprs32 (uint32_t iar, uint32_t ms
*** 1054,1060 ****
  	       uint32_t lr, uint32_t ctr, uint32_t xer)
  {
    int regno = FIRST_UISA_SP_REGNUM;
!   supply_register (regno, (char *) &iar);
    supply_register (regno + 1, (char *) &msr);
    supply_register (regno + 2, (char *) &cr);
    supply_register (regno + 3, (char *) &lr);
--- 1055,1062 ----
  	       uint32_t lr, uint32_t ctr, uint32_t xer)
  {
    int regno = FIRST_UISA_SP_REGNUM;
! 
!   supply_register (regno,     (char *) &iar);
    supply_register (regno + 1, (char *) &msr);
    supply_register (regno + 2, (char *) &cr);
    supply_register (regno + 3, (char *) &lr);
*************** ops_fetch_registers (int regno)
*** 1209,1214 ****
--- 1211,1246 ----
      }
  }
  
+ /* Store the gp registers into an array of uint32_t or uint64_t.  */
+ 
+ static void
+ fill_gprs64 (uint64_t *vals)
+ {
+   int regno;
+ 
+   for (regno = 0; regno < FP0_REGNUM; regno++)
+     regcache_collect (regno, vals + regno);
+ }
+ 
+ static void 
+ fill_gprs32 (uint32_t *vals)
+ {
+   int regno;
+ 
+   for (regno = 0; regno < FP0_REGNUM; regno++)
+     regcache_collect (regno, vals + regno);
+ }
+ 
+ /* Store the floating point registers into a double array.  */
+ static void
+ fill_fprs (double *vals)
+ {
+   int regno;
+ 
+   for (regno = FP0_REGNUM; regno < FPLAST_REGNUM; regno++)
+     regcache_collect (regno, vals + regno);
+ }
+ 
  /* Store the special registers into the specified 64-bit and 32-bit
     locations.  */
  
*************** static void
*** 1216,1228 ****
  fill_sprs64 (uint64_t *iar, uint64_t *msr, uint32_t *cr,
  	     uint64_t *lr, uint64_t *ctr, uint32_t *xer)
  {
    int regno = FIRST_UISA_SP_REGNUM;
!   *iar = read_register (regno);
!   *msr = read_register (regno + 1);
!   *cr = read_register (regno + 2);
!   *lr = read_register (regno + 3);
!   *ctr = read_register (regno + 4);
!   *xer = read_register (regno + 5);
  }
  
  /* Store all registers into pthread PDTID, which doesn't have a kernel
--- 1248,1275 ----
  fill_sprs64 (uint64_t *iar, uint64_t *msr, uint32_t *cr,
  	     uint64_t *lr, uint64_t *ctr, uint32_t *xer)
  {
+   int regno = FIRST_UISA_SP_REGNUM;
+ 
+   regcache_collect (regno,      (void *) iar);
+   regcache_collect (regno + 1,  (void *) msr);
+   regcache_collect (regno + 2,  (void *) cr);
+   regcache_collect (regno + 3,  (void *) lr);
+   regcache_collect (regno + 4,  (void *) ctr);
+   regcache_collect (regno + 5,  (void *) xer);
+ }
+ 
+ static void
+ fill_sprs32 (uint32_t *iar, uint32_t *msr, uint32_t *cr,
+ 	     uint32_t *lr,  uint32_t *ctr, uint32_t *xer)
+ {
    int regno = FIRST_UISA_SP_REGNUM;
! 
!   regcache_collect (regno,      (void *) iar);
!   regcache_collect (regno + 1,  (void *) msr);
!   regcache_collect (regno + 2,  (void *) cr);
!   regcache_collect (regno + 3,  (void *) lr);
!   regcache_collect (regno + 4,  (void *) ctr);
!   regcache_collect (regno + 5,  (void *) xer);
  }
  
  /* Store all registers into pthread PDTID, which doesn't have a kernel
*************** store_regs_lib (pthdb_pthread_t pdtid)
*** 1236,1241 ****
--- 1283,1291 ----
  {
    int status, i;
    pthdb_context_t ctx;
+   uint32_t int32;
+   uint64_t int64;
+   double   dbl;
  
    if (debug_aix_thread)
      fprintf_unfiltered (gdb_stdlog, 
*************** store_regs_lib (pthdb_pthread_t pdtid)
*** 1248,1266 ****
      error ("aix-thread: store_registers: pthdb_pthread_context returned %s",
             pd_status2str (status));
  
!   /* General-purpose registers.  */
  
    for (i = 0; i < 32; i++)
!     ctx.gpr[i] = read_register (i);
! 
!   /* Floating-point registers.  */
! 
!   for (i = 0; i < 32; i++)
!     ctx.fpr[i] = *(double *) &registers[REGISTER_BYTE (FP0_REGNUM + i)];
  
!   /* Special registers.  */
  
!   fill_sprs64 (&ctx.iar, &ctx.msr, &ctx.cr, &ctx.lr, &ctx.ctr, &ctx.xer);
  
    status = pthdb_pthread_setcontext (pd_session, pdtid, &ctx);
    if (status != PTHDB_SUCCESS)
--- 1298,1341 ----
      error ("aix-thread: store_registers: pthdb_pthread_context returned %s",
             pd_status2str (status));
  
!   /* Collect general-purpose register values from the regcache.  */
  
    for (i = 0; i < 32; i++)
!     {
!       if (arch64)
! 	{
! 	  regcache_collect (i, (void *) &int64);
! 	  ctx.gpr[i] = int64;
! 	}
!       else
! 	{
! 	  regcache_collect (i, (void *) &int32);
! 	  ctx.gpr[i] = int32;
! 	}
!     }
  
!   /* Collect floating-point register values from the regcache.  */
!   fill_fprs (ctx.fpr);
  
!   /* Special registers (always kept in ctx as 64 bits).  */
!   if (arch64)
!     {
!       fill_sprs64 (&ctx.iar, &ctx.msr, &ctx.cr, &ctx.lr, &ctx.ctr, &ctx.xer);
!     }
!   else
!     {
!       /* Problem: ctx.iar etc. are 64 bits, but raw_registers are 32.
! 	 Solution: use 32-bit temp variables.  */
!       uint32_t tmp1, tmp2, tmp3, tmp4, tmp5, tmp6;
! 
!       fill_sprs32 (&tmp1, &tmp2, &tmp3, &tmp4, &tmp5, &tmp6);
!       ctx.iar = tmp1;
!       ctx.msr = tmp2;
!       ctx.cr  = tmp3;
!       ctx.lr  = tmp4;
!       ctx.ctr = tmp5;
!       ctx.xer = tmp6;
!     }
  
    status = pthdb_pthread_setcontext (pd_session, pdtid, &ctx);
    if (status != PTHDB_SUCCESS)
*************** store_regs_lib (pthdb_pthread_t pdtid)
*** 1279,1287 ****
  static void
  store_regs_kern (int regno, pthdb_tid_t tid)
  {
    struct ptxsprs sprs64;
!   struct ptsprs sprs32;
!   char *regp;
  
    if (debug_aix_thread)
      fprintf_unfiltered (gdb_stdlog, "store_regs_kern tid=%lx regno=%d\n",
--- 1354,1365 ----
  static void
  store_regs_kern (int regno, pthdb_tid_t tid)
  {
+   uint64_t gprs64[32];
+   uint32_t gprs32[32];
+   double fprs[32];
    struct ptxsprs sprs64;
!   struct ptsprs  sprs32;
!   int i;
  
    if (debug_aix_thread)
      fprintf_unfiltered (gdb_stdlog, "store_regs_kern tid=%lx regno=%d\n",
*************** store_regs_kern (int regno, pthdb_tid_t 
*** 1290,1308 ****
    /* General-purpose registers.  */
    if (regno == -1 || regno < FP0_REGNUM)
      {
-       regp = &registers[REGISTER_BYTE (0)];
        if (arch64)
! 	ptrace64aix (PTT_WRITE_GPRS, tid, (unsigned long) regp, 0, NULL);
        else
! 	ptrace32 (PTT_WRITE_GPRS, tid, (int *) regp, 0, NULL);
      }
  
    /* Floating-point registers.  */
  
    if (regno == -1 || (regno >= FP0_REGNUM && regno <= FPLAST_REGNUM))
      {
!       regp = &registers[REGISTER_BYTE (FP0_REGNUM)];
!       ptrace32 (PTT_WRITE_FPRS, tid, (int *) regp, 0, NULL);
      }
  
    /* Special-purpose registers.  */
--- 1368,1391 ----
    /* General-purpose registers.  */
    if (regno == -1 || regno < FP0_REGNUM)
      {
        if (arch64)
! 	{
! 	  fill_gprs64 (gprs64);
! 	  ptrace64aix (PTT_WRITE_GPRS, tid, (unsigned long) gprs64, 0, NULL);
! 	}
        else
! 	{
! 	  fill_gprs32 (gprs32);
! 	  ptrace32 (PTT_WRITE_GPRS, tid, gprs32, 0, NULL);
! 	}
      }
  
    /* Floating-point registers.  */
  
    if (regno == -1 || (regno >= FP0_REGNUM && regno <= FPLAST_REGNUM))
      {
!       fill_fprs (fprs);
!       ptrace32 (PTT_WRITE_FPRS, tid, (int *) fprs, 0, NULL);
      }
  
    /* Special-purpose registers.  */
*************** store_regs_kern (int regno, pthdb_tid_t 
*** 1312,1338 ****
      {
        if (arch64)
  	{
  	  ptrace64aix (PTT_READ_SPRS, tid, 
  		       (unsigned long) &sprs64, 0, NULL);
  	  fill_sprs64 (&sprs64.pt_iar, &sprs64.pt_msr, &sprs64.pt_cr,
! 		       &sprs64.pt_lr, &sprs64.pt_ctr, &sprs64.pt_xer);
  	  ptrace64aix (PTT_WRITE_SPRS, tid, 
  		       (unsigned long) &sprs64, 0, NULL);
  	}
        else
  	{
  	  ptrace32 (PTT_READ_SPRS, tid, (int *) &sprs32, 0, NULL);
  
! 	  regno = FIRST_UISA_SP_REGNUM;
! 	  sprs32.pt_iar = read_register (regno);
! 	  sprs32.pt_msr = read_register (regno + 1);
! 	  sprs32.pt_cr = read_register (regno + 2);
! 	  sprs32.pt_lr = read_register (regno + 3);
! 	  sprs32.pt_ctr = read_register (regno + 4);
! 	  sprs32.pt_xer = read_register (regno + 5);
  
  	  if (REGISTER_RAW_SIZE (LAST_UISA_SP_REGNUM))
! 	    sprs32.pt_mq = read_register (LAST_UISA_SP_REGNUM);
  
  	  ptrace32 (PTT_WRITE_SPRS, tid, (int *) &sprs32, 0, NULL);
  	}
--- 1395,1418 ----
      {
        if (arch64)
  	{
+ 	  /* Must read first, not all of it's in the cache.  */
  	  ptrace64aix (PTT_READ_SPRS, tid, 
  		       (unsigned long) &sprs64, 0, NULL);
  	  fill_sprs64 (&sprs64.pt_iar, &sprs64.pt_msr, &sprs64.pt_cr,
! 		       &sprs64.pt_lr,  &sprs64.pt_ctr, &sprs64.pt_xer);
  	  ptrace64aix (PTT_WRITE_SPRS, tid, 
  		       (unsigned long) &sprs64, 0, NULL);
  	}
        else
  	{
+ 	  /* Must read first, not all of it's in the cache.  */
  	  ptrace32 (PTT_READ_SPRS, tid, (int *) &sprs32, 0, NULL);
  
! 	  fill_sprs32 (&sprs32.pt_iar, &sprs32.pt_msr, &sprs32.pt_cr,
! 		       &sprs32.pt_lr,  &sprs32.pt_ctr, &sprs32.pt_xer);
  
  	  if (REGISTER_RAW_SIZE (LAST_UISA_SP_REGNUM))
! 	    regcache_collect (LAST_UISA_SP_REGNUM, &sprs32.pt_mq);
  
  	  ptrace32 (PTT_WRITE_SPRS, tid, (int *) &sprs32, 0, NULL);
  	}
*************** ops_store_registers (int regno)
*** 1362,1376 ****
      }
  }
  
! /* Prepare to modify the registers array.  */
  
  static void
  ops_prepare_to_store (void)
  {
    if (!PD_TID (inferior_ptid))
      base_ops.to_prepare_to_store ();
    else
!     read_register_bytes (0, NULL, REGISTER_BYTES);
  }
  
  /* Transfer LEN bytes of memory from GDB address MYADDR to target
--- 1442,1461 ----
      }
  }
  
! /* Prepare to copy the register cache to the child:
!    The register cache must be fully fetched and up to date.  */
  
  static void
  ops_prepare_to_store (void)
  {
+   int i;
+ 
    if (!PD_TID (inferior_ptid))
      base_ops.to_prepare_to_store ();
    else
!     for (i = 0; i < NUM_REGS; i++)
!       if (!register_cached (i))
! 	target_fetch_registers (regnum);
  }
  
  /* Transfer LEN bytes of memory from GDB address MYADDR to target

Re: Another patch to aix-thread.c

[Kevin Buettner]

On Jul 18, 12:17pm, Michael Snyder wrote:

> In aix-thread.c there are some obsolete and/or dubious uses of the
> register cache (including direct reading/writing to the register cache).

I agree.  Thanks for helping me clean up this code.

> This patch (which I emphasize, I have not tested!)
> cleans them up and/or re-writes some of them.
> 
> Short summary: target_store_registers should copy the register cache
> into the child's register state, but not by reading from the registers
> array directly, and not by calling read_register.  The API for the 
> target layer to get the contents of the cache is register_collect().
> I also replaced a call to read_register_bytes from ops_prepare_to_store.

I've committed Michael's patch.  Prior to doing so, I made the following
changes:

	* aix-thread.c (gdb_assert.h): Include.
	(fill_sprs64, fill_sprs32): Add selected asserts to make sure that
	register sizes (from register cache) match size of buffer holding
	register data.
	(fill_sprs32): Change parameter types to match those in the ptrace()
	buffer.
	(store_regs_lib): Likewise, but for 32-bit temporary variables.
	(ops_prepare_to_store): Rename loop variable ``i'' to ``regno''.

--- aix-thread.c.snyder	Mon Jul 22 10:55:41 2002
+++ aix-thread.c	Mon Jul 22 11:52:12 2002
@@ -42,6 +42,7 @@
      */
 
 #include "defs.h"
+#include "gdb_assert.h"
 #include "gdbthread.h"
 #include "target.h"
 #include "inferior.h"
@@ -1250,26 +1251,34 @@ fill_sprs64 (uint64_t *iar, uint64_t *ms
 {
   int regno = FIRST_UISA_SP_REGNUM;
 
-  regcache_collect (regno,      (void *) iar);
-  regcache_collect (regno + 1,  (void *) msr);
-  regcache_collect (regno + 2,  (void *) cr);
-  regcache_collect (regno + 3,  (void *) lr);
-  regcache_collect (regno + 4,  (void *) ctr);
-  regcache_collect (regno + 5,  (void *) xer);
+  gdb_assert (sizeof (*iar) == REGISTER_RAW_SIZE (regno));
+
+  regcache_collect (regno,     iar);
+  regcache_collect (regno + 1, msr);
+  regcache_collect (regno + 2, cr);
+  regcache_collect (regno + 3, lr);
+  regcache_collect (regno + 4, ctr);
+  regcache_collect (regno + 5, xer);
 }
 
 static void
-fill_sprs32 (uint32_t *iar, uint32_t *msr, uint32_t *cr,
-	     uint32_t *lr,  uint32_t *ctr, uint32_t *xer)
+fill_sprs32 (unsigned long *iar, unsigned long *msr, unsigned long *cr,
+	     unsigned long *lr,  unsigned long *ctr, unsigned long *xer)
 {
   int regno = FIRST_UISA_SP_REGNUM;
 
-  regcache_collect (regno,      (void *) iar);
-  regcache_collect (regno + 1,  (void *) msr);
-  regcache_collect (regno + 2,  (void *) cr);
-  regcache_collect (regno + 3,  (void *) lr);
-  regcache_collect (regno + 4,  (void *) ctr);
-  regcache_collect (regno + 5,  (void *) xer);
+  /* If this assert() fails, the most likely reason is that GDB was
+     built incorrectly.  In order to make use of many of the header
+     files in /usr/include/sys, GDB needs to be configured so that
+     sizeof (long) == 4).  */
+  gdb_assert (sizeof (*iar) == REGISTER_RAW_SIZE (regno));
+
+  regcache_collect (regno,     iar);
+  regcache_collect (regno + 1, msr);
+  regcache_collect (regno + 2, cr);
+  regcache_collect (regno + 3, lr);
+  regcache_collect (regno + 4, ctr);
+  regcache_collect (regno + 5, xer);
 }
 
 /* Store all registers into pthread PDTID, which doesn't have a kernel
@@ -1325,16 +1334,18 @@ store_regs_lib (pthdb_pthread_t pdtid)
   else
     {
       /* Problem: ctx.iar etc. are 64 bits, but raw_registers are 32.
-	 Solution: use 32-bit temp variables.  */
-      uint32_t tmp1, tmp2, tmp3, tmp4, tmp5, tmp6;
-
-      fill_sprs32 (&tmp1, &tmp2, &tmp3, &tmp4, &tmp5, &tmp6);
-      ctx.iar = tmp1;
-      ctx.msr = tmp2;
-      ctx.cr  = tmp3;
-      ctx.lr  = tmp4;
-      ctx.ctr = tmp5;
-      ctx.xer = tmp6;
+	 Solution: use 32-bit temp variables.  (The assert() in fill_sprs32()
+	 will fail if the size of an unsigned long is incorrect.  If this
+	 happens, GDB needs to be reconfigured so that longs are 32-bits.)  */
+      unsigned long tmp_iar, tmp_msr, tmp_cr, tmp_lr, tmp_ctr, tmp_xer;
+
+      fill_sprs32 (&tmp_iar, &tmp_msr, &tmp_cr, &tmp_lr, &tmp_ctr, &tmp_xer);
+      ctx.iar = tmp_iar;
+      ctx.msr = tmp_msr;
+      ctx.cr  = tmp_cr;
+      ctx.lr  = tmp_lr;
+      ctx.ctr = tmp_ctr;
+      ctx.xer = tmp_xer;
     }
 
   status = pthdb_pthread_setcontext (pd_session, pdtid, &ctx);
@@ -1448,14 +1459,14 @@ ops_store_registers (int regno)
 static void
 ops_prepare_to_store (void)
 {
-  int i;
+  int regno;
 
   if (!PD_TID (inferior_ptid))
     base_ops.to_prepare_to_store ();
   else
-    for (i = 0; i < NUM_REGS; i++)
-      if (!register_cached (i))
-	target_fetch_registers (regnum);
+    for (regno = 0; regno < NUM_REGS; regno++)
+      if (!register_cached (regno))
+	target_fetch_registers (regno);
 }
 
 /* Transfer LEN bytes of memory from GDB address MYADDR to target

Re: Another patch to aix-thread.c

[Andrew Cagney]

> +  regcache_collect (regno,     iar);
> +  regcache_collect (regno + 1, msr);

Suggest ``regno + 0''.  Otherwize gdb_indent.sh will mess up your nice 
alignment.

Andrew

Re: Another patch to aix-thread.c

[Kevin Buettner]

On Jul 22,  3:43pm, Andrew Cagney wrote:

> > +  regcache_collect (regno,     iar);
> > +  regcache_collect (regno + 1, msr);
> 
> Suggest ``regno + 0''.  Otherwize gdb_indent.sh will mess up your nice 
> alignment.

I agree that this is a good idea.  However, I'm about to make some
changes which will obviate the use of regno + N.  I.e, I'm about to
transform the above into the following:

	struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
	...
	regcache_collect (PC_REGNUM, iar);
	regcache_collect (tdep->ppc_ps_regnum, msr);
	...

This will also eliminate the use of FIRST_UISA_SP_REGNUM from the
file.  While I'm at it, I'll also look at eliminating this macro
from rs6000-nat.c which means that it can be eliminated from GDB
entirely.

(It bothered me that aix-thread.c was making assumptions about the
order of the registers.  Also, it was more difficult to verify the
correctness by having expressions of the form "regno + small_offset".
I was having to look elsewhere to verify that the specified offset
was correct.)

Kevin