Re: [PATCH] Enable vector instruction debugging for AIX

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>Please find attached the patch. [See 0001-PATCH-Enable-Vector-support-
for-AIX.patch]

Some comments on the patch inline.

>diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
>index 57e5756e144..15400f5b90d 100644
--- a/gdb/aix-thread.c
>+++ b/gdb/aix-thread.c
>@@ -1347,11 +1347,75 @@ fetch_regs_kernel_thread (struct regcache
*regcache, int regno,
> 	  supply_sprs32 (regcache, sprs32.pt_iar, sprs32.pt_msr,
sprs32.pt_cr,
> 			 sprs32.pt_lr, sprs32.pt_ctr, sprs32.pt_xer,
> 			 sprs32.pt_fpscr);
>-
>-	  if (tdep->ppc_mq_regnum >= 0)
>-	    regcache->raw_supply (tdep->ppc_mq_regnum, (char *)
&sprs32.pt_mq);

Why remove this support?  That looks unrelated.

>+  /* vector registers */

Coding style, should use full sentence here (and elsewhere), e.g.:
  /* Vector registers. */

>+  if (tdep->ppc_vr0_regnum != -1 && regno >= tdep->ppc_vr0_regnum)
>+  {
>+    int ret = 0;
>+    int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep-
>ppc_vr0_regnum + 1;
>+    __vmx_context_t vmx;
>+    memset(&vmx, 0, sizeof(__vmx_context_t));
>+    if (__power_vmx())

Are these types/functions (__vmx_context_t, __power_vmx) available
unconditionally, or does there need to be some configure check
(to avoid breaking compilation on older AIX version for example).

>+    {
>+       ret = ptrace64 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);

The rest of the code uses "ptrace64aix" to use either ptrace64
or ptracex depending on what's available.  Why is this not needed
here?

>+       if (ret < 0)
>+       {
>+         if (errno == ENXIO)
>+           return;
>+         perror_with_name (_("Unable to fetch AltiVec registers"));
>+       }

In the rest of this function, a failing ptrace call causes the
registers to be initialized to zero.  Why is it different here?

>+static void
>+fill_altivec (const struct regcache *regcache, __vmx_context_t *vmx)

Please move the "fill" routines next to the other "fill" routines.
Also, add a comment before the routine like with the others.

>diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
>index cb141427696..e989a9a6739 100644
>--- a/gdb/rs6000-aix-nat.c
>+++ b/gdb/rs6000-aix-nat.c
>@@ -54,6 +54,19 @@
> #include <sys/ldr.h>
> #include <sys/systemcfg.h>
> 
>+#include "gdbthread.h"

What is this needed for here?

>+#include <sys/context.h>
>+#include <sys/pthdebug.h>

Are these available unconditionally?

>+int have_ptrace_getvrregs = 1;
>+int have_ptrace_getsetvsxregs = 1;

These aren't handled consistenly in the rest of the code.
You should decide how you plan to handle the non-availability
of these ptrace features (silently ignore, treat as zero,
treat as unavailable) and then handle them consistently.


>+static void
>+store_vsx_register_aix (struct regcache *regcache, int regno)
>+{
>+  int ret;
>+  struct gdbarch *gdbarch = regcache->arch ();
>+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
>+  struct thrdentry64 thrdentry;
>+  __vsx_context_t vsx;
>+  pid_t pid = inferior_ptid.pid ();
>+  tid64_t  thrd_i = 0;
>+
>+  if (getthrds64(pid, &thrdentry, sizeof(struct thrdentry64),
>+                                            &thrd_i, 1) == 1)
>+    thrd_i = thrdentry.ti_tid;
>+  memset(&vsx, 0, sizeof(__vsx_context_t));
>+  if (__power_vsx()) 
>+  {
>+    ret = ptrace64 (PTT_READ_VSX, thrd_i, (long long) &vsx, 0, 0);

Same question as in aix-thread.c - should this check ARCH64 to
see if ptrace64 is even available?  It's strange to have those
checks everywhere else in this file but not here.

Similar question whether getthrds64 etc. are available
unconditionally.

>+    if (ret < 0)
>+    {
>+       if (errno == ENXIO)
>+       {
>+         /* have_ptrace_getsetvsxregs = 0; */
>+         warning (_("Unable to fetch VSX registers."));
>+         return;
>+       }

See above.  Final patches submitted upstream should never have
any code commented out - please resolve these questions before.

>@@ -264,6 +355,17 @@ store_register (struct regcache *regcache, int
regno)
> 
>   nr = regmap (gdbarch, regno, &isfloat);
> 
>+  if (altivec_register_p (gdbarch, regno))
>+  {
>+    store_altivec_register_aix (regcache, regno);
>+    return;
>+  }
>+  if (vsx_register_p (gdbarch, regno))
>+  {
>+    store_vsx_register_aix (regcache, regno);
>+    return;
>+  }

If you do it this way, it should be *before* the regmap call
(which doesn't handle any of these registers.)

>@@ -312,10 +506,6 @@ void
> rs6000_nat_target::fetch_registers (struct regcache *regcache, int
regno)
> {
>   struct gdbarch *gdbarch = regcache->arch ();
>-  if (regno != -1)
>-    fetch_register (regcache, regno);
>-
>-  else

This doesn't look right?

>+      if (have_ptrace_getvrregs)
>+        if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum !=
-1) 
>+        {
>+           fetch_altivec_registers_aix (regcache);
>+        }
>+
>+     if (have_ptrace_getsetvsxregs)
>+         if (tdep->ppc_vsr0_upper_regnum != -1)
>+          {
>+            fetch_vsx_registers_aix (regcache);
>+          }

No need for { } when it's a single statement.

>+const struct target_desc *
>+rs6000_nat_target::read_description ()
>+{
>+   if (ARCH64())
>+      return tdesc_powerpc_vsx64;
>+   else
>+      return tdesc_powerpc_vsx32;
>+}

Well, is this true?  Shouldn't this check whether the machine
actually *has* VSX, and return a different tdesc otherwise?

>@@ -145,7 +161,7 @@ aix_sighandle_frame_prev_register (frame_info_ptr
this_frame,
> 
> static int
> aix_sighandle_frame_sniffer (const struct frame_unwind *self,
>-			     frame_info_ptr this_frame,
>+			     frame_info_ptr this_frame, 
> 			     void **this_prologue_cache)
> {
>   CORE_ADDR pc = get_frame_pc (this_frame);

Whitespace only change - please remove.

>+/* Return non-zero if the architecture described by GDBARCH has
>+   VSX registers (vsr0 --- vsr63).  */
>+static int
>+rs6000_aix_vsx_support_p (struct gdbarch *gdbarch)
>+{
>+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
>+  return tdep->ppc_vsr0_regnum >= 0;
>+}
>+
>+/* Return non-zero if the architecture described by GDBARCH has
>+   Altivec registers (vr0 --- vr31, vrsave and vscr).  */
>+static int
>+rs6000_aix_altivec_support_p (struct gdbarch *gdbarch)
>+{
>+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
>+  return (tdep->ppc_vr0_regnum >= 0
>+   && tdep->ppc_vrsave_regnum >= 0);
>+}

Are these helpers really needed?  In other places the same check
is just performed inline.  This should be consistent everywhere.

>+    for (i = tdep->ppc_vr0_regnum, offset = offsets->vr0_offset;
>+                         i < tdep->ppc_vr0_regnum + ppc_num_vrs;
>+                                              i++, offset += 16)

Formatting.

>+    for (i = tdep->ppc_vsr0_upper_regnum;
>+     i < tdep->ppc_vsr0_upper_regnum + 32;
>+                           i++, offset++)
>+      ppc_supply_reg (regcache, i, (const gdb_byte *) vsxregs, offset
* 8, 8);

Formatting.  Also you should be consistent with code above and use
offset += 8 instead of the multiplication.

>+    for (i = tdep->ppc_vsr0_upper_regnum;
>+     i < tdep->ppc_vsr0_upper_regnum + 32;
>+                                     i++)
>+      ppc_collect_reg (regcache, i, (gdb_byte *) vsxregs, 0, 8);

This looks wrong - it always uses offset zero?

>@@ -262,10 +462,19 @@ rs6000_aix_iterate_over_regset_sections (struct
gdbarch *gdbarch,
> 					 const struct regcache
*regcache)
> {
>   ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
>+  int have_altivec = tdep->ppc_vr0_regnum != -1;
>+  int have_vsx = tdep->ppc_vsr0_upper_regnum != -1;
>+   
>   if (tdep->wordsize == 4)
>     cb (".reg", 592, 592, &rs6000_aix32_regset, NULL, cb_data);
>   else
>     cb (".reg", 576, 576, &rs6000_aix64_regset, NULL, cb_data);
>+ 
>+  if (have_altivec)
>+   cb (".aix-vmx", 560, 560, &rs6000_aix32_vrregset, "AIX altivec",
cb_data);
>+
>+  if (have_vsx)
>+   cb (".aix-vsx", 256, 256, &rs6000_aix32_vsxregset, "AIX vsx",
cb_data);    

As those regsets are the same between 32- and 64-bit, they should
probably be just called rs6000_aix_vrregset etc.

>@@ -351,7 +560,7 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch,
struct value *function,
> 
>       arg = args[argno];
>       type = check_typedef (value_type (arg));
>-      len = type->length ();
>+      len = type->length (); 

Another whitespace-only change, please remove.
 
>index 8b6d666bbe7..6e8ffc5fb0d 100644
>--- a/gdb/rs6000-tdep.c
>+++ b/gdb/rs6000-tdep.c
>@@ -1972,7 +1972,8 @@ skip_prologue (struct gdbarch *gdbarch,
CORE_ADDR pc, CORE_ADDR lim_pc,
>       /* 001110 00000 00000 iiii iiii iiii iiii  */
>       /* 001110 01110 00000 iiii iiii iiii iiii  */
>       else if ((op & 0xffff0000) == 0x38000000         /* li r0, SIMM
*/
>-	       || (op & 0xffff0000) == 0x39c00000)     /* li r14, SIMM
*/
>+	       || (op & 0xffff0000) == 0x39c00000     /* li r14, SIMM
*/
>+               || (op & 0xff0f0000) == 0x38000000)     /* li, r4,
SIMM */ 
> 	{
> 	  if ((op & 0xffff0000) == 0x38000000)
> 	    r0_contains_arg = 0;
>@@ -1986,24 +1987,21 @@ skip_prologue (struct gdbarch *gdbarch,
CORE_ADDR pc, CORE_ADDR lim_pc,
> 	}
>       /* Store vector register S at (r31+r0) aligned to 16
bytes.  */      
>       /* 011111 sssss 11111 00000 00111001110 */
>-      else if ((op & 0xfc1fffff) == 0x7c1f01ce)   /* stvx Vs, R31, R0
*/
>-	{
>-	  if (pc == (li_found_pc + 4))
>-	    {
>-	      vr_reg = GET_SRC_REG (op);
>-	      /* If this is the first vector reg to be saved, or if
>-		 it has a lower number than others previously seen,
>-		 reupdate the frame info.  */
>-	      if (fdata->saved_vr == -1 || fdata->saved_vr > vr_reg)
>-		{
>-		  fdata->saved_vr = vr_reg;
>-		  fdata->vr_offset = vr_saved_offset + offset;
>-		}
>-	      vr_saved_offset = -1;
>-	      vr_reg = -1;
>-	      li_found_pc = 0;
>-	    }
>-	}
>+      else if ((op & 0xfc1fffff) == 0x7c1f01ce   /* stvx Vs, R31, R0
*/
>+               || (op & 0xfc1fffff) == 0x7c012f99 /* stxvd2x Vs,r1,
r5 */
>+               || (op & 0xfc1fffff) == 0x7c012f98 /* stxvd2x vs0, r1,
r5 */
>+               || (op & 0xfc1ff000) == 0x7c012000)   
>+      {
>+        vr_reg = GET_SRC_REG (op);
>+        /* If this is the first vector reg to be saved, or if
>+       	   it has a lower number than others previously seen,
>+       	   reupdate the frame info.  */
>+        fdata->saved_vr = vr_reg;
>+        fdata->vr_offset = vr_saved_offset;
>+        vr_saved_offset = -1;
>+        vr_reg = -1;
>+        li_found_pc = 0;
>+      }

So I don't know enough about the details of the prologue generated
by the various compilers to understand exactly what's going on here,
but this change looks wrong - it now completely ignores the
"li_found_pc", so why even compute it?  More specifically, the
intention of the previous code was to only look for the first
instances of stvx* in the prolog at a particular place (because
later instances might serve a different purpose).  This change
just ignores all this.

Is prologue parsing even still necessary on AIX?  On Linux
these days we should always get this info via DWARF CIF.

>@@ -2665,8 +2663,8 @@ rs6000_convert_register_p (struct gdbarch
*gdbarch, int regnum,
> 	  && regnum >= tdep->ppc_fp0_regnum
> 	  && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs
> 	  && type->code () == TYPE_CODE_FLT
>-	  && (type->length ()
>-	      != builtin_type (gdbarch)->builtin_double->length ()));
>+          && (type->length ()
>+              != builtin_type (gdbarch)->builtin_double->length ()));

Another white-space only change.

>@@ -3466,7 +3464,7 @@ struct ppc_variant
>     unsigned long mach;
> 
>     /* Target description for this variant.  */
>-    const struct target_desc **tdesc;
>+    struct target_desc **tdesc;

This looks just wrong.

>@@ -3504,7 +3502,7 @@ static struct ppc_variant variants[] =
>   {"powerpc64", "PowerPC 64-bit user-level", bfd_arch_powerpc,
>    bfd_mach_ppc64, &tdesc_powerpc_altivec64},
>   {"620", "Motorola PowerPC 620", bfd_arch_powerpc,
>-   bfd_mach_ppc_620, &tdesc_powerpc_64},
>+   bfd_mach_ppc_620, &tdesc_powerpc_vsx64},

Should we change the default for "Motorola PowerPC 620" 
at this stage?  Can we even test on that machine?

>@@ -3680,18 +3678,15 @@ rs6000_frame_cache (frame_info_ptr this_frame,
void **this_cache)
>   /* if != -1, fdata.saved_vr is the smallest number of saved_vr.
>      All vr's from saved_vr to vr31 are saved.  */
>   if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1)
>-    {
>-      if (fdata.saved_vr >= 0)
>-	{
>-	  int i;
>-	  CORE_ADDR vr_addr = cache->base + fdata.vr_offset;
>-	  for (i = fdata.saved_vr; i < 32; i++)
>-	    {
>-	      cache->saved_regs[tdep->ppc_vr0_regnum + i].set_addr
(vr_addr);
>-	      vr_addr += register_size (gdbarch, tdep->ppc_vr0_regnum);
>-	    }
>-	}
>-    }
>+  {
>+    int i;
>+    CORE_ADDR vr_addr = cache->base + fdata.vr_offset;
>+    for (i = 0; i < 66; i++)
>+    {
>+      cache->saved_regs[tdep->ppc_vsr0_upper_regnum + i].set_addr
(vr_addr);
>+      vr_addr += register_size (gdbarch, tdep-
>ppc_vsr0_upper_regnum);
>+    } 
>+  } 

This looks wrong:
- It completely removes loading stored values for altivec registers
- It always loads VSX registers even if they haven't been saved
- It uses up to 66 "VSX upper-half" registers even though only
  32 of those exist.
- Are there even any call-saved VSX registers in the AIX ABI?
  In the
Linux ABI there aren't.

Bye,
Ulrich