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

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

[Maciej W. Rozycki]

Hello,

 This is a change that makes breakpoints requested in a branch or jump 
delay slot be moved to the preceding instruction.  This removes a 
confusion that arises when such a breakpoint is hit and the resulting 
message like this:

(gdb) break *0x00400670
Breakpoint 1 at 0x400670: file foo.c, line 12.
(gdb) run
Starting program: .../foo

Program received signal SIGTRAP, Trace/breakpoint trap.
main (argc=1, argv=0x7fd7c874) at foo.c:12
12      }
(gdb) x /2i $pc
0x40066c <main+12>:     jr      ra
0x400670 <main+16>:     move    v0,zero

 This change has been tested natively for mips-unknown-linux-gnu and 
remotely for mipsisa32-sde-elf, using mips-sim-sde32/-EB, 
mips-sim-sde32/-mips16/-EB, mips-sim-sde32/-EL and 
mips-sim-sde32/-mips16/-EL as the targets, with no regressions.

2007-05-24  Chris Dearman  <chris@mips.com>
            Maciej W. Rozycki  <macro@mips.com>

	* 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.

 OK to apply?

  Maciej

12271-4
Index: binutils-quilt/src/gdb/mips-tdep.c
===================================================================
--- binutils-quilt.orig/src/gdb/mips-tdep.c	2007-05-23 17:50:36.000000000 +0100
+++ binutils-quilt/src/gdb/mips-tdep.c	2007-05-24 17:58:17.000000000 +0100
@@ -4817,6 +4817,186 @@
     }
 }
 
+/* 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;
+}
+
+static int
+mips16_instruction_has_delay_slot (CORE_ADDR addr, int mustbe32)
+{
+  gdb_byte buf[MIPS_INSN16_SIZE];
+  unsigned short inst;
+  int status;
+
+  status = read_memory_nobpt (addr, buf, MIPS_INSN16_SIZE);
+  if (status)
+    return 0;
+
+  inst = mips_fetch_instruction (addr);
+  if (!mustbe32)
+    return (inst & 0xf89f) == 0xe800;	/* jr/jalr (16-bit instruction)  */
+  return (inst & 0xf800) == 0x1800;	/* jal/jalx (32-bit instruction)  */
+}
+
+static CORE_ADDR
+mips_segment_boundary (CORE_ADDR bpaddr)
+{
+#ifdef BFD64
+  switch ((int)(bpaddr >> 62) & 0x3)
+    {
+    case 0x3:				/* xkseg  */
+      if (bpaddr == (bfd_signed_vma)(int)bpaddr)
+	return bpaddr & ~0x1fffffffLL;	/* 32-bit compatibility segment  */
+      break;
+    case 0x2:				/* xkphys  */
+      return bpaddr & 0xf800000000000000LL;
+    case 0x1:				/* xksseg  */
+    case 0x0:				/* xkuseg/kuseg  */
+      break;
+    }
+  return bpaddr & 0xc000000000000000LL;
+#else
+  if (bpaddr & (CORE_ADDR)0x80000000)	/* kernel segment  */
+    return bpaddr & ~(CORE_ADDR)0x1fffffff;
+  return 0x0000000;			/* user segment  */
+#endif
+}
+
+static CORE_ADDR
+mips_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
+{
+  CORE_ADDR prev_addr, next_addr;
+  CORE_ADDR boundary;
+
+  /* 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.
+
+     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. */
+
+  boundary = mips_segment_boundary (bpaddr);
+
+  if (!is_mips16_addr (bpaddr))
+    {
+      if (bpaddr == boundary)
+	return bpaddr;
+      /* If the previous instruction has a branch delay slot, we have
+         to move the breakpoint to the branch instruction. */
+      prev_addr = bpaddr - 4;
+      if (mips32_instruction_has_delay_slot (prev_addr))
+	{
+	  bpaddr = prev_addr;
+	}
+    }
+  else
+    {
+      struct minimal_symbol *sym;
+      CORE_ADDR addr, jmpaddr;
+      int i;
+
+      /* 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.  */
+
+      /* 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. :-(  */
+      if (find_pc_partial_function (bpaddr, NULL, &addr, NULL)
+	  && addr > boundary && addr < bpaddr)
+	  boundary = addr & ~1;
+
+      jmpaddr = 0;
+      addr = bpaddr;
+      for (i = 1; i < 4; i++)
+	{
+	  if ((addr & ~1) == boundary)
+	    break;
+	  addr -= 2;
+	  if (i == 1 && mips16_instruction_has_delay_slot (addr, 0))
+	    {
+	      /* Looks like a jr/jalr at [target-1], but it could be
+	         the second word of a previous jal, so record it and
+	         check back one more.  */
+	      jmpaddr = addr;
+	    }
+	  else if (i > 1 && mips16_instruction_has_delay_slot (addr, 1))
+	    {
+	      if (i == 2)
+		/* Looks like a jal at [target-2], but it could also
+		   be the second word of a previous jal, record it,
+		   and check back one more.  */
+		jmpaddr = addr;
+	      else
+		/* Looks like a jal at [target-3], so any previously
+		   recorded jal or jr must be wrong, because:
+
+		   >-3: jal
+		    -2: jal-ext (can't be jal)
+		    -1: bdslot (can't be jr)
+		     0: target insn
+
+		   Of course it could be another jal-ext which looks
+		   like a jal, but in that case we'd have broken out
+		   of this loop at [target-2]:
+
+		    -4: jal
+		   >-3: jal-ext
+		    -2: bdslot (can't be jmp)
+		    -1: jalr/jr
+		     0: target insn  */
+		jmpaddr = 0;
+	    }
+	  else
+	    {
+	      /* Not a jump instruction: if we're at [target-1] this
+	         could be the second word of a jal, so continue;
+	         otherwise we're done.  */
+	      if (i > 1)
+		break;
+	    }
+	}
+
+      if (jmpaddr)
+	bpaddr = jmpaddr;
+    }
+
+  return bpaddr;
+}
+
 /* If PC is in a mips16 call or return stub, return the address of the target
    PC, which is either the callee or the caller.  There are several
    cases which must be handled:
@@ -5514,6 +5694,8 @@
 
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, mips_breakpoint_from_pc);
+  set_gdbarch_adjust_breakpoint_address (gdbarch,
+					 mips_adjust_breakpoint_address);
 
   set_gdbarch_skip_prologue (gdbarch, mips_skip_prologue);
 

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

[Mark Kettenis]

> Date: Thu, 24 May 2007 19:07:46 +0100 (BST)
> From: "Maciej W. Rozycki" <macro@mips.com>
> 
> Hello,
> 
>  This is a change that makes breakpoints requested in a branch or jump 
> delay slot be moved to the preceding instruction.  This removes a 
> confusion that arises when such a breakpoint is hit and the resulting 
> message like this:
> 
> (gdb) break *0x00400670
> Breakpoint 1 at 0x400670: file foo.c, line 12.
> (gdb) run
> Starting program: .../foo
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> main (argc=1, argv=0x7fd7c874) at foo.c:12
> 12      }
> (gdb) x /2i $pc
> 0x40066c <main+12>:     jr      ra
> 0x400670 <main+16>:     move    v0,zero
> 
>  This change has been tested natively for mips-unknown-linux-gnu and 
> remotely for mipsisa32-sde-elf, using mips-sim-sde32/-EB, 
> mips-sim-sde32/-mips16/-EB, mips-sim-sde32/-EL and 
> mips-sim-sde32/-mips16/-EL as the targets, with no regressions.
> 
> 2007-05-24  Chris Dearman  <chris@mips.com>
>             Maciej W. Rozycki  <macro@mips.com>
> 
> 	* 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.
> 
>  OK to apply?

Do we have a testcase for this?  I also would like to test this on
OpenBSD/mips64.  Unfortunately I won't be able to test this in the
next three weeks.  Can this wait until then?

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

[Daniel Jacobowitz]

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

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

[Maciej W. Rozycki]

On Fri, 25 May 2007, Maciej W. Rozycki wrote:

>  In principle on bare iron it should be doable, but the problem is, IIRC, 
> the state of cp0.cause.bd is not propagated under Linux to userland in any 
> way.  And at the moment I'm not sure how this would affect EJTAG debugging 
> either.

 FYI, I have now checked Linux and it should be fine -- must have been my
bad memory.  EJTAG has provisions for handling this correctly
(cp0.debug.dbd), but I'm not sure if associated software gets it right.

  Maciej

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

[Maciej W. Rozycki]

On Fri, 25 May 2007, Maciej W. Rozycki wrote:

>  In principle on bare iron it should be doable, but the problem is, IIRC, 
> the state of cp0.cause.bd is not propagated under Linux to userland in any 
> way.  And at the moment I'm not sure how this would affect EJTAG debugging 
> either.

 FYI, I have now checked Linux and it should be fine -- must have been my
bad memory.  EJTAG has provisions for handling this correctly
(cp0.debug.dbd), but I'm not sure if associated software gets it right.

  Maciej

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

[Daniel Jacobowitz]

On Fri, May 25, 2007 at 01:21:57PM +0100, Maciej W. Rozycki wrote:
> On Fri, 25 May 2007, Maciej W. Rozycki wrote:
> 
> >  In principle on bare iron it should be doable, but the problem is, IIRC, 
> > the state of cp0.cause.bd is not propagated under Linux to userland in any 
> > way.  And at the moment I'm not sure how this would affect EJTAG debugging 
> > either.
> 
>  FYI, I have now checked Linux and it should be fine -- must have been my 
> bad memory.  EJTAG has provisions for handling this correctly 
> (cp0.debug.dbd), but I'm not sure if associated software gets it right.

Great.  Want to take a look at doing this the other way then?

The best option I can think of would be to fake the PC value based on
the value of BD.  You might be able to do this cleverly; have a
register named "$pc" which shows the hardware contents of the PC
register, and have read_pc / unwind_pc / write_pc adjust based on BD.
I'm not sure if that will work, but it does mimic the architecture
behavior, which is an encouraging sign; and some other platforms do
similar magic (e.g. HP/UX and IA64).

It might be best to eliminate the setting of PC_REGNUM first, but
that's multi-arch work; I see that dwarf2-frame.c relies on it unless
you set dwarf2_frame_default_init_reg.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Maciej W. Rozycki]

On Fri, 25 May 2007, Daniel Jacobowitz wrote:

> >  FYI, I have now checked Linux and it should be fine -- must have been my 
> > bad memory.  EJTAG has provisions for handling this correctly 
> > (cp0.debug.dbd), but I'm not sure if associated software gets it right.

 EJTAG seems to be getting it right in principle -- there is one corner 
case affecting us which I have identified, but it should be taken care of 
elsewhere.

> Great.  Want to take a look at doing this the other way then?

 As much as I'd like to I may not be able to do it right now.

> The best option I can think of would be to fake the PC value based on
> the value of BD.  You might be able to do this cleverly; have a
> register named "$pc" which shows the hardware contents of the PC
> register, and have read_pc / unwind_pc / write_pc adjust based on BD.

 Note that there is nothing special needed for write_pc -- if you modify 
the PC, you just write the new value to the "register" (there is actually 
no such register as the PC in the MIPS architecture -- it's merely a 
useful notion of "the next instruction to execute", which depending on the 
way a debugger accesses the processor may happen to be recorded in various 
cp0 registers).

 BD is a merely a status bit which does not affect subsequent ERET/DERET.

> I'm not sure if that will work, but it does mimic the architecture
> behavior, which is an encouraging sign; and some other platforms do
> similar magic (e.g. HP/UX and IA64).

 This is not rocket science -- the basic issue is to make gdb "accept" a 
breakpoint that signals at a different address to one it was set at.

> It might be best to eliminate the setting of PC_REGNUM first, but
> that's multi-arch work; I see that dwarf2-frame.c relies on it unless
> you set dwarf2_frame_default_init_reg.

 I am not sure what you mean here...

  Maciej

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

[Maciej W. Rozycki]

[Sorry about the mess -- the bloody MIPS mail server keeps failing today.]

On Thu, 24 May 2007, Daniel Jacobowitz 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).

 Oh yes, indeed -- I have not looked too deeply into this bit of the 
patch, sorry.

> > +  /* 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?

 Oh absolutely.  Actually the very complaint I quoted was taken from 
Linux.

> > +     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.

 In principle on bare iron it should be doable, but the problem is, IIRC, 
the state of cp0.cause.bd is not propagated under Linux to userland in any 
way.  And at the moment I'm not sure how this would affect EJTAG debugging 
either.

 I do agree the first solution would be a bit better, because in reality 
(yes I did write such code myself!) you can have control flow which jumps 
into a branch delay slot under certain circumstances (and obviously at 
times executes the preceding branch normally as well).

> 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?

 There are both kinds of jumps/branches in MIPS16e:

- jal, jalr, jalx and jr do have a delay slot,

- branches and jalrc and jrc have no delay slot (the two latters are 
  MIPS16e additions over "traditional" MIPS16).

> > +      /* 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.

 Well, if Linux and EJTAG can be done at all, I might have a look into it.

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

 I have thought a general test case would be tough, but a MIPS-specific 
one should be easy indeed.

> Thanks for the patch!

 Credit goes to Chris!

  Maciej

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

[Daniel Jacobowitz]

On Fri, May 25, 2007 at 02:20:55PM +0100, Maciej W. Rozycki wrote:
> > 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?
> 
>  There are both kinds of jumps/branches in MIPS16e:
> 
> - jal, jalr, jalx and jr do have a delay slot,
> 
> - branches and jalrc and jrc have no delay slot (the two latters are 
>   MIPS16e additions over "traditional" MIPS16).

You might want to fix mips_single_step_through_delay at some point,
then; it doesn't know about any MIPS16 instructions with delay slots.

-- 
Daniel Jacobowitz
CodeSourcery