Re: [PATCH] Enable vector instruction debugging for AIX

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

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

>So I connected with my seniors on this. So AIX folks are using 7.1 now. 
>So I have changed this to minimum 7.x.. Kindly see it in this patch. 
>For those who are below 7.1, we are not supporting. 

OK, this makes sense to me.  But it looks like you've added this
to the wrong section now, see below.

>>Fix coding style ({ should be indented).
>This is done. I have eliminated all RHS space and aligned the flower brackets wherever I saw it was not aligned.

Unfortunately it seems you changed many of these
in the wrong direction now.  GNU coding style is
to indent the { by two spaces, and then indent the
block inside by *another* two spaces:

  if (condition)
    {
      /* code */
    }

Also, the patch still introduces various instances of
8 spaces (that should be tabs), and whitespace at the
end of the line (which should be removed).


>@@ -938,6 +938,9 @@ info sources
> 
> ARM Symbian			arm*-*-symbianelf*
> 
>+GDB no longer supports AIX 4.x, AIX 5.x and AIX 6.x.  The minimum supported
>+AIX version is now AIX 7.1.
>+

You've added this under the GDB 11 news section, which is wrong.

You should instead add a new section 
  * Removed targets and native configurations
in the topmost 
  *** Changes since GDB 13
section.

>+  __vmx_context_t vmx;
>+  if (__power_vmx() && (flags & PTHDB_FLAG_REGS))
>+  {

Wrong indentation.

>+  /* vsx registers.  */
>+  __vsx_context_t vsx;
>+  if (__power_vsx() && (flags & PTHDB_FLAG_REGS))
>+  {

Likewise.

>+  /* vector registers.  */
>+  if (__power_vmx() && (flags & PTHDB_FLAG_REGS))
>+  {

Likewise.

>+  /* vsx registers.  */
>+  if (__power_vsx() && (flags & PTHDB_FLAG_REGS))
>+  {

Likewise.

>+  /* Fill altivec-registers.  */
>+
>+  if (__power_vmx())
>+  {

Likewise throughout the block

>+    memset(&vmx, 0, sizeof(__vmx_context_t));
>+    if (data->arch64)

Why is this check necessary, and what happens on 32-bit targets?

>+  /* Fill vsx registers. */
>+
>+  if (__power_vsx())
>+  {
>+    memset(&vsx, 0, sizeof(__vsx_context_t));
>+    if (data->arch64)

Same question, also same indentation issues.

>+    /* Vector registers.  */
>+    if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1
>+	&& (regno == -1 || (regno >= tdep->ppc_vr0_regnum
>+	&& regno <= tdep->ppc_vrsave_regnum)))
>+    {

Also indentation issues throughout the block.

>+    /* VSX registers.  */
>+    if (tdep->ppc_vsr0_upper_regnum != -1 && (regno == -1
>+	|| (regno >=tdep->ppc_vsr0_upper_regnum)))

You've now removed the upper limit completely.  I think this needs an 
  && regno < tdep->ppc_vsr0_upper_regnum + ppc_num_vshrs

>+  memset(&vsx, 0, sizeof(__vsx_context_t));
>+  if (__power_vsx() && thrd_i > 0)
>+  {

Indentation.

>+  memset(&vmx, 0, sizeof(__vmx_context_t));
>+  if (__power_vmx() && thrd_i > 0)
>+  {

Likewise.

>+  memset(&vmx, 0, sizeof(__vmx_context_t));
>+  if (__power_vmx() && thrd_i > 0)
>+  {

Likewise.

>+  memset(&vsx, 0, sizeof(__vsx_context_t));
>+  if (__power_vsx() && thrd_i > 0)
>+  {

Likewise.

>+  /* Alti-vec register.  */
>+  if (altivec_register_p (gdbarch, regno))
>+  {
>+    fetch_altivec_registers_aix (regcache);
>+    return;
>+  }
>+
>+  /* VSX register.  */
>+  if (vsx_register_p (gdbarch, regno))
>+  {
>+    fetch_vsx_registers_aix (regcache);
>+    return;
>+  }

Likewise.

>+  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;
>+  }

Likewise.

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

Likewise.

>+  if (regnum == -1)
>+  {

Likewise.

>+  if (regnum == -1)
>+  {

Likewise.

>+  if (regnum == -1)
>+  {

Likewise.

>+  if (regnum == -1)
>+  {

Likewise.

Bye,
Ulrich