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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

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