[PATCH] MIPS: Avoid breakpoints in branch delay slots

[Joel Brobecker]

> 2011-11-22  Chris Dearman  <chris@mips.com>
>             Nathan Froyd  <froydnj@codesourcery.com>
>             Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gdb/
> 	* mips-tdep.c (mips32_instruction_has_delay_slot): New function.
> 	(mips16_instruction_has_delay_slot): Likewise.
> 	(mips_segment_boundary): Likewise.
> 	(mips_adjust_breakpoint_address): Likewise.
> 	(mips_gdbarch_init): Use mips_adjust_breakpoint_address.

Just a few minor comments.

> +    return (op >> 2 == 5)	/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
> +	   || (op == 29)	/* JALX: bits 011101 */
> +	   || (op == 17 && itype_rs (inst) == 8);
> +				/* BC1F, BC1FL, BC1T, BC1TL: 010001 01000 */

In this case, can you add a pair of parentheses around the return
expression.  I know that are useless, but it is part of our coding
standard, because they help formatting tools format the expression
correctly.  Thus:

    return ((op >> 2 == 5)       /* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
            || (op == 29)        /* JALX: bits 011101 */
            || (op == 17 && itype_rs (inst) == 8));
                                 /* BC1F, BC1FL, BC1T, BC1TL: 010001 01000 */
> +	  return (op == 8)	/* JR */
> +		 || (op == 9);	/* JALR */

Same here, please.

> +static int
> +mips16_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr,
> +				   int mustbe32)

This function needs an introductory comment. In particular, mustbe32...

> +static CORE_ADDR
> +mips_segment_boundary (CORE_ADDR bpaddr)
> +{
> +#ifdef BFD64

Function needs an introductory comment. All functions do, in fact, so
can you fix them all?

Why are the codes different if BFD64 is defined or not? That doesn't
look right to me. Can you explain? Isn't there another dynamic property
in the target that we can use for that?

> +      if (mips32_instruction_has_delay_slot (gdbarch, prev_addr))
> +	{
> +	  bpaddr = prev_addr;
> +	}

Extraneous curly braces. In GDB, for one-statement if blocks, we
prefer:

        if (mips32_instruction_has_delay_slot (gdbarch, prev_addr))
          bpaddr = prev_addr;

Same elsewhere below. Can you fix them all?

> +      /* The only MIPS16 instructions with delay slots are jal, jalr
> +         and jr.  An absolute jal is always 4 bytes long, so try for
> +         that first, then try the 2 byte jalr/jal.
> +         XXX We have to assume that bpaddr is not the second half of
> +         an extended instruction.  */

I don't think there is a rule against that, but let's not use XXX.
Please use FIXME (I personally also like to put my name and date,
to have it there, but optional). However, before letting a FIXME in,
I'd like to understand what the extent of the problem is, and why
it's not possible to fix it now.

-- 
Joel