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-Enable-vector-instruction-debugging-for-AIX.patch}.

This should be nearly ready to commit, I've just found a
few minor issues - see below.

Also, please provide commit message.

>--- a/gdb/NEWS
>+++ b/gdb/NEWS
>@@ -3,6 +3,8 @@
> 
> *** Changes since GDB 13
> 
>+* AIX now needs to be of version 5.2 or higher to install GDB.
>+
> * Multi-target feature configuration
> 
>   GDB now supports the individual configuration of remote targets' feature

I think these are usually under a separate section, something like:

 * Removed targets and native configurations

 GDB no longer supports AIX 4.x and AIX 5.1.  The minimum supported
 AIX version is now AIX 5.2.

As a separate note, I see that even AIX 5.2 itself has been out of
support for over 13 years now, so I'm wondering how much sense it
makes to still claim support for this in GDB.  If we're already
deprecating old versions, maybe it would be better to set the
minimum supported version at 6.1 or even 7.1 at this point? 

>@@ -7606,10 +7608,10 @@ the possible architectures.
> 
> Windows 95, x86 Windows NT			i[345]86-*-cygwin32
> M68K NetBSD					m68k-*-netbsd*
>-PowerPC AIX 4.x					powerpc-*-aix*
>+PowerPC AIX 5.2 or higher			powerpc-*-aix*
> PowerPC MacOS					powerpc-*-macos*
> PowerPC Windows NT				powerpcle-*-cygwin32
>-RS/6000 AIX 4.x					rs6000-*-aix4*
>+RS/6000 AIX 5.2 or higher			rs6000-*-aix4*

This describes what was added in GDB 4.18 - I don't think this
section should be retroactively modified.
 
>+static void 
>+supply_altivec_regs (struct regcache *regcache, __vmx_context_t vmx)
>+{ 
>+  ppc_gdbarch_tdep *tdep
>+    = gdbarch_tdep<ppc_gdbarch_tdep> (regcache->arch ());
>+  int regno; 
>+  int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;
>+  for (regno = 0; regno < num_of_vrregs; regno++)
>+     regcache->raw_supply (tdep->ppc_vr0_regnum + regno, 
>+			   &(vmx.__vr[regno]));

Now that you've taken VRSAVE and VSCR separately, this loop now
only needs to execute 32 times, but you still do it 34 times.
I think you should just use ppc_num_vrs instead of this
num_of_vrregs you compute here.

>+  regcache->raw_supply (tdep->ppc_vrsave_regnum, &(vmx.__vrsave));
>+  regcache->raw_supply (tdep->ppc_vrsave_regnum - 1, &(vmx.__vscr));

>+  /* vector registers.  */
>+  if (tdep->ppc_vr0_regnum != -1)
>+    {
>+      int ret = 0;
>+      int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;
>+      __vmx_context_t vmx;
>+      if (data->arch64)
>+	ret = ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
>+      else
>+	ret = ptrace32 (PTT_READ_VEC, tid, (uintptr_t) &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]));

Same problem here.

>+      regcache->raw_supply (tdep->ppc_vrsave_regnum, &(vmx.__vrsave));
>+      regcache->raw_supply (tdep->ppc_vrsave_regnum - 1, &(vmx.__vscr));

>+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[regno]));

And here.

>+  if (REG_VALID == regcache->get_register_status (tdep->ppc_vrsave_regnum))
>+  { 
>+    regcache->raw_collect (tdep->ppc_vrsave_regnum, &(vmx->__vrsave));
>+    regcache->raw_collect (tdep->ppc_vrsave_regnum - 1, &(vmx->__vscr));

You should separately check VSCR for REG_VALID.

>+  int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;

Same problem here.
 
>+      if (REG_VALID == regcache->get_register_status (tdep->ppc_vrsave_regnum))
>+      {
>+	ctx.vmx.__vrsave = vmx.__vrsave;
>+	ctx.vmx.__vscr = vmx.__vscr;

Also should do a separate REG_VALID check.

>@@ -1766,6 +1979,56 @@ store_regs_kernel_thread (const struct regcache *regcache, int regno,
>+    /* vmx registers.  */
Should be VSX ?
>+    if (tdep->ppc_vsr0_upper_regnum != -1  && tdep->ppc_vrsave_regnum != -1
>+       && (regno == -1 || (regno >= tdep->ppc_vsr0_upper_regnum
>+       && regno <= tdep->ppc_vrsave_regnum))) 

The regno comparison looks inconsistent (lower bound VSX; upper bound VMX).

>+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]));

Same loop count problem here.

>+  regcache->raw_supply (tdep->ppc_vrsave_regnum, &(vmx->__vrsave));
>+  regcache->raw_supply (tdep->ppc_vrsave_regnum - 1, &(vmx->__vscr));

>+  /*Alti-vec register.  */
Missing space
>+  /*VSX register.  */
Here as well.
 
>+  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;
>+  }
Fix coding style ({ should be indented).


Bye,
Ulrich