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

[Andrew Pinski <andrew dot pinski at caviumnetworks dot com>]

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.

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

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

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