RE: [PATCH/MIPS] Add support Octeon's bbit instructions

["Pinski, Andrew" <Andrew.Pinski@caviumnetworks.com>]

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

On Thu, 2012-04-19 at 06:18 -0700, Maciej W. Rozycki wrote:
Hi Andrew,
> 
> >   Currently gdb does not support stepping over Octeon's bbit
> > instructions.  These instructions are branch instructions with a delay
> > slot but in the cop2 instruction area.
> > 
> > This adds the support to both mips32_next_pc and
> > mips32_instruction_has_delay_slot.
> > 
> > OK?   Built and tested on mips64-linux-gnu on an Octeon2.
> 
>  Thanks for your contribution.  Overall your change is good, but has a few 
> small problems, including but not limited to formatting.  Please make the 
> adjustments as requested below.  I realise that you may have copied from 
> or modelled your code after pieces elsewhere that have coding problems, 
> however new submissions should follow the GNU coding standard even if the 
> existing code base has problems with that.
> 
Thanks for the review, I should have been more careful about the style.  This is not my first patch to a GNU project so I know better.

Here is the updated patch with the style fixes and one extra change as I noticed itype_op (inst) was being called a few times in mips32_next_pc, I merged all of them into one variable.  And renamed is_octeon_bit_op to
is_octeon_bbit_op since the instructions are named bbit and not bit.

OK?  Build and tested on mips64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

ChangeLog:
* mips-tdep.c (is_octeon): New function.
(is_octeon_bbit_op): New function.
(mips32_next_pc): Handle Octeon's bbit instructions.
(mips32_instruction_has_delay_slot): Likewise.


> > ChangeLog:
> > * mips-tdep.c (is_octeon): New function.
> > (isocteonbitinsn): New function.
> > (mips32_next_pc): Handle Octeon's bbit insturctions.
> 
> s/insturctions/instructions/
> 
> > (mips32_instruction_has_delay_slot): Likewise.
> > 
> > Index: mips-tdep.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/mips-tdep.c,v
> > retrieving revision 1.539
> > diff -u -p -r1.539 mips-tdep.c
> > --- mips-tdep.c	10 Apr 2012 23:06:57 -0000	1.539
> > +++ mips-tdep.c	14 Apr 2012 23:33:02 -0000
> > @@ -219,6 +219,18 @@ mips_abi (struct gdbarch *gdbarch)
> >    return gdbarch_tdep (gdbarch)->mips_abi;
> >  }
> >  
> > +static int
> > +is_octeon (struct gdbarch *gdbarch, const struct bfd_arch_info *info)
> > +{
> > +  if (!info)
> > +    info = gdbarch_bfd_arch_info (gdbarch);
> > +
> > +  return (info->mach == bfd_mach_mips_octeon
> > +         || info->mach == bfd_mach_mips_octeonp
> > +         || info->mach == bfd_mach_mips_octeon2);
> > +}
> > +
> > +
> >  int
> >  mips_isa_regsize (struct gdbarch *gdbarch)
> >  {
> 
>  Where is the INFO argument supplied?  It looks to me like it can be 
> removed, please do so (if you have a follow-up change that needs it, then 
> fold it into there).
> 
>  This function doesn't seem to logically belong between mips_abi and 
> mips_isa_regsize, and is only used by isocteonbitinsn.  Please move it 
> down to immediately precede isocteonbitinsn.
> 
>  Please add a short comment before the function, explaining what it does 
> (even if it might seem obvious).
> 
> > @@ -1162,6 +1174,23 @@ mips32_bc1_pc (struct gdbarch *gdbarch, 
> >    return pc;
> >  }
> >  
> > +/* Return true if the INST is the Octeon's BBIT instruction. */
> 
>  Two spaces after the full stop.  Add an extra line between the comment 
> and the function definition.
> 
> > +static int
> > +isocteonbitinsn (int inst, struct gdbarch *gdbarch)
> 
>  The naming convention is to use underscores in function names, this would 
> have to be is_octeon_bit_insn (but see below).
> 
> > +{
> > +  int op;
> 
>  Add an extra line after the last block variable definition.
> 
> > +  if (!is_octeon (gdbarch, NULL))
> > +    return 0;
> > +  op = itype_op (inst);
> > +  /* BBIT0 is encoded as LWC2: 110 010.  */
> > +  /* BBIT032 is encoded as LDC2: 110 110.  */
> > +  /* BBIT1 is encoded as SWC2: 111 010.  */
> > +  /* BBIT132 is encoded as SDC2: 111 110.  */
> > +  if (op == 50 || op == 54 || op == 58 || op == 62)
> > +    return 1;
> > +  return 0;
> > +}
> > +
> >  /* Determine where to set a single step breakpoint while considering
> >     branch prediction.  */
> >  static CORE_ADDR
> 
>  However, it seems to me is_octeon_bit_insn would be a bit cleaner if it 
> operated on op already extracted as this is how 
> mips32_instruction_has_delay_slot and mips32_next_pc generally operate, 
> please convert it, renaming to is_octeon_bit_op.
> 
> > @@ -1213,6 +1242,21 @@ mips32_next_pc (struct frame_info *frame
> >  	  /* Add 1 to indicate 16-bit mode -- invert ISA mode.  */
> >  	  pc = ((pc + 4) & ~(CORE_ADDR) 0x0fffffff) + reg + 1;
> >  	}
> > +      else if (isocteonbitinsn (inst, gdbarch))
> > +        {
> > +          int bit, branch_if;
> 
>  Indentation, both lines -- please convert each 8 spaces to tabs.
> 
> > +	  int op = itype_op (inst);
> 
>  Insert an empty line after the last block variable definition.
> 
> > +	  branch_if = op == 58 || op == 62;
> > +	  bit = itype_rt (inst);
> > +	  if (op == 54 || op == 62)
> > +	    bit += 32;
> > +          if (((get_frame_register_signed (frame,
> > +                                           itype_rs (inst)) >> bit) & 1)
> > +              == branch_if)
> > +            pc += mips32_relative_offset (inst) + 4;
> > +          else
> > +           pc += 8;        /* after the delay slot */
> > +        }
> 
>  Please pay attention to correct comment formatting (start with a capital 
> letter, end with a full stop, followed by two spaces), preexisting 
> breakage is not an excuse.  Also indentation is botched in the last 7 
> lines -- please convert each 8 spaces to tabs (also before the comment) 
> and add a missing leading space in the second last line.
> 
> >        else
> >  	pc += 4;		/* Not a branch, next instruction is easy.  */
> >      }
> > @@ -5397,6 +5441,8 @@ mips32_instruction_has_delay_slot (struc
> >    op = itype_op (inst);
> >    if ((inst & 0xe0000000) != 0)
> >      {
> > +      if (isocteonbitinsn (inst, gdbarch))
> > +	return 1;
> >        rs = itype_rs (inst);
> >        rt = itype_rt (inst);
> >        return (op >> 2 == 5	/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx  */
> 
>  Make it:
> 
>   if (is_octeon_bit_op (op))
>     return 1;
>   else if ((inst & 0xe0000000) != 0)
> 
>   Maciej
> 
> 

[-- Attachment #2: addgdbbbit.diff.txt --]
[-- Type: text/plain, Size: 3642 bytes --]

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.539
diff -u -p -r1.539 mips-tdep.c
--- mips-tdep.c	10 Apr 2012 23:06:57 -0000	1.539
+++ mips-tdep.c	20 Apr 2012 18:50:40 -0000
@@ -1162,6 +1162,32 @@ mips32_bc1_pc (struct gdbarch *gdbarch, 
   return pc;
 }
 
+/* Return nonzero if the gdbarch is an Octeon series. */
+static int
+is_octeon (struct gdbarch *gdbarch)
+{
+  const struct bfd_arch_info *info = gdbarch_bfd_arch_info (gdbarch);
+
+  return (info->mach == bfd_mach_mips_octeon
+         || info->mach == bfd_mach_mips_octeonp
+         || info->mach == bfd_mach_mips_octeon2);
+}
+
+/* Return true if the OP represents the Octeon's BBIT instruction. */
+static int
+is_octeon_bbit_op (int op, struct gdbarch *gdbarch)
+{
+  if (!is_octeon (gdbarch))
+    return 0;
+  /* BBIT0 is encoded as LWC2: 110 010.  */
+  /* BBIT032 is encoded as LDC2: 110 110.  */
+  /* BBIT1 is encoded as SWC2: 111 010.  */
+  /* BBIT132 is encoded as SDC2: 111 110.  */
+  if (op == 50 || op == 54 || op == 58 || op == 62)
+    return 1;
+  return 0;
+}
+
 /* Determine where to set a single step breakpoint while considering
    branch prediction.  */
 static CORE_ADDR
@@ -1174,7 +1200,8 @@ mips32_next_pc (struct frame_info *frame
   if ((inst & 0xe0000000) != 0)		/* Not a special, jump or branch
 					   instruction.  */
     {
-      if (itype_op (inst) >> 2 == 5)
+      op = itype_op (inst);
+      if (op >> 2 == 5)
 	/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
 	{
 	  op = (itype_op (inst) & 0x03);
@@ -1192,18 +1219,18 @@ mips32_next_pc (struct frame_info *frame
 	      pc += 4;
 	    }
 	}
-      else if (itype_op (inst) == 17 && itype_rs (inst) == 8)
+      else if (op == 17 && itype_rs (inst) == 8)
 	/* BC1F, BC1FL, BC1T, BC1TL: 010001 01000 */
 	pc = mips32_bc1_pc (gdbarch, frame, inst, pc + 4, 1);
-      else if (itype_op (inst) == 17 && itype_rs (inst) == 9
+      else if (op == 17 && itype_rs (inst) == 9
 	       && (itype_rt (inst) & 2) == 0)
 	/* BC1ANY2F, BC1ANY2T: 010001 01001 xxx0x */
 	pc = mips32_bc1_pc (gdbarch, frame, inst, pc + 4, 2);
-      else if (itype_op (inst) == 17 && itype_rs (inst) == 10
+      else if (op == 17 && itype_rs (inst) == 10
 	       && (itype_rt (inst) & 2) == 0)
 	/* BC1ANY4F, BC1ANY4T: 010001 01010 xxx0x */
 	pc = mips32_bc1_pc (gdbarch, frame, inst, pc + 4, 4);
-      else if (itype_op (inst) == 29)
+      else if (op == 29)
 	/* JALX: 011101 */
 	/* The new PC will be alternate mode.  */
 	{
@@ -1213,6 +1240,21 @@ mips32_next_pc (struct frame_info *frame
 	  /* Add 1 to indicate 16-bit mode -- invert ISA mode.  */
 	  pc = ((pc + 4) & ~(CORE_ADDR) 0x0fffffff) + reg + 1;
 	}
+      else if (is_octeon_bbit_op (op, gdbarch))
+	{
+	  int bit, branch_if;
+
+	  branch_if = op == 58 || op == 62;
+	  bit = itype_rt (inst);
+	  if (op == 54 || op == 62)
+	    bit += 32;
+	  if (((get_frame_register_signed (frame,
+					   itype_rs (inst)) >> bit) & 1)
+              == branch_if)
+	    pc += mips32_relative_offset (inst) + 4;
+          else
+	    pc += 8;        /* After the delay slot.  */
+	}
       else
 	pc += 4;		/* Not a branch, next instruction is easy.  */
     }
@@ -5399,7 +5441,8 @@ mips32_instruction_has_delay_slot (struc
     {
       rs = itype_rs (inst);
       rt = itype_rt (inst);
-      return (op >> 2 == 5	/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx  */
+      return (is_octeon_bbit_op (op, gdbarch) 
+	      || op >> 2 == 5	/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx  */
 	      || op == 29	/* JALX: bits 011101  */
 	      || (op == 17
 		  && (rs == 8