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

["Pinski, Andrew" <Andrew dot Pinski at caviumnetworks dot com>]

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: addgdbbbit.diff.txt
Description: addgdbbbit.diff.txt