Re: mips-tdep.c: Adjust breakpoints in branch delay slots

[Daniel Jacobowitz <drow@false.org>]

On Thu, May 24, 2007 at 07:07:46PM +0100, Maciej W. Rozycki wrote:
> +/* Return non-zero if the ADDR instruction has a branch delay slot
> +   (i.e. it is a jump or branch instruction).  This function was based
> +   on mips32_next_pc().  */
> +
> +static int
> +mips32_instruction_has_delay_slot (CORE_ADDR addr)
> +{
> +  gdb_byte buf[MIPS_INSN32_SIZE];
> +  int status;
> +
> +  status = read_memory_nobpt (addr, buf, MIPS_INSN32_SIZE);
> +  return !status && mips32_next_pc (addr) != addr + 4;
> +}

That's not what you want.  mips32_next_pc calls read_register to
figure out if the branch condition is true, and we don't have any idea
yet (and might not be connected to a target).

> +  /* If a breakpoint is set on the instruction in a branch delay slot,
> +     GDB gets confused.  When the breakpoint is hit, the PC isn't on
> +     the instruction in the branch delay slot, the PC will point to
> +     the branch instruction.  Since the PC doesn't match any known
> +     breakpoints, GDB reports a trap exception.

Just checking, but does this happen in a userspace environment like
Linux too?

> +     There are two possible fixes for this problem.
> +
> +     1) When the breakpoint gets hit, see if the BD bit is set in the
> +     Cause register (which indicates the last exception occurred in a
> +     branch delay slot).  If the BD bit is set, fix the PC to point to
> +     the instruction in the branch delay slot.
> +
> +     2) When the user sets the breakpoint, don't allow him to set the
> +     breakpoint on the instruction in the branch delay slot.  Instead
> +     move the breakpoint to the branch instruction (which will have
> +     the same result).
> +
> +     The problem with the first solution is that if the user then
> +     single-steps the processor, the branch instruction will get
> +     skipped (since GDB thinks the PC is on the instruction in the
> +     branch delay slot).
> +
> +     So, we'll use the second solution.  To do this we need to know if
> +     the instruction we're trying to set the breakpoint on is in the
> +     branch delay slot. */

Abstractly, which of these solutions would you say was better?  I bet
we could make the first one work if we tried hard enough.  One way
would be to fake the PC, but that's pretty gross.  Still, it might be
possible if it was worthwhile.

BTW, see mips_single_step_through_delay, which could probably be
improved.  Doesn't its comment about MIPS16 having no delay slots
disagree with this patch?  Which is right?

> +      /* Make sure we don't scan back before the beginning of the
> +         current function, since we may fetch constant data or MIPS32
> +         insns that looks like a MIPS16 jump.  Of course we might do
> +         that anyway if the compiler has moved constants inline. :-(  */

This can happen for MIPS32 too at the start of a function.  All in all
I'm a little bit nervous about this adjustment, since it will fail
very confusingly if you get unlucky with constant pool data.  That's
why I was asking about the alternative solution above.

Oh, and I wholeheartedly agree with Mark about testcases.  It
shouldn't be hard to add this to gdb.arch/.

Thanks for the patch!

-- 
Daniel Jacobowitz
CodeSourcery