Re: [PATCH] Enable vector instruction debugging for AIX

[Aditya Kamath1 <Aditya.Kamath1@ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 22323 bytes --]

Hi all,

Thank you for the feedback. Please find attached the new patch. [See: 0001-Enable-Vector-instruction-debugging-support-for-AIX.patch ]

Kindly let me know if any changes required.

I have answered point by point for the feedback below. Please find below.

Have a nice day ahead.

Thanks and regards,
Aditya.

---------------------------------------------------------------------------------------------------------------
>-
>-        if (tdep->ppc_mq_regnum >= 0)
>-          regcache->raw_supply (tdep->ppc_mq_regnum, (char *)
&sprs32.pt_mq);

Why remove this support?  That looks unrelated.

Ans = Agreed. I made a mistake. I have corrected the same.

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

Ans = Taken care in all comments.


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).

Ans = They are all available since AIX version 5.3. So it should not break.


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

Ans = Taken care in complete aix-thread.c file changes

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

Ans = You were right I have taken care of it. See memset () function.

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

Ans = I have done it.

>+#include "gdbthread.h"

What is this needed for here?

Ans = I have removed it.

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

Are these available unconditionally?

 ANS = Available from AIX 5.3

>+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.

ANS = Taken care and eliminated. Thank you for the suggestion.

+    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?

ANS = Taken care here as well

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

ANS = removed. Thanks

>   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.)

ANS = Implemented. Thanks


>-  if (regno != -1)
>-    fetch_register (regcache, regno);
>-
>-  else

This doesn't look right?

ANS = Yes, I understood that sometimes when we know what register to write we need it. I have made some changed here in the patch.

+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?

ANS = AIX 5.3 onwards we get those _vmx_context_t, __power_vmx variables to support these target descriptions.

 Whitespace only change - please remove.

ANS = Done

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


ANS = Done


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.

ANS = I have taken care. But in case there is an update for large for loop statements let me know.


>+      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.

ANS = Done

>+      ppc_collect_reg (regcache, i, (gdb_byte *) vsxregs, 0, 8);

This looks wrong - it always uses offset zero?

ANS = Corrected


>+  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.

ANS = No, their offsets are different. Hence, we have different variables for them.


>-      {
>-        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;
>-          }
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.

ANS = I agree, I got this wrong in the previous patch. I have corrected it now. Kindly let me know.

>-   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?
ANS = No. No we cannot test on that machine.



>   /* 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

ANS =  I got this wrong in the previous patch. I have corrected it now. Kindly let me know.




________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 14 October 2022 18:11
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simark@simark.ca <simark@simark.ca>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Enable vector instruction debugging for AIX

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


[-- Attachment #2: 0001-Enable-Vector-instruction-debugging-support-for-AIX.patch --]
[-- Type: application/octet-stream, Size: 22827 bytes --]

From 43b8eebc56715fde2dc701e52789e1845c0c95f9 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 28 Oct 2022 04:59:06 -0500
Subject: [PATCH] Enable Vector instruction debugging support for AIX

---
 gdb/aix-thread.c      | 125 +++++++++++++++++++++++++
 gdb/rs6000-aix-nat.c  | 210 +++++++++++++++++++++++++++++++++++++++++-
 gdb/rs6000-aix-tdep.c | 179 +++++++++++++++++++++++++++++++++++
 gdb/rs6000-tdep.c     |  12 ++-
 4 files changed, 520 insertions(+), 6 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index e556c153576..6bcb9af7771 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -1352,6 +1352,43 @@ fetch_regs_kernel_thread (struct regcache *regcache, int regno,
 	    regcache->raw_supply (tdep->ppc_mq_regnum, (char *) &sprs32.pt_mq);
 	}
     }
+   
+  /* 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;
+      if (__power_vmx())
+      {
+        if (arch64)
+          ret = ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
+        else
+          ret = ptrace32 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
+        if (ret < 0)
+          memset(&vmx, 0, sizeof(__vmx_context_t));
+        for (i = 0; i < num_of_vrregs; i++)
+          regcache->raw_supply (tdep->ppc_vr0_regnum + i, &(vmx.__vr[i]));
+      }
+    }
+
+  /* vsx registers.  */
+  if (tdep->ppc_vsr0_upper_regnum != -1 && regno >= tdep->ppc_vsr0_upper_regnum)
+    {
+      __vsx_context_t vsx;
+      if (__power_vsx())
+      {
+        int ret = 0;
+        if (arch64)
+          ret = ptrace64aix (PTT_READ_VSX, tid, (long long) &vsx, 0, 0);
+        else
+          ret = ptrace32 (PTT_READ_VSX, tid, (long long) &vsx, 0, 0);
+        if (ret < 0)
+          memset(&vsx, 0, sizeof(__vsx_context_t));
+        for (i = 0; i < ppc_num_vshrs; i++)
+          regcache->raw_supply (tdep->ppc_vsr0_upper_regnum + i, &(vsx.__vsr_dw1[i]));
+        }
+    }
 }
 
 /* Fetch register REGNO if != -1 or all registers otherwise from the
@@ -1496,6 +1533,37 @@ fill_sprs32 (const struct regcache *regcache,
     regcache->raw_collect (tdep->ppc_fpscr_regnum, fpscr);
 }
 
+/* Fill altivec registers.  */
+
+static void
+fill_altivec (const struct regcache *regcache, __vmx_context_t *vmx)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+  int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;
+  int regno;
+
+  for (regno = 0; regno < num_of_vrregs; regno++)
+    if (REG_VALID == regcache->get_register_status (tdep->ppc_vr0_regnum + regno))
+      regcache->raw_collect (tdep->ppc_vr0_regnum + regno,
+                                 &(vmx->__vr[0]) + regno);
+}
+
+/* Fill vsx registers. */
+
+static void
+fill_vsx (const struct regcache *regcache, __vsx_context_t  *vsx)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+  int regno;
+
+  for (regno = 0; regno < ppc_num_vshrs; regno++)
+    if (REG_VALID == regcache->get_register_status ( tdep->ppc_vsr0_upper_regnum + regno))
+      regcache->raw_collect (tdep->ppc_vsr0_upper_regnum + regno,
+                                   &(vsx->__vsr_dw1[0]) + regno);
+}
+
 /* Store all registers into pthread PDTID, which doesn't have a kernel
    thread.
 
@@ -1602,6 +1670,7 @@ store_regs_kernel_thread (const struct regcache *regcache, int regno,
   double fprs[ppc_num_fprs];
   struct ptxsprs sprs64;
   struct ptsprs  sprs32;
+  int ret = 0; 
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -1692,6 +1761,62 @@ store_regs_kernel_thread (const struct regcache *regcache, int regno,
 	  ptrace32 (PTT_WRITE_SPRS, tid, (uintptr_t) &sprs32, 0, NULL);
 	}
     }
+
+    /* Vector registers.  */
+    if (tdep->ppc_vr0_regnum != -1 && regno >= tdep->ppc_vr0_regnum)
+    {
+      __vmx_context_t vmx;
+      if (__power_vmx())
+      {
+        if (arch64)
+          ret = ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
+        else
+          ret = ptrace32 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
+        if (ret < 0)
+        {
+          if (errno == ENXIO)
+          {
+            warning (_("Unable to fetch AltiVec registers."));
+            memset(&vmx, 0, sizeof(__vmx_context_t)); 
+          }
+          fill_altivec(regcache, &vmx);
+          if (arch64)
+            ret = ptrace64aix (PTT_WRITE_VEC, tid, (long long) &vmx, 0, 0);
+          else
+            ret = ptrace32 (PTT_WRITE_VEC, tid, (long long) &vmx, 0, 0);
+          if (ret < 0)
+            perror_with_name (_("Unable to store AltiVec register."));
+        }
+      }
+    }
+ 
+    /* vmx registers.  */
+    if (tdep->ppc_vsr0_upper_regnum != -1 && regno >= tdep->ppc_vsr0_upper_regnum)
+    {
+      __vsx_context_t vsx;
+      if (__power_vsx())
+      {
+        if (arch64)
+          ret =  ptrace64aix (PTT_READ_VSX, tid, (long long) &vsx, 0, 0);
+        else
+          ret =  ptrace32 (PTT_READ_VSX, tid, (long long) &vsx, 0, 0);
+        if (ret < 0)
+        {
+          if (errno == ENXIO)
+          {
+            warning (_("Unable to fetch VSX registers."));
+            memset(&vsx, 0, sizeof(__vsx_context_t)); 
+          }
+        }
+        fill_vsx (regcache, &vsx);
+        if (arch64)
+          ret = ptrace64aix (PTT_WRITE_VSX, tid, (long long) &vsx, 0, 0);
+        else
+          ret = ptrace32 (PTT_WRITE_VSX, tid, (long long) &vsx, 0, 0);
+        if (ret < 0)
+          perror_with_name (_("Unable to store VSX register."));
+      }
+   } 
 }
 
 /* Store gdb's current view of the register set into the
diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index cb141427696..3a5f23b4ba7 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -54,6 +54,14 @@
 #include <sys/ldr.h>
 #include <sys/systemcfg.h>
 
+#include <sys/context.h>
+#include <sys/pthdebug.h>
+
+#include "features/rs6000/powerpc-vsx64.c"
+#include "features/rs6000/powerpc-vsx32.c"
+#include "features/rs6000/powerpc-altivec32.c"
+#include "features/rs6000/powerpc-altivec64.c"
+
 /* On AIX4.3+, sys/ldr.h provides different versions of struct ld_info for
    debugging 32-bit and 64-bit processes.  Define a typedef and macros for
    accessing fields in the appropriate structures.  */
@@ -91,6 +99,8 @@ class rs6000_nat_target final : public inf_ptrace_target
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
+  const struct target_desc *read_description ()  override;
+
 protected:
 
   void post_startup_inferior (ptid_t ptid) override
@@ -187,6 +197,174 @@ rs6000_ptrace64 (int req, int id, long long addr, int data, void *buf)
   return ret;
 }
 
+/* Store the vsx registers.  */
+
+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())
+  {
+    if (ARCH64 ())
+      ret = rs6000_ptrace64 (PTT_READ_VSX, thrd_i, (long long) &vsx, 0, 0);
+    else
+      ret = rs6000_ptrace32 (PTT_READ_VSX, thrd_i, (int *)&vsx, 0, 0);
+    if (ret < 0)
+    {
+       if (errno == ENXIO)
+       {
+         warning (_("Unable to fetch VSX registers."));
+         return;
+       }
+    }
+  }
+  regcache->raw_collect (regno, &(vsx.__vsr_dw1[0])+
+                       regno - tdep->ppc_vsr0_upper_regnum);
+
+  if (ARCH64 ())
+    ret = rs6000_ptrace64 (PTT_WRITE_VSX, thrd_i, (long long) &vsx, 0, 0);
+  else
+    ret = rs6000_ptrace32 (PTT_WRITE_VSX, thrd_i, (int *) &vsx, 0, 0);
+
+  if (ret < 0)
+    perror_with_name (_("Unable to store VSX register."));
+}
+
+/* Store Altivec registers.  */
+
+static void
+store_altivec_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;
+  __vmx_context_t vmx;
+  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(&vmx, 0, sizeof(__vmx_context_t));
+  if (__power_vmx())
+  {
+    if (ARCH64 ())
+      ret = rs6000_ptrace64 (PTT_READ_VEC, thrd_i, (long long) &vmx, 0, 0);
+    else
+      ret = rs6000_ptrace32 (PTT_READ_VEC, thrd_i, (int *) &vmx, 0, 0);
+    if (ret < 0)
+    {
+       if (errno == ENXIO)
+       {
+         warning (_("Unable to fetch AltiVec registers."));
+         return;
+       }
+    }
+  }
+
+  regcache->raw_collect (regno, &(vmx.__vr[0]) + regno
+                                - tdep->ppc_vr0_regnum);
+
+  if (ARCH64 ())
+    ret = rs6000_ptrace64 (PTT_WRITE_VEC, thrd_i, (long long) &vmx, 0, 0);
+  else
+    ret = rs6000_ptrace32 (PTT_WRITE_VEC, thrd_i, (int *) &vmx, 0, 0);
+  if (ret < 0)
+     perror_with_name (_("Unable to store AltiVec register."));
+
+}
+
+/* Supply altivec registers.  */
+
+static void
+supply_vrregset_aix (struct regcache *regcache, __vmx_context_t *vmx)
+{
+  int i;
+  struct gdbarch *gdbarch = regcache->arch ();
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+  int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;
+
+  for (i = 0; i < num_of_vrregs; i++)
+    regcache->raw_supply (tdep->ppc_vr0_regnum + i,
+                                    &(vmx->__vr[i]));
+}
+
+/* Fetch altivec register.  */
+
+static void
+fetch_altivec_registers_aix (struct regcache *regcache)
+{
+  struct thrdentry64 thrdentry;
+  __vmx_context_t vmx;
+  pid_t pid = current_inferior ()->pid;
+  tid64_t  thrd_i = 0;
+
+  if (getthrds64(pid, &thrdentry, sizeof(struct thrdentry64),
+                               &thrd_i, 1) == 1)
+    thrd_i = thrdentry.ti_tid;
+
+  memset(&vmx, 0, sizeof(__vmx_context_t));
+  if (__power_vmx())
+  {
+    if (ARCH64 ())
+      rs6000_ptrace64 (PTT_READ_VEC, thrd_i, (long long) &vmx, 0, 0);
+    else
+      rs6000_ptrace32 (PTT_READ_VEC, thrd_i, (int *) &vmx, 0, 0);
+    supply_vrregset_aix (regcache, &vmx);      
+  }
+}
+
+/* supply vsx register.  */
+
+static void
+supply_vsxregset_aix (struct regcache *regcache, __vsx_context_t *vsx)
+{
+
+  int i;
+  struct gdbarch *gdbarch = regcache->arch ();
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+
+  for (i = 0; i < ppc_num_vshrs; i++)
+   regcache->raw_supply (tdep->ppc_vsr0_upper_regnum + i,
+                                   &(vsx->__vsr_dw1[i]));
+}
+
+/* Fetch vsx registers.  */
+
+static void
+fetch_vsx_registers_aix (struct regcache *regcache)
+{
+  struct thrdentry64 thrdentry;
+  __vsx_context_t vsx;
+  pid_t pid = current_inferior ()->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())
+  {
+    if (ARCH64 ())
+      rs6000_ptrace64 (PTT_READ_VSX, thrd_i, (long long) &vsx, 0, 0);
+    else
+      rs6000_ptrace32 (PTT_READ_VSX, thrd_i, (int *) &vsx, 0, 0);
+    supply_vsxregset_aix (regcache, &vsx);
+  }
+}
+
 /* Fetch register REGNO from the inferior.  */
 
 static void
@@ -233,7 +411,7 @@ fetch_register (struct regcache *regcache, int regno)
 	    *addr = buf;
 	}
     }
-
+  
   if (!errno)
     regcache->raw_supply (regno, (char *) addr);
   else
@@ -244,6 +422,9 @@ fetch_register (struct regcache *regcache, int regno)
 #endif
       errno = 0;
     }
+
+  fetch_altivec_registers_aix (regcache);
+  fetch_vsx_registers_aix (regcache);
 }
 
 /* Store register REGNO back into the inferior.  */
@@ -262,6 +443,17 @@ store_register (struct regcache *regcache, int regno)
   /* -1 can be a successful return value, so infer errors from errno.  */
   errno = 0;
 
+  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;
+  }
+
   nr = regmap (gdbarch, regno, &isfloat);
 
   /* Floating-point registers.  */
@@ -314,7 +506,6 @@ rs6000_nat_target::fetch_registers (struct regcache *regcache, int regno)
   struct gdbarch *gdbarch = regcache->arch ();
   if (regno != -1)
     fetch_register (regcache, regno);
-
   else
     {
       ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
@@ -331,7 +522,13 @@ rs6000_nat_target::fetch_registers (struct regcache *regcache, int regno)
       if (tdep->ppc_fp0_regnum >= 0)
 	for (regno = 0; regno < ppc_num_fprs; regno++)
 	  fetch_register (regcache, tdep->ppc_fp0_regnum + regno);
+      
+      if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1) 
+        fetch_altivec_registers_aix (regcache);
 
+      if (tdep->ppc_vsr0_upper_regnum != -1)
+        fetch_vsx_registers_aix (regcache);
+          
       /* Read special registers.  */
       fetch_register (regcache, gdbarch_pc_regnum (gdbarch));
       fetch_register (regcache, tdep->ppc_ps_regnum);
@@ -346,6 +543,15 @@ rs6000_nat_target::fetch_registers (struct regcache *regcache, int regno)
     }
 }
 
+const struct target_desc *
+rs6000_nat_target::read_description ()
+{
+   if (ARCH64())
+      return tdesc_powerpc_vsx64;
+   else
+      return tdesc_powerpc_vsx32;
+}
+
 /* Store our register values back into the inferior.
    If REGNO is -1, do this for all registers.
    Otherwise, REGNO specifies which register (so we can save time).  */
diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
index d47974b51d1..dc3508e503d 100644
--- a/gdb/rs6000-aix-tdep.c
+++ b/gdb/rs6000-aix-tdep.c
@@ -68,6 +68,176 @@
 /* Minimum possible text address in AIX.  */
 #define AIX_TEXT_SEGMENT_BASE 0x10000000
 
+struct rs6000_aix_reg_vrreg_offset
+{
+  int vr0_offset;
+  int vscr_offset;
+  int vrsave_offset;
+};
+
+static struct rs6000_aix_reg_vrreg_offset rs6000_aix_vrreg_offset =
+{
+   /* AltiVec registers.  */
+  32, /* vr0_offset */
+  544, /* vscr_offset. */
+  560 /* vrsave_offset */
+};
+
+static int
+rs6000_aix_get_vrreg_offset (ppc_gdbarch_tdep *tdep,
+  const struct rs6000_aix_reg_vrreg_offset *offsets,
+                                         int regnum)
+{
+  if (regnum >= tdep->ppc_vr0_regnum &&
+  regnum < tdep->ppc_vr0_regnum + ppc_num_vrs)
+    return offsets->vr0_offset + (regnum - tdep->ppc_vr0_regnum) * 16;
+
+  if (regnum == tdep->ppc_vrsave_regnum - 1)
+    return offsets->vscr_offset;
+
+  if (regnum == tdep->ppc_vrsave_regnum)
+    return offsets->vrsave_offset;
+
+  return -1;
+}
+
+static void
+rs6000_aix_supply_vrregset (const struct regset *regset, struct regcache *regcache,
+                                        int regnum, const void *vrregs, size_t len)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  const struct rs6000_aix_reg_vrreg_offset  *offsets;
+  size_t offset;
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+  if (!(tdep->ppc_vr0_regnum >= 0  && tdep->ppc_vrsave_regnum >= 0))
+    return;
+
+  offsets = (const struct rs6000_aix_reg_vrreg_offset *) regset->regmap;
+  if (regnum == -1)
+  {
+    int i;
+
+    for (i = tdep->ppc_vr0_regnum, offset = offsets->vr0_offset;
+                         i < tdep->ppc_vr0_regnum + ppc_num_vrs;
+                                              i++, offset += 16)
+        ppc_supply_reg (regcache, i, (const gdb_byte *) vrregs, offset, 16);
+
+    ppc_supply_reg (regcache, (tdep->ppc_vrsave_regnum - 1),
+        (const gdb_byte *) vrregs, offsets->vscr_offset, 4);
+
+    ppc_supply_reg (regcache, tdep->ppc_vrsave_regnum,
+    (const gdb_byte *) vrregs, offsets->vrsave_offset, 4);
+
+    return;
+  }
+  offset = rs6000_aix_get_vrreg_offset (tdep, offsets, regnum);
+  if (regnum != tdep->ppc_vrsave_regnum &&
+      regnum != tdep->ppc_vrsave_regnum - 1)
+    ppc_supply_reg (regcache, regnum, (const gdb_byte *) vrregs, offset, 16);
+  else
+    ppc_supply_reg (regcache, regnum,
+     (const gdb_byte *) vrregs, offset, 4);
+
+}
+
+static void
+rs6000_aix_supply_vsxregset (const struct regset *regset, struct regcache *regcache,
+                                        int regnum, const void *vsxregs, size_t len)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+  if (!(tdep->ppc_vsr0_regnum >= 0))
+    return;
+
+  if (regnum == -1)
+  {
+    int i, offset = 0;
+
+    for (i = tdep->ppc_vsr0_upper_regnum; i < tdep->ppc_vsr0_upper_regnum 
+                                                   + 32; i++, offset += 8)
+      ppc_supply_reg (regcache, i, (const gdb_byte *) vsxregs, offset, 8);
+
+    return;
+  }
+  else
+    ppc_supply_reg (regcache, regnum, (const gdb_byte *) vsxregs, 0, 8);
+}
+
+static void
+rs6000_aix_collect_vsxregset (const struct regset *regset,
+                          const struct regcache *regcache,
+                    int regnum, void *vsxregs, size_t len)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+  if (!(tdep->ppc_vsr0_regnum >= 0))
+    return;
+
+  if (regnum == -1)
+  {
+    int i;
+    int offset = 0;
+    for (i = tdep->ppc_vsr0_upper_regnum; i < tdep->ppc_vsr0_upper_regnum
+                                                  + 32; i++, offset += 8)
+      ppc_collect_reg (regcache, i, (gdb_byte *) vsxregs, offset, 8);
+
+    return;
+  }
+
+ else
+    ppc_collect_reg (regcache, regnum, (gdb_byte *) vsxregs, 0, 8);
+}
+
+static void
+rs6000_aix_collect_vrregset (const struct regset *regset,
+                         const struct regcache *regcache,
+                    int regnum, void *vrregs, size_t len)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  const struct rs6000_aix_reg_vrreg_offset *offsets;
+  size_t offset;
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+  if (!(tdep->ppc_vr0_regnum >= 0 && tdep->ppc_vrsave_regnum >= 0))
+    return;
+
+  offsets = (const struct rs6000_aix_reg_vrreg_offset *) regset->regmap;
+  if (regnum == -1)
+  {
+    int i;
+
+    for (i = tdep->ppc_vr0_regnum, offset = offsets->vr0_offset; i <
+              tdep->ppc_vr0_regnum + ppc_num_vrs; i++, offset += 16)
+      ppc_collect_reg (regcache, i, (gdb_byte *) vrregs, offset, 16);
+
+    ppc_collect_reg (regcache, (tdep->ppc_vrsave_regnum - 1),
+               (gdb_byte *) vrregs, offsets->vscr_offset, 4);
+
+    ppc_collect_reg (regcache, tdep->ppc_vrsave_regnum,
+             (gdb_byte *) vrregs, offsets->vrsave_offset, 4);
+
+    return;
+  }
+  offset = rs6000_aix_get_vrreg_offset (tdep, offsets, regnum);
+  if (regnum != tdep->ppc_vrsave_regnum
+      && regnum != tdep->ppc_vrsave_regnum - 1)
+    ppc_collect_reg (regcache, regnum, (gdb_byte *) vrregs, offset, 16);
+  else
+    ppc_collect_reg (regcache, regnum,
+                   (gdb_byte *) vrregs, offset, 4);
+}
+
+static const struct regset rs6000_aix32_vrregset = {
+  &rs6000_aix_vrreg_offset,
+  rs6000_aix_supply_vrregset,
+  rs6000_aix_collect_vrregset
+};
+
+static const struct regset rs6000_aix32_vsxregset = {
+  &rs6000_aix_vrreg_offset,
+  rs6000_aix_supply_vsxregset,
+  rs6000_aix_collect_vsxregset
+};
+
 static struct trad_frame_cache *
 aix_sighandle_frame_cache (frame_info_ptr this_frame,
 			   void **this_cache)
@@ -262,10 +432,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); 
 }
 
 
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 8b6d666bbe7..9588068bfa9 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,7 +1987,10 @@ 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 */
+      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)   
 	{
 	  if (pc == (li_found_pc + 4))
 	    {
@@ -3474,7 +3478,7 @@ static struct ppc_variant variants[] =
   {"powerpc", "PowerPC user-level", bfd_arch_powerpc,
    bfd_mach_ppc, &tdesc_powerpc_altivec32},
   {"power", "POWER user-level", bfd_arch_rs6000,
-   bfd_mach_rs6k, &tdesc_rs6000},
+   bfd_mach_rs6k, &tdesc_powerpc_vsx32},
   {"403", "IBM PowerPC 403", bfd_arch_powerpc,
    bfd_mach_ppc_403, &tdesc_powerpc_403},
   {"405", "IBM PowerPC 405", bfd_arch_powerpc,
@@ -3504,7 +3508,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},
   {"630", "Motorola PowerPC 630", bfd_arch_powerpc,
    bfd_mach_ppc_630, &tdesc_powerpc_64},
   {"a35", "PowerPC A35", bfd_arch_powerpc,
-- 
2.31.1