[PATCH] MIPS: Avoid breakpoints in branch delay slots

[PATCH] MIPS: Avoid breakpoints in branch delay slots

[Maciej W. Rozycki]

Hi,

 This change makes GDB avoid placing breakpoints in a branch delay slot.  
The rationale for this change is explained within the patch itself in the 
comment within the mips_adjust_breakpoint_address's body, so I won't 
repeat it here; see below.

 Regression tested successfully for mips-sde-elf (MIPS32 and MIPS16 
multilibs, big- and little-endian each) and mips-linux-gnu.  OK to apply?

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.

  Maciej

gdb-mips-breakpoint-adjust.diff
Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c	2011-11-23 02:48:41.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c	2011-11-23 12:51:00.525461787 +0000
@@ -5351,6 +5351,223 @@ mips_breakpoint_from_pc (struct gdbarch 
     }
 }
 
+/* Return non-zero if the ADDR instruction has a branch delay slot
+   (i.e. it is a jump or branch instruction).  This function is based
+   on mips32_next_pc.  */
+
+static int
+mips32_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  gdb_byte buf[MIPS_INSN32_SIZE];
+  unsigned long inst;
+  int status;
+  int op;
+
+  status = target_read_memory (addr, buf, MIPS_INSN32_SIZE);
+  if (status)
+    return 0;
+
+  inst = mips_fetch_instruction (gdbarch, addr);
+  op = itype_op (inst);
+  if ((inst & 0xe0000000) != 0)
+    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 */
+  else
+    {
+      switch (op & 0x07)	/* extract bits 28,27,26 */
+	{
+	case 0:			/* SPECIAL */
+	  op = rtype_funct (inst);
+	  return (op == 8)	/* JR */
+		 || (op == 9);	/* JALR */
+	  break;		/* end SPECIAL */
+	case 1:			/* REGIMM */
+	  op = itype_rt (inst);	/* branch condition */
+	  return (op & 0xc) == 0;
+				/* BLTZ, BLTZL, BGEZ, BGEZL: bits 000xx */
+				/* BLTZAL, BLTZALL, BGEZAL, BGEZALL: 100xx */
+	  break;		/* end REGIMM */
+	default:		/* J, JAL, BEQ, BNE, BLEZ, BGTZ */
+	  return 1;
+	  break;
+	}
+    }
+}
+
+static int
+mips16_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr,
+				   int mustbe32)
+{
+  gdb_byte buf[MIPS_INSN16_SIZE];
+  unsigned short inst;
+  int status;
+
+  status = target_read_memory (addr, buf, MIPS_INSN16_SIZE);
+  if (status)
+    return 0;
+
+  inst = mips_fetch_instruction (gdbarch, 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 0x00000000;			/* 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;
+  CORE_ADDR func_addr;
+
+  /* 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);
+
+  /* Make sure we don't scan back before the beginning of the current
+     function, since we may fetch constant data or insns that look like
+     a jump.  Of course we might do that anyway if the compiler has
+     moved constants inline. :-(  */
+  if (find_pc_partial_function (bpaddr, NULL, &func_addr, NULL)
+      && func_addr > boundary && func_addr <= bpaddr)
+    boundary = func_addr;
+
+  if (!mips_pc_is_mips16 (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 (gdbarch, prev_addr))
+	{
+	  bpaddr = prev_addr;
+	}
+    }
+  else
+    {
+      struct minimal_symbol *sym;
+      CORE_ADDR addr, jmpaddr;
+      int i;
+
+      boundary = unmake_mips16_addr (boundary);
+
+      /* 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.  */
+
+      jmpaddr = 0;
+      addr = bpaddr;
+      for (i = 1; i < 4; i++)
+	{
+	  if (unmake_mips16_addr (addr) == boundary)
+	    break;
+	  addr -= 2;
+	  if (i == 1 && mips16_instruction_has_delay_slot (gdbarch, 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 (gdbarch, 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:
@@ -6337,6 +6554,8 @@ mips_gdbarch_init (struct gdbarch_info i
 
   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: [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

Re: [PATCH] MIPS: Avoid breakpoints in branch delay slots

[Maciej W. Rozycki]

Hi Joel,

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

 Always welcome! :)

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

 Yes, I know, sorry about this.  These are pretty damn long-lived patches, 
comprising a mixture of very old code and some newer updates, and while I 
did try to catch all such nits while looking through them before posting, 
I must have obviously missed some places.  It's easier to write code 
following the standard from start than to chase such discrepancies later 
on, sigh...

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

 I'll look through all the pieces again and see if I haven't missed 
anything.  In this case I removed some redundant brackets too.

> > +static int
> > +mips16_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr,
> > +				   int mustbe32)
> 
> This function needs an introductory comment. In particular, mustbe32...

 Sure.

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

 Yes, I'll look through for any missing too.

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

 Well, 64-bit immediates may be truncated, unless we have 64-bit BFD 
support, due to the lack of a 64-bit integer data type I believe.  Please 
straighten me out if I am wrong though.

 Perhaps we could get away by using "sizeof (bpaddr)" instead, but I'm not 
sure if the compiler is not going to bail out on the truncation of 
out-of-range literals even if they appear in pieces of code that are 
effectively "if (0)".  Let me cook some code like this and we shall see.

 Perhaps we should actually think of phasing out pre-C9x compilers that 
lack the long long type (or any 64-bit integer equivalent) on 32-bit 
platforms.  That would help elsewhere too; in particular what I have in 
mind are some limitations in libopcodes that we recently hit with the 
addition of microMIPS instruction set support.

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

 Yes, of course.  I have removed unnecessary curly braces around a switch 
statement earlier on too.

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

 OK.

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

 Oh, actually that comment is not accurate either -- the JALX instruction 
also has a delay slot, so I'll mention it here too 
(mips16_instruction_has_delay_slot handles it correctly already).  The 
comment is by Chris and from 2004-09-27 it would seem, umm...

 As to bpaddr pointing to the second half of an extended instruction -- 
if you specify breakpoint by address, e.g.:

(gdb) break *0xdeadbeef

then I guess there is no way to determine if that address points to the 
beginning or the middle of an instruction (and of course, if software 
breakpoints are used, then executing this patched instruction will make 
the program go astray rather than hitting the breakpoint).  So I'm not 
quite sure if that's a FIXME or something we need to live with.

 Long-term it might be a good idea to investigate if proper support for 
breakpoints in delay slots can be implemented instead.  The problem is not 
all debug stubs provide the necessary information -- a breakpoint in a 
delay slot can be hit as a result of falling through from the branch (in 
which case the branch has to be reexecuted or emulated upon resumption) or 
after a jump into the delay slot from elsewhere (in which case no special 
treatment must be applied).

 The two cases can be told apart by examining status, either the BD bit of 
the CP0 Cause register in OS debugging, or the DBD bit of the CP0 Debug 
register in JTAG debugging, or in a target-specific manner in the case of 
a simulator (some simulators do not maintain/expose full CP0 state at all) 
-- and even if that's available within the target itself, the remote debug 
stub or the OS may not expose the information to the debugger.

 Anyway, I believe I have addressed all your concerns now.

 I have rewritten mips_segment_boundary to avoid long long literals too -- 
at least x86 GCC version I use seems to produce reasonable code after the 
change, expanding all the shifts to inline masks as with original code. 

 I'm still feeling a bit uneasy about the double cast used to sign-extend 
bpaddr from bit #31, but I was unable to find another way of coding it 
that GCC would compile to decent code.  At least I replaced int with 
explicit int32_t.

 So far so good, but then I actually went as far as to force 32-bit BFD to 
check all is right -- it's always 64-bit with the configurations I use, 
but there are some MIPS targets that are not necessarily such.  That 
confirmed my concerns with a:

mips-tdep.c:5503: error: right shift count >= width of type

warning that -Werror converted to fatal.

 I agree that #ifdefs scattered throughout code are ugly and best avoided, 
so I have rewritten this piece so as to derive the shift count from the 
width of the type.  The result may not be the prettiest piece of code I 
have ever seen, but there you go.  As I say, it may be more reasonable to 
consider deprecating pre-C9x compilers, or at least those that do not 
provide a "long long" alternative.

 Here's the result.  Any further comments?

  Maciej

2011-12-07  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.

gdb-mips-breakpoint-adjust.diff
Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c	2011-12-07 00:24:01.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c	2011-12-07 18:23:48.725620992 +0000
@@ -5353,6 +5353,234 @@ mips_breakpoint_from_pc (struct gdbarch 
     }
 }
 
+/* Return non-zero if the ADDR instruction has a branch delay slot
+   (i.e. it is a jump or branch instruction).  This function is based
+   on mips32_next_pc.  */
+
+static int
+mips32_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  gdb_byte buf[MIPS_INSN32_SIZE];
+  unsigned long inst;
+  int status;
+  int op;
+
+  status = target_read_memory (addr, buf, MIPS_INSN32_SIZE);
+  if (status)
+    return 0;
+
+  inst = mips_fetch_instruction (gdbarch, addr);
+  op = itype_op (inst);
+  if ((inst & 0xe0000000) != 0)
+    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  */
+  else
+    switch (op & 0x07)		/* extract bits 28,27,26  */
+      {
+      case 0:			/* SPECIAL  */
+	op = rtype_funct (inst);
+	return (op == 8		/* JR  */
+		|| op == 9);	/* JALR  */
+	break;			/* end SPECIAL  */
+      case 1:			/* REGIMM  */
+	op = itype_rt (inst);	/* branch condition  */
+	return (op & 0xc) == 0;
+				/* BLTZ, BLTZL, BGEZ, BGEZL: bits 000xx  */
+				/* BLTZAL, BLTZALL, BGEZAL, BGEZALL: 100xx  */
+	break;			/* end REGIMM  */
+      default:			/* J, JAL, BEQ, BNE, BLEZ, BGTZ  */
+	return 1;
+	break;
+      }
+}
+
+/* Return non-zero if the ADDR instruction, which must be a 32-bit
+   instruction if MUSTBE32 is set or can be any instruction otherwise,
+   has a branch delay slot (i.e. it is a non-compact jump instruction).  */
+
+static int
+mips16_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr,
+				   int mustbe32)
+{
+  gdb_byte buf[MIPS_INSN16_SIZE];
+  unsigned short inst;
+  int status;
+
+  status = target_read_memory (addr, buf, MIPS_INSN16_SIZE);
+  if (status)
+    return 0;
+
+  inst = mips_fetch_instruction (gdbarch, addr);
+  if (!mustbe32)
+    return (inst & 0xf89f) == 0xe800;	/* JR/JALR (16-bit instruction)  */
+  return (inst & 0xf800) == 0x1800;	/* JAL/JALX (32-bit instruction)  */
+}
+
+/* Calculate the starting address of the MIPS memory segment BPADDR is in.
+   This assumes KSSEG exists.  */
+
+static CORE_ADDR
+mips_segment_boundary (CORE_ADDR bpaddr)
+{
+  CORE_ADDR mask = CORE_ADDR_MAX;
+  int segsize;
+
+  if (sizeof (CORE_ADDR) == 8)
+    /* Get the topmost two bits of bpaddr in a 32-bit safe manner.  */
+    switch (bpaddr >> ((sizeof (CORE_ADDR) << 3) - 2) & 3)
+      {
+      case 3:
+	if (bpaddr == (bfd_signed_vma) (int32_t) bpaddr)
+	  segsize = 29;			/* 32-bit compatibility segment  */
+	else
+	  segsize = 62;			/* xkseg  */
+	break;
+      case 2:				/* xkphys  */
+	segsize = 59;
+	break;
+      default:				/* xksseg (1), xkuseg/kuseg (0)  */
+	segsize = 62;
+	break;
+      }
+  else if (bpaddr & 0x80000000)		/* kernel segment  */
+    segsize = 29;
+  else
+    segsize = 31;			/* user segment  */
+  mask <<= segsize;
+  return bpaddr & mask;
+}
+
+/* Move the breakpoint at BPADDR out of any branch delay slot by shifting
+   it backwards if necessary.  Return the address of the new location.  */
+
+static CORE_ADDR
+mips_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
+{
+  CORE_ADDR prev_addr, next_addr;
+  CORE_ADDR boundary;
+  CORE_ADDR func_addr;
+
+  /* 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);
+
+  /* Make sure we don't scan back before the beginning of the current
+     function, since we may fetch constant data or insns that look like
+     a jump.  Of course we might do that anyway if the compiler has
+     moved constants inline. :-(  */
+  if (find_pc_partial_function (bpaddr, NULL, &func_addr, NULL)
+      && func_addr > boundary && func_addr <= bpaddr)
+    boundary = func_addr;
+
+  if (!mips_pc_is_mips16 (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 (gdbarch, prev_addr))
+	bpaddr = prev_addr;
+    }
+  else
+    {
+      struct minimal_symbol *sym;
+      CORE_ADDR addr, jmpaddr;
+      int i;
+
+      boundary = unmake_mips16_addr (boundary);
+
+      /* The only MIPS16 instructions with delay slots are JAL, JALX,
+         JALR and JR.  An absolute JAL/JALX is always 4 bytes long,
+         so try for that first, then try the 2 byte JALR/JR.
+         FIXME: We have to assume that bpaddr is not the second half
+         of an extended instruction.  */
+
+      jmpaddr = 0;
+      addr = bpaddr;
+      for (i = 1; i < 4; i++)
+	{
+	  if (unmake_mips16_addr (addr) == boundary)
+	    break;
+	  addr -= 2;
+	  if (i == 1 && mips16_instruction_has_delay_slot (gdbarch, addr, 0))
+	    /* Looks like a JR/JALR at [target-1], but it could be
+	       the second word of a previous JAL/JALX, so record it
+	       and check back one more.  */
+	    jmpaddr = addr;
+	  else if (i > 1
+		   && mips16_instruction_has_delay_slot (gdbarch, addr, 1))
+	    {
+	      if (i == 2)
+		/* Looks like a JAL/JALX at [target-2], but it could also
+		   be the second word of a previous JAL/JALX, record it,
+		   and check back one more.  */
+		jmpaddr = addr;
+	      else
+		/* Looks like a JAL/JALX at [target-3], so any previously
+		   recorded JAL/JALX or JR/JALR must be wrong, because:
+
+		   >-3: JAL
+		    -2: JAL-ext (can't be JAL/JALX)
+		    -1: bdslot (can't be JR/JALR)
+		     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: JR/JALR
+		     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/JALX, 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:
@@ -6339,6 +6567,8 @@ mips_gdbarch_init (struct gdbarch_info i
 
   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: [PATCH] MIPS: Avoid breakpoints in branch delay slots

[Joel Brobecker]

> Well, 64-bit immediates may be truncated, unless we have 64-bit BFD 
> support, due to the lack of a 64-bit integer data type I believe.

OK, so that was the goal.

>  I agree that #ifdefs scattered throughout code are ugly and best avoided, 
> so I have rewritten this piece so as to derive the shift count from the 
> width of the type.  The result may not be the prettiest piece of code I 
> have ever seen, but there you go.  As I say, it may be more reasonable to 
> consider deprecating pre-C9x compilers, or at least those that do not 
> provide a "long long" alternative.

I am having doubts again, but I think we do require C90 to compile GDB.
It's C99 that is not mandatory yet (hence the use of gnulib for some of
the C99 headers). Based on that, can we consider that we're always going
to have a 64bit "long long"?

> 2011-12-07  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.

The code looks good to me. I will let you decide which version you
think is going to be better to maintain. I was just afraid that you
needed to know what the *target* address size was, in which case the
define would have been the wrong solution.

Thanks for the detailed explanations...

-- 
Joel

Re: [PATCH] MIPS: Avoid breakpoints in branch delay slots

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I am having doubts again, but I think we do require C90 to compile GDB.

We do.

Joel> It's C99 that is not mandatory yet (hence the use of gnulib for some of
Joel> the C99 headers). Based on that, can we consider that we're always going
Joel> to have a 64bit "long long"?

I think we can't.

If debugging a particular target needs a 64 bit type on the host, and
such a type isn't available, then there is just no way to do that
debugging.  But, this situation is already accounted for, see
ALL_64_TARGET_OBS in gdb/Makefile.in.

I think this setup is increasingly rare, though.

Joel> The code looks good to me. I will let you decide which version you
Joel> think is going to be better to maintain. I was just afraid that you
Joel> needed to know what the *target* address size was, in which case the
Joel> define would have been the wrong solution.

I didn't really read the thread, but I would much prefer following the
existing approach over adding more BFD64 checks, if possible.

Tom

Re: [PATCH] MIPS: Avoid breakpoints in branch delay slots

[Maciej W. Rozycki]

On Fri, 9 Dec 2011, Tom Tromey wrote:

> Joel> I am having doubts again, but I think we do require C90 to compile GDB.
> 
> We do.
> 
> Joel> It's C99 that is not mandatory yet (hence the use of gnulib for some of
> Joel> the C99 headers). Based on that, can we consider that we're always going
> Joel> to have a 64bit "long long"?
> 
> I think we can't.
> 
> If debugging a particular target needs a 64 bit type on the host, and
> such a type isn't available, then there is just no way to do that
> debugging.  But, this situation is already accounted for, see
> ALL_64_TARGET_OBS in gdb/Makefile.in.
> 
> I think this setup is increasingly rare, though.
> 
> Joel> The code looks good to me. I will let you decide which version you
> Joel> think is going to be better to maintain. I was just afraid that you
> Joel> needed to know what the *target* address size was, in which case the
> Joel> define would have been the wrong solution.
> 
> I didn't really read the thread, but I would much prefer following the
> existing approach over adding more BFD64 checks, if possible.

 The problem here is a piece of code has to handle target addresses using 
conditionals against values both inside and outside the range possible to 
express with a 32-bit host data type.  It is possible to do so if 
CORE_ADDR is 64-bit data type which can be qualified with BFD64 at the 
preprocessor level.  Otherwise addresses outside the 32-bit range will 
never be considered and the respective conditionals will not be relevant 
anymore, but these within that range still need to work.

 All of this can be done by checking the width of CORE_ADDR at the 
compiler level instead which I am actually all in favour to, but that 
requires some care because when CORE_ADDR is a 32-bit data type, the 
compiler will produce warnings about a shift amount exceeding the width of 
the data type operated on even though it only happens in dead code 
(perhaps dumber compilers will only evaluate expressions whose value is 
known at the compilation time, such as ones involving sizeof, at the run 
time and then actually emit conditional code that is supposed to be dead, 
hmm?).

 Hence this strange:

bpaddr >> ((sizeof (CORE_ADDR) << 3) - 2)

calculation that makes sure the right amount is used.  Frankly, on second 
thoughts, I could have used an expression like this:

bpaddr >> (sizeof (CORE_ADDR) == 8 ? 62 : 0)

But in the context of the:

if (sizeof (CORE_ADDR) == 8)

conditional this expression is calculated in it wouldn't be any less 
obscure, so I have only updated the comment to clarify what is going on 
here to say:

    /* Get the topmost two bits of bpaddr in a 32-bit safe manner (avoid
       a compiler warning produced where CORE_ADDR is a 32-bit type even
       though in that case this is dead code).  */

Committed now with this change, thanks for your review.

  Maciej