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

Pinski, Andrew
2012-04-20T21:28:03Z

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