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

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

[Andrew Pinski]

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

Hi,
  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,
Andrew Pinski

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

[-- Attachment #2: addgdbbbit.diff.txt --]
[-- Type: text/plain, Size: 2523 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	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)
 {
@@ -1162,6 +1174,23 @@ mips32_bc1_pc (struct gdbarch *gdbarch, 
   return pc;
 }
 
+/* Return true if the INST is the Octeon's BBIT instruction. */
+static int
+isocteonbitinsn (int inst, struct gdbarch *gdbarch)
+{
+  int op;
+  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
@@ -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;
+	  int op = itype_op (inst);
+	  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.  */
     }
@@ -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  */

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

[Maciej W. Rozycki]

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.

> 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

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

[Pinski, Andrew]

[-- 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

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

[Joel Brobecker]

Hi Andrew,

This is a superficial review, I was just glancing at the diff
for no real reason when I spotted a couple of little nits. They
may be specific to the GDB project...

> +/* Return nonzero if the gdbarch is an Octeon series. */
> +static int
> +is_octeon (struct gdbarch *gdbarch)

We'd like to have one empty line between the function documentation
and its declaration.  Also, 2 spaces after the period :-).

-- 
Joel

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

[Maciej W. Rozycki]

On Fri, 20 Apr 2012, Pinski, Andrew wrote:

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

 Thanks for doing this, I meant to do such a change as the next step.  
However I'd prefer functionally separate changes to be made as separate 
commits, so please split this change into two, first that eliminates the 
repetitive itype_op (inst) operations, and second that adds your new 
feature.

>  And renamed is_octeon_bit_op to 
> is_octeon_bbit_op since the instructions are named bbit and not bit.

 Thanks, that looks reasonable to me.  Please also take into account my 
previous comment about function documentation that Joel has been kind 
enough to reiterate.

 OK with these changes.

  Maciej

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

[Andrew Pinski]

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

On Tue, Apr 24, 2012 at 12:27 PM, Maciej W. Rozycki
<macro@codesourcery.com> wrote:
> On Fri, 20 Apr 2012, Pinski, Andrew wrote:
>
>> 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.
>
>  Thanks for doing this, I meant to do such a change as the next step.
> However I'd prefer functionally separate changes to be made as separate
> commits, so please split this change into two, first that eliminates the
> repetitive itype_op (inst) operations, and second that adds your new
> feature.
>
>>  And renamed is_octeon_bit_op to
>> is_octeon_bbit_op since the instructions are named bbit and not bit.
>
>  Thanks, that looks reasonable to me.  Please also take into account my
> previous comment about function documentation that Joel has been kind
> enough to reiterate.
>
>  OK with these changes.

These are the three patches I committed in the end.  I also added a
testcase for testing bbit.

Thanks,
Andrew Pinski

Patch 1:
ChangeLog:
* mips-tdep.c (mips32_next_pc): Consolidate calls to itype_op.

Patch 2:
ChangeLog:
* mips-tdep.c (mips32_next_pc): Fix line spacing of the comment
before the function.

Patch 3:
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.

testsuite/ChangeLog:
* gdb.arch/mips-octeon-bbit.c: New file.
* gdb.arch/mips-octeon-bbit.exp: New Test.

[-- Attachment #2: 0001-mips-tdep.c-mips32_next_pc-Consolidate-calls-to-ityp.patch --]
[-- Type: text/x-patch, Size: 2356 bytes --]

From a90e6c9cac622722a0dc046a19ab63e8fc23d9fe Mon Sep 17 00:00:00 2001
From: Andrew Pinski <apinski@cavium.com>
Date: Sun, 19 Aug 2012 11:24:48 -0700
Subject: [PATCH 1/3] 	* mips-tdep.c (mips32_next_pc): Consolidate calls to itype_op.

---
 gdb/mips-tdep.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index a001424..07ae405 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -1475,14 +1475,14 @@ mips32_next_pc (struct frame_info *frame, CORE_ADDR pc)
   unsigned long inst;
   int op;
   inst = mips_fetch_instruction (gdbarch, ISA_MIPS, pc, NULL);
+  op = itype_op (inst);
   if ((inst & 0xe0000000) != 0)		/* Not a special, jump or branch
 					   instruction.  */
     {
-      if (itype_op (inst) >> 2 == 5)
+      if (op >> 2 == 5)
 	/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
 	{
-	  op = (itype_op (inst) & 0x03);
-	  switch (op)
+	  switch (op & 0x03)
 	    {
 	    case 0:		/* BEQL */
 	      goto equal_branch;
@@ -1496,18 +1496,18 @@ mips32_next_pc (struct frame_info *frame, CORE_ADDR pc)
 	      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.  */
 	{
@@ -1524,7 +1524,7 @@ mips32_next_pc (struct frame_info *frame, CORE_ADDR pc)
     {				/* This gets way messy.  */
 
       /* Further subdivide into SPECIAL, REGIMM and other.  */
-      switch (op = itype_op (inst) & 0x07)	/* Extract bits 28,27,26.  */
+      switch (op & 0x07)	/* Extract bits 28,27,26.  */
 	{
 	case 0:		/* SPECIAL */
 	  op = rtype_funct (inst);
-- 
1.7.2.5


[-- Attachment #3: 0002-mips-tdep.c-mips32_next_pc-Fix-line-spacing-of-the-c.patch --]
[-- Type: text/x-patch, Size: 742 bytes --]

From e62b978677e9eab70b71a49f395aabcca2fae109 Mon Sep 17 00:00:00 2001
From: Andrew Pinski <apinski@cavium.com>
Date: Sun, 19 Aug 2012 11:57:39 -0700
Subject: [PATCH 2/3] 	* mips-tdep.c (mips32_next_pc): Fix line spacing of the comment
 	before the function.

---
 gdb/mips-tdep.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 07ae405..751d7d6 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -1468,6 +1468,7 @@ mips32_bc1_pc (struct gdbarch *gdbarch, struct frame_info *frame,
 
 /* Determine where to set a single step breakpoint while considering
    branch prediction.  */
+
 static CORE_ADDR
 mips32_next_pc (struct frame_info *frame, CORE_ADDR pc)
 {
-- 
1.7.2.5


[-- Attachment #4: 0003-mips-tdep.c-is_octeon-New-function.patch --]
[-- Type: text/x-patch, Size: 7892 bytes --]

From 2a46d4e5ea7349ffe1deabee65379bfebf4ece46 Mon Sep 17 00:00:00 2001
From: Andrew Pinski <apinski@cavium.com>
Date: Sun, 19 Aug 2012 12:05:56 -0700
Subject: [PATCH 3/3] 	* 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.

	* gdb.arch/mips-octeon-bbit.c: New file.
	* gdb.arch/mips-octeon-bbit.exp: New Test.
---
 gdb/mips-tdep.c                             |   51 ++++++++++++-
 gdb/testsuite/gdb.arch/mips-octeon-bbit.c   |   49 ++++++++++++
 gdb/testsuite/gdb.arch/mips-octeon-bbit.exp |  112 +++++++++++++++++++++++++++
 3 files changed, 211 insertions(+), 1 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/mips-octeon-bbit.c
 create mode 100644 gdb/testsuite/gdb.arch/mips-octeon-bbit.exp

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 751d7d6..56fa56c 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -1466,6 +1466,35 @@ mips32_bc1_pc (struct gdbarch *gdbarch, struct frame_info *frame,
   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.  */
 
@@ -1518,6 +1547,25 @@ mips32_next_pc (struct frame_info *frame, CORE_ADDR pc)
 	  /* 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);
+
+	  /* Take into account the *32 instructions.  */
+	  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.  */
     }
@@ -6947,7 +6995,8 @@ mips32_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr)
     {
       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
diff --git a/gdb/testsuite/gdb.arch/mips-octeon-bbit.c b/gdb/testsuite/gdb.arch/mips-octeon-bbit.c
new file mode 100644
index 0000000..e3df4a4
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/mips-octeon-bbit.c
@@ -0,0 +1,49 @@
+typedef unsigned long long uint64_t;
+void abort (void);
+
+#define BASE 0x1234567812345678ull
+
+#define DEF_BBIT_TAKEN(BRANCH_IF, BIT)					\
+  int bbit_is_taken_##BRANCH_IF##_##BIT (volatile uint64_t *p)		\
+  {									\
+    int ret;								\
+    asm (".set push				\n\t"			\
+	 ".set noreorder			\n\t"			\
+	 "bbit" #BRANCH_IF " %1, " #BIT ", 1f	\n\t"			\
+	 "nop					\n\t"			\
+	 "li %0, 0				\n\t"			\
+	 "b 2f					\n\t"			\
+	 "nop					\n\t"			\
+	 "1:					\n\t"			\
+	 "li %0, 1				\n\t"			\
+	 "2:					\n\t"			\
+	 ".set pop"							\
+	 : "=r"(ret) : "r"(*p));					\
+    return ret;								\
+  }									\
+  volatile uint64_t taken_##BRANCH_IF##_##BIT =				\
+    BASE & (~(1ull << BIT)) | ((uint64_t) BRANCH_IF << BIT);		\
+  volatile uint64_t not_taken_##BRANCH_IF##_##BIT =			\
+    BASE & (~(1ull << BIT)) | (((uint64_t) !BRANCH_IF) << BIT);  
+
+DEF_BBIT_TAKEN (0, 10);
+DEF_BBIT_TAKEN (0, 36);
+DEF_BBIT_TAKEN (1, 20);
+DEF_BBIT_TAKEN (1, 49);
+
+#define EXPECT(X) if (!(X)) abort ();
+
+main ()
+{
+  EXPECT (bbit_is_taken_0_10 (&taken_0_10));
+  EXPECT (!bbit_is_taken_0_10 (&not_taken_0_10));
+
+  EXPECT (bbit_is_taken_0_36 (&taken_0_36));
+  EXPECT (!bbit_is_taken_0_36 (&not_taken_0_36));
+
+  EXPECT (bbit_is_taken_1_20 (&taken_1_20));
+  EXPECT (!bbit_is_taken_1_20 (&not_taken_1_20));
+
+  EXPECT (bbit_is_taken_1_49 (&taken_1_49));
+  EXPECT (!bbit_is_taken_1_49 (&not_taken_1_49));
+}
diff --git a/gdb/testsuite/gdb.arch/mips-octeon-bbit.exp b/gdb/testsuite/gdb.arch/mips-octeon-bbit.exp
new file mode 100644
index 0000000..de6db0f
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/mips-octeon-bbit.exp
@@ -0,0 +1,112 @@
+# Copyright 2007 Cavium Networks, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+# Test single-step on bbit.  
+
+if ![istarget "*octeon*"] {
+  return -1
+}
+
+proc current_insn {} {
+    global gdb_prompt
+
+    send_gdb "x/i \$pc\n"
+    gdb_expect {
+	-re ".*?:\\s+\(.*?\)\\s*$gdb_prompt $" {
+	    set insn $expect_out(1,string)
+	    return $insn
+	}
+    }
+    return ""
+}
+
+proc single_step {} {
+    global gdb_prompt
+
+    send_gdb "si\n"
+    gdb_expect {
+	-re "$gdb_prompt \$" {
+	    return 1
+	}
+    }
+    return 0;
+}
+
+proc single_step_until { match } {
+    global timeout
+
+    set insn [current_insn]
+    set start [timestamp]
+    while { $insn != "" && [timestamp] - $start < 3*$timeout } {
+	if [regexp $match $insn] {
+	    return 1
+	}
+	if {![single_step]} {
+	    return 0
+	}
+	set insn [current_insn]
+    }
+    return 0;
+}
+
+proc test_bbit { name taken } {
+    if {![single_step_until "bbit"]} {
+	fail "$name single-step until bbit"
+	return
+    }
+    pass "$name single-step until bbit"
+    gdb_test "si" "" "$name single-step on bbit"
+    if [regexp "li\\s+\[sv\]0,$taken" [current_insn]] {
+	pass "$name check insn after bbit"
+    } else {
+	send_log "expected: li\\s+\[sv\]0,$taken found [current_insn]\n"
+	fail "$name check insn after bbit"
+    }
+}
+
+set testfile "mips-octeon-bbit"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
+       {debug nowarnings}] != "" } {
+     fail "compilation"
+     return
+}
+
+pass "compilation"
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+# Native needs run.
+runto_main
+
+set tests ""
+foreach n [list "0_10" "0_36" "1_20" "1_49"] {
+    lappend tests "bbit_is_taken_$n"
+}
+foreach func $tests {
+    gdb_test "break $func" "Breakpoint.*at.*" "set breakpoint on $func"
+}
+
+foreach func $tests {
+    gdb_test "continue" "Continuing.*Breakpoint.*$func.*" "hit $func first"
+    test_bbit "$func branch taken" 1
+    gdb_test "continue" "Continuing.*Breakpoint.*$func.*" "hit $func second"
+    test_bbit "$func branch not taken" 0
+}
-- 
1.7.2.5

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Pinski <andrew.pinski@caviumnetworks.com> writes:

I know this is in already, but at least for future reference (or now if
you don't mind changing it)...

Andrew> +set testfile "mips-octeon-bbit"
Andrew> +set srcfile ${testfile}.c
Andrew> +set binfile ${objdir}/${subdir}/${testfile}

Use standard_testfile.c

Andrew> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
Andrew> +       {debug nowarnings}] != "" } {
Andrew> +     fail "compilation"
Andrew> +     return
Andrew> +}
Andrew> +
Andrew> +pass "compilation"
Andrew> +
Andrew> +gdb_exit
Andrew> +gdb_start
Andrew> +gdb_reinitialize_dir $srcdir/$subdir
Andrew> +gdb_load ${binfile}

All this can be replaced with prepare_for_testing.

If you want, I can write a patch, if you would test it.

thanks,
Tom

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

[Andrew Pinski]

On Thu, 2012-08-23 at 10:27 -0600, Tom Tromey wrote:
> >>>>> "Andrew" == Andrew Pinski <andrew.pinski@caviumnetworks.com> writes:
> 
> I know this is in already, but at least for future reference (or now if
> you don't mind changing it)...

Thanks, I will remember this for next time.

> 
> Andrew> +set testfile "mips-octeon-bbit"
> Andrew> +set srcfile ${testfile}.c
> Andrew> +set binfile ${objdir}/${subdir}/${testfile}
> 
> Use standard_testfile.c
> 
> Andrew> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
> Andrew> +       {debug nowarnings}] != "" } {
> Andrew> +     fail "compilation"
> Andrew> +     return
> Andrew> +}
> Andrew> +
> Andrew> +pass "compilation"
> Andrew> +
> Andrew> +gdb_exit
> Andrew> +gdb_start
> Andrew> +gdb_reinitialize_dir $srcdir/$subdir
> Andrew> +gdb_load ${binfile}
> 
> All this can be replaced with prepare_for_testing.
> 
> If you want, I can write a patch, if you would test it.

Yes please write the patch and I will test it.  


Thanks,
Andrew Pinski

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

[Tom Tromey]

Andrew> Yes please write the patch and I will test it.  

Give this a try.

Also, I think the copyright info in this file is wrong.
It says copyright Cavium Networks -- but presumably it was assigned.
And it uses GPL v2, not v3.

Tom

diff --git a/gdb/testsuite/gdb.arch/mips-octeon-bbit.exp b/gdb/testsuite/gdb.arch/mips-octeon-bbit.exp
index de6db0f..096c56f 100644
--- a/gdb/testsuite/gdb.arch/mips-octeon-bbit.exp
+++ b/gdb/testsuite/gdb.arch/mips-octeon-bbit.exp
@@ -77,22 +77,13 @@ proc test_bbit { name taken } {
     }
 }
 
-set testfile "mips-octeon-bbit"
-set srcfile ${testfile}.c
-set binfile ${objdir}/${subdir}/${testfile}
+standard_testfile .c
 
-if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
-       {debug nowarnings}] != "" } {
-     fail "compilation"
-     return
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+	  {debug nowarnings}] } {
+    return -1
 }
 
-pass "compilation"
-
-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-gdb_load ${binfile}
 # Native needs run.
 runto_main