[PATCH/MIPS] Add support Octeon's bbit instructions
Pinski, Andrew
Andrew.Pinski@caviumnetworks.com
Fri Apr 20 22:56:00 GMT 2012
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
>
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: addgdbbbit.diff.txt
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20120420/61b53ca0/attachment.txt>
More information about the Gdb-patches
mailing list